ERC20Votes Interface implementation

[pscott]

I am trying to implement an ERC20 Votes token using the ERC20Votes extension. It worked fine on 0.13.0, but it's not working since 0.14.0.

📝 Details

The ERC20Votes interface mentions the presence of num_checkpoints() and checkpoint(). Those functions are not part of the ERC20VotesImpl, so my understanding is they were supposed to be defined by the implementor (me).

I did it with 0.13.0 (see code). Ever since 0.14.0 (this commit), Checkpoint is a pub(crate) which means it's only accessible to the crate.

This means that trying to use openzeppelin::utils::structs::checkpoints::Checkpoint; will error. But it's a type required in the function signature of checkpoints, which means it's impossible for the implementor to implement it.

But if I don't implement it, then the contract will be just fine; however i won't respect the ERC20VotesABI interface.

So the question is:

1. Are those two functions really supposed to be part of the interface?
2. Is there another way of implementing them rather than manually implementing them on the contract?
3. Is the Checkpoint struct exported elsewhere?
4. Am I misunderstanding the interface?

🔢 Code to reproduce bug

Token contract:

#[starknet::contract]
pub mod GenericERC20Votes {
    use openzeppelin::governance::utils::interfaces::IVotes;
    use openzeppelin::token::erc20::{ERC20Component};
    use openzeppelin::token::erc20::extensions::{ERC20VotesComponent};
    use openzeppelin::utils::nonces::NoncesComponent;
    use openzeppelin::utils::cryptography::snip12::{SNIP12Metadata};
    use starknet::ContractAddress;
    use openzeppelin::utils::structs::checkpoints::Checkpoint; // ?? This will error with 0.14.0 but is required to implement the full interface

    component!(path: ERC20Component, storage: erc20, event: ERC20Event);
    component!(path: ERC20VotesComponent, storage: erc20_votes, event: ERC20VotesEvent);
    component!(path: NoncesComponent, storage: nonces, event: NoncesEvent);

    #[abi(embed_v0)]
    impl ERC20MixinImpl = ERC20Component::ERC20MixinImpl<ContractState>;
    impl ERC20InternalImpl = ERC20Component::InternalImpl<ContractState>;

    #[abi(embed_v0)]
    impl ERC20VotesImpl = ERC20VotesComponent::ERC20VotesImpl<ContractState>;
    impl ERC20VotesInternalImpl = ERC20VotesComponent::InternalImpl<ContractState>;

    impl NoncesImpl = NoncesComponent::NoncesImpl<ContractState>;

    impl SNIP12MetadataImpl of SNIP12Metadata {
        fn name() -> felt252 {
            'toto' // TODO
        }

        fn version() -> felt252 {
            '1.0.0' // TODO
        }
    }

    #[storage]
    struct Storage {
        #[substorage(v0)]
        erc20: ERC20Component::Storage,
        #[substorage(v0)]
        erc20_votes: ERC20VotesComponent::Storage,
        #[substorage(v0)]
        nonces: NoncesComponent::Storage,
    }

    #[event]
    #[derive(Drop, starknet::Event)]
    enum Event {
        #[flat]
        ERC20Event: ERC20Component::Event,
        #[flat]
        ERC20VotesEvent: ERC20VotesComponent::Event,
        #[flat]
        NoncesEvent: NoncesComponent::Event,
    }

    //
    // Hooks
    //

    impl ERC20VotesHooksImpl of ERC20Component::ERC20HooksTrait<ContractState> {
        fn before_update(
            ref self: ERC20Component::ComponentState<ContractState>,
            from: ContractAddress,
            recipient: ContractAddress,
            amount: u256
        ) { // Nothing to do
        }

        fn after_update(
            ref self: ERC20Component::ComponentState<ContractState>,
            from: ContractAddress,
            recipient: ContractAddress,
            amount: u256
        ) {
            // Access local state from component state
            let mut contract_state = ERC20Component::HasComponent::get_contract_mut(ref self);
            // Function from integrated component
            contract_state.erc20_votes.transfer_voting_units(from, recipient, amount);
        }
    }

     /// Get number of checkpoints for `account`.
    #[external(v0)]
    fn num_checkpoints(self: @ContractState, account: ContractAddress) -> u32 {
        let unsafe_state = ERC20Votes::unsafe_new_contract_state();
        ERC20Votes::InternalImpl::num_checkpoints(@unsafe_state, account)
    }

    /// Get the `pos`-th checkpoint for `account`.
    #[external(v0)]
    fn checkpoints(self: @ContractState, account: ContractAddress, pos: u32) -> Checkpoint {
        let unsafe_state = ERC20Votes::unsafe_new_contract_state();
        ERC20Votes::InternalImpl::checkpoints(@unsafe_state, account, pos)
    }

    #[constructor]
    fn constructor(
        ref self: ContractState,
        name: ByteArray,
        symbol: ByteArray,
        initial_supply: u256,
        recipient: ContractAddress
    ) {
        self.erc20.initializer(name, symbol);
        self.erc20.mint(recipient, initial_supply);
    }
}

[ericnordelo]

Hey @pscott, thanks for the detail report. To answer your questions:

Are those two functions really supposed to be part of the interface?

They should be optional and not part of the ABI as they currently are since they are not exposed in any embeddable implementation.

Is there another way of implementing them rather than manually implementing them on the contract?

There's not.

Is the Checkpoint struct exported elsewhere?

Not but it should, that is an issue of the latest release, and should be fixed. The idea was to make StorageArray private, since the intention is to wait for the cairo native one. We should make Checkpoint public again.

Am I misunderstanding the interface?

The interface represents the interface of a preset that was removed (which included those two methods). It should be fixed.

I created this PR and we should be releasing a fix soon. Thanks again.