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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b31db97
feat(pallet): follow psp22 standard for `decrease_allowance` and `bur…
chungquantin Sep 27, 2024
437d71b
refactor: convert to `ok_or`
chungquantin Sep 27, 2024
bf474c6
fix: apply suggestions from code review
chungquantin Sep 28, 2024
7fd90b5
feat: integration-tests
chungquantin Sep 30, 2024
3f7a0fb
fix: formatting
chungquantin Sep 30, 2024
eb11d7d
fix: burn integration tests
chungquantin Sep 30, 2024
53fac61
chore: update weights
chungquantin Oct 1, 2024
4c20399
fix: resolve comments
chungquantin Oct 1, 2024
2d2be28
fix: return weight on burn error
chungquantin Oct 1, 2024
2e7c3ec
fix: formatting
chungquantin Oct 1, 2024
64ac345
chore: update weights
chungquantin Oct 1, 2024
93f048c
fix: file permission
chungquantin Oct 2, 2024
d7b5be9
Merge branch 'main' into chungquantin/main-psp22_pallet
chungquantin Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Cargo.toml
View file
Open in desktop
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
17 changes: 12 additions & 5 deletions pallets/api/src/fungibles/mod.rs
View file
Open in desktop
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 @@ -460,21 +462,26 @@ pub mod pallet {
/// - `account` - The account from which the tokens will be destroyed.
/// - `value` - The number of tokens to destroy.
#[pallet::call_index(20)]
#[pallet::weight(AssetsWeightInfoOf::<T>::burn())]
#[pallet::weight(<T as Config>::WeightInfo::balance_of() + AssetsWeightInfoOf::<T>::burn())]
pub fn burn(
origin: OriginFor<T>,
token: TokenIdOf<T>,
account: AccountIdOf<T>,
value: BalanceOf<T>,
) -> DispatchResult {
) -> DispatchResultWithPostInfo {
let current_balance = AssetsOf::<T>::balance(token.clone(), &account);
if current_balance < value {
return Err(AssetsErrorOf::<T>::BalanceLow
.with_weight(<T as Config>::WeightInfo::balance_of()));
}
Daanvdplas marked this conversation as resolved.
Show resolved Hide resolved
AssetsOf::<T>::burn(
origin,
token.clone().into(),
T::Lookup::unlookup(account.clone()),
value,
)?;
Self::deposit_event(Event::Transfer { token, from: Some(account), to: None, value });
Ok(())
Ok(().into())
}
}

Expand Down
36 changes: 22 additions & 14 deletions pallets/api/src/fungibles/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,20 +272,28 @@ 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 amount to decrease
// with.
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 +303,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 +417,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 amount to burn.
assert_noop!(
Fungibles::burn(signed(owner), token, from, total_supply * 2),
AssetsError::BalanceLow.with_weight(WeightInfo::balance_of())
);
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 +685,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
54 changes: 27 additions & 27 deletions pallets/api/src/fungibles/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//! Autogenerated weights for `fungibles`
//!
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 40.0.0
//! DATE: 2024-09-13, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! DATE: 2024-10-01, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well done for re-running! It might be nice to have devnet benchmarking run automatically on PRs in the future, so that changes included in the comments above the weight function signatures in terms of storage reads/writes can be checked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Love your suggestion. Created an issue for this: #321

//! WORST CASE MAP SIZE: `1000000`
//! HOSTNAME: `R0GUE`, CPU: `<UNKNOWN>`
//! WASM-EXECUTION: `Compiled`, CHAIN: `Some("dev")`, DB CACHE: `1024`
Expand Down Expand Up @@ -57,12 +57,12 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Proof Size summary in bytes:
// Measured: `413 + c * (102 ±0)`
// Estimated: `3675`
// Minimum execution time: 20_000_000 picoseconds.
Weight::from_parts(1_473_469, 3675)
// Standard Error: 89_329
.saturating_add(Weight::from_parts(19_606_122, 0).saturating_mul(a.into()))
// Standard Error: 89_329
.saturating_add(Weight::from_parts(30_920_408, 0).saturating_mul(c.into()))
// Minimum execution time: 26_000_000 picoseconds.
Weight::from_parts(2_877_551, 3675)
// Standard Error: 100_168
.saturating_add(Weight::from_parts(24_846_938, 0).saturating_mul(a.into()))
// Standard Error: 100_168
.saturating_add(Weight::from_parts(38_375_510, 0).saturating_mul(c.into()))
.saturating_add(T::DbWeight::get().reads(2_u64))
.saturating_add(T::DbWeight::get().reads((1_u64).saturating_mul(c.into())))
.saturating_add(T::DbWeight::get().writes(2_u64))
Expand All @@ -74,7 +74,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3675`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3675)
.saturating_add(T::DbWeight::get().reads(1_u64))
}
Expand All @@ -84,7 +84,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3599`
// Minimum execution time: 2_000_000 picoseconds.
// Minimum execution time: 3_000_000 picoseconds.
Weight::from_parts(3_000_000, 3599)
.saturating_add(T::DbWeight::get().reads(1_u64))
}
Expand All @@ -94,8 +94,8 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3613`
// Minimum execution time: 4_000_000 picoseconds.
Weight::from_parts(4_000_000, 3613)
// Minimum execution time: 5_000_000 picoseconds.
Weight::from_parts(5_000_000, 3613)
.saturating_add(T::DbWeight::get().reads(1_u64))
}
/// Storage: `Assets::Metadata` (r:1 w:0)
Expand All @@ -104,7 +104,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3605`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3605)
.saturating_add(T::DbWeight::get().reads(1_u64))
}
Expand All @@ -124,7 +124,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3605`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3605)
.saturating_add(T::DbWeight::get().reads(1_u64))
}
Expand All @@ -134,7 +134,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3675`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3675)
.saturating_add(T::DbWeight::get().reads(1_u64))
}
Expand All @@ -154,12 +154,12 @@ impl WeightInfo for () {
// Proof Size summary in bytes:
// Measured: `413 + c * (102 ±0)`
// Estimated: `3675`
// Minimum execution time: 20_000_000 picoseconds.
Weight::from_parts(1_473_469, 3675)
// Standard Error: 89_329
.saturating_add(Weight::from_parts(19_606_122, 0).saturating_mul(a.into()))
// Standard Error: 89_329
.saturating_add(Weight::from_parts(30_920_408, 0).saturating_mul(c.into()))
// Minimum execution time: 26_000_000 picoseconds.
Weight::from_parts(2_877_551, 3675)
// Standard Error: 100_168
.saturating_add(Weight::from_parts(24_846_938, 0).saturating_mul(a.into()))
// Standard Error: 100_168
.saturating_add(Weight::from_parts(38_375_510, 0).saturating_mul(c.into()))
.saturating_add(RocksDbWeight::get().reads(2_u64))
.saturating_add(RocksDbWeight::get().reads((1_u64).saturating_mul(c.into())))
.saturating_add(RocksDbWeight::get().writes(2_u64))
Expand All @@ -171,7 +171,7 @@ impl WeightInfo for () {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3675`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3675)
.saturating_add(RocksDbWeight::get().reads(1_u64))
}
Expand All @@ -181,7 +181,7 @@ impl WeightInfo for () {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3599`
// Minimum execution time: 2_000_000 picoseconds.
// Minimum execution time: 3_000_000 picoseconds.
Weight::from_parts(3_000_000, 3599)
.saturating_add(RocksDbWeight::get().reads(1_u64))
}
Expand All @@ -191,8 +191,8 @@ impl WeightInfo for () {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3613`
// Minimum execution time: 4_000_000 picoseconds.
Weight::from_parts(4_000_000, 3613)
// Minimum execution time: 5_000_000 picoseconds.
Weight::from_parts(5_000_000, 3613)
.saturating_add(RocksDbWeight::get().reads(1_u64))
}
/// Storage: `Assets::Metadata` (r:1 w:0)
Expand All @@ -201,7 +201,7 @@ impl WeightInfo for () {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3605`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3605)
.saturating_add(RocksDbWeight::get().reads(1_u64))
}
Expand All @@ -221,7 +221,7 @@ impl WeightInfo for () {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3605`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3605)
.saturating_add(RocksDbWeight::get().reads(1_u64))
}
Expand All @@ -231,7 +231,7 @@ impl WeightInfo for () {
// Proof Size summary in bytes:
// Measured: `6`
// Estimated: `3675`
// Minimum execution time: 1_000_000 picoseconds.
// Minimum execution time: 2_000_000 picoseconds.
Weight::from_parts(2_000_000, 3675)
.saturating_add(RocksDbWeight::get().reads(1_u64))
}
Expand Down
Empty file modified pop-api/examples/.gitignore
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/balance-transfer/Cargo.toml
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/balance-transfer/lib.rs
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/fungibles/Cargo.toml
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/fungibles/lib.rs
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/nfts/Cargo.toml
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/nfts/lib.rs
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/place-spot-order/Cargo.toml
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/place-spot-order/lib.rs
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/read-runtime-state/Cargo.toml
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/examples/read-runtime-state/lib.rs
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/integration-tests/contracts/create_token_in_constructor/Cargo.toml
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/integration-tests/contracts/create_token_in_constructor/lib.rs
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/integration-tests/contracts/fungibles/Cargo.toml
100755 → 100644
View file
Open in desktop
Empty file.
Empty file modified pop-api/integration-tests/contracts/fungibles/lib.rs
100755 → 100644
View file
Open in desktop
Empty file.
22 changes: 11 additions & 11 deletions pop-api/integration-tests/src/fungibles/mod.rs
View file
Open in desktop
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 All @@ -267,15 +262,20 @@ fn decrease_allowance_works() {
Err(Module { index: 52, error: [16, 0] }),
);
assets::thaw(&addr, token);
// "Unapproved" error is returned if the current allowance is less than `value`.
assert_eq!(
decrease_allowance(&addr, token, &BOB, amount * 2),
Err(Module { index: 52, error: [10, 0] }),
);
// Successfully decrease allowance.
let allowance_before = Assets::allowance(token, &addr, &BOB);
assert_ok!(decrease_allowance(&addr, 0, &BOB, amount / 2 - 1 * UNIT));
assert_ok!(decrease_allowance(&addr, token, &BOB, amount / 2 - 1 * UNIT));
let allowance_after = Assets::allowance(token, &addr, &BOB);
assert_eq!(allowance_before - allowance_after, amount / 2 - 1 * UNIT);
// Token is not live, i.e. frozen or being destroyed.
assets::start_destroy(&addr, token);
assert_eq!(
decrease_allowance(&addr, token, &BOB, amount),
decrease_allowance(&addr, token, &BOB, 1 * UNIT),
Err(Module { index: 52, error: [16, 0] }),
);
});
Expand Down Expand Up @@ -530,14 +530,14 @@ fn burn_works() {
let amount: Balance = 100 * UNIT;

// Token does not exist.
assert_eq!(burn(&addr, 1, &BOB, amount), Err(Module { index: 52, error: [3, 0] }));
assert_eq!(burn(&addr, 1, &BOB, 0), Err(Module { index: 52, error: [3, 0] }));
let token = assets::create(&ALICE, 1, 1);
// Bob has no tokens and therefore doesn't exist.
assert_eq!(burn(&addr, token, &BOB, 1), Err(Module { index: 52, error: [1, 0] }));
// Bob has no tokens.
assert_eq!(burn(&addr, token, &BOB, amount), Err(Module { index: 52, error: [0, 0] }));
// Burning can only be done by the manager.
assets::mint(&ALICE, token, &BOB, amount);
assert_eq!(burn(&addr, token, &BOB, 1), Err(Module { index: 52, error: [2, 0] }));
let token = assets::create_and_mint_to(&addr, 2, &BOB, amount);
let token = assets::create_and_mint_to(&addr, 2, &BOB, amount * 2);
// Token is not live, i.e. frozen or being destroyed.
assets::freeze(&addr, token);
assert_eq!(burn(&addr, token, &BOB, amount), Err(Module { index: 52, error: [16, 0] }));
Expand Down
Loading