Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Mar 9, 2025

Toward https://github.com/snapshot-labs/workflow/issues/227

This PR adds validation to the space top level strategies.

It will ensure that:

  • top level strategies are existing strategies
  • top level strategies are not disabled

Test

  • Use SDK to send space setting with inexisting strategies
  • It should return error
  • Use SDK to send space setting with multichain strategy
  • It should return error

@wa0x6e wa0x6e requested a review from Copilot March 9, 2025 22:37
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR introduces a new validation mechanism for space strategies by fetching a list of approved strategies from an external API.

  • Importing fetchWithKeepAlive and a new SCORE_API_URL from environment variables
  • Adding a try-catch block in the verify function to validate strategy names and deprecation status
  • Rejecting the process if any strategy is invalid or deprecated

Reviewed Changes

File Description
src/writer/settings.ts Validates strategies by calling an external API to ensure strategy validity and status

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@wa0x6e wa0x6e force-pushed the feat-validate-space-strategies branch from 0188a18 to 5d8276d Compare March 9, 2025 22:58
@wa0x6e wa0x6e force-pushed the feat-validate-space-strategies branch from 5d8276d to fac6311 Compare March 9, 2025 23:15
@wa0x6e wa0x6e marked this pull request as ready for review March 10, 2025 17:49
@wa0x6e wa0x6e requested a review from ChaituVR March 11, 2025 11:45
@@ -80,6 +81,26 @@ export async function verify(body): Promise<any> {
return Promise.reject(`max number of strategies is ${strategiesLimit}`);
}

try {
const strategiesList = await (await fetchWithKeepAlive(`${scoreAPIUrl}/api/strategies`)).json();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should call it every 5 mins instead of calling it everytime, but can make things look complicated 🙈

@@ -80,6 +81,26 @@ export async function verify(body): Promise<any> {
return Promise.reject(`max number of strategies is ${strategiesLimit}`);
}

try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should move this check and strategies limit check to a new function similar to validateSpaceSettings

Copy link
Member

@ChaituVR ChaituVR left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants