Erc721 votes and general Votes component #1114

ggonzalez94 · 2024-08-22T19:40:05Z

Fixes #984

This PR introduces a generic Votes component that contains reusable logic for other token voting mechanisms. When we implemented ERC20Votes we did it in a single component that used storage variable names specific to ERC20 and it also uses StorageArray behind the scenes(Vec wasn't available at that moment), so the design is suboptimal and we would like to move away from it at least for ERC721Votes and future voting contracts.

This PR contains:

A proposed design that has a Votes component that holds all the common logic for voting.
An impl inside that component that has logic specific to ERC721Votes and ERC20Votes. If we want to add another form of voting we just need to add a new impl block to the same component that implements the get_voting_units function from the TokenVotesTrait trait.
Moves all interfaces and utils for voting and delegation to the governance module.
Remove ERC20Votes.

Things that are still missing

Create a new version of checkpoints that uses Vec instead of StorageArray. This is being challenging given that Vecis built to work with Storage and not regular structs

PR Checklist

Tests
Documentation
Added entry to CHANGELOG.md
Tried the feature on a public network

ericnordelo · 2024-08-23T18:00:41Z

Hey @ggonzalez94. I have some small comments, but since the design can still change, I won't submit those to avoid unnecessary noise. I will leave my thoughts on this comment:

TLDR:

Using traits to pass custom functionality to component implementations is a powerful feature, that we've been using to provide some capabilities hard to implement otherwise, like Hooks for token transfers. With this said, abusing this feature is an antipattern IMO, since users of the library need to define/import extra code that is called in a different context, which can be both confusing and easy to misuse.

In this context, requiring an extra implementation (ERC721Votes in the example) for a logic that could be component-provided is an antipattern, since users won't ever need to modify this implementation (different from Hooks). In this case it is better to provide two different components, to relay on them to provide the full implementation of the logic.

Regarding the proposed design, I'm concerned about the UX for users of the library. The flow looks like this:

Current design:

The contract needs to add the ERC20Component
The contract needs to add the VotesComponent
The contract needs to add an ERC20Hooks impl with the after_update logic
The contract needs to add an extra ERC721Votes implementation with the get_voting_units (which name may be a bit confusing btw).

This can get quite verbose and somehow not as simple as it could be, I think we should favor having two separate components even if it includes some code repetition, just to improve the UX, since the user would only need to:

Add the ERC20Component
Add the ERC20VotesComponent
Add an ERC20Hooks impl with the after_update logic

With this said, I think we should be able to reuse code in a separate module (not a component), where we can have the common storage and the common embeddable implementation defined using storage_node and StoragePath, with the corresponding extra traits and implementations, but this would be the first time we use this in the library, and there's no clear documentation on how to work with this at least in components yet.

I recommend we separate the components now even if we have to reuse the Storage struct and the common logic in the embeddable implementation.

PD: When proposed to use two different implementations of the same component, I was thinking of something like Ownable, where the only thing that needs to change in the contract is the embedded implementations

ggonzalez94 · 2024-08-25T21:20:01Z

Hey @ericnordelo! Thanks for the detailed feedback. I see your point, so I've been playing a bit with this and I don't think the DEX is that different between two different components vs single component with two different impl (unless there's something big I'm missing - if that's the case let me know).
This is how a contract that uses the proposed design for ERC721Votes would look like(removing some usings, etc to make the example shorter)

#[starknet::contract]
pub mod ERC721VotesMock {
    component!(path: VotesComponent, storage: erc721_votes, event: ERC721VotesEvent);
    component!(path: ERC721Component, storage: erc721, event: ERC721Event);
    component!(path: SRC5Component, storage: src5, event: SRC5Event);
    component!(path: NoncesComponent, storage: nonces, event: NoncesEvent);

    //Votes and ERC721Votes
    // Here's the only major difference - where instead of having two `impl`(internal and external)
    // We have three(common internal and external) + ERC721 specific
    #[abi(embed_v0)]
    impl VotesImpl = VotesComponent::VotesImpl<ContractState>;
    impl VotesInternalImpl = VotesComponent::InternalImpl<ContractState>;
    impl ERC721VotesImpl = VotesComponent::ERC721VotesImpl<ContractState>;

    // ERC721
    #[abi(embed_v0)]
    impl ERC721MixinImpl = ERC721Component::ERC721MixinImpl<ContractState>;
    impl ERC721InternalImpl = ERC721Component::InternalImpl<ContractState>;

    // Nonces
    #[abi(embed_v0)]
    impl NoncesImpl = NoncesComponent::NoncesImpl<ContractState>;

    #[storage]
    struct Storage {
        #[substorage(v0)]
        erc721_votes: VotesComponent::Storage,
        #[substorage(v0)]
        erc721: ERC721Component::Storage,
        #[substorage(v0)]
        src5: SRC5Component::Storage,
        #[substorage(v0)]
        nonces: NoncesComponent::Storage
    }

    #[event]
    #[derive(Drop, starknet::Event)]
    enum Event {
        #[flat]
        ERC721VotesEvent: VotesComponent::Event,
        #[flat]
        ERC721Event: ERC721Component::Event,
        #[flat]
        SRC5Event: SRC5Component::Event,
        #[flat]
        NoncesEvent: NoncesComponent::Event
    }

    /// Required for hash computation.
    pub(crate) impl SNIP12MetadataImpl of SNIP12Metadata {
        fn name() -> felt252 {
            'DAPP_NAME'
        }
        fn version() -> felt252 {
            'DAPP_VERSION'
        }
    }

    //
    // Hooks and ctor not included to make the example shorter
    //
}

As you can see the only big difference vs how this looks on a contract that uses ERC20Votes is only one impl(actually I think this one is missing refering to the internal impl). In my opinion it is a tradeoff that is worth taking or at least analyzing more carefully for the amount of code repetition we are saving, which makes our code more auditable and less error prone(we only need to change one place).
Regarding wether people could misuse the component I haven't tested that much, but I imagine that is solvable with dependencies and trait bounds? (e.g. you can only use the ERC721Votes impl or ERC20Votes impl if your contract has implementations for the common trait).
Let me know if you have thoughts!

ericnordelo · 2024-08-26T14:15:24Z

My main concern was the extra impl, because while it is true we want to avoid code repetition, I'm hesitant regarding adding trait requirements in component impls, when the implementation of the trait doesn't need to be modified by the user. With this said, I realized now that with the approach you propose users won't even need to explicitly add the extra ERC721VotesImpl in the contract, because as you mentioned, one implementation will depend on the ERC721Component, and the other one on the ERC20Component, then the compiler can automatically get the right impl as long as the parent module (VotesComponent) is in scope.

The preset can look like this:

#[starknet::contract]
pub mod ERC721Votes {
    (...)
    component!(path: ERC721Component, storage: erc721, event: ERC721Event);
    component!(path: VotesComponent, storage: votes, event: VotesEvent);
    component!(path: SRC5Component, storage: src5, event: SRC5Event);
    component!(path: NoncesComponent, storage: nonces, event: NoncesEvent);

    // ERC721 Mixin
    #[abi(embed_v0)]
    impl ERC721MixinImpl = ERC721Component::ERC721MixinImpl<ContractState>;
    impl ERC721InternalImpl = ERC721Component::InternalImpl<ContractState>;

    // Votes
    #[abi(embed_v0)]
    impl VotesImpl = VotesComponent::VotesImpl<ContractState>;

    // Nonces
    #[abi(embed_v0)]
    impl NoncesImpl = NoncesComponent::NoncesImpl<ContractState>;

    #[storage]
    struct Storage {
        #[substorage(v0)]
        erc721: ERC721Component::Storage,
        #[substorage(v0)]
        votes: VotesComponent::Storage,
        #[substorage(v0)]
        src5: SRC5Component::Storage,
        #[substorage(v0)]
        nonces: NoncesComponent::Storage
    }

    #[event]
    #[derive(Drop, starknet::Event)]
    enum Event {
        #[flat]
        ERC721Event: ERC721Component::Event,
        #[flat]
        VotesEvent: VotesComponent::Event,
        #[flat]
        SRC5Event: SRC5Component::Event,
        #[flat]
        NoncesEvent: NoncesComponent::Event
    }
    
    impl ERC721HooksImpl of ERC721Component::ERC721HooksTrait<ContractState> {
        fn after_update(
            ref self: ERC721Component::ComponentState<ContractState>,
            to: ContractAddress,
            token_id: u256,
            auth: ContractAddress,
        ) {
            let mut contract_state = ERC721Component::HasComponent::get_contract_mut(ref self);
            contract_state.votes.transfer_voting_units(from, to, 1);
        }
    }

    /// Required for hash computation.
    pub(crate) impl SNIP12MetadataImpl of SNIP12Metadata {
        fn name() -> felt252 {
            'DAPP_NAME'
        }
        fn version() -> felt252 {
            'DAPP_VERSION'
        }
    }

    #[constructor]
    fn constructor(
        ref self: ContractState,
        name: ByteArray,
        symbol: ByteArray,
        fixed_supply: u256,
        recipient: ContractAddress,
    ) {
        self.erc721.initializer(name, symbol, "");
    }
}

Notice that it is enough to add just the VotesImpl as long as VotesComponent is in scope.

And an ERC20Votes preset would only need to change the ERC721Component, the VotesImpl would work out of the box if the ERC20Votes impl is defined correctly in the VotesComponent.

I have nothing against this approach of one component then. Let's add the ERC20Votes impl to the PR too. Even if we are not removing the Legacy ERC20Votes yet, we can mark it in the CHANGELOG as deprecated, and we can start favoring the new VotesComponent.

andrew-fleming · 2024-08-27T07:47:55Z

I think the overall design looks really good! Some notes:

the compiler can automatically get the right impl as long as the parent module (VotesComponent) is in scope.

Great point

Regarding wether people could misuse the component I haven't tested that much, but I imagine that is solvable with dependencies and trait bounds? (e.g. you can only use the ERC721Votes impl or ERC20Votes impl if your contract has implementations for the common trait).

One case to consider is if a contract wanted to declare both ERC20 and ERC721 components. The contract can't expose both interfaces, but they could have the both InternalImpls in scope. The current design can't support that, but maybe we could leverage negative traits in the token votes impls?

This was briefly discussed offline, but I'll repeat it here: I think we should remove the IVotesToken interface because its contents (get_voting_units) isn't meant to be externally called. Instead, we can define it as a basic trait. Something like:

#[starknet::component]
pub mod VotesComponent {
    (...)

    trait TokenVotesTrait<TContractState> {
        fn get_voting_units(
            self: @TContractState,
            account: ContractAddress,
        ) -> u256;
    }

    impl ERC20Votes<...> of TokenVotesTrait<...> { }

    impl ERC721Votes<...> of TokenVotesTrait<...> { }
}

ericnordelo · 2024-08-28T11:03:44Z

packages/governance/src/votes/interface.cairo

@@ -35,7 +35,6 @@ pub trait IVotes<TState> {
 }

 /// Common interface for tokens used for voting(e.g. `ERC721Votes` or `ERC20Votes`)


We should update accordingly, or remove the comment since the user shouldn't implement this trait.

You mean removing the trait or removing the comment or making it just a code comment and not ///? I think having the trait is valuable since it forces contracts that use the VotesComponent to have an implementation of this trait and it allows someone that wants to use their own token voting mechanism to implement it

I'd adjust the comment because it's not an interface. This is an important distinction because trait and interface definitions look (unfortunately) similar. I might even consider moving this away from interface.cairo and into the Votes file or mod to make it clear this isn't meant to be exposed

packages/governance/src/votes/votes.cairo

ggonzalez94 · 2024-09-05T02:22:02Z

Now that we settled on the design, I cleaned up the component and added a bunch of tests. I still need to finish the tests, add documentation and implement checkpoints using Vec(so far I'm reusing the current implementation that uses StorageArray).
But I think this is ready for another round of review as I work on the rest of the things

andrew-fleming

Looking good, sir! I left some comments and questions

packages/governance/src/votes/votes.cairo

andrew-fleming · 2024-09-06T22:20:48Z

packages/governance/src/votes/interface.cairo

@@ -35,7 +35,6 @@ pub trait IVotes<TState> {
 }

 /// Common interface for tokens used for voting(e.g. `ERC721Votes` or `ERC20Votes`)


I'd adjust the comment because it's not an interface. This is an important distinction because trait and interface definitions look (unfortunately) similar. I might even consider moving this away from interface.cairo and into the Votes file or mod to make it clear this isn't meant to be exposed

packages/governance/src/votes/votes.cairo

andrew-fleming · 2024-09-06T22:54:42Z

packages/governance/src/tests/test_votes.cairo

+    let state = setup_erc721_votes();
+
+    assert_eq!(state.get_voting_units(OWNER()), 10);
+    assert_eq!(state.get_voting_units(RECIPIENT()), 0);


Small nit: I'd change RECIPIENT to OTHER or something like that and only use RECIPIENT for cases where the account is the actual recipient of a tx

packages/governance/src/tests/test_votes.cairo

andrew-fleming · 2024-09-07T00:06:24Z

packages/governance/src/tests/test_votes.cairo

+            break;
+        }
+        mock_state.erc721.mint(OWNER(), i);
+        // We manually transfer voting units here, since this is usually implemented in the hook


Why don't we integrate the logic in the mock contract hook?

andrew-fleming · 2024-09-07T01:14:35Z

packages/governance/src/tests/test_votes.cairo

+    start_cheat_caller_address(contract_address, OWNER());
+
+    state.delegate(RECIPIENT());
+    spy.assert_event_delegate_changed(contract_address, OWNER(), ZERO(), RECIPIENT());
+    spy.assert_only_event_delegate_votes_changed(contract_address, RECIPIENT(), 0, 10);
+    assert_eq!(state.get_votes(RECIPIENT()), 10);
+    assert_eq!(state.get_votes(OWNER()), 0);


I think we can improve the readability of the tests by using variables that better describe what they represent. For example, delegator, delegatee, and total_votes instead of OWNER, RECIPIENT, and 10. This is especially helpful for longer test cases with event assertions. Forgive the nitpickiness 😅 I know this is a WIP

packages/governance/src/tests/mocks/votes_mocks.cairo

packages/governance/src/votes/votes.cairo

codecov · 2024-10-04T20:57:58Z

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.62%. Comparing base (32a8d2b) to head (4ca0ae4).

Files with missing lines	Patch %	Lines
packages/governance/src/votes/utils.cairo	50.00%	1 Missing ⚠️
packages/governance/src/votes/votes.cairo	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1114      +/-   ##
==========================================
- Coverage   88.87%   88.62%   -0.25%     
==========================================
  Files          57       58       +1     
  Lines        1375     1372       -3     
==========================================
- Hits         1222     1216       -6     
- Misses        153      156       +3

Files with missing lines	Coverage Δ
packages/governance/src/votes/utils.cairo	`50.00% <50.00%> (ø)`
packages/governance/src/votes/votes.cairo	`95.91% <94.11%> (ø)`

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32a8d2b...4ca0ae4. Read the comment docs.

ggonzalez94 added 2 commits August 19, 2024 17:30

votes first iteration

4324006

consolidate votes and delegation to governace module

dd0eb38

ggonzalez94 requested review from ericnordelo, andrew-fleming and immrsd August 22, 2024 19:53

fmt

5141fb5

add ERC20Votes impl, code docs and remove interface for internal iml

2a76341