-
Notifications
You must be signed in to change notification settings - Fork 14
feat: validate space strategies #513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0188a18
to
5d8276d
Compare
5d8276d
to
fac6311
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utAck, other than above this looks good
* fix: duplicate request preventor for EIP-1271 requests * fix test
* chore: allows 50 follows for turbo users * chore: allows 50 follows for turbo users
* fix: all copeland on production * fix: remove unused variable
Updated mantle to mnt as it's new NetworkID for it.
…524) Bumps [@snapshot-labs/snapshot.js](https://github.com/snapshot-labs/snapshot.js) from 0.12.54 to 0.12.55. - [Release notes](https://github.com/snapshot-labs/snapshot.js/releases) - [Commits](snapshot-labs/snapshot.js@v0.12.54...v0.12.55) --- updated-dependencies: - dependency-name: "@snapshot-labs/snapshot.js" dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…525) Bumps [@snapshot-labs/snapshot.js](https://github.com/snapshot-labs/snapshot.js) from 0.12.55 to 0.12.56. - [Release notes](https://github.com/snapshot-labs/snapshot.js/releases) - [Commits](snapshot-labs/snapshot.js@v0.12.55...v0.12.56) --- updated-dependencies: - dependency-name: "@snapshot-labs/snapshot.js" dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…526) Bumps [@snapshot-labs/snapshot.js](https://github.com/snapshot-labs/snapshot.js) from 0.12.56 to 0.12.57. - [Release notes](https://github.com/snapshot-labs/snapshot.js/releases) - [Commits](snapshot-labs/snapshot.js@v0.12.56...v0.12.57) --- updated-dependencies: - dependency-name: "@snapshot-labs/snapshot.js" dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) * feat: sync spaces custom domain with walletconnect allowed origins * refactor: better variable name * Update .env.example Co-authored-by: Less <[email protected]> * chore: only use domain without protocol prefix --------- Co-authored-by: Chaitanya <[email protected]> Co-authored-by: Less <[email protected]>
* feat: prevent non-premium networks * add testcases * Improve network filtering and fix test case
* feat: use correct network when fetching shib space controller * chore: use network realm instead of chain id * fix: allow network param override
…531) Bumps [@snapshot-labs/snapshot.js](https://github.com/snapshot-labs/snapshot.js) from 0.12.57 to 0.12.58. - [Release notes](https://github.com/snapshot-labs/snapshot.js/releases) - [Commits](snapshot-labs/snapshot.js@v0.12.57...v0.12.58) --- updated-dependencies: - dependency-name: "@snapshot-labs/snapshot.js" dependency-version: 0.12.58 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat: add turbo tracking every 10s * fix: correct syntax for GetSpaces query * ci: deactivate schnaps for tests * refactor: rename to turbo_expiration * chore: remove quotes around url in .env.example Co-authored-by: Chaitanya <[email protected]> * refactor: only track if SCHNAPS_API_URL is set * chore: remove log * chore: remove dead env var Co-authored-by: Wan <[email protected]> * refactor: move while loop out of if condition Co-authored-by: Wan <[email protected]> * lint: fix * chore: remove track turbo status * ci: move loop in if condition * refactor: move while loop out of if condition * fix: process only relevant spaces for the instance network * fix: use sentry to capture errors * fix: use fech with keep alive * fix: fix invalid comparison * docs: add note about future feature * fix: only update `turbo_expiration` field * fix: improve sql query --------- Co-authored-by: Chaitanya <[email protected]> Co-authored-by: Wan <[email protected]>
…532) Bumps [@snapshot-labs/snapshot.js](https://github.com/snapshot-labs/snapshot.js) from 0.12.58 to 0.12.59. - [Release notes](https://github.com/snapshot-labs/snapshot.js/releases) - [Commits](snapshot-labs/snapshot.js@v0.12.58...v0.12.59) --- updated-dependencies: - dependency-name: "@snapshot-labs/snapshot.js" dependency-version: 0.12.59 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…533) Bumps [@snapshot-labs/snapshot.js](https://github.com/snapshot-labs/snapshot.js) from 0.12.59 to 0.12.60. - [Release notes](https://github.com/snapshot-labs/snapshot.js/releases) - [Commits](snapshot-labs/snapshot.js@v0.12.59...v0.12.60) --- updated-dependencies: - dependency-name: "@snapshot-labs/snapshot.js" dependency-version: 0.12.60 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* fix: infer turbo status from turbo expiration date * fix: update schema.sql
6cfb5ea
to
ea54370
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds validation for space strategies to ensure they are valid and enabled before allowing space settings to be saved or proposals to be created. The validation includes checking that strategies exist in the official list and are not disabled (such as the multichain
strategy).
- Extracts space validation logic into a dedicated helper module for better code organization
- Implements strategy validation against a dynamically updated list from the Score API
- Adds comprehensive test coverage for the new validation functionality
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/helpers/strategies.ts |
New module that polls and caches valid strategies from the Score API |
src/helpers/spaceValidation.ts |
Extracted and enhanced space validation logic with strategy validation |
src/writer/settings.ts |
Updated to use the new validation helper |
src/writer/proposal.ts |
Updated to use the new validation helper |
src/index.ts |
Added graceful startup/shutdown for strategy polling |
test/unit/helpers/spaceValidation.test.ts |
Comprehensive tests for the new validation logic |
test/unit/writer/settings.test.ts |
Refactored tests to use mocked validation |
test/unit/writer/proposal.test.ts |
Refactored tests to use mocked validation |
test/integration/ingestor.test.ts |
Added strategy mocking for integration tests |
test/e2e/api.test.ts |
Fixed missing return statements in async operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -58,7 +58,7 @@ describe('POST /flag', () => { | |||
settings: JSON.stringify(space.settings) | |||
})) | |||
.map(async space => { | |||
db.queryAsync('INSERT INTO snapshot_sequencer_test.spaces SET ?', space); | |||
return db.queryAsync('INSERT INTO snapshot_sequencer_test.spaces SET ?', space); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This return statement inside a map callback doesn't affect the outer function's return value. The map is creating an array of promises but they're not being awaited. Consider using Promise.all()
to properly handle the async operations.
Copilot uses AI. Check for mistakes.
@@ -75,7 +75,7 @@ describe('POST /flag', () => { | |||
vp_value_by_strategy: JSON.stringify(proposal.vp_value_by_strategy || []) | |||
})) | |||
.map(async proposal => { | |||
db.queryAsync('INSERT INTO snapshot_sequencer_test.proposals SET ?', proposal); | |||
return db.queryAsync('INSERT INTO snapshot_sequencer_test.proposals SET ?', proposal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above - this return statement inside a map callback doesn't affect the outer function's return value. The map is creating an array of promises but they're not being awaited. Consider using Promise.all()
to properly handle the async operations.
Copilot uses AI. Check for mistakes.
08ab602
to
cb2a274
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tAck
except few small comments above
4217967
to
01a27e4
Compare
we can merge when you are ready |
Toward https://github.com/snapshot-labs/workflow/issues/227
Toward https://github.com/snapshot-labs/workflow/issues/631
This PR adds validation to the space top level strategies.
Summary
It will ensure that:
On both cases, it will return an error, preventing:
The only disabled strategies right now is
multichain
Changes
In order to accomplish the strategies validation, we needed to have a source of truth for valid and enabled strategies. This list will come from https://score.snapshot.org/api/strategies , and there is a new file which will poll and refresh a
strategies
object regularly, like on the hub.The infinite loop is started on express.js app start, and will end gracefully on shutdown.
The
validateSpaceSettings
function, which was located in writer/settings.ts, has been extracted into its own file, in helper/validation.ts, since this function was also used by writer/proposal.tsIt makes more sense to have this common function inside a helper, which is also simplifying the test considerably, and avoiding duplicates tests.
Finally, there's been some refactoring in the tests, to:
validateSpaceSettings
into its own fileTest
multichain
strategyIf not using an SDK, you can: