Skip to content
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

feat(pallet): follow psp22 standard for decrease_allowance and burn #311

Closed
wants to merge 13 commits into from
6 changes: 5 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ license = "Unlicense"
repository = "https://github.com/r0gue-io/pop-node/"

[workspace]
exclude = [ "extension/contract", "pop-api", "tests/contracts" ]
exclude = [
"extension/contract",
"pop-api",
"tests/contracts",
]
members = [
"integration-tests",
"node",
Expand Down
10 changes: 8 additions & 2 deletions pallets/api/src/fungibles/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type AccountIdOf<T> = <T as frame_system::Config>::AccountId;
type TokenIdOf<T> = <AssetsOf<T> as Inspect<<T as frame_system::Config>::AccountId>>::AssetId;
type TokenIdParameterOf<T> = <T as pallet_assets::Config<AssetsInstanceOf<T>>>::AssetIdParameter;
type AssetsOf<T> = pallet_assets::Pallet<T, AssetsInstanceOf<T>>;
type AssetsErrorOf<T> = pallet_assets::Error<T, AssetsInstanceOf<T>>;
type AssetsInstanceOf<T> = <T as Config>::AssetsInstance;
type AssetsWeightInfoOf<T> = <T as pallet_assets::Config<AssetsInstanceOf<T>>>::WeightInfo;
type BalanceOf<T> = <AssetsOf<T> as Inspect<<T as frame_system::Config>::AccountId>>::Balance;
Expand All @@ -33,7 +34,7 @@ pub mod pallet {
};
use frame_system::pallet_prelude::*;
use sp_runtime::{
traits::{StaticLookup, Zero},
traits::{CheckedSub, StaticLookup, Zero},
Saturating,
};
use sp_std::vec::Vec;
Expand Down Expand Up @@ -338,13 +339,14 @@ pub mod pallet {
let token_param: TokenIdParameterOf<T> = token.clone().into();

// Cancel the approval and approve `new_allowance` if difference is more than zero.
let new_allowance =
current_allowance.checked_sub(&value).ok_or(AssetsErrorOf::<T>::Unapproved)?;
AssetsOf::<T>::cancel_approval(
origin.clone(),
token_param.clone(),
spender_source.clone(),
)
.map_err(|e| e.with_weight(WeightOf::<T>::approve(0, 1)))?;
let new_allowance = current_allowance.saturating_sub(value);
let weight = if new_allowance.is_zero() {
WeightOf::<T>::approve(0, 1)
} else {
Expand Down Expand Up @@ -467,6 +469,10 @@ pub mod pallet {
account: AccountIdOf<T>,
value: BalanceOf<T>,
) -> DispatchResult {
let current_balance = AssetsOf::<T>::balance(token.clone(), &account);
if current_balance < value {
return Err(AssetsErrorOf::<T>::BalanceLow.into());
}
Daanvdplas marked this conversation as resolved.
Show resolved Hide resolved
AssetsOf::<T>::burn(
origin,
token.clone().into(),
Expand Down
35 changes: 21 additions & 14 deletions pallets/api/src/fungibles/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,20 +272,27 @@ fn decrease_allowance_works() {
BadOrigin.with_weight(WeightInfo::approve(0, 0))
);
}
assets::create_mint_and_approve(owner, token, owner, value, spender, value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to this test demonstrate why a test per behaviour might be preferred. Such an introduction of a feat should of just had a new test added for a new behaviour, which would have been much cleaner to both write and review.

assert_eq!(Assets::allowance(token, &owner, &spender), value);
// Check error works for `Assets::cancel_approval()`. No error test for `approve_transfer`
// because it is not possible.
assert_ok!(Assets::freeze_asset(signed(owner), token));
assert_noop!(
Fungibles::decrease_allowance(signed(owner), token, spender, value / 2),
AssetsError::Unknown.with_weight(WeightInfo::approve(0, 1))
AssetsError::AssetNotLive.with_weight(WeightInfo::approve(0, 1))
);
assets::create_mint_and_approve(owner, token, owner, value, spender, value);
assert_eq!(Assets::allowance(token, &owner, &spender), value);
assert_ok!(Assets::thaw_asset(signed(owner), token));
// Owner balance is not changed if decreased by zero.
assert_eq!(
Fungibles::decrease_allowance(signed(owner), token, spender, 0),
Ok(Some(WeightInfo::approve(0, 0)).into())
);
assert_eq!(Assets::allowance(token, &owner, &spender), value);
// "Unapproved" error is returned if the current allowance is less than `value`.
chungquantin marked this conversation as resolved.
Show resolved Hide resolved
assert_noop!(
Fungibles::decrease_allowance(signed(owner), token, spender, value * 2),
AssetsError::Unapproved
);
// Decrease allowance successfully.
assert_eq!(
Fungibles::decrease_allowance(signed(owner), token, spender, value / 2),
Expand All @@ -295,13 +302,6 @@ fn decrease_allowance_works() {
System::assert_last_event(
Event::Approval { token, owner, spender, value: value / 2 }.into(),
);
// Saturating if current allowance is decreased more than the owner balance.
assert_eq!(
Fungibles::decrease_allowance(signed(owner), token, spender, value),
Ok(Some(WeightInfo::approve(0, 1)).into())
);
assert_eq!(Assets::allowance(token, &owner, &spender), 0);
System::assert_last_event(Event::Approval { token, owner, spender, value: 0 }.into());
});
}

Expand Down Expand Up @@ -416,10 +416,17 @@ fn burn_works() {
let from = BOB;
let total_supply = value * 2;

// Check error works for `Assets::burn()`.
assert_noop!(Fungibles::burn(signed(owner), token, from, value), AssetsError::Unknown);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst the assets error assertions were to ensure that errors are properly handled, which is the case on line 424, this line could have been left in.

assets::create_and_mint_to(owner, token, from, total_supply);
assert_eq!(Assets::total_supply(TOKEN), total_supply);
// Check error works for `Assets::burn()`.
assert_ok!(Assets::freeze_asset(signed(owner), token));
assert_noop!(Fungibles::burn(signed(owner), token, from, value), AssetsError::AssetNotLive);
assert_ok!(Assets::thaw_asset(signed(owner), token));
// "BalanceLow" error is returned if the balance is less than `value`.
chungquantin marked this conversation as resolved.
Show resolved Hide resolved
assert_noop!(
Fungibles::burn(signed(owner), token, from, total_supply * 2),
AssetsError::BalanceLow
);
let balance_before_burn = Assets::balance(token, &from);
assert_ok!(Fungibles::burn(signed(owner), token, from, value));
assert_eq!(Assets::total_supply(TOKEN), total_supply - value);
Expand Down Expand Up @@ -677,8 +684,8 @@ mod read_weights {
}

mod ensure_codec_indexes {
use super::{Encode, RuntimeCall, *};
use crate::{fungibles, fungibles::Call::*, mock::RuntimeCall::Fungibles};
use super::{Encode, *};
use crate::{fungibles, mock::RuntimeCall::Fungibles};

#[test]
fn ensure_read_variant_indexes() {
Expand Down
Empty file modified pop-api/examples/.gitignore
100755 → 100644
Empty file.
Empty file modified pop-api/examples/balance-transfer/Cargo.toml
100755 → 100644
Empty file.
Empty file modified pop-api/examples/balance-transfer/lib.rs
100755 → 100644
Empty file.
Empty file modified pop-api/examples/fungibles/Cargo.toml
100755 → 100644
Empty file.
Empty file modified pop-api/examples/fungibles/lib.rs
100755 → 100644
Empty file.
Empty file modified pop-api/examples/nfts/Cargo.toml
100755 → 100644
Empty file.
Empty file modified pop-api/examples/nfts/lib.rs
100755 → 100644
Empty file.
Empty file modified pop-api/examples/place-spot-order/Cargo.toml
100755 → 100644
Empty file.
Empty file modified pop-api/examples/place-spot-order/lib.rs
100755 → 100644
Empty file.
Empty file modified pop-api/examples/read-runtime-state/Cargo.toml
100755 → 100644
Empty file.
Empty file modified pop-api/examples/read-runtime-state/lib.rs
100755 → 100644
Empty file.
Empty file.
Empty file.
Empty file modified pop-api/integration-tests/contracts/fungibles/Cargo.toml
100755 → 100644
Empty file.
Empty file modified pop-api/integration-tests/contracts/fungibles/lib.rs
100755 → 100644
Empty file.
5 changes: 0 additions & 5 deletions pop-api/integration-tests/src/fungibles/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,6 @@ fn decrease_allowance_works() {
let addr = instantiate(CONTRACT, INIT_VALUE, vec![]);
let amount: Balance = 100 * UNIT;

// Token does not exist.
assert_eq!(
decrease_allowance(&addr, 0, &BOB, amount),
Err(Module { index: 52, error: [3, 0] }),
);
// Create token and mint `amount` to contract address, then approve Bob to spend `amount`.
let token = assets::create_mint_and_approve(&addr, 0, &addr, amount, &BOB, amount);
// Token is not live, i.e. frozen or being destroyed.
Expand Down
Loading