Asset Conversion: Pool Account ID derivation with additional Pallet ID seed #3250

muharem · 2024-02-08T03:38:14Z

Introduce PalletId as an additional seed parameter for pool's account id derivation.

The PR also introduces the pallet_asset_conversion_ops pallet with a call to migrate a given pool to thew new account. Additionally fungibles::lifetime::ResetTeam and fungible::lifetime::Refund traits, to facilitate the migration of pools.

substrate/frame/asset-conversion/src/migration.rs

…rsion-pool-account

substrate/frame/asset-conversion/src/migration.rs

This reverts commit 03363c8.

…rsion-pool-account

paritytech-cicd-pr · 2024-04-15T13:18:17Z

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/5930659

This reverts commit 9247584.

This reverts commit 6915afe.

github-actions · 2024-04-15T15:16:45Z

Review required! Latest push from author must always be reviewed

franciscoaguirre · 2024-04-16T09:51:09Z

substrate/bin/node/runtime/src/lib.rs

@@ -2474,6 +2496,9 @@ mod runtime {

 	#[runtime::pallet_index(78)]
 	pub type PalletExampleMbms = pallet_example_mbm;
+
+	#[runtime::pallet_index(79)]
+	pub type AssetConversionMigration = pallet_asset_conversion_ops;


Do you need to add the migration to this runtime? It's not live, is it?

This runtime serves as an example of pallets integration. I think it make sense to have here.

Not required, I'm guessing it was probably added for pallet benchmarks.

true, we also need it for the benchmarks.

franciscoaguirre · 2024-04-16T09:51:25Z

prdoc/pr_3250.prdoc

+
+doc:
+  - audience: Runtime Dev
+    description: |


Shouldn't you mention the migration?

Yes, I can improve this

franciscoaguirre · 2024-04-16T09:59:10Z

substrate/frame/asset-conversion/ops/src/lib.rs

+			if let Some((depositor, deposit)) =
+				T::AssetsRefund::deposit_held(asset1.clone(), prior_account.clone())
+			{
+				T::DepositAsset::mint_into(&depositor, deposit + deposit_asset_ed)?;


Why do you mint deposit + deposit_asset_ed? Doesn't the depositor already have the deposit?

No, depositor might now have it. There is a comment above.

franciscoaguirre · 2024-04-16T10:02:23Z

substrate/frame/asset-conversion/ops/src/mock.rs

+  }
+);
+
+#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]


You can remove many types given that you're deriving the defaults. Take a look at:

polkadot-sdk/substrate/frame/balances/src/tests/mod.rs

Line 93 in 4b5c3fd

#[derive_impl(frame_system::config_preludes::TestDefaultConfig)]

franciscoaguirre · 2024-04-16T10:04:42Z

substrate/frame/asset-conversion/ops/src/mock.rs

+	type MaxConsumers = ConstU32<16>;
+}
+
+impl pallet_balances::Config for Test {


If you use derive_impl here, it means less maintenance when we add new config items to these traits.

franciscoaguirre · 2024-04-16T10:04:49Z

substrate/frame/asset-conversion/ops/src/mock.rs

+	type RuntimeFreezeReason = ();
+}
+
+impl pallet_assets::Config<Instance1> for Test {


franciscoaguirre · 2024-04-16T10:11:31Z

substrate/frame/assets/src/functions.rs

@@ -1013,4 +1018,28 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			})
 			.collect::<Vec<_>>()
 	}
+
+	/// Reset the team for the asset with the given `id`.


What's the "team" of an asset?

is not it clear from arguments? this also how we name it in pallet's call

muharem · 2024-04-16T13:14:13Z

@franciscoaguirre all the comments addressed, can you have a look again please

liamaharon · 2024-04-16T12:58:43Z

substrate/bin/node/runtime/src/lib.rs

@@ -2474,6 +2496,9 @@ mod runtime {

 	#[runtime::pallet_index(78)]
 	pub type PalletExampleMbms = pallet_example_mbm;
+
+	#[runtime::pallet_index(79)]
+	pub type AssetConversionMigration = pallet_asset_conversion_ops;


Not required, I'm guessing it was probably added for pallet benchmarks.

substrate/frame/asset-conversion/ops/Cargo.toml

substrate/frame/asset-conversion/ops/src/lib.rs

liamaharon · 2024-04-16T13:30:02Z

substrate/frame/support/src/traits/tokens/fungibles/lifetime.rs

+/// Trait for resetting the team configuration of an existing fungible asset.
+pub trait ResetTeam<AccountId>: Inspect<AccountId> {
+	/// Reset the team for the asset with the given `id`.
+	///
+	/// ### Parameters
+	/// - `id`: The identifier of the asset for which the team is being reset.
+	/// - `owner`: The new `owner` account for the asset.
+	/// - `admin`: The new `admin` account for the asset.
+	/// - `issuer`: The new `issuer` account for the asset.
+	/// - `freezer`: The new `freezer` account for the asset.
+	fn reset_team(
+		id: Self::AssetId,
+		owner: AccountId,
+		admin: AccountId,
+		issuer: AccountId,
+		freezer: AccountId,
+	) -> DispatchResult;
+}


Not sure this belongs in lifetime.rs.. Maybe roles.rs?

substrate/frame/support/src/traits/tokens/fungibles/lifetime.rs

…D seed (#3250) Introduce `PalletId` as an additional seed parameter for pool's account id derivation. The PR also introduces the `pallet_asset_conversion_ops` pallet with a call to migrate a given pool to thew new account. Additionally `fungibles::lifetime::ResetTeam` and `fungible::lifetime::Refund` traits, to facilitate the migration of pools. --------- Co-authored-by: command-bot <>

muharem added 8 commits February 2, 2024 12:32

account id converter with pallet id salt

21d1df4

migration for pool account

ebbad9f

refund trait and impl

1e2a925

drop super trait for reset team trait

a45b298

account id types

d8f7b4e

fungibles

1d30b89

migration

cb3b79d

setup migration for asset hubs

9f2c71a

muharem added the T2-pallets This PR/Issue is related to a particular pallet. label Feb 8, 2024

muharem requested a review from a team as a code owner February 8, 2024 03:38

muharem commented Feb 8, 2024

View reviewed changes

substrate/frame/asset-conversion/src/migration.rs Outdated Show resolved Hide resolved

muharem added 7 commits February 8, 2024 11:49

doc fixes

f9a0edc

Merge remote-tracking branch 'origin/master' into muharem-asset-conve…

61245e3

…rsion-pool-account

prdoc

6fce4bf

try runtime imports

2321031

change account id format for logs

3d3e5b3

import

ff19ff1

rename deposit fn to deposit_held

4dd4334

muharem commented Feb 8, 2024

View reviewed changes

substrate/frame/asset-conversion/src/migration.rs Outdated Show resolved Hide resolved

muharem added 11 commits February 9, 2024 13:42

migration adjustments

3537f90

migration asserts reduce to zero instead success

0eee02e

correct doc

11e3004

balance for destroying asset cannot be increased/decreased

03363c8

migration

58f0a66

make migration idempotent

d14b412

Merge branch 'master' into muharem-asset-conversion-pool-account

10713ad

asset conversion ops pallet

979ae0c

Revert "balance for destroying asset cannot be increased/decreased"

3be3789

This reverts commit 03363c8.

Merge remote-tracking branch 'origin/master' into muharem-asset-conve…

f3ea112

…rsion-pool-account

todos and unused import

086198c

muharem added 3 commits April 15, 2024 14:33

update reset team trait

8ce6f7c

update refund trait

9247584

remove unused imports

6915afe

muharem added 3 commits April 15, 2024 17:15

Revert "update refund trait"

7398696

This reverts commit 9247584.

Revert "remove unused imports"

7bfd1ff

This reverts commit 6915afe.

update doc

7a579a9

github-actions bot requested review from georgepisaltu and NachoPal April 15, 2024 15:16

georgepisaltu approved these changes Apr 15, 2024

View reviewed changes

NachoPal approved these changes Apr 16, 2024

View reviewed changes

franciscoaguirre reviewed Apr 16, 2024

View reviewed changes

update pr doc and use default configs for pallets in test env

fba27bf

paritytech-review-bot bot requested a review from a team April 16, 2024 13:13

liamaharon reviewed Apr 16, 2024

View reviewed changes

update docs

82b0aca

paritytech-review-bot bot requested a review from a team April 16, 2024 14:30

liamaharon approved these changes Apr 16, 2024

View reviewed changes

move reset team trait to roles module

496e2d7

muharem added this pull request to the merge queue Apr 17, 2024

Merged via the queue into master with commit 4e10d3b Apr 17, 2024
131 of 136 checks passed

muharem deleted the muharem-asset-conversion-pool-account branch April 17, 2024 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset Conversion: Pool Account ID derivation with additional Pallet ID seed #3250

Asset Conversion: Pool Account ID derivation with additional Pallet ID seed #3250

muharem commented Feb 8, 2024 •

edited

Loading

paritytech-cicd-pr commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

franciscoaguirre Apr 16, 2024

muharem Apr 16, 2024

liamaharon Apr 16, 2024

muharem Apr 16, 2024

franciscoaguirre Apr 16, 2024

muharem Apr 16, 2024

muharem Apr 16, 2024

franciscoaguirre Apr 16, 2024

muharem Apr 16, 2024

franciscoaguirre Apr 16, 2024

muharem Apr 16, 2024

franciscoaguirre Apr 16, 2024

muharem Apr 16, 2024

franciscoaguirre Apr 16, 2024

muharem Apr 16, 2024

franciscoaguirre Apr 16, 2024

muharem Apr 16, 2024

muharem commented Apr 16, 2024

liamaharon Apr 16, 2024

liamaharon Apr 16, 2024

Asset Conversion: Pool Account ID derivation with additional Pallet ID seed #3250

Asset Conversion: Pool Account ID derivation with additional Pallet ID seed #3250

Conversation

muharem commented Feb 8, 2024 • edited Loading

paritytech-cicd-pr commented Apr 15, 2024

github-actions bot 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

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

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

muharem commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem commented Feb 8, 2024 •

edited

Loading