Add ERC20 preset #810
Add ERC20 preset #810
Conversation
There was a problem hiding this comment.
Looking good! We still need to add the class_hash table somehow in the docs, but I think we can merge this PR and tackle that in another one, because that one is going to take some time no design and back and forth, and having these PRs staled is not necessary imo.
That we do. Yeah, agreed on not stalling all the PRs |
| @@ -147,7 +147,7 @@ fn test__approve_from_zero() { | |||
| fn test__approve_to_zero() { | |||
| let mut state = setup(); | |||
| testing::set_caller_address(OWNER()); | |||
| state.erc20._approve(OWNER(), Zeroable::zero(), VALUE); | |||
| state.erc20._approve(OWNER(), ZERO(), VALUE); | |||
There was a problem hiding this comment.
why do we have ZERO() instead of using Zeroable::zero()?
There was a problem hiding this comment.
IMO shorter and easier to read and follow (is not the same to ctrl+f Zeroable::zero that is used for everything zeroable, than to look for the ZERO() constant)
There was a problem hiding this comment.
we could simply use zero() from Zeroable if it's a matter of length. I don't see much value in it and the fact that it's used for everything zero (address, uint, etc) is not a problem to me. Otoh I don't think it's that important, even though I prefer minimalism and only adding something if it's really needed, there's no problem with ZERO() either.
Co-authored-by: Martín Triay <martriay@gmail.com>
Fixes (partial) #804.
PR Checklist