From 20500f623af36b5db4e236a78c1bf400ec4e2e32 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Wed, 14 Feb 2024 14:09:44 -0500 Subject: [PATCH] Remove increase / decrease and MAX allowance from ERC20 --- .../token/erc20/test_erc20_allowance.cairo | 174 ------------------ .../token/erc20/erc20_allowance.cairo | 81 +------- token/src/erc20/erc20.cairo | 51 +---- token/src/erc20/interface.cairo | 24 --- token/src/erc20/tests.cairo | 132 ------------- token/src/presets/erc20/bridgeable.cairo | 10 - .../src/presets/erc20/tests_bridgeable.cairo | 53 ------ 7 files changed, 6 insertions(+), 519 deletions(-) diff --git a/token/src/components/tests/token/erc20/test_erc20_allowance.cairo b/token/src/components/tests/token/erc20/test_erc20_allowance.cairo index e16e045b..aeb7b8bb 100644 --- a/token/src/components/tests/token/erc20/test_erc20_allowance.cairo +++ b/token/src/components/tests/token/erc20/test_erc20_allowance.cairo @@ -91,34 +91,6 @@ fn test_erc20_allowance_approve_to_zero() { state.erc20_allowance.approve(ZERO(), VALUE); } - -// -// update_allowance (increase_allowance, decrease_allowance) -// - -#[test] -#[available_gas(100000000)] -fn test_erc20_allowance_update_allowance() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - - state.erc20_allowance.approve(SPENDER(), VALUE); - utils::drop_event(ZERO()); - - state.erc20_allowance.update_allowance(OWNER(), SPENDER(), 0, SUPPLY); - assert( - state.erc20_allowance.allowance(OWNER(), SPENDER()) == VALUE + SUPPLY, - 'should be VALUE+SUPPLY' - ); - assert_only_event_approval(ZERO(), OWNER(), SPENDER(), VALUE + SUPPLY); - - state.erc20_allowance.update_allowance(OWNER(), SPENDER(), VALUE, 0); - assert(state.erc20_allowance.allowance(OWNER(), SPENDER()) == SUPPLY, 'should be SUPPLY'); - assert_only_event_approval(ZERO(), OWNER(), SPENDER(), SUPPLY); -} - - // // spend_allowance // @@ -159,149 +131,3 @@ fn test_erc20_allowance_spend_allowance_with_max_allowance() { utils::assert_no_events_left(ZERO()); } - - -// -// increase_allowance & increaseAllowance -// - -#[test] -#[available_gas(25000000)] -fn test_erc20_allowance_increase_allowance() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.approve(SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(state.erc20_allowance.increase_allowance(SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(ZERO(), OWNER(), SPENDER(), VALUE * 2); - assert( - state.erc20_allowance.allowance(OWNER(), SPENDER()) == VALUE * 2, 'Should be amount * 2' - ); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve to 0',))] -fn test_erc20_allowance_increase_allowance_to_zero_address() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.increase_allowance(ZERO(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve from 0',))] -fn test_erc20_allowance_increase_allowance_from_zero_address() { - let (world, mut state) = STATE(); - state.erc20_allowance.increase_allowance(SPENDER(), VALUE); -} - -#[test] -#[available_gas(25000000)] -fn test_erc20_allowance_increaseAllowance() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.approve(SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(state.erc20_allowance.increaseAllowance(SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(ZERO(), OWNER(), SPENDER(), 2 * VALUE); - assert( - state.erc20_allowance.allowance(OWNER(), SPENDER()) == VALUE * 2, 'Should be amount * 2' - ); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve to 0',))] -fn test_erc20_allowance_increaseAllowance_to_zero_address() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.increaseAllowance(ZERO(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve from 0',))] -fn test_erc20_allowance_increaseAllowance_from_zero_address() { - let (world, mut state) = STATE(); - state.erc20_allowance.increaseAllowance(SPENDER(), VALUE); -} - -// -// decrease_allowance & decreaseAllowance -// - -#[test] -#[available_gas(25000000)] -fn test_erc20_allowance_decrease_allowance() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.approve(SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(state.erc20_allowance.decrease_allowance(SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(ZERO(), OWNER(), SPENDER(), 0); - assert(state.erc20_allowance.allowance(OWNER(), SPENDER()) == VALUE - VALUE, 'Should be 0'); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_erc20_allowance_decrease_allowance_to_zero_address() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.decrease_allowance(ZERO(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_erc20_allowance_decrease_allowance_from_zero_address() { - let (world, mut state) = STATE(); - state.erc20_allowance.decrease_allowance(SPENDER(), VALUE); -} - -#[test] -#[available_gas(25000000)] -fn test_erc20_allowance_decreaseAllowance() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.approve(SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(state.erc20_allowance.decreaseAllowance(SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(ZERO(), OWNER(), SPENDER(), 0); - assert(state.erc20_allowance.allowance(OWNER(), SPENDER()) == VALUE - VALUE, 'Should be 0'); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_erc20_allowance_decreaseAllowance_to_zero_address() { - let (world, mut state) = STATE(); - - testing::set_caller_address(OWNER()); - state.erc20_allowance.decreaseAllowance(ZERO(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_erc20_allowance_decreaseAllowance_from_zero_address() { - let (world, mut state) = STATE(); - state.erc20_allowance.decreaseAllowance(SPENDER(), VALUE); -} - diff --git a/token/src/components/token/erc20/erc20_allowance.cairo b/token/src/components/token/erc20/erc20_allowance.cairo index b4102364..d226416e 100644 --- a/token/src/components/token/erc20/erc20_allowance.cairo +++ b/token/src/components/token/erc20/erc20_allowance.cairo @@ -25,20 +25,6 @@ trait IERC20Allowance { fn approve(ref self: TState, spender: ContractAddress, amount: u256) -> bool; } -#[starknet::interface] -trait IERC20SafeAllowance { - fn increase_allowance(ref self: TState, spender: ContractAddress, added_value: u256) -> bool; - fn decrease_allowance( - ref self: TState, spender: ContractAddress, subtracted_value: u256 - ) -> bool; -} - -#[starknet::interface] -trait IERC20SafeAllowanceCamel { - fn increaseAllowance(ref self: TState, spender: ContractAddress, addedValue: u256) -> bool; - fn decreaseAllowance(ref self: TState, spender: ContractAddress, subtractedValue: u256) -> bool; -} - /// /// ERC20Allowance Component /// @@ -100,52 +86,6 @@ mod erc20_allowance_component { } } - #[embeddable_as(ERC20SafeAllowanceImpl)] - impl ERC20SafeAllowance< - TContractState, - +HasComponent, - +IWorldProvider, - +Drop - > of IERC20SafeAllowance> { - fn increase_allowance( - ref self: ComponentState, spender: ContractAddress, added_value: u256 - ) -> bool { - self.update_allowance(get_caller_address(), spender, 0, added_value); - true - } - - fn decrease_allowance( - ref self: ComponentState, - spender: ContractAddress, - subtracted_value: u256 - ) -> bool { - self.update_allowance(get_caller_address(), spender, subtracted_value, 0); - true - } - } - - #[embeddable_as(ERC20SafeAllowanceCamelImpl)] - impl ERC20SafeAllowanceCamel< - TContractState, - +HasComponent, - +IWorldProvider, - +Drop - > of IERC20SafeAllowanceCamel> { - fn increaseAllowance( - ref self: ComponentState, spender: ContractAddress, addedValue: u256 - ) -> bool { - self.increase_allowance(spender, addedValue) - } - - fn decreaseAllowance( - ref self: ComponentState, - spender: ContractAddress, - subtractedValue: u256 - ) -> bool { - self.decrease_allowance(spender, subtractedValue) - } - } - /// /// Internal /// @@ -179,20 +119,6 @@ mod erc20_allowance_component { self.emit_event(approval_event); } - fn update_allowance( - ref self: ComponentState, - owner: ContractAddress, - spender: ContractAddress, - subtract: u256, - add: u256 - ) { - let mut allowance = self.get_allowance(owner, spender); - // adding and subtracting is fewer steps than if - allowance.amount = allowance.amount - subtract; - allowance.amount = allowance.amount + add; - self.set_allowance(allowance); - } - // use in transfer_from fn spend_allowance( ref self: ComponentState, @@ -200,10 +126,9 @@ mod erc20_allowance_component { spender: ContractAddress, amount: u256 ) { - let current_allowance = self.get_allowance(owner, spender).amount; - if current_allowance != BoundedInt::max() { - self.update_allowance(owner, spender, amount, 0); - } + let mut allowance = self.get_allowance(owner, spender); + allowance.amount = allowance.amount - amount; + self.set_allowance(allowance); } fn emit_event, +Drop, +Clone>( diff --git a/token/src/erc20/erc20.cairo b/token/src/erc20/erc20.cairo index 198c24b1..034ecbc9 100644 --- a/token/src/erc20/erc20.cairo +++ b/token/src/erc20/erc20.cairo @@ -141,36 +141,6 @@ mod ERC20 { } } - #[abi(embed_v0)] - fn increase_allowance( - ref self: ContractState, spender: ContractAddress, added_value: u256 - ) -> bool { - self.update_allowance(get_caller_address(), spender, 0, added_value); - true - } - - #[abi(embed_v0)] - fn increaseAllowance( - ref self: ContractState, spender: ContractAddress, addedValue: u256 - ) -> bool { - increase_allowance(ref self, spender, addedValue) - } - - #[abi(embed_v0)] - fn decrease_allowance( - ref self: ContractState, spender: ContractAddress, subtracted_value: u256 - ) -> bool { - self.update_allowance(get_caller_address(), spender, subtracted_value, 0); - true - } - - #[abi(embed_v0)] - fn decreaseAllowance( - ref self: ContractState, spender: ContractAddress, subtractedValue: u256 - ) -> bool { - decrease_allowance(ref self, spender, subtractedValue) - } - // // Internal // @@ -216,20 +186,6 @@ mod ERC20 { get!(self.world(), (get_contract_address(), owner, spender), ERC20Allowance) } - fn update_allowance( - ref self: ContractState, - owner: ContractAddress, - spender: ContractAddress, - subtract: u256, - add: u256 - ) { - let mut allowance = self.get_allowance(owner, spender); - // adding and subtracting is fewer steps than if - allowance.amount = allowance.amount - subtract; - allowance.amount = allowance.amount + add; - self.set_allowance(allowance); - } - fn set_allowance(ref self: ContractState, allowance: ERC20Allowance) { assert(!allowance.owner.is_zero(), Errors::APPROVE_FROM_ZERO); assert(!allowance.spender.is_zero(), Errors::APPROVE_TO_ZERO); @@ -298,10 +254,9 @@ mod ERC20 { fn _spend_allowance( ref self: ContractState, owner: ContractAddress, spender: ContractAddress, amount: u256 ) { - let current_allowance = self.get_allowance(owner, spender).amount; - if current_allowance != BoundedInt::max() { - self.update_allowance(owner, spender, amount, 0); - } + let mut allowance = self.get_allowance(owner, spender); + allowance.amount = allowance.amount - amount; + self.set_allowance(allowance); } } } diff --git a/token/src/erc20/interface.cairo b/token/src/erc20/interface.cairo index 89939915..577ce7db 100644 --- a/token/src/erc20/interface.cairo +++ b/token/src/erc20/interface.cairo @@ -19,14 +19,6 @@ trait IERC20Metadata { fn decimals(self: @TState) -> u8; } -#[starknet::interface] -trait ISafeAllowance { - fn increase_allowance(ref self: TState, spender: ContractAddress, added_value: u256) -> bool; - fn decrease_allowance( - ref self: TState, spender: ContractAddress, subtracted_value: u256 - ) -> bool; -} - #[starknet::interface] trait IERC20Camel { fn totalSupply(self: @TState) -> u256; @@ -48,12 +40,6 @@ trait IERC20CamelOnly { ) -> bool; } -#[starknet::interface] -trait ISafeAllowanceCamel { - fn increaseAllowance(ref self: TState, spender: ContractAddress, addedValue: u256) -> bool; - fn decreaseAllowance(ref self: TState, spender: ContractAddress, subtractedValue: u256) -> bool; -} - #[starknet::interface] trait ERC20ABI { // IERC20 @@ -71,20 +57,10 @@ trait ERC20ABI { fn symbol(self: @TState) -> felt252; fn decimals(self: @TState) -> u8; - // IERC20SafeAllowance - fn increase_allowance(ref self: TState, spender: ContractAddress, added_value: u256) -> bool; - fn decrease_allowance( - ref self: TState, spender: ContractAddress, subtracted_value: 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; - - // IERC20CamelSafeAllowance - fn increaseAllowance(ref self: TState, spender: ContractAddress, addedValue: u256) -> bool; - fn decreaseAllowance(ref self: TState, spender: ContractAddress, subtractedValue: u256) -> bool; } diff --git a/token/src/erc20/tests.cairo b/token/src/erc20/tests.cairo index 3f088e4b..ca7dc860 100644 --- a/token/src/erc20/tests.cairo +++ b/token/src/erc20/tests.cairo @@ -305,138 +305,6 @@ fn test_transfer_from_from_zero_address() { ERC20Impl::transfer_from(ref state, Zeroable::zero(), RECIPIENT(), VALUE); } -// -// increase_allowance & increaseAllowance -// - -#[test] -#[available_gas(25000000)] -fn test_increase_allowance() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20Impl::approve(ref state, SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(ERC20::increase_allowance(ref state, SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(OWNER(), SPENDER(), VALUE * 2); - assert(ERC20Impl::allowance(@state, OWNER(), SPENDER()) == VALUE * 2, 'Should be amount * 2'); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve to 0',))] -fn test_increase_allowance_to_zero_address() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20::increase_allowance(ref state, Zeroable::zero(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve from 0',))] -fn test_increase_allowance_from_zero_address() { - let mut state = setup(); - ERC20::increase_allowance(ref state, SPENDER(), VALUE); -} - -#[test] -#[available_gas(25000000)] -fn test_increaseAllowance() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20Impl::approve(ref state, SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(ERC20::increaseAllowance(ref state, SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(OWNER(), SPENDER(), 2 * VALUE); - assert(ERC20Impl::allowance(@state, OWNER(), SPENDER()) == VALUE * 2, 'Should be amount * 2'); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve to 0',))] -fn test_increaseAllowance_to_zero_address() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20::increaseAllowance(ref state, Zeroable::zero(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('ERC20: approve from 0',))] -fn test_increaseAllowance_from_zero_address() { - let mut state = setup(); - ERC20::increaseAllowance(ref state, SPENDER(), VALUE); -} - -// -// decrease_allowance & decreaseAllowance -// - -#[test] -#[available_gas(25000000)] -fn test_decrease_allowance() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20Impl::approve(ref state, SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(ERC20::decrease_allowance(ref state, SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(OWNER(), SPENDER(), 0); - assert(ERC20Impl::allowance(@state, OWNER(), SPENDER()) == VALUE - VALUE, 'Should be 0'); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_decrease_allowance_to_zero_address() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20::decrease_allowance(ref state, Zeroable::zero(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_decrease_allowance_from_zero_address() { - let mut state = setup(); - ERC20::decrease_allowance(ref state, SPENDER(), VALUE); -} - -#[test] -#[available_gas(25000000)] -fn test_decreaseAllowance() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20Impl::approve(ref state, SPENDER(), VALUE); - utils::drop_event(ZERO()); - - assert(ERC20::decreaseAllowance(ref state, SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(OWNER(), SPENDER(), 0); - assert(ERC20Impl::allowance(@state, OWNER(), SPENDER()) == VALUE - VALUE, 'Should be 0'); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_decreaseAllowance_to_zero_address() { - let mut state = setup(); - testing::set_caller_address(OWNER()); - ERC20::decreaseAllowance(ref state, Zeroable::zero(), VALUE); -} - -#[test] -#[available_gas(25000000)] -#[should_panic(expected: ('u256_sub Overflow',))] -fn test_decreaseAllowance_from_zero_address() { - let mut state = setup(); - ERC20::decreaseAllowance(ref state, SPENDER(), VALUE); -} - // // _spend_allowance // diff --git a/token/src/presets/erc20/bridgeable.cairo b/token/src/presets/erc20/bridgeable.cairo index 56a85061..addd81f4 100644 --- a/token/src/presets/erc20/bridgeable.cairo +++ b/token/src/presets/erc20/bridgeable.cairo @@ -37,16 +37,6 @@ trait IERC20BridgeablePreset { fn allowance(self: @TState, owner: ContractAddress, spender: ContractAddress) -> u256; fn approve(ref self: TState, spender: ContractAddress, amount: u256) -> bool; - // IERC20SafeAllowance - fn decrease_allowance( - ref self: TState, spender: ContractAddress, subtracted_value: u256 - ) -> bool; - fn increase_allowance(ref self: TState, spender: ContractAddress, added_value: u256) -> bool; - - // IERC20SafeAllowanceCamel - fn decreaseAllowance(ref self: TState, spender: ContractAddress, subtractedValue: u256) -> bool; - fn increaseAllowance(ref self: TState, spender: ContractAddress, addedValue: u256) -> bool; - // IERC20Bridgeable fn burn(ref self: TState, account: ContractAddress, amount: u256); fn l2_bridge_address(self: @TState,) -> ContractAddress; diff --git a/token/src/presets/erc20/tests_bridgeable.cairo b/token/src/presets/erc20/tests_bridgeable.cairo index 0c3fe8a7..3a5d5d9e 100644 --- a/token/src/presets/erc20/tests_bridgeable.cairo +++ b/token/src/presets/erc20/tests_bridgeable.cairo @@ -222,59 +222,6 @@ fn test_transfer_from_doesnt_consume_infinite_allowance() { ); } - -// -// increase_allowance -// - -#[test] -#[available_gas(25000000)] -fn test_increase_allowance() { - let (world, mut erc20_bridgeable) = setup(); - - utils::impersonate(OWNER()); - erc20_bridgeable.approve(SPENDER(), VALUE); - - utils::drop_all_events(erc20_bridgeable.contract_address); - utils::drop_all_events(world.contract_address); - - assert(erc20_bridgeable.increase_allowance(SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(erc20_bridgeable.contract_address, OWNER(), SPENDER(), VALUE * 2); - - // drop StoreSetRecord ERC20AllowanceModel - utils::drop_event(world.contract_address); - assert_only_event_approval(world.contract_address, OWNER(), SPENDER(), VALUE * 2); - - assert(erc20_bridgeable.allowance(OWNER(), SPENDER()) == VALUE * 2, 'Should be amount * 2'); -} - -// -// decrease_allowance -// - -#[test] -#[available_gas(25000000)] -fn test_decrease_allowance() { - let (world, mut erc20_bridgeable) = setup(); - - utils::impersonate(OWNER()); - erc20_bridgeable.approve(SPENDER(), VALUE); - - utils::drop_all_events(erc20_bridgeable.contract_address); - utils::drop_all_events(world.contract_address); - - assert(erc20_bridgeable.decrease_allowance(SPENDER(), VALUE), 'Should return true'); - - assert_only_event_approval(erc20_bridgeable.contract_address, OWNER(), SPENDER(), 0); - - // drop StoreSetRecord ERC20AllowanceModel - utils::drop_event(world.contract_address); - assert_only_event_approval(world.contract_address, OWNER(), SPENDER(), 0); - - assert(erc20_bridgeable.allowance(OWNER(), SPENDER()) == VALUE - VALUE, 'Should be 0'); -} - // // bridgeable //