ERC20 Permit Component #1140

immrsd · 2024-09-10T11:59:25Z

PR Checklist

Tests
Documentation in a separate PR
Added entry to CHANGELOG.md
Tried the feature on a public network

ericnordelo

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.

CHANGELOG.md

ericnordelo · 2024-09-11T11:02:34Z

packages/presets/src/erc20_permit.cairo

+/// signature (following SNIP-12 standard) used for ERC20Permit functionality. It's crucial that the
+/// SNIP12Metadata name remains unique to avoid confusion and potential security issues.
+///
+/// For more complex or custom contracts, use Wizard for Cairo


Will we have this release together with a Wizard update?

Removed ERC20Permit preset and this message also as a result

packages/token/src/erc20/extensions.cairo

ericnordelo · 2024-09-11T11:44:20Z

packages/token/src/erc20/extensions/erc20_permit/erc20_permit.cairo

+/// EIP-2612: https://eips.ethereum.org/EIPS/eip-2612
+/// SNIP-12:  https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-12.md
+#[starknet::component]
+pub mod ERC20PermitComponent {


Since this extension doesn't need extra storage or events, it doesn't need to be a component, but can be implemented as an extra embeddable_impl in ERC20Component. This module can still exist as a container for helpers.

All we need is this extra impl in the ERC20Component (with the corresponding imports):

#[embeddable_as(ERC20PermitImpl)] impl ERC20Permit< TContractState, +HasComponent<TContractState>, +ERC20HooksTrait<TContractState>, impl Nonces: NoncesComponent::HasComponent<TContractState>, impl Metadata: SNIP12Metadata, +Drop<TContractState> > of IERC20Permit<ComponentState<TContractState>> { /// Sets the allowance of the `spender` over `owner`'s tokens after validating the signature /// generated off-chain and signed by the `owner`. /// /// Requirements: /// /// - `owner` is a deployed account contract. /// - `spender` is not the zero address. /// - `deadline` is a timestamp in the future. /// - `signature` is a valid signature that can be validated with a call to `owner` account. /// - `signature` must use the current nonce of the `owner`. /// /// Emits an `Approval` event. fn permit( ref self: ComponentState<TContractState>, owner: ContractAddress, spender: ContractAddress, amount: u256, deadline: u64, signature: Array<felt252> ) { assert(get_block_timestamp() <= deadline, Errors::EXPIRED_SIGNATURE); // Get current nonce and increment it let mut nonces_component = get_dep_component_mut!(ref self, Nonces); let nonce = nonces_component.use_nonce(owner); // Compute hash for permit let permit = Permit { token: get_contract_address(), spender, amount, nonce, deadline }; let permit_hash = permit.get_message_hash(owner); // Make a call to the account to validate permit signature let is_valid_sig_felt = DualCaseAccount { contract_address: owner } .is_valid_signature(permit_hash, signature); // Check the response is either 'VALID' or True (for backwards compatibility) let is_valid_sig = is_valid_sig_felt == starknet::VALIDATED || is_valid_sig_felt == 1; assert(is_valid_sig, Errors::INVALID_SIGNATURE); // Approve self._approve(owner, spender, amount); } /// Returns the current nonce of the `owner`. A nonce value must be /// included whenever a signature for `permit` is generated. fn nonces(self: @ComponentState<TContractState>, owner: ContractAddress) -> felt252 { let nonces_component = get_dep_component!(self, Nonces); nonces_component.nonces(owner) } /// Returns the domain separator used in generating a message hash for `permit` signature. /// The domain hashing logic follows SNIP-12 standard. fn DOMAIN_SEPARATOR(self: @ComponentState<TContractState>) -> felt252 { let domain = StarknetDomain { name: Metadata::name(), version: Metadata::version(), chain_id: get_tx_info().unbox().chain_id, revision: 1 }; domain.hash_struct() } }

And then the preset can avoid the extra component boilerplate, and can just embed the impl (together with the NoncesComponent) when the permit logic is required.

As a general rule, we should avoid components without storage and events (and sometimes even with events), since they can be regular modules.

As a general rule, we should avoid components without storage and events (and sometimes even with events), since they can be regular modules.

+1

Completely agree, refactored it to be a trait of the ERC20 component

andrew-fleming

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 :)

andrew-fleming · 2024-09-12T02:07:05Z

packages/token/src/erc20/extensions/erc20_permit/erc20_permit.cairo

+/// EIP-2612: https://eips.ethereum.org/EIPS/eip-2612
+/// SNIP-12:  https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-12.md
+#[starknet::component]
+pub mod ERC20PermitComponent {


As a general rule, we should avoid components without storage and events (and sometimes even with events), since they can be regular modules.

+1

andrew-fleming · 2024-09-12T02:34:56Z

packages/presets/src/erc20_permit.cairo

+/// # ERC20Permit Preset
+///
+/// The ERC20Permit preset integrates standard ERC20 token functionality with the ERC20Permit
+/// extension, as defined by EIP-2612. This preset allows for token approvals via off-chain
+/// signatures, thus enhancing transaction efficiency by reducing the need for on-chain approval
+/// transactions.


Do we really want a preset for erc20permit? I don't see why we'd include this and not erc20votes (that's not to suggest we should have a preset for erc20votes)

If we did include this as a preset, it should be upgradeable and its interface should be defined in presets::src::interfaces

Decided to go without the preset. It doesn't make sense to have it, especially once it was refactored to be a trait instead of a component

andrew-fleming · 2024-09-12T02:59:46Z

packages/token/src/tests/mocks/erc20_permit_mocks.cairo

+    pub(crate) impl SNIP12MetadataImpl of SNIP12Metadata {
+        fn name() -> felt252 {


Do we need the pub(crate)?

Yes, now I import the implementation to use the same name and version in the test setup function

andrew-fleming · 2024-09-12T03:42:39Z

packages/token/src/tests/erc20/test_erc20_permit.cairo

+#[test]
+fn test_valid_permit_default_data() {
+    let data = TEST_DATA();
+    let (owner, spender, amount, deadline) = (data.owner, data.spender, data.amount, data.deadline);
+    let mock = setup(data);
+
+    assert_valid_allowance(owner, spender, 0);
+
+    let nonce = mock.nonces(owner);
+    let signature = prepare_permit_signature(data, nonce);
+    mock.permit(owner, spender, amount, deadline, signature);
+
+    assert_valid_allowance(owner, spender, amount);
+    assert_valid_nonce(owner, nonce + 1);
+}
+
+#[test]
+fn test_valid_permit_other_data() {
+    let mut data = TEST_DATA();
+    data.spender = constants::OTHER();
+    data.amount = constants::TOKEN_VALUE_2;
+    let (owner, spender, amount, deadline) = (data.owner, data.spender, data.amount, data.deadline);
+    let mock = setup(data);


Since the test bodies are the same, can't we get around writing them twice? Something like this:

fn valid_permit_assertions(data: TestData) {...} #[test] fn test_valid_permit_default_data() { valid_permit_assertions(TEST_DATA()); }

Just an idea, not sure if it's worth it

Frankly, I don't think this would be helpful. There would be only 2 test cases covered by this helper, all other cases have slight differences

packages/token/src/tests/erc20/test_erc20_permit.cairo

codecov · 2024-09-27T16:25:04Z

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.95%. Comparing base (bf5d02c) to head (3e48a9a).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/token/src/erc20/erc20.cairo	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1140      +/-   ##
==========================================
+ Coverage   88.87%   88.95%   +0.07%     
==========================================
  Files          57       58       +1     
  Lines        1375     1394      +19     
==========================================
+ Hits         1222     1240      +18     
- Misses        153      154       +1

Files with missing lines	Coverage Δ
packages/testing/src/constants.cairo	`100.00% <100.00%> (ø)`
...ages/token/src/erc20/extensions/erc20_permit.cairo	`100.00% <100.00%> (ø)`
packages/utils/src/cryptography/snip12.cairo	`83.33% <ø> (ø)`
packages/token/src/erc20/erc20.cairo	`95.34% <93.33%> (-0.43%)`	⬇️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf5d02c...3e48a9a. Read the comment docs.

immrsd added 6 commits September 5, 2024 16:43

Add ERC20Permit component

a055099

Add ERC20Permit preset

cfa836a

Add ERC20Pemit mock and update visibility modifiers

9973d9a

Add tests for ERC20Permit component

23ba5da

Add tests for ERC20Permit preset

87f1859

Run linter

67c3fc7

immrsd requested review from ggonzalez94, ericnordelo and andrew-fleming September 10, 2024 11:59

immrsd self-assigned this Sep 10, 2024

ericnordelo reviewed Sep 11, 2024

View reviewed changes

andrew-fleming reviewed Sep 12, 2024

View reviewed changes

immrsd added 6 commits September 25, 2024 13:02

Merge main

a499394

Remove ERC20Permit preset

4bfbe36

Add ERC20Permit as a trait of ERC20Component

09fb2d2

Remove ERC20Permit component, restructure files

950205e

Update tests

1c2fc82

Fix test error messages

5702d0c

immrsd requested review from ericnordelo and andrew-fleming September 25, 2024 16:47

Make slight changes to functions doc

3e48a9a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERC20 Permit Component #1140

ERC20 Permit Component #1140

immrsd commented Sep 10, 2024

ericnordelo left a comment

ericnordelo Sep 11, 2024

immrsd Sep 25, 2024

ericnordelo Sep 11, 2024

andrew-fleming Sep 12, 2024

immrsd Sep 25, 2024

andrew-fleming left a comment

andrew-fleming Sep 12, 2024

andrew-fleming Sep 12, 2024

andrew-fleming Sep 12, 2024

immrsd Sep 25, 2024

andrew-fleming Sep 12, 2024

immrsd Sep 25, 2024

andrew-fleming Sep 12, 2024

immrsd Sep 25, 2024

codecov bot commented Sep 27, 2024 •

edited

Loading

		pub(crate) impl SNIP12MetadataImpl of SNIP12Metadata {
		fn name() -> felt252 {

ERC20 Permit Component #1140

Are you sure you want to change the base?

ERC20 Permit Component #1140

Conversation

immrsd commented Sep 10, 2024

PR Checklist

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

codecov bot commented Sep 27, 2024 •

edited

Loading