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

[Assets] Call implementation for transfer_all #4527

Merged
merged 14 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -531,4 +531,14 @@ impl<T: frame_system::Config> pallet_assets::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}

fn transfer_all() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `3593`
// Minimum execution time: 46_573_000 picoseconds.
Weight::from_parts(47_385_000, 3593)
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -528,4 +528,14 @@ impl<T: frame_system::Config> pallet_assets::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}

fn transfer_all() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `3593`
// Minimum execution time: 46_573_000 picoseconds.
Weight::from_parts(47_385_000, 3593)
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -528,4 +528,14 @@ impl<T: frame_system::Config> pallet_assets::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}

fn transfer_all() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `3593`
// Minimum execution time: 46_573_000 picoseconds.
Weight::from_parts(47_385_000, 3593)
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -537,4 +537,14 @@ impl<T: frame_system::Config> pallet_assets::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}

fn transfer_all() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `3593`
// Minimum execution time: 46_573_000 picoseconds.
Weight::from_parts(47_385_000, 3593)
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -535,4 +535,14 @@ impl<T: frame_system::Config> pallet_assets::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}

fn transfer_all() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `3593`
// Minimum execution time: 46_573_000 picoseconds.
Weight::from_parts(47_385_000, 3593)
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -529,4 +529,14 @@ impl<T: frame_system::Config> pallet_assets::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}

fn transfer_all() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `3593`
// Minimum execution time: 46_573_000 picoseconds.
Weight::from_parts(47_385_000, 3593)
.saturating_add(T::DbWeight::get().reads(1_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
}
27 changes: 27 additions & 0 deletions prdoc/pr_4527.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Call implementation for `transfer_all`

doc:
- audience: Runtime Dev
description: |
This PR introduces the `transfer_all` call for `pallet-assets`.
The parameters are analog to the same call in `pallet-balances`.
This change shouldn't break the existing APIs.
Copy link
Contributor

Choose a reason for hiding this comment

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

new call is new enum variant within Call enum. so this is a breaking change as for a rust crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's rough. I guess we have to follow crates rules but it's not breaking for transaction encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change it with "This change is expected to be backwards-compatible". While technically it is true that the encoding changes, @joepetrowski is right in the sense that it doesn't change transaction encoding, as it is appending a new call at the end of the Call enum. Therefore, it is safe to say it is backwards compatible (which is the expected behaviour in these cases).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but for crates versioning I think it should follow the rust semver rules. At least that's what we have not in guidelines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, @ggwpez also pointed out that it breaks the WeightInfo trait for anyone using this pallet. So I agree it is (unfortunately) major.

This change requires running benchmarkings to set accurate weights for
the call.
- audience: Runtime User
description: |
This PR introduces the `transfer_all` call for `pallet-assets`.
The parameters are analog to the same call in `pallet-balances`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- audience: Runtime User
description: |
This PR introduces the `transfer_all` call for `pallet-assets`.
The parameters are analog to the same call in `pallet-balances`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consider it's still useful to let runtime users know about this update, as this potentially impacts users like CEXes that will come to the changelog to know about how this change works.

When using this call, consider that transferring all funds of an asset
account might kill it when setting `keep_alive` as false.

crates:
- name: pallet-assets
bump: minor
pandres95 marked this conversation as resolved.
Show resolved Hide resolved
- name: asset-hub-rococo-runtime
bump: minor
- name: asset-hub-westend-runtime
bump: minor
10 changes: 10 additions & 0 deletions substrate/frame/assets/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -553,5 +553,15 @@ benchmarks_instance_pallet! {
assert_last_event::<T, I>(Event::Blocked { asset_id: asset_id.into(), who: caller }.into());
}

transfer_all {
let amount = T::Balance::from(100u32);
pandres95 marked this conversation as resolved.
Show resolved Hide resolved
let (asset_id, caller, caller_lookup) = create_default_minted_asset::<T, I>(true, amount);
let target: T::AccountId = account("target", 0, SEED);
let target_lookup = T::Lookup::unlookup(target.clone());
}: _(SystemOrigin::Signed(caller.clone()), asset_id.clone(), target_lookup, false)
verify {
assert_last_event::<T, I>(Event::Transferred { asset_id: asset_id.into(), from: caller, to: target, amount }.into());
}

impl_benchmark_test_suite!(Assets, crate::mock::new_test_ext(), crate::mock::Test)
}
49 changes: 48 additions & 1 deletion substrate/frame/assets/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,11 @@ use frame_support::{
pallet_prelude::DispatchResultWithPostInfo,
storage::KeyPrefixIterator,
traits::{
tokens::{fungibles, DepositConsequence, WithdrawConsequence},
tokens::{
fungibles, DepositConsequence, Fortitude,
Preservation::{Expendable, Preserve},
WithdrawConsequence,
},
BalanceStatus::Reserved,
Currency, EnsureOriginWithArg, ReservableCurrency, StoredMap,
},
Expand Down Expand Up @@ -1687,6 +1691,49 @@ pub mod pallet {
Self::deposit_event(Event::<T, I>::Blocked { asset_id: id, who });
Ok(())
}

/// Transfer the entire transferable balance from the caller asset account.
///
/// NOTE: This function only attempts to transfer _transferable_ balances. This means that
/// any held, frozen, or minimum balance (when `keep_alive` is `true`), will not be
/// transferred by this function. To ensure that this function results in a killed account,
/// you might need to prepare the account by removing any reference counters, storage
/// deposits, etc...
///
/// The dispatch origin of this call must be Signed.
///
/// - `id`: The identifier of the asset for the account holding a deposit.
/// - `dest`: The recipient of the transfer.
/// - `keep_alive`: A boolean to determine if the `transfer_all` operation should send all
/// of the funds the asset account has, causing the sender asset account to be killed
/// (false), or transfer everything except at least the minimum balance, which will
/// guarantee to keep the sender asset account alive (true).
#[pallet::call_index(32)]
#[pallet::weight(T::WeightInfo::refund_other())]
pub fn transfer_all(
joepetrowski marked this conversation as resolved.
Show resolved Hide resolved
origin: OriginFor<T>,
id: T::AssetIdParameter,
dest: AccountIdLookupOf<T>,
keep_alive: bool,
) -> DispatchResult {
let transactor = ensure_signed(origin)?;
let keep_alive = if keep_alive { Preserve } else { Expendable };
let reducible_balance = <Self as fungibles::Inspect<_>>::reducible_balance(
id.clone().into(),
&transactor,
keep_alive,
Fortitude::Polite,
);
let dest = T::Lookup::lookup(dest)?;
<Self as fungibles::Mutate<_>>::transfer(
id.into(),
&transactor,
&dest,
reducible_balance,
keep_alive,
)?;
Ok(())
}
}

/// Implements [`AccountTouch`] trait.
Expand Down
44 changes: 44 additions & 0 deletions substrate/frame/assets/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,50 @@ fn transferring_to_blocked_account_should_not_work() {
});
}

#[test]
fn transfer_all_works_1() {
new_test_ext().execute_with(|| {
// setup
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 0, true, 100));
assert_ok!(Assets::mint(RuntimeOrigin::signed(0), 0, 1, 200));
assert_ok!(Assets::mint(RuntimeOrigin::signed(0), 0, 2, 100));
// transfer all and allow death
assert_ok!(Assets::transfer_all(Some(1).into(), 0, 2, false));
assert_eq!(Assets::balance(0, &1), 0);
assert_eq!(Assets::balance(0, &2), 300);
});
}

#[test]
fn transfer_all_works_2() {
new_test_ext().execute_with(|| {
// setup
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 0, true, 100));
assert_ok!(Assets::mint(RuntimeOrigin::signed(0), 0, 1, 200));
assert_ok!(Assets::mint(RuntimeOrigin::signed(0), 0, 2, 100));
// transfer all and allow death
assert_ok!(Assets::transfer_all(Some(1).into(), 0, 2, true));
assert_eq!(Assets::balance(0, &1), 100);
assert_eq!(Assets::balance(0, &2), 200);
});
}

// /// TODO: Cover this case once #3951 is merged
muharem marked this conversation as resolved.
Show resolved Hide resolved
// #[test]
// fn transfer_all_works_3() {
// new_test_ext().execute_with(|| {
// // setup
// assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 0, true, 100));
// assert_ok!(Assets::mint(RuntimeOrigin::signed(0), 0, 1, 200));
// assert_ok!(AssetFreezer::set_frozen(&TestId::Foo, 0, &1, 10));
// assert_ok!(Assets::mint(RuntimeOrigin::signed(0), 0, 2, 100));
// // transfer all and allow death w/ reserved
// assert_ok!(Assets::transfer_all(Some(1).into(), 0, 2, true));
// assert_eq!(Assets::balance(0, &1), 110);
// assert_eq!(Assets::balance(0, &2), 200);
// });
// }

#[test]
fn origin_guards_should_work() {
new_test_ext().execute_with(|| {
Expand Down
21 changes: 21 additions & 0 deletions substrate/frame/assets/src/weights.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading