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

Create ERC721-With-Balance strategy #207

Closed
wants to merge 4 commits into from

Conversation

willhblackburn
Copy link

This creates the erc721-with-balance strategy.

It allows a strategy on ERC721 token balances with a minimum balance. The strategy returns 0 VP if the account has less than the minimum balance or 1 VP if the account has greater than or equal to the minimum balance.

This will allow token gated (ERC721) communities and protocols to have egalitarian voting power.

@ChaituVR
Copy link
Member

ChaituVR commented Dec 1, 2021

Hello @willhblackburn Thanks for the PR but erc20-with-balance works with ERC721 too, sorry for the confusion, we are actually trying to rename erc20-with-balance to with-balance to avoid confusion #114

@ChaituVR ChaituVR added the duplicate This issue or pull request already exists label Dec 1, 2021
@willhblackburn
Copy link
Author

Hey @ChaituVR,

Thanks, I see that history now.

I didn't like combining these strategies for two reasons (we are currently using the erc20-with-balance strategy). First, separating the strategies allow for removing decimals altogether for the erc721 strategy. Secondly, the current erc20-with-balance strategy is confusing logically. It requests a minimum balance option, but only returns voting power if the balance is greater than this minimum balance.

@ChaituVR
Copy link
Member

ChaituVR commented Dec 2, 2021

First, separating the strategies allow for removing decimals altogether for the erc721 strategy.
you can pass decimals in options to erc20-with-balance right? 🤔

but only returns voting power if the balance is greater than this minimum balance.

@willhblackburn minBalance option is optional and if not passed it will check balance is more than 0 or not

@willhblackburn
Copy link
Author

@ChaituVR

you can pass decimals in options to erc20-with-balance right? 🤔

Right, my point is you can remove decimals altogether in the erc721 strategy, thus reducing confusion.

minBalance option is optional and if not passed it will check balance is more than 0 or not

That's correct, but the logic is still unintuitive. The minBalance is a greater than check even when passed. If you specify a minimum balance for voting power, the logic should be greater than or equal to. A separate ERC721 strategy allows for resolving to an integer and a proper greater than or equal to check.

It's not that it cannot be done in one strategy; it is that separating them provides a more intuitive strategy for those trying to use Snapshot. But I understand if this is not accepted.

@ChaituVR
Copy link
Member

ChaituVR commented Dec 3, 2021

Right, my point is you can remove decimals altogether in the erc721 strategy, thus reducing confusion.

For confusion, yes, but we want to keep the strategies as optimal as possible. maybe we should make the decimal option optional in erc20-with-balance

That's correct, but the logic is still unintuitive. The minBalance is greater than check even when passed. If you specify a minimum balance for voting power, the logic should be greater than or equal to. A separate ERC721 strategy allows for resolving to an integer and a proper greater than or equal to check.

True let's wait for @bonustrack to see what he think we can do

@ChaituVR
Copy link
Member

Closing this for now, please reopen this if needed

@ChaituVR ChaituVR closed this Dec 13, 2021
@willhblackburn
Copy link
Author

@ChaituVR - we are waiting for @bonustrack to comment, right?

@ChaituVR ChaituVR reopened this Dec 13, 2021
@ChaituVR
Copy link
Member

@willhblackburn Maybe I think I can try to answer, there is no way to modify greater than to greater than or equal to on the existing strategy. because many spaces depend on that and new change can be confusing them, and greater than just works fine for all of them. Can I know your requirement for having greater than or equal to?

@willhblackburn
Copy link
Author

@ChaituVR - it took hours to find a strategy that worked and have it setup correctly due to the fact that the strategy uses greater than unintuitively and the strategy is centered around fungible tokens w/ decimals. Using this new strategy would help new communities by fixing both of those issues.

@ChaituVR
Copy link
Member

@willhblackburn we are actually trying to find better ways to rename a few strategies and use new strategies without affecting old proposals, we can merge this but I am thinking if this will add up to that trouble list 😅 ya Fabien is the better person to take comment on this, I will ask Fabien to reply to this :)

@ChaituVR
Copy link
Member

ChaituVR commented Jan 26, 2022

@willhblackburn We don't want to update the existing strategy to have greater than or equal to (it can affect existing spaces)
and we can't have a new strategy erc721-with-balance (duplicate - so closing this) Can you suggest any other idea? we can continue the discussion here #107 if needed

@ChaituVR ChaituVR closed this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants