From d3d49f32a7766a07ba6b2b7a6fc4b2712710ee60 Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Thu, 5 Oct 2023 17:37:36 +0200 Subject: [PATCH 1/5] Fix bug with infinite allowance --- .../orml-currencies-allowance-extension/src/lib.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/pallets/orml-currencies-allowance-extension/src/lib.rs b/pallets/orml-currencies-allowance-extension/src/lib.rs index e8dfb3fa8..3b8ac4d08 100644 --- a/pallets/orml-currencies-allowance-extension/src/lib.rs +++ b/pallets/orml-currencies-allowance-extension/src/lib.rs @@ -298,7 +298,7 @@ impl Pallet { Approvals::::try_mutate_exists( (id, &owner, delegate), |maybe_approved| -> DispatchResult { - let mut approved = maybe_approved.take().ok_or(Error::::Unapproved)?; + let approved = maybe_approved.take().ok_or(Error::::Unapproved)?; let remaining = approved.checked_sub(&amount).ok_or(Error::::Unapproved)?; as MultiCurrency>::transfer( @@ -308,14 +308,13 @@ impl Pallet { amount, )?; - if remaining.is_zero() { + // Don't decrement allowance if it is set to the max value (which acts as infinite allowance) + if approved == BalanceOf::::max_value() { + *maybe_approved = Some(approved); + } else if remaining.is_zero() { *maybe_approved = None; } else { - //decrement allowance only if it isn't max value (which acts as infinite allowance) - if approved != BalanceOf::::max_value() { - approved = remaining; - } - *maybe_approved = Some(approved); + *maybe_approved = Some(remaining); } Ok(()) }, From fc54e64bf750fe7c5915a8e27c17ba01ba2b3aa6 Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Thu, 5 Oct 2023 17:45:24 +0200 Subject: [PATCH 2/5] Add more tests --- .../src/tests.rs | 237 +++++++++++++++++- 1 file changed, 227 insertions(+), 10 deletions(-) diff --git a/pallets/orml-currencies-allowance-extension/src/tests.rs b/pallets/orml-currencies-allowance-extension/src/tests.rs index cc81f26b3..86ffbf5fe 100644 --- a/pallets/orml-currencies-allowance-extension/src/tests.rs +++ b/pallets/orml-currencies-allowance-extension/src/tests.rs @@ -1,6 +1,7 @@ -use frame_support::{assert_err, assert_ok, error::BadOrigin}; +use frame_support::{assert_err, assert_ok, error::BadOrigin, traits::Get}; +use orml_traits::MultiCurrency; -use crate::{mock::*, AllowedCurrencies, CurrencyOf}; +use crate::{mock::*, AllowedCurrencies, Config, CurrencyOf, Error}; #[test] fn should_add_allowed_currencies() { @@ -12,6 +13,27 @@ fn should_add_allowed_currencies() { }) } +#[test] +fn should_not_exceed_allowed_currencies() { + run_test(|| { + let max_allowed_currencies: u32 = ::MaxAllowedCurrencies::get(); + let too_many_currencies = (0..(max_allowed_currencies as u64) + 1).collect::>(); + + assert_err!( + TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + too_many_currencies.clone() + ), + Error::::ExceedsNumberOfAllowedCurrencies + ); + + assert_err!( + TokenAllowance::remove_allowed_currencies(RuntimeOrigin::root(), too_many_currencies), + Error::::ExceedsNumberOfAllowedCurrencies + ); + }) +} + #[test] fn should_remove_allowed_currencies() { run_test(|| { @@ -56,10 +78,9 @@ fn should_not_remove_allowed_currencies_with_not_root_origin() { #[test] fn should_add_few_allowed_currencies() { run_test(|| { - let native_currency_id = ::GetNativeCurrencyId::get(); - let added_currencies: Vec> = vec![native_currency_id, 1, 2, 3]; + let added_currencies: Vec> = vec![0, 1, 2, 3]; assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); - assert_eq!(AllowedCurrencies::::get(native_currency_id), Some(())); + assert_eq!(AllowedCurrencies::::get(0), Some(())); assert_eq!(AllowedCurrencies::::get(1), Some(())); assert_eq!(AllowedCurrencies::::get(2), Some(())); assert_eq!(AllowedCurrencies::::get(3), Some(())); @@ -69,27 +90,223 @@ fn should_add_few_allowed_currencies() { #[test] fn should_remove_few_allowed_currencies() { run_test(|| { - let native_currency_id = ::GetNativeCurrencyId::get(); - let added_currencies: Vec> = vec![native_currency_id, 1, 2, 3, 4]; + let added_currencies: Vec> = vec![0, 1, 2, 3, 4]; assert_ok!(TokenAllowance::add_allowed_currencies( RuntimeOrigin::root(), added_currencies.clone() )); - assert_eq!(AllowedCurrencies::::get(native_currency_id), Some(())); + assert_eq!(AllowedCurrencies::::get(0), Some(())); assert_eq!(AllowedCurrencies::::get(1), Some(())); assert_eq!(AllowedCurrencies::::get(2), Some(())); assert_eq!(AllowedCurrencies::::get(3), Some(())); - let removed_currencies: Vec> = vec![native_currency_id, 1, 2, 3]; + let removed_currencies: Vec> = vec![0, 1, 2, 3]; assert_ok!(TokenAllowance::remove_allowed_currencies( RuntimeOrigin::root(), removed_currencies )); - assert_eq!(AllowedCurrencies::::get(native_currency_id), None); + assert_eq!(AllowedCurrencies::::get(0), None); assert_eq!(AllowedCurrencies::::get(1), None); assert_eq!(AllowedCurrencies::::get(2), None); assert_eq!(AllowedCurrencies::::get(3), None); assert_eq!(AllowedCurrencies::::get(4), Some(())); }) } + +#[test] +fn should_approve_transfer() { + run_test(|| { + let currency_id: ::CurrencyId = 0; + let owner: ::AccountId = 0; + let delegate: ::AccountId = 1; + let amount: ::Balance = 1_000_000_000u32 as Balance; + + // Will not work yet + assert_err!( + TokenAllowance::approve( + RuntimeOrigin::signed(owner.clone()), + currency_id, + delegate.clone(), + amount + ), + Error::::CurrencyNotLive + ); + + // We need to add the currency first + assert_ok!(TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + vec![currency_id] + )); + + // Should work now + assert_ok!(TokenAllowance::approve( + RuntimeOrigin::signed(owner.clone()), + currency_id, + delegate.clone(), + amount + )); + }) +} + +#[test] +fn should_do_approved_transfer() { + run_test(|| { + let currency_id: ::CurrencyId = 0; + let owner: ::AccountId = 0; + let delegate: ::AccountId = 1; + let destination: ::AccountId = 2; + let amount: ::Balance = 1_000_000_000u32 as Balance; + + // Mint some tokens + assert_ok!(Tokens::deposit(currency_id, &owner, amount)); + + // Check the balances + assert_eq!(Tokens::free_balance(currency_id, &owner), amount); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), 0); + + // We need to add the currency first + assert_ok!(TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + vec![currency_id] + )); + + // Approve the amount + assert_ok!(TokenAllowance::approve( + RuntimeOrigin::signed(owner.clone()), + currency_id, + delegate.clone(), + amount + )); + + // Transfer the approved amount + assert_ok!(TokenAllowance::transfer_from( + RuntimeOrigin::signed(delegate.clone()), + currency_id, + owner.clone(), + destination.clone(), + amount + )); + + // Check the balances + assert_eq!(Tokens::free_balance(currency_id, &owner), 0); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), amount); + }) +} + +#[test] +fn should_transfer_from_keeping_infinite_allowance() { + run_test(|| { + let currency_id: ::CurrencyId = 0; + let owner: ::AccountId = 0; + let delegate: ::AccountId = 1; + let destination: ::AccountId = 2; + // We use the max value of u128 as the amount to approve because it represents an infinite allowance + let allowance_amount: ::Balance = + ::Balance::max_value(); + + // Mint some tokens + assert_ok!(Tokens::deposit(currency_id, &owner, allowance_amount)); + + // Check the balances + assert_eq!(Tokens::free_balance(currency_id, &owner), allowance_amount); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), 0); + + // We need to add the currency first + assert_ok!(TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + vec![currency_id] + )); + + // Approve infinite spending + assert_ok!(TokenAllowance::approve( + RuntimeOrigin::signed(owner.clone()), + currency_id, + delegate.clone(), + allowance_amount, + )); + + // Check the allowance of the delegate + assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), allowance_amount); + + // Transfer the approved amount once + assert_ok!(TokenAllowance::transfer_from( + RuntimeOrigin::signed(delegate.clone()), + currency_id, + owner.clone(), + destination.clone(), + allowance_amount + )); + + // Check the balances + assert_eq!(Tokens::free_balance(currency_id, &owner), 0); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), allowance_amount); + + // Check that the allowance of the delegate is still the same since it should be infinite + assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), allowance_amount); + + // Move the tokens from `destination` to the `owner` again to avoid overflow but allow for testing the same amount again + assert_ok!(Tokens::transfer( + RuntimeOrigin::signed(destination.clone()), + owner, + currency_id, + allowance_amount + )); + + // Transfer the approved amount again + assert_ok!(TokenAllowance::transfer_from( + RuntimeOrigin::signed(delegate.clone()), + currency_id, + owner.clone(), + destination.clone(), + allowance_amount + )); + }) +} + +#[test] +fn should_not_transfer_from_for_invalid_origin() { + run_test(|| { + let currency_id: ::CurrencyId = 0; + let owner: ::AccountId = 0; + let destination: ::AccountId = 2; + let amount: ::Balance = 1_000_000_000u32 as Balance; + + assert_err!( + TokenAllowance::transfer_from( + RuntimeOrigin::none(), + currency_id, + owner.clone(), + destination.clone(), + amount + ), + BadOrigin + ); + }) +} + +#[test] +fn should_not_transfer_from_for_invalid_currency() { + run_test(|| { + let currency_id: ::CurrencyId = 0; + let owner: ::AccountId = 0; + let delegate: ::AccountId = 1; + let destination: ::AccountId = 2; + let amount: ::Balance = 1_000_000_000u32 as Balance; + + assert_err!( + TokenAllowance::transfer_from( + RuntimeOrigin::signed(delegate.clone()), + currency_id, + owner.clone(), + destination.clone(), + amount + ), + Error::::CurrencyNotLive + ); + }) +} From bfc79750c634d577178cdb9fad9d2120a2a18f5a Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Mon, 9 Oct 2023 16:45:49 +0200 Subject: [PATCH 3/5] Add even more tests --- .../src/tests.rs | 246 ++++++++++++++---- 1 file changed, 191 insertions(+), 55 deletions(-) diff --git a/pallets/orml-currencies-allowance-extension/src/tests.rs b/pallets/orml-currencies-allowance-extension/src/tests.rs index 86ffbf5fe..2a2d73859 100644 --- a/pallets/orml-currencies-allowance-extension/src/tests.rs +++ b/pallets/orml-currencies-allowance-extension/src/tests.rs @@ -6,10 +6,39 @@ use crate::{mock::*, AllowedCurrencies, Config, CurrencyOf, Error}; #[test] fn should_add_allowed_currencies() { run_test(|| { - let native_currency_id = ::GetNativeCurrencyId::get(); - let added_currencies: Vec> = vec![native_currency_id]; + let max_allowed_currencies: u32 = ::MaxAllowedCurrencies::get(); + let added_currencies = (0..max_allowed_currencies as u64).collect::>(); + + assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); + for i in 0..max_allowed_currencies { + assert_eq!(AllowedCurrencies::::get(i), Some(())); + } + }) +} + +#[test] +fn should_remove_allowed_currencies() { + run_test(|| { + let max_allowed_currencies: u32 = ::MaxAllowedCurrencies::get(); + let mut added_currencies = (0..max_allowed_currencies as u64).collect::>(); + assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); - assert_eq!(AllowedCurrencies::::get(native_currency_id), Some(())); + for i in 0..max_allowed_currencies { + assert_eq!(AllowedCurrencies::::get(i), Some(())); + } + + let existing_currency: CurrencyOf = + added_currencies.pop().expect("Should have a currency"); + assert_ok!(TokenAllowance::remove_allowed_currencies( + RuntimeOrigin::root(), + added_currencies + )); + + for i in 0..added_currencies.len() as u32 { + assert_eq!(AllowedCurrencies::::get(i), None); + } + // The existing currency should remain + assert_eq!(AllowedCurrencies::::get(existing_currency), Some(())); }) } @@ -19,6 +48,7 @@ fn should_not_exceed_allowed_currencies() { let max_allowed_currencies: u32 = ::MaxAllowedCurrencies::get(); let too_many_currencies = (0..(max_allowed_currencies as u64) + 1).collect::>(); + // We can't add more than the maximum allowed currencies assert_err!( TokenAllowance::add_allowed_currencies( RuntimeOrigin::root(), @@ -31,86 +61,90 @@ fn should_not_exceed_allowed_currencies() { TokenAllowance::remove_allowed_currencies(RuntimeOrigin::root(), too_many_currencies), Error::::ExceedsNumberOfAllowedCurrencies ); - }) -} -#[test] -fn should_remove_allowed_currencies() { - run_test(|| { - let native_currency_id = ::GetNativeCurrencyId::get(); - let added_currencies: Vec> = vec![native_currency_id]; + // Fill the allowed currencies to the maximum + let mut added_currencies = (0..max_allowed_currencies as u64).collect::>(); + assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); + for i in 0..max_allowed_currencies { + assert_eq!(AllowedCurrencies::::get(i), Some(())); + } + + // Try to add a duplicate currency (should not fail because we don't store duplicates) + let already_added_currency = added_currencies[0]; assert_ok!(TokenAllowance::add_allowed_currencies( RuntimeOrigin::root(), - added_currencies.clone() + already_added_currency )); - assert_eq!(AllowedCurrencies::::get(native_currency_id), Some(())); + assert_eq!(AllowedCurrencies::::get(already_added_currency), Some(())); - assert_ok!(TokenAllowance::remove_allowed_currencies( - RuntimeOrigin::root(), - added_currencies - )); - assert_eq!(AllowedCurrencies::::get(native_currency_id), None); + // Try to add a new distinct currency (should fail since we reached the maximum) + let illegal_currency: CurrencyOf = max_allowed_currencies as u64; + assert_err!( + TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), illegal_currency), + Error::::ExceedsNumberOfAllowedCurrencies + ); }) } #[test] -fn should_not_add_allowed_currencies_with_not_root_origin() { +fn should_not_add_allowed_currencies_with_non_root_origin() { run_test(|| { let native_currency_id = ::GetNativeCurrencyId::get(); let added_currencies: Vec> = vec![native_currency_id]; - let result = - TokenAllowance::add_allowed_currencies(RuntimeOrigin::signed(1), added_currencies); - assert_err!(result, BadOrigin); + assert_err!( + TokenAllowance::add_allowed_currencies(RuntimeOrigin::signed(1), added_currencies), + BadOrigin + ); + assert_err!( + TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies), + BadOrigin + ); }) } #[test] -fn should_not_remove_allowed_currencies_with_not_root_origin() { +fn should_not_remove_allowed_currencies_with_non_root_origin() { run_test(|| { let native_currency_id = ::GetNativeCurrencyId::get(); let added_currencies: Vec> = vec![native_currency_id]; - let result = - TokenAllowance::remove_allowed_currencies(RuntimeOrigin::signed(1), added_currencies); - assert_err!(result, BadOrigin); + assert_err!( + TokenAllowance::remove_allowed_currencies(RuntimeOrigin::signed(1), added_currencies), + BadOrigin + ); + assert_err!( + TokenAllowance::remove_allowed_currencies(RuntimeOrigin::root(), added_currencies), + BadOrigin + ); }) } #[test] -fn should_add_few_allowed_currencies() { +fn should_return_allowance() { run_test(|| { - let added_currencies: Vec> = vec![0, 1, 2, 3]; - assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); - assert_eq!(AllowedCurrencies::::get(0), Some(())); - assert_eq!(AllowedCurrencies::::get(1), Some(())); - assert_eq!(AllowedCurrencies::::get(2), Some(())); - assert_eq!(AllowedCurrencies::::get(3), Some(())); - }) -} + let currency_id: ::CurrencyId = 0; + let owner: ::AccountId = 0; + let delegate: ::AccountId = 1; + let amount: ::Balance = 1_000_000_000u32 as Balance; -#[test] -fn should_remove_few_allowed_currencies() { - run_test(|| { - let added_currencies: Vec> = vec![0, 1, 2, 3, 4]; + // We need to add the currency first assert_ok!(TokenAllowance::add_allowed_currencies( RuntimeOrigin::root(), - added_currencies.clone() + vec![currency_id] )); - assert_eq!(AllowedCurrencies::::get(0), Some(())); - assert_eq!(AllowedCurrencies::::get(1), Some(())); - assert_eq!(AllowedCurrencies::::get(2), Some(())); - assert_eq!(AllowedCurrencies::::get(3), Some(())); - let removed_currencies: Vec> = vec![0, 1, 2, 3]; - assert_ok!(TokenAllowance::remove_allowed_currencies( - RuntimeOrigin::root(), - removed_currencies + // Check allowance + assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), 0); + + // Approve the amount + assert_ok!(TokenAllowance::approve( + RuntimeOrigin::signed(owner.clone()), + currency_id, + delegate.clone(), + amount )); - assert_eq!(AllowedCurrencies::::get(0), None); - assert_eq!(AllowedCurrencies::::get(1), None); - assert_eq!(AllowedCurrencies::::get(2), None); - assert_eq!(AllowedCurrencies::::get(3), None); - assert_eq!(AllowedCurrencies::::get(4), Some(())); + // Check allowance again + assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), amount); }) } @@ -146,11 +180,14 @@ fn should_approve_transfer() { delegate.clone(), amount )); + + // Check allowance + assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), amount); }) } #[test] -fn should_do_approved_transfer() { +fn should_transfer_from_for_approved_transfer() { run_test(|| { let currency_id: ::CurrencyId = 0; let owner: ::AccountId = 0; @@ -180,7 +217,7 @@ fn should_do_approved_transfer() { amount )); - // Transfer the approved amount + // Transfer all of the approved amount assert_ok!(TokenAllowance::transfer_from( RuntimeOrigin::signed(delegate.clone()), currency_id, @@ -193,11 +230,95 @@ fn should_do_approved_transfer() { assert_eq!(Tokens::free_balance(currency_id, &owner), 0); assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); assert_eq!(Tokens::free_balance(currency_id, &destination), amount); + + // Test again but this time only using a partial amount of what was approved + let partial_amount = amount / 2; + assert_ok!(Tokens::deposit(currency_id, &owner, amount)); + assert_ok!(TokenAllowance::approve( + RuntimeOrigin::signed(owner.clone()), + currency_id, + delegate.clone(), + amount + )); + assert_ok!(TokenAllowance::transfer_from( + RuntimeOrigin::signed(delegate.clone()), + currency_id, + owner.clone(), + destination.clone(), + partial_amount + )); + + // Check the balances again + assert_eq!(Tokens::free_balance(currency_id, &owner), amount - partial_amount); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), amount + partial_amount); }) } #[test] -fn should_transfer_from_keeping_infinite_allowance() { +fn should_not_transfer_from_without_approved_transfer() { + run_test(|| { + let currency_id: ::CurrencyId = 0; + let owner: ::AccountId = 0; + let delegate: ::AccountId = 1; + let destination: ::AccountId = 2; + let amount: ::Balance = 1_000_000_000u32 as Balance; + + // Mint some tokens + assert_ok!(Tokens::deposit(currency_id, &owner, amount)); + + // Check the balances + assert_eq!(Tokens::free_balance(currency_id, &owner), amount); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), 0); + + // We need to add the currency first + assert_ok!(TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + vec![currency_id] + )); + + // Try to `transfer_from` without having approved the transfer + assert_err!( + TokenAllowance::transfer_from( + RuntimeOrigin::signed(delegate.clone()), + currency_id, + owner.clone(), + destination.clone(), + amount + ), + Error::::Unapproved + ); + + // Approve the amount + assert_ok!(TokenAllowance::approve( + RuntimeOrigin::signed(owner.clone()), + currency_id, + delegate.clone(), + amount + )); + + // Try to `transfer_from` for amount larger than what was approved + assert_err!( + TokenAllowance::transfer_from( + RuntimeOrigin::signed(delegate.clone()), + currency_id, + owner.clone(), + destination.clone(), + amount + 1 + ), + Error::::Unapproved + ); + + // Check the balances + assert_eq!(Tokens::free_balance(currency_id, &owner), amount); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), 0); + }) +} + +#[test] +fn should_transfer_from_while_keeping_infinite_allowance() { run_test(|| { let currency_id: ::CurrencyId = 0; let owner: ::AccountId = 0; @@ -265,6 +386,11 @@ fn should_transfer_from_keeping_infinite_allowance() { destination.clone(), allowance_amount )); + + // Check the balances again + assert_eq!(Tokens::free_balance(currency_id, &owner), 0); + assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); + assert_eq!(Tokens::free_balance(currency_id, &destination), allowance_amount); }) } @@ -286,6 +412,16 @@ fn should_not_transfer_from_for_invalid_origin() { ), BadOrigin ); + assert_err!( + TokenAllowance::transfer_from( + RuntimeOrigin::root(), + currency_id, + owner.clone(), + destination.clone(), + amount + ), + BadOrigin + ); }) } From 6bb68f33c40d0164c649517899f4f3440a6b79cd Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Mon, 9 Oct 2023 17:21:20 +0200 Subject: [PATCH 4/5] Some refactoring --- .../src/tests.rs | 58 ++++++++++++------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/pallets/orml-currencies-allowance-extension/src/tests.rs b/pallets/orml-currencies-allowance-extension/src/tests.rs index 2a2d73859..fcf59ea79 100644 --- a/pallets/orml-currencies-allowance-extension/src/tests.rs +++ b/pallets/orml-currencies-allowance-extension/src/tests.rs @@ -7,7 +7,8 @@ use crate::{mock::*, AllowedCurrencies, Config, CurrencyOf, Error}; fn should_add_allowed_currencies() { run_test(|| { let max_allowed_currencies: u32 = ::MaxAllowedCurrencies::get(); - let added_currencies = (0..max_allowed_currencies as u64).collect::>(); + let max_allowed_currencies: u64 = max_allowed_currencies as u64; + let added_currencies = (0..max_allowed_currencies).collect::>(); assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); for i in 0..max_allowed_currencies { @@ -20,9 +21,13 @@ fn should_add_allowed_currencies() { fn should_remove_allowed_currencies() { run_test(|| { let max_allowed_currencies: u32 = ::MaxAllowedCurrencies::get(); - let mut added_currencies = (0..max_allowed_currencies as u64).collect::>(); + let max_allowed_currencies: u64 = max_allowed_currencies as u64; + let mut added_currencies = (0..max_allowed_currencies).collect::>(); - assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); + assert_ok!(TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + added_currencies.clone() + )); for i in 0..max_allowed_currencies { assert_eq!(AllowedCurrencies::::get(i), Some(())); } @@ -31,10 +36,10 @@ fn should_remove_allowed_currencies() { added_currencies.pop().expect("Should have a currency"); assert_ok!(TokenAllowance::remove_allowed_currencies( RuntimeOrigin::root(), - added_currencies + added_currencies.clone() )); - for i in 0..added_currencies.len() as u32 { + for i in 0..added_currencies.len() as u64 { assert_eq!(AllowedCurrencies::::get(i), None); } // The existing currency should remain @@ -46,7 +51,8 @@ fn should_remove_allowed_currencies() { fn should_not_exceed_allowed_currencies() { run_test(|| { let max_allowed_currencies: u32 = ::MaxAllowedCurrencies::get(); - let too_many_currencies = (0..(max_allowed_currencies as u64) + 1).collect::>(); + let max_allowed_currencies: u64 = max_allowed_currencies as u64; + let too_many_currencies = (0..max_allowed_currencies + 1).collect::>(); // We can't add more than the maximum allowed currencies assert_err!( @@ -63,8 +69,11 @@ fn should_not_exceed_allowed_currencies() { ); // Fill the allowed currencies to the maximum - let mut added_currencies = (0..max_allowed_currencies as u64).collect::>(); - assert_ok!(TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies)); + let added_currencies = (0..max_allowed_currencies).collect::>(); + assert_ok!(TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + added_currencies.clone() + )); for i in 0..max_allowed_currencies { assert_eq!(AllowedCurrencies::::get(i), Some(())); } @@ -73,14 +82,14 @@ fn should_not_exceed_allowed_currencies() { let already_added_currency = added_currencies[0]; assert_ok!(TokenAllowance::add_allowed_currencies( RuntimeOrigin::root(), - already_added_currency + vec![already_added_currency] )); assert_eq!(AllowedCurrencies::::get(already_added_currency), Some(())); // Try to add a new distinct currency (should fail since we reached the maximum) - let illegal_currency: CurrencyOf = max_allowed_currencies as u64; + let illegal_currency: CurrencyOf = max_allowed_currencies; assert_err!( - TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), illegal_currency), + TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), vec![illegal_currency]), Error::::ExceedsNumberOfAllowedCurrencies ); }) @@ -92,11 +101,14 @@ fn should_not_add_allowed_currencies_with_non_root_origin() { let native_currency_id = ::GetNativeCurrencyId::get(); let added_currencies: Vec> = vec![native_currency_id]; assert_err!( - TokenAllowance::add_allowed_currencies(RuntimeOrigin::signed(1), added_currencies), + TokenAllowance::add_allowed_currencies( + RuntimeOrigin::signed(1), + added_currencies.clone() + ), BadOrigin ); assert_err!( - TokenAllowance::add_allowed_currencies(RuntimeOrigin::root(), added_currencies), + TokenAllowance::add_allowed_currencies(RuntimeOrigin::none(), added_currencies), BadOrigin ); }) @@ -108,11 +120,14 @@ fn should_not_remove_allowed_currencies_with_non_root_origin() { let native_currency_id = ::GetNativeCurrencyId::get(); let added_currencies: Vec> = vec![native_currency_id]; assert_err!( - TokenAllowance::remove_allowed_currencies(RuntimeOrigin::signed(1), added_currencies), + TokenAllowance::remove_allowed_currencies( + RuntimeOrigin::signed(1), + added_currencies.clone() + ), BadOrigin ); assert_err!( - TokenAllowance::remove_allowed_currencies(RuntimeOrigin::root(), added_currencies), + TokenAllowance::remove_allowed_currencies(RuntimeOrigin::none(), added_currencies), BadOrigin ); }) @@ -126,12 +141,6 @@ fn should_return_allowance() { let delegate: ::AccountId = 1; let amount: ::Balance = 1_000_000_000u32 as Balance; - // We need to add the currency first - assert_ok!(TokenAllowance::add_allowed_currencies( - RuntimeOrigin::root(), - vec![currency_id] - )); - // Check allowance assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), 0); @@ -230,6 +239,8 @@ fn should_transfer_from_for_approved_transfer() { assert_eq!(Tokens::free_balance(currency_id, &owner), 0); assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); assert_eq!(Tokens::free_balance(currency_id, &destination), amount); + // Check that the allowance is now empty since we transferred the whole amount + assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), 0); // Test again but this time only using a partial amount of what was approved let partial_amount = amount / 2; @@ -252,6 +263,11 @@ fn should_transfer_from_for_approved_transfer() { assert_eq!(Tokens::free_balance(currency_id, &owner), amount - partial_amount); assert_eq!(Tokens::free_balance(currency_id, &delegate), 0); assert_eq!(Tokens::free_balance(currency_id, &destination), amount + partial_amount); + // Check that the allowance is now reduced by the partial amount + assert_eq!( + TokenAllowance::allowance(currency_id, &owner, &delegate), + amount - partial_amount + ); }) } From db7c5deb0f58d6cc706c60668e4beb75e2774d8b Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Tue, 10 Oct 2023 12:05:03 +0200 Subject: [PATCH 5/5] Fix `should_return_allowance()` test --- pallets/orml-currencies-allowance-extension/src/tests.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pallets/orml-currencies-allowance-extension/src/tests.rs b/pallets/orml-currencies-allowance-extension/src/tests.rs index fcf59ea79..cb97aaacb 100644 --- a/pallets/orml-currencies-allowance-extension/src/tests.rs +++ b/pallets/orml-currencies-allowance-extension/src/tests.rs @@ -141,6 +141,12 @@ fn should_return_allowance() { let delegate: ::AccountId = 1; let amount: ::Balance = 1_000_000_000u32 as Balance; + // We need to add the currency first + assert_ok!(TokenAllowance::add_allowed_currencies( + RuntimeOrigin::root(), + vec![currency_id] + )); + // Check allowance assert_eq!(TokenAllowance::allowance(currency_id, &owner, &delegate), 0);