Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ERC20 Permit Component #1140
ERC20 Permit Component #1140
Changes from 19 commits
a055099
cfa836a
9973d9a
23ba5da
87f1859
67c3fc7
a499394
4bfbe36
09fb2d2
950205e
1c2fc82
5702d0c
3e48a9a
8fbbe48
26ce488
acad601
2cef6d1
4d23651
72ab63d
d90a50d
0204425
68e45a4
e023847
78fa616
6776641
c44cf42
979117b
34a3d72
a144d66
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to add an extra ABI trait, but include the new implementations in the ERC20ABI trait, even if the user can choose not to use some implementations, the ABI should contain the full ABI of the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERC20Mixin
actually implementsERC20ABI
. If we include permit fns intoERC20ABI
, we'll have to requireERC20Permit
to be implemented forERC20Mixin
to become available. I don't think we should do that and thatERC20Permit
is the feature that should be expected to be ON by defaultThis way it makes total sense to have an additional interface named
ERC20PermitABI
, so it can be used for the cases when the trait is included and implementedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERC20Mixin (and other mixins) implement the ABI of the component by coincidence, but not by definition. Mixin and ABIs are different things, even when they sometimes match.
A Mixin is a set of implementations that are commonly used together (as mentioned here) whose objective is to avoid verbosity. A component ABI trait, on the other hand, is intended as a single dispatcher for the component full interface, even if it doesn't have to be implemented all-together, as explained in this recently added note:
We didn't follow this principle in Ownable and OwnableTwoStep because the same function (transfer ownership) with the same interface has different implementations that can be problematic if they are used thinking the other one is in place, so we favoured clarity over the rule. In this case, the permit implementation doesn't have a problematic intersection and can be added to the ABI as intended.
In this case, we can use
#[generate_trait]
for the mixin instead of implementing from the ABI trait, and we should add permit to the existing ABI.Note that adding a different ABI per feature would create a lot of repetition in the interface file if the component grows bigger in features.
TLDR: Mixin shouldn't be necessarily pegged to component ABIs, since they are not defined as the same thing.
cc @andrew-fleming that may correct me if I recall something the wrong way from when we worked on this definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds accurate to me. Looking at the docs now, I do think we can improve our definition of ABI traits. We mention that they're useful for preset contracts; however, this isn't necessarily true anymore. We have a preset interfaces dir now and the ABI doesn't include the interface for upgrades. Further clarity and purpose would be helpful IMO
Maybe it's worth using
#[generate_trait]
for all the mixins to establish a better differentiationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[generate_trait]
won't help here since the embeddable trait has to implement a starknet interface. I decoupled mixin impl from ABI and added aIERC20Mixin
interface