Skip to content
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

[delegate-registry-v2] Add the delegate registry v2 startegy #1205

Merged
merged 23 commits into from
Oct 7, 2023

Conversation

asgeir-s
Copy link
Contributor

Add the delegate-registry-v2 strategy.

@ChaituVR ChaituVR changed the title Add the delegate registry v2 startegy [delegate-registry-v2] Add the delegate registry v2 startegy Jul 1, 2023
ChaituVR
ChaituVR previously approved these changes Jul 1, 2023
@ChaituVR
Copy link
Member

ChaituVR commented Jul 1, 2023

@manboy-eth We need to add this strategy to src/strategies/index.ts

@ChaituVR
Copy link
Member

ChaituVR commented Jul 1, 2023

@manboy-eth Can take a look at failing test cases, try running `yarn test --strategy=delegate-registry-v2 in your local

@ChaituVR ChaituVR dismissed their stale review July 1, 2023 06:49

tests failing

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Jul 3, 2023

@manboy-eth Can take a look at failing test cases, try running `yarn test --strategy=delegate-registry-v2 in your local

I appreciate your help, @ChaituVR.

I am a little unsure how to proceed. Here's how the voting strategy is supposed to work:

  1. Our backend (https://delegate-registry-backend.vercel.app) will return the amount that is delegated to an address, provided the address is not delegating further.
  2. This result is merged with other strategies in the Snapshot space, such as erc20-balance-of.
  3. We plan to add a validation strategy that ensures only addresses that are not delegating can vote (so that addresses that are delegating can not vote with the score returned by the erc20-balance-of strategy).

Does this approach make sense, or should we structure it differently? Also, is there a way to automatically force a space to enable a validation strategy if a voting strategy is enabled?

Also, since the strategy has to return at least one address with a score, is it possible to select what snapshot space should be used in the test? If not, could someone send some tokens (the ones referenced in the yam.eth's voting erc20 strategy) to 0xD028d504316FEc029CFa36bdc3A8f053F6E5a6e4? This would allow me to make a delegation for the yam.eth space.

@ChaituVR
Copy link
Member

Hi @manboy-eth

Does this approach make sense,

Makes sense, but also you could return 0 voting power from this strategy itself and use this strategy in basic validation and check min score. https://docs.snapshot.org/user-guides/strategies/validation-strategies#voting-validation-in-space-settings then we don't need separate validation

is there a way to automatically force a space to enable a validation strategy if a voting strategy is enabled?

There is no way to force, spaces should enable validation if they want to (maybe you can add this in README)

since the strategy has to return at least one address with a score, is it possible to select what snapshot space should be used in the test?

You can easily create one on demo.snapshot.org, using goerli. it should be free :)

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Jul 31, 2023

Makes sense, but also you could return 0 voting power from this strategy itself and use this strategy in basic validation and check min score. https://docs.snapshot.org/user-guides/strategies/validation-strategies#voting-validation-in-space-settings then we don't need separate validation

I am unsure how I can make that work since we need to distinguish token holders who have delegated and token holders who have not delegated (but still has a score from other strategies). In both cases, one or more other strategies in the space can return a non-zero value and should be merged.

We need a validation strategy that can make it possible to vote as long as the delegation registry strategy does not return "0".

I think we have three scenarios (numbers 2 and 3 can probably be counted as the same, I am separating them here for clearity):

  1. When the strategy returns 0, it should not be possible to vote - the address has delegated.
  2. When the strategy returns undefined it should be able to vote, merging all the scores in the space - the address has not delegated and has no delegators delegating to it.
  3. When the strategy returns a positive number, it should be able to vote, merging all the scores in the space - the address has delegations to it and possibly a score from other strategies in the space.

So basically, the validation strategy becomes:

score  = getScoreFromVotingStrategy()
canVote = score !== "0" // undefined and a number can vote

There is no way to force, spaces should enable validation if they want to (maybe you can add this in README)

Okay.

You can easily create one on demo.snapshot.org, using goerli. it should be free :)

I meant the space used in the tests when I run yarn test --strategy=delegate-registry-v2, it looks like it is hardcoded to use the yam.eth space.

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Aug 3, 2023

I created and added the required validation strategy and added it to the readme.

@asgeir-s asgeir-s requested a review from ChaituVR August 10, 2023 09:05
@ChaituVR
Copy link
Member

Hi @manboy-eth I meant adding basic validation directly to your space 😅 Don't have to create a seperate validation

@asgeir-s
Copy link
Contributor Author

Hi @manboy-eth I meant adding basic validation directly to your space 😅 Don't have to create a seperate validation

But the problem is that basic validation does not work for our requirements (check the comment above: #1205 (comment)).
Because of this, we had to create a new validation strategy.

@ChaituVR
Copy link
Member

oh i get it now, thanks!

But what will be the voting power if delegate-registry-v2 returns 0 ? 🤔

@ChaituVR
Copy link
Member

Can't depend on undefined from strategies, better if strategy can decide on who got voting power or who don't have voting power

@auryn-macmillan
Copy link
Contributor

@manboy-eth it seems like we should just encapsulate everything in the delegation strategy, perhaps?

So rather than the current setup, where there is a delegation strategy alongside any number of other strategies...

"strategies": [
    {
      "name": "gno"
    },
    {
      "name": "delegation",
      "params": {
        "strategies": [
          {
            "name": "gno"
          }
        ]
      }
    },
]

Perhaps everything should be encapsulated by the delegation strategy. This way there is no need for additional validation outside of the strategy.

"strategies": [
    {
      "name": "delegation",
      "params": {
        "strategies": [
          {
            "name": "gno"
          }
        ]
      }
    },
]

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Sep 5, 2023

oh i get it now, thanks!

But what will be the voting power if delegate-registry-v2 returns 0 ? 🤔

Then the address has delegated its voting power and should not be able to vote.

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Sep 5, 2023

This is the way it currently works:

  • undefined means "address not in the registry",
  • 0 means, address have delegated and
  • a positive number is the number of votes delegated to the address.

Regarding @auryn-macmillan 's suggested approach:

  1. @ChaituVR: Can you make the UI for setting up and managing strategies handle this?
  2. @ChaituVR: Does this have any impact on your caching systems etc.?
  3. Just to make sure I understand, then we would compute everything like we do today but with two adjustments:
    a. When retrieving strategies during the creation of a snapshot, we will get it from the internal strategy settings.
    b. When returning the vote weight, we get our snapshot of vote weights from the delegation registry, then we get the vote weight from each strategy, merge the vote weights as intended, and return the results.

src/strategies/delegate-registry-v2/index.ts Outdated Show resolved Hide resolved
src/validations/strategy-zero-gated/index.ts Outdated Show resolved Hide resolved
src/validations/strategy-zero-gated/index.ts Outdated Show resolved Hide resolved
@ChaituVR
Copy link
Member

ChaituVR commented Sep 6, 2023

undefined means "address not in the registry",
0 means, address have delegated and
a positive number is the number of votes delegated to the address.

Better to handle this in strategy itself

@ChaituVR
Copy link
Member

ChaituVR commented Sep 6, 2023

@ChaituVR: Can you make the UI for setting up and managing strategies handle this?

Sure, create a ticket on https://discord.snapshot.org/ will get in touch to create a test space for you

@ChaituVR: Does this have any impact on your caching systems etc.?

Using a delegation strategy won't have any impact on caching

Just to make sure I understand, then we would compute everything like we do today but with two adjustments:
a. When retrieving strategies during the creation of a snapshot, we will get it from the internal strategy settings.

Correct, whenever a proposal is created - we copy strategies to the proposal so it will use these strategies for ever

b. When returning the vote weight, we get our snapshot of vote weights from the delegation registry, then we get the vote weight from each strategy, merge the vote weights as intended, and return the results.

didn't get it sorry 🙈

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Sep 7, 2023

It sounds like we might have some miscommunication, so I just have to make sure we are on the same page before I start working on this:

So if I create a ticket in Discord, you will make the user interface for configuring any snapshot strategy inside of our strategy settings like Auryn proposes here: #1205 (comment). This seems like a potentially big and complicated task to me 🤔
After you work on the user interface, users will be able to manage strategies (add, remove, or edit settings) nested inside our strategies params. This setting will not be at the normal strategy configuration location strategies but will rater be located inside of our strategy configuration like this strategies[0].params.strategies (the index is 0 because it will always be only one top-level strategy when the delegation registry strategy is added - the delegation registry strategy).

In other words, you will handle all these special cases (only applicable to the delegation registry strategy):

  • When the delegation registry strategy is added to a space, the preexisting strategies will be moved inside of the delegation registry's params settings.
  • When the delegation registry is removed, all the strategies inside of the delegation registry's params settings will be placed back in the top-level strategy location.
  • When a strategy is added to a space with the delegation registry strategy already added, it will be added under the delegation registry's params. strategies`?
    and so on.

This way we will be able to do all the vote weight computations for all strategies in the space, inside of our strategy.

@ChaituVR
Copy link
Member

ChaituVR commented Sep 8, 2023

I am sorry but i am having a hard time understanding your requirements, What interface you are talking about? Can you explain me an example situation of how you want to calculate your voting power?

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Sep 8, 2023

I am sorry but i am having a hard time understanding your requirements, What interface you are talking about? Can you explain me an example situation of how you want to calculate your voting power?

We wonder if we can have all the strategies inside of our strategy like @auryn-macmillan suggested (look at the strategy configuration he has suggested in his comment here: #1205 (comment)).

I am talking about the user interface / GUI / frontend at "https://snapshot.org", and what I think would be required if we were going to do it as @auryn-macmillan suggested.

@auryn-macmillan
Copy link
Contributor

I don't think this necessarily requires UI changes in order to function as intended.
With the existing UI, you should already be able to configure the delegation strategy.

@asgeir-s
Copy link
Contributor Author

asgeir-s commented Sep 11, 2023

Okay, so users will have to manually add the strategy settings for all the strategies they want to use here:
Screenshot 2023-09-11 at 14 11 40

And will not be able to use Snapshot's normal strategies management UI for adding, removing or editing strategies (they must do it manually as specified above):
Screenshot 2023-09-11 at 14 13 23
Screenshot 2023-09-11 at 14 12 31

Also, we must inform users not to add other strategies using Snapshot's normal strategies management UI.

@auryn-macmillan and @ChaituVR: That's what we want?

@auryn-macmillan
Copy link
Contributor

I think that works for the time being, we can improve on it in a subsequent PR.

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.

Everything else looks good to me

src/strategies/delegate-registry-v2/index.ts Show resolved Hide resolved
src/strategies/delegate-registry-v2/index.ts Show resolved Hide resolved
@asgeir-s asgeir-s requested a review from ChaituVR October 4, 2023 14:45
@ChaituVR ChaituVR merged commit ccfecb7 into snapshot-labs:master Oct 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants