Migrate ERC20Votes to component #951
Conversation
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
… into feat/snip-12
…eat/erc20-votes-component
There was a problem hiding this comment.
The component migration looks really, really good!
I left some mostly small comments and questions—the most noteworthy, I think, is the idea of adding a default hook impl for ERC20 contracts in the ERC20 module. The benefit of this is that contracts can just import the default impl in their contract if there's no additional before/after_update logic (instead of recreating the impl that essentially does nothing). Let me know what you think
| /// Returns the checkpoint at given position. | ||
| fn at(self: @Trace, pos: u32) -> Checkpoint { | ||
| assert(pos < self.length(), 'Array overflow'); | ||
| self.checkpoints.read_at(pos) | ||
| } |
There was a problem hiding this comment.
So I'm still in favor of unit testing all these functions. I know we concluded in the initial PR that tests are good but shouldn't be a priority (in this particular case). I vote we make an issue for it (after we merge this) so this doesn't get lost
There was a problem hiding this comment.
We have time to add the unit tests before merging this PR. I will work on it.
There was a problem hiding this comment.
I was working on this already, but I realized that these unit tests will most likely change when the native StorageArray (that Ariel mentioned is coming) arrives, at that point we will probably remove our own StorageArray implementation, and refactor Trace and Checkpoint from this. I lean toward creating the issue and waiting for the new storage array to come before adding the unit tests, since the module is already released, audited, and used in production. Wdyt?
There was a problem hiding this comment.
Mmmm right. Yeah, creating an issue works for me
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
…airo-contracts into feat/erc20-votes-component
…eat/erc20-votes-component
src/presets/erc20_votes.cairo
Outdated
| } | ||
|
|
||
| /// Required for hash computation. | ||
| impl SNIP12MetadataImpl of SNIP12Metadata { |
There was a problem hiding this comment.
@andrew-fleming even if we decide to use storage, I would need to update the SNIP12 utilities and add a component for that, and change the documentation. I still lean towards keeping the current implementation and removing the preset, but even if we decide to keep it, I think we should remove it for this PR, and add it again on the future after updating the SNIP12 implementations.
There was a problem hiding this comment.
Maybe we could write an instructional blog post or guide on creating an ERC20Votes contract. We have the wizard, of course, but maybe a guide could help compliment it? I do agree though that removing the preset is the best approach right now
Fixes #815
PR Checklist