-
Notifications
You must be signed in to change notification settings - Fork 329
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
base: main
Are you sure you want to change the base?
ERC20 Permit Component #1140
Conversation
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.
Hey Alejandro, the implementation is looking good, but I think we should refactor this into an extra embeddable implementation in the ERC20Component, instead of an extra component, since this would reduce the boilerplate in the final contract, and a component without storage members and events can be just an extra module.
packages/token/src/erc20/extensions/erc20_permit/erc20_permit.cairo
Outdated
Show resolved
Hide resolved
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.
Looking good, sir! I do agree with Eric with having ERC20Permit as an embeddable impl inside of ERC20Component. This should be a little easier to integrate into a contract. Aside from this, I left a few comments :)
packages/token/src/erc20/extensions/erc20_permit/erc20_permit.cairo
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1140 +/- ##
==========================================
+ Coverage 88.87% 89.01% +0.14%
==========================================
Files 57 59 +2
Lines 1375 1393 +18
==========================================
+ Hits 1222 1240 +18
Misses 153 153
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Nice job on the improvements! This looks almost ready to me. I left a small suggestion and a question
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.
Sooo it seems a little misleading to me to have the impl in the core module and keep the erc20_permit
module name under extensions. Do you think erc20_permit_utils
would be better?
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.
Actually, that was my initial decision while refactoring. But having two files under extensions erc20_votes
and erc20_permit_utils
seemed inconsistent and the "utils" suffix felt redundant. I don't have a strong opinion on this matter, I'm okay with both of the names
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.
I agree, it's not great. I'm okay with the current erc20_permit
for now. @ericnordelo any strong opinions on this?
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.
For consistency with the recent SNIP-9 addition, I would move the file under erc20
but outside of extensions
, and rename it to snip12_utils.cairo
, since it only contains snip12 utilities. We should also move the erc20_votes
snip12 utilities that are currently in the same file of the component, to that new file outside extensions.
We should probably have only one snip12_utils
module per package, meaning that a refactor of SNIP-9 would also be required, but I think is worth doing that to improve/keep organization.
Something like:
token
└─── erc20
│ └─── extensions
│ │ │ erc20_votes.cairo
│ └─── snip12_utils
│ │ │ votes.cairo
│ │ │ permit.cairo
│ │ erc20.cairo
│ │ extensions.cairo
│ │ interface.cairo
│ │ snip12_utils.cairo
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.
Restructured the files accordingly
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.
Great job with the refactor @immrsd! Left some comments.
spender: ContractAddress, | ||
amount: u256, | ||
deadline: u64, | ||
signature: Array<felt252> |
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.
signature: Array<felt252> | |
signature: Span<felt252> |
We should favor Span<felt252>
over Array<felt252>
since it is significantly cheaper as parameter for external functions (related to Serde and compiler internal implementation of the type).
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 should favor Span over Array, but in this case we need Array (to make the is_valid_signature
call to the owner's account, which expects a signature as Array<felt252>
)
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.
Similar to the implementation in SRC9, we should receive the Span, and call into when calling is_valid_signature
, since we only keep that Array for BC with the interface Id, but we should not continue adding more Array when not needed. It would be nice if you could profile the calls using Span and into vs and Array to compare steps.
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.
For consistency with the recent SNIP-9 addition, I would move the file under erc20
but outside of extensions
, and rename it to snip12_utils.cairo
, since it only contains snip12 utilities. We should also move the erc20_votes
snip12 utilities that are currently in the same file of the component, to that new file outside extensions.
We should probably have only one snip12_utils
module per package, meaning that a refactor of SNIP-9 would also be required, but I think is worth doing that to improve/keep organization.
Something like:
token
└─── erc20
│ └─── extensions
│ │ │ erc20_votes.cairo
│ └─── snip12_utils
│ │ │ votes.cairo
│ │ │ permit.cairo
│ │ erc20.cairo
│ │ extensions.cairo
│ │ interface.cairo
│ │ snip12_utils.cairo
spender: ContractAddress, | ||
amount: u256, | ||
deadline: u64, | ||
signature: Array<felt252> |
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.
signature: Array<felt252> | |
signature: Span<felt252> |
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 have to require a signature in the format of an array
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.
See #1140 (comment)
@@ -107,3 +121,43 @@ pub trait ERC20VotesABI<TState> { | |||
ref self: TState, sender: ContractAddress, recipient: ContractAddress, amount: u256 | |||
) -> bool; | |||
} | |||
|
|||
#[starknet::interface] | |||
pub trait ERC20PermitABI<TState> { |
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 implements ERC20ABI
. If we include permit fns into ERC20ABI
, we'll have to require ERC20Permit
to be implemented for ERC20Mixin
to become available. I don't think we should do that and that ERC20Permit
is the feature that should be expected to be ON by default
This 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 implemented
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 (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
TLDR: Mixin shouldn't be necessarily pegged to component ABIs, since they are not defined as the same thing.
Maybe it's worth using #[generate_trait]
for all the mixins to establish a better differentiation
utils::declare_and_deploy_at( | ||
"DualCaseAccountMock", data.owner, array![data.key_pair.public_key] | ||
); |
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.
Is something preventing us from using COMPONENT_STATE
instead of deploying and dispatchers? Note that using COMPONENT_STATE should speed up running the tests.
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.
There's nothing preventing us from following the approach with CONTRACT_STATE
, but there's also no strong reason why this approach is better. I reckon that having a contract deployed and interacting with it through dispatcher feels closer to the real-world usage scenario and also prevents from using any internal functions that
are not supposed and even can't be called externally
Either way tests run pretty fast. That's compilation that takes most time. With that said, I think it's fine to keep interacting with the contract under test through a dispatcher. What do you think?
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.
I had the same thoughts when we started following this logic, we already had a discussion about it and decided to test components as separate modules, since they are not deployed contracts, but modules to extend those. Declaring and deploying in tests take time and adds up while we continue adding more tests. I'm not against reopening the discussion about it, but for now, since we should always favor consistency, let's refactor into using COMPONENT_STATE as we've been doing when possible for component tests.
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.
I'm not against reopening the discussion about it, but for now, since we should always favor consistency, let's refactor into using COMPONENT_STATE as we've been doing when possible for component tests.
+1
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.
Agreed with Eric's review, namely:
- Span (+ into) > Array
- ComponentState testing pattern > Dispatchers
- If you feel strongly about favoring dispatchers for testing, let's discuss outside of the PR
/// - The address of the owner | ||
/// - The parameters specified in the `approve` function (spender and amount) | ||
/// - The address of the token contract itself | ||
/// - A nonce, which must be unique for each operation, incrementing after each use to prevent | ||
/// reuse of the signature | ||
/// - The chain ID, which protects against cross-chain replay attacks |
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.
/// - The address of the owner | |
/// - The parameters specified in the `approve` function (spender and amount) | |
/// - The address of the token contract itself | |
/// - A nonce, which must be unique for each operation, incrementing after each use to prevent | |
/// reuse of the signature | |
/// - The chain ID, which protects against cross-chain replay attacks | |
/// - The address of the owner. | |
/// - The parameters specified in the `approve` function (spender and amount). | |
/// - The address of the token contract itself. | |
/// - A nonce, which must be unique for each operation, incrementing after each use to prevent. | |
/// reuse of the signature. | |
/// - The chain ID, which protects against cross-chain replay attacks. |
Small nit: we end list items with a period (like in the Requirements section for permit
)
utils::declare_and_deploy_at( | ||
"DualCaseAccountMock", data.owner, array![data.key_pair.public_key] | ||
); |
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.
I'm not against reopening the discussion about it, but for now, since we should always favor consistency, let's refactor into using COMPONENT_STATE as we've been doing when possible for component tests.
+1
Implements ERC20Permit extension
PR Checklist