Implement Inactive balance tracking in Assets pallet #1747

muraca · 2023-09-28T20:11:26Z

The trait fungibles::Unbalanced provides methods to increase and decrease active issuance.
This PR addresses the lack of this feature in Assets, by introducing a new field inactive in the Asset struct and implementing the methods accordingly.

closes #228

Polkadot address: 12poSUQPtcF1HUPQGY3zZu2P8emuW9YnsPduA4XG3oCEfJVp

substrate/frame/assets/src/impl_fungibles.rs

substrate/frame/assets/src/migration/mod.rs

substrate/frame/assets/src/migration/v1.rs

substrate/frame/assets/src/migration/v2.rs

liamaharon · 2023-10-01T11:55:24Z

substrate/frame/assets/src/tests.rs

+		// Define a constant Vec for migration.
+		// This is a workaround for the fact that we can't implement Get for Vec.
+		struct ConstVecForMigrationToV2;
+		impl Get<Vec<(u32, u64)>> for ConstVecForMigrationToV2 {
+			fn get() -> Vec<(u32, u64)> {
+				// AssetId 1, AccountId 2 will be inactive.
+				vec![(1, 2)]
+			}
+		}


Can you just define a constant vector in the migrations file the same as it would be defined in the runtime?

also how do we calculate what this vec should be when we go to run this for real?

This pattern is not something I came up with, rather a solution I found in balances, introduced by @gavofyork in Substrate PR 12832, and used again later in society, introduced in Substrate PR 11324 - not sure on which one was written first, since society was merged later but was opened before.

In the latter, I see the migration has an internal method from_original, which is the one called in the test instead of building a struct as I did. Should I proceed with the same approach?

I can try to search how this pattern is used in runtimes if needed.

I may be missing something but I'm a bit confused actually why any accounts will need to be seeded with inactive balance when we merge this PR.

Isn't the only way their bal can be made inactive is by calling deactivate, which we are introducing in this PR?

When introducing the inactive balance feature, one might want to populate the state during the migration, by passing a list of accounts which balances are to be considered inactive.

On pallet balances I found:

// Generally this will be used for the XCM teleport checking account.

@KiChjang does xcm need this?

XCM needs this but I don't think Assets does, at least to the extent the pallet is used in Asset Hub. AH doesn't trust any other chains as teleporters for its locally created assets, and for foreign assets it does not maintain a checking account (it expects the asset creator to).

Local Assets uses checking,

but no teleporters

Foreign Assets no checking

Thanks @joepetrowski. @muraca let's leave it in just in case there's a use case outside of AH.

substrate/frame/assets/src/migration/v2.rs

juangirini

LGTM

Please bear in mind that this might not be merged before MBM is ready #1781.
In which case you will need to adapt the migration, I will approve it afterwards.

Also please confirm that you have tested this with try-runtime

muraca · 2023-10-28T08:59:15Z

I tested with try-runtime on Asset Hub Rococo and it works as expected.
To reproduce:

Create the snapshot at block 3_075_000

try-runtime \
  --runtime existing \
  create-snapshot block.snap \
  --uri wss://rococo-asset-hub-rpc.polkadot.io:443 \
  --at 0xed9b3c06621f649106eef7c6e6fd53ffceca2cfba0cf3039993b7ce50626ca4a

Apply try-runtime-assets-v2.patch, which includes the missing runtime upgrades for other pallets, and the v1 to v2 runtime upgrade for pallet assets. That the pallet assets storage version is moved back to 1 to let the first migration go smoothly.
Build the new runtime.

cargo build -p asset-hub-rococo-runtime --features try-runtime --release

Run the runtime upgrade.

try-runtime \
  --runtime target/release/wbuild/asset-hub-rococo-runtime/asset_hub_rococo_runtime.wasm \
  on-runtime-upgrade \
  snap -p block.snap

The expected output is the following:

As you can see, migration v1 to v2 goes smoothly. The error is wanted because the pallet storage version to v1 otherwise would fail, and we would not be able to perform the v1 to v2 migration.

P.S. in the patch, there's an upgrade to uniques' migration to v1, which was not using the OnRuntimeUpgrade traits. If you think it is worth to be pushed, I can open another PR. @liamaharon

liamaharon · 2023-10-30T07:11:09Z

The error is wanted because the pallet storage version to v1 otherwise would fail, and we would not be able to perform the v1 to v2 migration.

Shouldn't the current storage versions be set to v2?

P.S. in the patch, there's an upgrade to uniques' migration to v1, which was not using the OnRuntimeUpgrade traits. If you think it is worth to be pushed, I can open another PR. @liamaharon

Not sure what you mean here, could you link to some code?

muraca · 2023-10-30T07:40:47Z

@liamaharon the changes I mentioned are in the patch file I linked:

Apply try-runtime-assets-v2.patch

To answer your question

Shouldn't the current storage versions be set to v2?

The actual on-chain storage version is currently 0, and migration to v2 requires version 1. For this reason, we need to perform a migration from 0 to 1 first.
If we set the current storage version to 2, the first migration will fail, so in the patch I created for this test we set it to 1.
Then when we perform the migration to v2, it works as expected, but the final check made by some procedural macro will fail and produce the output you see in the screenshot.

liamaharon · 2023-10-30T07:49:33Z

If we set the current storage version to 2, the first migration will fail

This is due to poorly written pre/post_upgrade checks in v0 -> v1 migration. you should remove the current version checks.

started working on migration Signed-off-by: muraca <mmuraca247@gmail.com>

Signed-off-by: muraca <mmuraca247@gmail.com>

bkchr · 2024-02-05T11:33:25Z

@muraca could you merge master?

@liamaharon could you give this another round of reviews?

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>

paritytech-cicd-pr · 2024-04-15T10:30:55Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5928578

liamaharon · 2024-04-29T05:51:08Z

substrate/frame/assets/src/impl_fungibles.rs

+				// Inactive amount can't exceed supply
+				asset.inactive = asset.inactive.saturating_add(amount).max(asset.supply);


Seems this be a defensive check if the runtime is trying to set the inactive amt > total supply, probably indicates a bug?

liamaharon · 2024-04-29T05:51:56Z

substrate/frame/assets/src/impl_fungibles.rs

+
+	fn reactivate(asset: Self::AssetId, amount: Self::Balance) {
+		Asset::<T, I>::mutate(&asset, |maybe_asset| match maybe_asset {
+			Some(ref mut asset) => asset.inactive.saturating_reduce(amount),


Should be defensive in case > total inactive amount is attempted to be reactivated?

liamaharon · 2024-04-29T05:52:08Z

substrate/frame/assets/src/lib.rs

@@ -218,8 +218,8 @@ pub mod pallet {
 	};
 	use frame_system::pallet_prelude::*;

-	/// The in-code storage version.
-	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+	/// The current storage version.


Suggested change

/// The current storage version.

/// The in-code storage version.

liamaharon · 2024-04-29T05:53:08Z

substrate/frame/assets/src/migration/v1.rs

+}
+
+pub struct MigrateToV1<T, I>(core::marker::PhantomData<(T, I)>);
+impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for MigrateToV1<T, I> {


Can use VersionedMigration for this and avoid doing version checks/sets yourself

liamaharon · 2024-04-29T05:56:13Z

substrate/frame/assets/src/migration/v1.rs

+			let mut translated = 0u64;
+			Asset::<T, I>::translate::<
+				old::AssetDetails<T::Balance, T::AccountId, DepositBalanceOf<T, I>>,
+				_,
+			>(|_key, old_value| {
+				translated.saturating_inc();
+				Some(old_value.migrate_to_v1())
+			});


Hmm is there a bound on the number of Assets that could exist for a runtime? Otherwise we may need to write this as an MBM.

Someone could potentially spam the chain with thousands of dummy Assets right before the migration is scheduled to take place, which would cause the migration block PoV size to be too large and brick the chain.

muraca requested review from a team September 28, 2023 20:11

muraca had a problem deploying to master September 29, 2023 07:32 — with GitHub Actions Failure

muraca had a problem deploying to master September 29, 2023 07:50 — with GitHub Actions Failure

muraca force-pushed the issue#228 branch from 7d4013b to b003c9f Compare September 30, 2023 07:24

liamaharon added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 1, 2023

liamaharon reviewed Oct 1, 2023

View reviewed changes

paritytech-ci requested a review from a team October 1, 2023 11:57

muraca force-pushed the issue#228 branch 4 times, most recently from dd29ead to 57a0787 Compare October 9, 2023 08:15

muraca requested a review from liamaharon October 9, 2023 08:15

muraca force-pushed the issue#228 branch 2 times, most recently from ea8203b to 2f1cfe7 Compare October 23, 2023 07:25

muraca force-pushed the issue#228 branch from 2f1cfe7 to be9cbe8 Compare October 24, 2023 19:46

juangirini reviewed Oct 26, 2023

View reviewed changes

substrate/frame/assets/src/migration/v2.rs Show resolved Hide resolved

paritytech-ci requested a review from a team October 26, 2023 07:21

muraca requested a review from juangirini October 26, 2023 08:54

juangirini reviewed Oct 27, 2023

View reviewed changes

paritytech-ci requested a review from a team October 27, 2023 09:44

muraca added 5 commits November 10, 2023 15:30

implemented deactivate and reactivate,

58a50da

started working on migration Signed-off-by: muraca <mmuraca247@gmail.com>

fixed migrations

1167d68

Signed-off-by: muraca <mmuraca247@gmail.com>

inactive computation in migration

ebdb21e

Signed-off-by: muraca <mmuraca247@gmail.com>

feature try-runtime for migration test

8af1f39

Signed-off-by: muraca <mmuraca247@gmail.com>

fixed leak crate-private type

632d1a8

Signed-off-by: muraca <mmuraca247@gmail.com>

muraca added 8 commits November 10, 2023 15:30

import Vec for migration v1

57ec30a

Signed-off-by: muraca <mmuraca247@gmail.com>

addressed review comments

180d91b

Signed-off-by: muraca <mmuraca247@gmail.com>

assert to ensure in migration post-upgrade

ea52ad0

Signed-off-by: muraca <mmuraca247@gmail.com>

migration v1 doesn't depend on v2 anymore

1b61d66

Signed-off-by: muraca <mmuraca247@gmail.com>

migration v1 now supports pallet instance

4550656

Signed-off-by: muraca <mmuraca247@gmail.com>

bump pallet version to v2

f69f85a

Signed-off-by: muraca <mmuraca247@gmail.com>

migration to v1 only checks on chain storage versions

ecea308

Signed-off-by: muraca <mmuraca247@gmail.com>

renamed VersionCheckedMigrateToV2 to MigrateToV2 paritytech#2264

7af51dd

Signed-off-by: muraca <mmuraca247@gmail.com>

muraca force-pushed the issue#228 branch from e1e2b15 to 7af51dd Compare November 10, 2023 14:32

paritytech-review-bot bot requested a review from a team November 10, 2023 14:33

Merge branch 'master' into issue#228

b0d6691

muraca requested a review from a team as a code owner February 5, 2024 18:24

muraca and others added 7 commits March 7, 2024 10:18

Merge branch 'master' into issue#228

889a898

inactive amount can't exceed supply

91ac946

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>

prdoc for paritytech#1747

c2b3f2b

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>

fixed fmt & clippy

6c7a197

Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>

Merge branch 'master' into issue#228

6a436d2

Merge branch 'master' into issue#228

401a229

Merge branch 'master' into issue#228

ce71499

liamaharon reviewed Apr 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Inactive balance tracking in Assets pallet #1747

Implement Inactive balance tracking in Assets pallet #1747

muraca commented Sep 28, 2023 •

edited

Loading

liamaharon Oct 1, 2023

liamaharon Oct 1, 2023

muraca Oct 2, 2023

liamaharon Oct 2, 2023

muraca Oct 2, 2023

liamaharon Oct 16, 2023

joepetrowski Oct 16, 2023

liamaharon Oct 16, 2023 •

edited

Loading

juangirini left a comment •

edited

Loading

muraca commented Oct 28, 2023 •

edited

Loading

liamaharon commented Oct 30, 2023

muraca commented Oct 30, 2023

liamaharon commented Oct 30, 2023 •

edited

Loading

bkchr commented Feb 5, 2024

paritytech-cicd-pr commented Apr 15, 2024

liamaharon Apr 29, 2024

liamaharon Apr 29, 2024

liamaharon Apr 29, 2024

liamaharon Apr 29, 2024

liamaharon Apr 29, 2024

		// Inactive amount can't exceed supply
		asset.inactive = asset.inactive.saturating_add(amount).max(asset.supply);

	/// The current storage version.
	/// The in-code storage version.

Implement Inactive balance tracking in Assets pallet #1747

Are you sure you want to change the base?

Implement Inactive balance tracking in Assets pallet #1747

Conversation

muraca commented Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamaharon Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

juangirini left a comment • edited Loading

Choose a reason for hiding this comment

muraca commented Oct 28, 2023 • edited Loading

liamaharon commented Oct 30, 2023

muraca commented Oct 30, 2023

liamaharon commented Oct 30, 2023 • edited Loading

bkchr commented Feb 5, 2024

paritytech-cicd-pr commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muraca commented Sep 28, 2023 •

edited

Loading

liamaharon Oct 16, 2023 •

edited

Loading

juangirini left a comment •

edited

Loading

muraca commented Oct 28, 2023 •

edited

Loading

liamaharon commented Oct 30, 2023 •

edited

Loading