-
Notifications
You must be signed in to change notification settings - Fork 665
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
Implement Inactive balance tracking in Assets pallet #1747
base: master
Are you sure you want to change the base?
Conversation
// 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)] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just define a constant vector in the migrations file the same as it would be defined in the runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also how do we calculate what this vec should be when we go to run this for real?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KiChjang does xcm need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joepetrowski. @muraca let's leave it in just in case there's a use case outside of AH.
dd29ead
to
57a0787
Compare
ea8203b
to
2f1cfe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I tested with
try-runtime \
--runtime existing \
create-snapshot block.snap \
--uri wss://rococo-asset-hub-rpc.polkadot.io:443 \
--at 0xed9b3c06621f649106eef7c6e6fd53ffceca2cfba0cf3039993b7ce50626ca4a
cargo build -p asset-hub-rococo-runtime --features try-runtime --release
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: P.S. in the patch, there's an upgrade to uniques' migration to v1, which was not using the |
Shouldn't the current storage versions be set to v2?
Not sure what you mean here, could you link to some code? |
@liamaharon the changes I mentioned are in the patch file I linked: To answer your question
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. |
This is due to poorly written |
started working on migration Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
@muraca could you merge master? @liamaharon could you give this another round of reviews? |
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
Signed-off-by: Matteo Muraca <mmuraca247@gmail.com>
The CI pipeline was cancelled due to failure one of the required jobs. |
// Inactive amount can't exceed supply | ||
asset.inactive = asset.inactive.saturating_add(amount).max(asset.supply); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this be a defensive check if the runtime is trying to set the inactive amt > total supply, probably indicates a bug?
|
||
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be defensive in case > total inactive amount is attempted to be reactivated?
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The current storage version. | |
/// The in-code storage version. |
} | ||
|
||
pub struct MigrateToV1<T, I>(core::marker::PhantomData<(T, I)>); | ||
impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for MigrateToV1<T, I> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use VersionedMigration
for this and avoid doing version checks/sets yourself
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()) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 theAsset
struct and implementing the methods accordingly.closes #228
Polkadot address: 12poSUQPtcF1HUPQGY3zZu2P8emuW9YnsPduA4XG3oCEfJVp