-
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
Changes from all commits
a055099
cfa836a
9973d9a
23ba5da
87f1859
67c3fc7
a499394
4bfbe36
09fb2d2
950205e
1c2fc82
5702d0c
3e48a9a
8fbbe48
26ce488
acad601
2cef6d1
4d23651
72ab63d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -12,15 +12,20 @@ | |||||
/// for examples. | ||||||
#[starknet::component] | ||||||
pub mod ERC20Component { | ||||||
use core::num::traits::Bounded; | ||||||
use core::num::traits::Zero; | ||||||
use core::num::traits::{Bounded, Zero}; | ||||||
use crate::erc20::interface; | ||||||
use starknet::ContractAddress; | ||||||
use starknet::get_caller_address; | ||||||
use starknet::storage::{ | ||||||
Map, StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, | ||||||
StoragePointerWriteAccess | ||||||
use crate::erc20::snip12_utils::permit::Permit; | ||||||
use openzeppelin_account::interface::{ISRC6Dispatcher, ISRC6DispatcherTrait}; | ||||||
use openzeppelin_utils::cryptography::interface::{INonces, ISNIP12Metadata}; | ||||||
use openzeppelin_utils::cryptography::snip12::{ | ||||||
StructHash, OffchainMessageHash, SNIP12Metadata, StarknetDomain | ||||||
}; | ||||||
use openzeppelin_utils::nonces::NoncesComponent::InternalTrait as NoncesInternalTrait; | ||||||
use openzeppelin_utils::nonces::NoncesComponent; | ||||||
use starknet::ContractAddress; | ||||||
use starknet::storage::{Map, StorageMapReadAccess, StorageMapWriteAccess}; | ||||||
use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess}; | ||||||
use starknet::{get_block_timestamp, get_caller_address, get_contract_address, get_tx_info}; | ||||||
|
||||||
#[storage] | ||||||
pub struct Storage { | ||||||
|
@@ -68,6 +73,8 @@ pub mod ERC20Component { | |||||
pub const MINT_TO_ZERO: felt252 = 'ERC20: mint to 0'; | ||||||
pub const INSUFFICIENT_BALANCE: felt252 = 'ERC20: insufficient balance'; | ||||||
pub const INSUFFICIENT_ALLOWANCE: felt252 = 'ERC20: insufficient allowance'; | ||||||
pub const EXPIRED_PERMIT_SIGNATURE: felt252 = 'ERC20: expired permit signature'; | ||||||
pub const INVALID_PERMIT_SIGNATURE: felt252 = 'ERC20: invalid permit signature'; | ||||||
} | ||||||
|
||||||
// | ||||||
|
@@ -288,6 +295,104 @@ pub mod ERC20Component { | |||||
} | ||||||
} | ||||||
|
||||||
/// The ERC20Permit impl implements the EIP-2612 standard, facilitating token approvals via | ||||||
/// off-chain signatures. This approach allows token holders to delegate their approval to spend | ||||||
/// tokens without executing an on-chain transaction, reducing gas costs and enhancing | ||||||
/// usability. | ||||||
/// See https://eips.ethereum.org/EIPS/eip-2612. | ||||||
/// | ||||||
/// The message signed and the signature must follow the SNIP-12 standard for hashing and | ||||||
/// signing typed structured data. | ||||||
/// See https://github.com/starknet-io/SNIPs/blob/main/SNIPS/snip-12.md. | ||||||
/// | ||||||
/// To safeguard against replay attacks and ensure the uniqueness of each approval via `permit`, | ||||||
/// the data signed includes: | ||||||
/// - 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 | ||||||
#[embeddable_as(ERC20PermitImpl)] | ||||||
impl ERC20Permit< | ||||||
TContractState, | ||||||
+HasComponent<TContractState>, | ||||||
+ERC20HooksTrait<TContractState>, | ||||||
impl Nonces: NoncesComponent::HasComponent<TContractState>, | ||||||
impl Metadata: SNIP12Metadata, | ||||||
+Drop<TContractState> | ||||||
> of interface::IERC20Permit<ComponentState<TContractState>> { | ||||||
/// Sets `amount` as the allowance of `spender` over `owner`'s tokens after validating the | ||||||
/// signature. | ||||||
/// | ||||||
/// Requirements: | ||||||
/// | ||||||
/// - `owner` is a deployed account contract. | ||||||
/// - `spender` is not the zero address. | ||||||
/// - `deadline` is not a timestamp in the past. | ||||||
/// - `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> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We should favor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
) { | ||||||
// 1. Ensure the deadline is not missed | ||||||
assert(get_block_timestamp() <= deadline, Errors::EXPIRED_PERMIT_SIGNATURE); | ||||||
|
||||||
// 2. Get the current nonce and increment it | ||||||
let mut nonces_component = get_dep_component_mut!(ref self, Nonces); | ||||||
let nonce = nonces_component.use_nonce(owner); | ||||||
|
||||||
// 3. Make a call to the account to validate permit signature | ||||||
let permit = Permit { token: get_contract_address(), spender, amount, nonce, deadline }; | ||||||
let permit_hash = permit.get_message_hash(owner); | ||||||
let is_valid_sig_felt = ISRC6Dispatcher { contract_address: owner } | ||||||
.is_valid_signature(permit_hash, signature); | ||||||
|
||||||
// 4. 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_PERMIT_SIGNATURE); | ||||||
|
||||||
// 5. Approve | ||||||
self._approve(owner, spender, amount); | ||||||
} | ||||||
|
||||||
/// Returns the current nonce of `owner`. A nonce value must be | ||||||
/// included whenever a signature for `permit` call 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() | ||||||
} | ||||||
} | ||||||
|
||||||
#[embeddable_as(SNIP12MetadataExternalImpl)] | ||||||
impl SNIP12MetadataExternal< | ||||||
TContractState, +HasComponent<TContractState>, impl Metadata: SNIP12Metadata | ||||||
> of ISNIP12Metadata<ComponentState<TContractState>> { | ||||||
/// Returns domain name and version used for generating a message hash for permit signature. | ||||||
fn snip12_metadata(self: @ComponentState<TContractState>) -> (felt252, felt252) { | ||||||
(Metadata::name(), Metadata::version()) | ||||||
} | ||||||
} | ||||||
|
||||||
// | ||||||
// Internal | ||||||
// | ||||||
|
@@ -333,7 +438,6 @@ pub mod ERC20Component { | |||||
self.update(account, Zero::zero(), amount); | ||||||
} | ||||||
|
||||||
|
||||||
/// Transfers an `amount` of tokens from `from` to `to`, or alternatively mints (or burns) | ||||||
/// if `from` (or `to`) is the zero address. | ||||||
/// | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -43,6 +43,20 @@ pub trait IERC20CamelOnly<TState> { | |||||
) -> bool; | ||||||
} | ||||||
|
||||||
#[starknet::interface] | ||||||
pub trait IERC20Permit<TState> { | ||||||
fn permit( | ||||||
ref self: TState, | ||||||
owner: ContractAddress, | ||||||
spender: ContractAddress, | ||||||
amount: u256, | ||||||
deadline: u64, | ||||||
signature: Array<felt252> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. See #1140 (comment) |
||||||
); | ||||||
fn nonces(self: @TState, owner: ContractAddress) -> felt252; | ||||||
fn DOMAIN_SEPARATOR(self: @TState) -> felt252; | ||||||
} | ||||||
|
||||||
#[starknet::interface] | ||||||
pub trait ERC20ABI<TState> { | ||||||
// IERC20 | ||||||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
This way it makes total sense to have an additional interface named There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe 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 |
||||||
// IERC20 | ||||||
fn total_supply(self: @TState) -> u256; | ||||||
fn balance_of(self: @TState, account: ContractAddress) -> u256; | ||||||
fn allowance(self: @TState, owner: ContractAddress, spender: ContractAddress) -> u256; | ||||||
fn transfer(ref self: TState, recipient: ContractAddress, amount: u256) -> bool; | ||||||
fn transfer_from( | ||||||
ref self: TState, sender: ContractAddress, recipient: ContractAddress, amount: u256 | ||||||
) -> bool; | ||||||
fn approve(ref self: TState, spender: ContractAddress, amount: u256) -> bool; | ||||||
|
||||||
// IERC20CamelOnly | ||||||
fn totalSupply(self: @TState) -> u256; | ||||||
fn balanceOf(self: @TState, account: ContractAddress) -> u256; | ||||||
fn transferFrom( | ||||||
ref self: TState, sender: ContractAddress, recipient: ContractAddress, amount: u256 | ||||||
) -> bool; | ||||||
|
||||||
// IERC20Metadata | ||||||
fn name(self: @TState) -> ByteArray; | ||||||
fn symbol(self: @TState) -> ByteArray; | ||||||
fn decimals(self: @TState) -> u8; | ||||||
|
||||||
// IERC20Permit | ||||||
fn permit( | ||||||
ref self: TState, | ||||||
owner: ContractAddress, | ||||||
spender: ContractAddress, | ||||||
amount: u256, | ||||||
deadline: u64, | ||||||
signature: Array<felt252> | ||||||
); | ||||||
fn nonces(self: @TState, owner: ContractAddress) -> felt252; | ||||||
fn DOMAIN_SEPARATOR(self: @TState) -> felt252; | ||||||
|
||||||
// ISNIP12Metadata | ||||||
fn snip12_metadata(self: @TState) -> (felt252, felt252); | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
pub mod permit; | ||
pub mod votes; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// SPDX-License-Identifier: MIT | ||
// OpenZeppelin Contracts for Cairo v0.17.0 (token/erc20/snip12_utils/permit.cairo) | ||
|
||
use core::hash::{HashStateTrait, HashStateExTrait}; | ||
use core::poseidon::PoseidonTrait; | ||
use openzeppelin_utils::cryptography::snip12::StructHash; | ||
use starknet::ContractAddress; | ||
|
||
#[derive(Copy, Drop, Hash)] | ||
pub struct Permit { | ||
pub token: ContractAddress, | ||
pub spender: ContractAddress, | ||
pub amount: u256, | ||
pub nonce: felt252, | ||
pub deadline: u64, | ||
} | ||
|
||
// Since there's no u64 type in SNIP-12, the type used for `deadline` parameter is u128 | ||
// selector!( | ||
// "\"Permit\"( | ||
// \"token\":\"ContractAddress\", | ||
// \"spender\":\"ContractAddress\", | ||
// \"amount\":\"u256\", | ||
// \"nonce\":\"felt\", | ||
// \"deadline\":\"u128\" | ||
// )" | ||
// ); | ||
pub const PERMIT_TYPE_HASH: felt252 = | ||
0x2a8eb238e7cde741a544afcc79fe945d4292b089875fd068633854927fd5a96; | ||
|
||
impl StructHashImpl of StructHash<Permit> { | ||
fn hash_struct(self: @Permit) -> felt252 { | ||
PoseidonTrait::new().update_with(PERMIT_TYPE_HASH).update_with(*self).finalize() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// SPDX-License-Identifier: MIT | ||
// OpenZeppelin Contracts for Cairo v0.17.0 (token/erc20/snip12_utils/votes.cairo) | ||
|
||
use core::hash::{HashStateTrait, HashStateExTrait}; | ||
use core::poseidon::PoseidonTrait; | ||
use openzeppelin_utils::cryptography::snip12::StructHash; | ||
use starknet::ContractAddress; | ||
|
||
// sn_keccak("\"Delegation\"(\"delegatee\":\"ContractAddress\",\"nonce\":\"felt\",\"expiry\":\"u128\")") | ||
// | ||
// Since there's no u64 type in SNIP-12, we use u128 for `expiry` in the type hash generation. | ||
pub const DELEGATION_TYPE_HASH: felt252 = | ||
0x241244ac7acec849adc6df9848262c651eb035a3add56e7f6c7bcda6649e837; | ||
|
||
#[derive(Copy, Drop, Hash)] | ||
pub struct Delegation { | ||
pub delegatee: ContractAddress, | ||
pub nonce: felt252, | ||
pub expiry: u64 | ||
} | ||
|
||
impl StructHashImpl of StructHash<Delegation> { | ||
fn hash_struct(self: @Delegation) -> felt252 { | ||
PoseidonTrait::new().update_with(DELEGATION_TYPE_HASH).update_with(*self).finalize() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
mod test_dual20; | ||
mod test_erc20; | ||
mod test_erc20_permit; | ||
mod test_erc20_votes; |
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.
Small nit: we end list items with a period (like in the Requirements section for
permit
)