From 5ac5a9a520dea12f8d8c14e1bd7cfeb57a3ec7bd Mon Sep 17 00:00:00 2001 From: Tin Chung <56880684+chungquantin@users.noreply.github.com> Date: Wed, 31 Jul 2024 14:12:25 +0700 Subject: [PATCH] feat: transfer_from and decrease_allowance (#134) Co-authored-by: Daan van der Plas <93204684+Daanvdplas@users.noreply.github.com> --- pallets/api/src/fungibles/mod.rs | 80 ++++++++++-- pallets/api/src/fungibles/tests.rs | 37 ++++++ pallets/api/src/mock.rs | 3 +- .../integration-tests/src/local_fungibles.rs | 115 +++++++++++++++++- runtime/devnet/src/config/api.rs | 7 +- 5 files changed, 228 insertions(+), 14 deletions(-) diff --git a/pallets/api/src/fungibles/mod.rs b/pallets/api/src/fungibles/mod.rs index 32f9af67..9fb444f8 100644 --- a/pallets/api/src/fungibles/mod.rs +++ b/pallets/api/src/fungibles/mod.rs @@ -110,6 +110,28 @@ pub mod pallet { AssetsOf::::transfer_keep_alive(origin, id.into(), target, amount) } + /// Transfers `value` amount of tokens from the delegated account approved by the `owner` to + /// account `to`, with additional `data` in unspecified format. + /// + /// # Parameters + /// * `id` - The ID of the asset. + /// * `owner` - The account from which the asset balance will be withdrawn. + /// * `to` - The recipient account. + /// * `value` - The number of tokens to transfer. + #[pallet::call_index(4)] + #[pallet::weight(AssetsWeightInfoOf::::transfer_approved())] + pub fn transfer_from( + origin: OriginFor, + id: AssetIdOf, + owner: AccountIdOf, + target: AccountIdOf, + amount: BalanceOf, + ) -> DispatchResult { + let owner = T::Lookup::unlookup(owner); + let target = T::Lookup::unlookup(target); + AssetsOf::::transfer_approved(origin, id.into(), owner, target, amount) + } + /// Approves an account to spend a specified number of tokens on behalf of the caller. /// /// # Parameters @@ -124,17 +146,15 @@ pub mod pallet { spender: AccountIdOf, value: BalanceOf, ) -> DispatchResultWithPostInfo { - let weight = |approve: u32, cancel: u32| -> Weight { - ::WeightInfo::approve(cancel, approve) - }; - let who = ensure_signed(origin.clone()).map_err(|e| e.with_weight(weight(0, 0)))?; + let who = ensure_signed(origin.clone()) + .map_err(|e| e.with_weight(Self::weight_approve(0, 0)))?; let current_allowance = AssetsOf::::allowance(id.clone(), &who, &spender); let spender = T::Lookup::unlookup(spender); let id: AssetIdParameterOf = id.into(); // If the new value is equal to the current allowance, do nothing. let return_weight = if value == current_allowance { - weight(0, 0) + Self::weight_approve(0, 0) } // If the new value is greater than the current allowance, approve the difference // because `approve_transfer` works additively (see `pallet-assets`). @@ -145,17 +165,17 @@ pub mod pallet { spender, value.saturating_sub(current_allowance), ) - .map_err(|e| e.with_weight(weight(1, 0)))?; - weight(1, 0) + .map_err(|e| e.with_weight(Self::weight_approve(1, 0)))?; + Self::weight_approve(1, 0) } else { // If the new value is less than the current allowance, cancel the approval and set the new value AssetsOf::::cancel_approval(origin.clone(), id.clone(), spender.clone()) - .map_err(|e| e.with_weight(weight(0, 1)))?; + .map_err(|e| e.with_weight(Self::weight_approve(0, 1)))?; if value.is_zero() { - return Ok(Some(weight(0, 1)).into()); + return Ok(Some(Self::weight_approve(0, 1)).into()); } AssetsOf::::approve_transfer(origin, id, spender, value)?; - weight(1, 1) + Self::weight_approve(1, 1) }; Ok(Some(return_weight).into()) } @@ -177,6 +197,42 @@ pub mod pallet { let spender = T::Lookup::unlookup(spender); AssetsOf::::approve_transfer(origin, id.into(), spender, value) } + + /// Decreases the allowance of a spender. + /// + /// # Parameters + /// * `id` - The ID of the asset. + /// * `spender` - The account that is allowed to spend the tokens. + /// * `value` - The number of tokens to decrease the allowance by. + #[pallet::call_index(7)] + #[pallet::weight(::WeightInfo::approve(1, 1))] + pub fn decrease_allowance( + origin: OriginFor, + id: AssetIdOf, + spender: AccountIdOf, + value: BalanceOf, + ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin.clone()) + .map_err(|e| e.with_weight(Self::weight_approve(0, 0)))?; + let mut current_allowance = AssetsOf::::allowance(id.clone(), &who, &spender); + let spender = T::Lookup::unlookup(spender); + let id: AssetIdParameterOf = id.into(); + + if value.is_zero() { + return Ok(Some(Self::weight_approve(0, 0)).into()); + } + + current_allowance.saturating_reduce(value); + // Cancel the aproval and set the new value if `current_allowance` is more than zero. + AssetsOf::::cancel_approval(origin.clone(), id.clone(), spender.clone()) + .map_err(|e| e.with_weight(Self::weight_approve(0, 1)))?; + + if current_allowance.is_zero() { + return Ok(Some(Self::weight_approve(0, 1)).into()); + } + AssetsOf::::approve_transfer(origin, id, spender, current_allowance)?; + Ok(().into()) + } } impl Pallet { @@ -208,5 +264,9 @@ pub mod pallet { }, } } + + pub fn weight_approve(approve: u32, cancel: u32) -> Weight { + ::WeightInfo::approve(cancel, approve) + } } } diff --git a/pallets/api/src/fungibles/tests.rs b/pallets/api/src/fungibles/tests.rs index c8de1020..4377fc83 100644 --- a/pallets/api/src/fungibles/tests.rs +++ b/pallets/api/src/fungibles/tests.rs @@ -19,6 +19,43 @@ fn transfer_works() { }); } +#[test] +fn transfer_from_works() { + new_test_ext().execute_with(|| { + let amount: Balance = 100 * UNIT; + // Approve CHARLIE to transfer up to `amount` to BOB. + create_asset_mint_and_approve(ALICE, ASSET, ALICE, amount * 2, CHARLIE, amount / 2); + let transferred = amount / 2; + // Successfully call transfer from. + let alice_balance_before_transfer = Assets::balance(ASSET, &ALICE); + let balance_before_transfer = Assets::balance(ASSET, &BOB); + assert_ok!(Fungibles::transfer_from(signed(CHARLIE), ASSET, ALICE, BOB, transferred)); + let alice_balance_after_transfer = Assets::balance(ASSET, &ALICE); + let balance_after_transfer = Assets::balance(ASSET, &BOB); + // Check that BOB receives the `amount` and ALICE `amount` is spent successfully by CHARLIE. + assert_eq!(balance_after_transfer, balance_before_transfer + transferred); + assert_eq!(alice_balance_after_transfer, alice_balance_before_transfer - transferred); + }); +} + +#[test] +fn decrease_allowance_works() { + new_test_ext().execute_with(|| { + let amount: Balance = 100 * UNIT; + create_asset_mint_and_approve(ALICE, ASSET, ALICE, amount, BOB, amount); + assert_eq!(Assets::allowance(ASSET, &ALICE, &BOB), amount); + // Owner balance is not changed if decreased by zero. + assert_ok!(Fungibles::decrease_allowance(signed(ALICE), ASSET, BOB, 0)); + assert_eq!(Assets::allowance(ASSET, &ALICE, &BOB), amount); + // Decrease allowance successfully. + assert_ok!(Fungibles::decrease_allowance(signed(ALICE), ASSET, BOB, amount / 2 - 1 * UNIT)); + assert_eq!(Assets::allowance(ASSET, &ALICE, &BOB), amount / 2 + 1 * UNIT); + // Saturating if current allowance is decreased more than the owner balance. + assert_ok!(Fungibles::decrease_allowance(signed(ALICE), ASSET, BOB, amount)); + assert_eq!(Assets::allowance(ASSET, &ALICE, &BOB), 0); + }); +} + // Non-additive, sets new value. #[test] fn approve_works() { diff --git a/pallets/api/src/mock.rs b/pallets/api/src/mock.rs index f5d155ef..b20e2635 100644 --- a/pallets/api/src/mock.rs +++ b/pallets/api/src/mock.rs @@ -103,6 +103,7 @@ impl crate::fungibles::Config for Test { pub(crate) const ALICE: AccountId = 1; pub(crate) const BOB: AccountId = 2; +pub(crate) const CHARLIE: AccountId = 3; pub(crate) const INIT_AMOUNT: Balance = 100_000_000 * UNIT; pub(crate) const UNIT: Balance = 10_000_000_000; @@ -112,7 +113,7 @@ pub(crate) fn new_test_ext() -> sp_io::TestExternalities { .expect("Frame system builds valid default genesis config"); pallet_balances::GenesisConfig:: { - balances: vec![(ALICE, INIT_AMOUNT), (BOB, INIT_AMOUNT)], + balances: vec![(ALICE, INIT_AMOUNT), (BOB, INIT_AMOUNT), (CHARLIE, INIT_AMOUNT)], } .assimilate_storage(&mut t) .expect("Pallet balances storage can be assimilated"); diff --git a/pop-api/integration-tests/src/local_fungibles.rs b/pop-api/integration-tests/src/local_fungibles.rs index 6de57ca4..15a644ad 100644 --- a/pop-api/integration-tests/src/local_fungibles.rs +++ b/pop-api/integration-tests/src/local_fungibles.rs @@ -78,6 +78,22 @@ fn transfer( result } +fn transfer_from( + addr: AccountId32, + asset_id: AssetId, + from: AccountId32, + to: AccountId32, + value: Balance, +) -> ExecReturnValue { + let function = function_selector("transfer_from"); + let data: Vec = vec![]; + let params = + [function, asset_id.encode(), from.encode(), to.encode(), value.encode(), data.encode()] + .concat(); + let result = bare_call(addr, params, 0).expect("should work"); + result +} + fn approve( addr: AccountId32, asset_id: AssetId, @@ -102,6 +118,18 @@ fn increase_allowance( result } +fn decrease_allowance( + addr: AccountId32, + asset_id: AssetId, + spender: AccountId32, + value: Balance, +) -> ExecReturnValue { + let function = function_selector("decrease_allowance"); + let params = [function, asset_id.encode(), spender.encode(), value.encode()].concat(); + let result = bare_call(addr, params, 0).expect("should work"); + result +} + // fn asset_exists(addr: AccountId32, asset_id: AssetId) -> bool { // let function = function_selector("asset_exists"); // let params = [function, asset_id.encode()].concat(); @@ -171,14 +199,15 @@ fn create_asset_mint_and_approve( mint: Balance, spender: AccountId32, approve: Balance, -) { - create_asset_and_mint_to(owner.clone(), asset_id, to.clone(), mint); +) -> AssetId { + let asset = create_asset_and_mint_to(owner.clone(), asset_id, to.clone(), mint); assert_ok!(Assets::approve_transfer( RuntimeOrigin::signed(to.into()), asset_id.into(), spender.into(), approve, )); + asset } // Freeze an asset. @@ -366,6 +395,52 @@ fn transfer_works() { }); } +#[test] +fn transfer_from_works() { + new_test_ext().execute_with(|| { + let _ = env_logger::try_init(); + let addr = instantiate(CONTRACT, INIT_VALUE, vec![]); + let amount: Balance = 100 * UNIT; + + // Asset does not exist. + assert_eq!( + decoded::(transfer_from(addr.clone(), 1, ALICE, BOB, amount / 2)), + Ok(Module { index: 52, error: 3 }), + ); + // Create asset with Alice as owner and mint `amount` to contract address. + let asset = create_asset_and_mint_to(ALICE, 1, ALICE, amount); + // Unapproved transfer. + assert_eq!( + decoded::(transfer_from(addr.clone(), asset, ALICE, BOB, amount / 2)), + Ok(Module { index: 52, error: 10 }) + ); + assert_ok!(Assets::approve_transfer( + RuntimeOrigin::signed(ALICE.into()), + asset.into(), + addr.clone().into(), + amount + 1 * UNIT, + )); + // Asset is not live, i.e. frozen or being destroyed. + freeze_asset(ALICE, asset); + assert_eq!( + decoded::(transfer_from(addr.clone(), asset, ALICE, BOB, amount)), + Ok(Module { index: 52, error: 16 }), + ); + thaw_asset(ALICE, asset); + // Not enough balance. + assert_eq!( + decoded::(transfer_from(addr.clone(), asset, ALICE, BOB, amount + 1 * UNIT,)), + Ok(Module { index: 52, error: 0 }), + ); + // Successful transfer. + let balance_before_transfer = Assets::balance(asset, &BOB); + let result = transfer_from(addr.clone(), asset, ALICE, BOB, amount / 2); + assert!(!result.did_revert(), "Contract reverted!"); + let balance_after_transfer = Assets::balance(asset, &BOB); + assert_eq!(balance_after_transfer, balance_before_transfer + amount / 2); + }); +} + #[test] fn approve_works() { new_test_ext().execute_with(|| { @@ -458,6 +533,42 @@ fn increase_allowance_works() { }); } +#[test] +fn decrease_allowance_works() { + new_test_ext().execute_with(|| { + let _ = env_logger::try_init(); + let addr = instantiate(CONTRACT, INIT_VALUE, vec![]); + let amount: Balance = 100 * UNIT; + // Asset does not exist. + assert_eq!( + decoded::(decrease_allowance(addr.clone(), 0, BOB, amount)), + Ok(Module { index: 52, error: 3 }), + ); + // Create asset and mint to the address contract, delegate Bob to spend the `amount`. + let asset = + create_asset_mint_and_approve(addr.clone(), 0, addr.clone(), amount, BOB, amount); + // Asset is not live, i.e. frozen or being destroyed. + freeze_asset(addr.clone(), asset); + assert_eq!( + decoded::(decrease_allowance(addr.clone(), asset, BOB, amount)), + Ok(Module { index: 52, error: 16 }), + ); + thaw_asset(addr.clone(), asset); + // Successfully decrease allowance. + let bob_allowance_before = Assets::allowance(asset, &addr, &BOB); + let result = decrease_allowance(addr.clone(), 0, BOB, amount / 2 - 1 * UNIT); + assert!(!result.did_revert(), "Contract reverted!"); + let bob_allowance_after = Assets::allowance(asset, &addr, &BOB); + assert_eq!(bob_allowance_before - bob_allowance_after, amount / 2 - 1 * UNIT); + // Asset is not live, i.e. frozen or being destroyed. + start_destroy_asset(addr.clone(), asset); + assert_eq!( + decoded::(decrease_allowance(addr.clone(), asset, BOB, amount)), + Ok(Module { index: 52, error: 16 }), + ); + }); +} + /// 2. PSP-22 Metadata Interface: /// - token_name /// - token_symbol diff --git a/runtime/devnet/src/config/api.rs b/runtime/devnet/src/config/api.rs index 884128f2..b3932a79 100644 --- a/runtime/devnet/src/config/api.rs +++ b/runtime/devnet/src/config/api.rs @@ -20,7 +20,12 @@ impl Contains for AllowedApiCalls { use fungibles::Call::*; matches!( c, - RuntimeCall::Fungibles(transfer { .. } | approve { .. } | increase_allowance { .. }) + RuntimeCall::Fungibles( + transfer { .. } + | transfer_from { .. } + | approve { .. } | increase_allowance { .. } + | decrease_allowance { .. } + ) ) } }