-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[pallet-assets] add ForeignAssetIdExtractor to assets precompile #10869
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
base: master
Are you sure you want to change the base?
Conversation
This reverts commit dc1b49d.
|
/cmd prdoc --audience runtime_dev --bump minor |
…time_dev --bump minor'
…ch/polkadot-sdk into rve/8659-assets-precompile
|
Another general comment is that it needs more tests (easy to use Claude for writing tests fast). |
27ff790 to
06b9f65
Compare
done |
|
/cmd bench --runtime dev --pallet pallet_assets_precompiles |
|
Command "bench --runtime dev --pallet pallet_assets_precompiles" has started 🚀 See logs here |
|
Command "bench --runtime dev --pallet pallet_assets_precompiles" has failed ❌! See logs here |
|
/cmd bench --runtime dev --pallet pallet_assets_precompiles |
|
Command "bench --runtime dev --pallet pallet_assets_precompiles" has started 🚀 See logs here |
…--pallet pallet_assets_precompiles'
|
Command "bench --runtime dev --pallet pallet_assets_precompiles" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
| /// | ||
| /// # Safety | ||
| /// | ||
| /// - Idempotent: Skips assets that already have mappings |
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 mapping didn't exist before the migration, so I don't see why we need to specify 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.
Maybe I dont understand the way migrations work, but this is what I thought:
The migration runs on runtime upgrades. Which means this migration will run several time in the future. So running it twice on the same set of foreign assets should not change anything. So it is required to be idempotent.
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.
no once they are passed they don't re-run
see
polkadot-sdk/substrate/frame/migrations/src/lib.rs
Lines 52 to 61 in 02f16b7
| //! ### Design Goals | |
| //! | |
| //! 1. Must automatically execute migrations over multiple blocks. | |
| //! 2. Must expose information about whether migrations are ongoing. | |
| //! 3. Must respect pessimistic weight bounds of migrations. | |
| //! 4. Must execute migrations in order. Skipping is not allowed; migrations are run on a | |
| //! all-or-nothing basis. | |
| //! 5. Must prevent re-execution of past migrations. | |
| //! 6. Must provide transactional storage semantics for migrations. | |
| //! 7. Must guarantee progress. |
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.
done
| } | ||
| } | ||
|
|
||
| /// Benchmark the case when there are no more assets to iterate. |
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 be removed as well, it's fine to over-estimate for the last step
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.
done
| pub trait Config: frame_system::Config { | ||
| /// The foreign asset ID type. This must match the `AssetId` type used by the | ||
| /// `pallet_assets` instance for foreign assets. | ||
| type ForeignAssetId: Member + Parameter + Clone + MaybeSerializeDeserialize + MaxEncodedLen; |
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 really think we should simplify where we can and not use an associated type here where we can hardcode it to Location, will test what it looks like without it
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.
@franciscoaguirre what do you think ? or maybe it's annoying because we need to update the Location version everytime you bump it here
580 impl pallet_assets::Config<ForeignAssetsInstance> for Runtime {
1 ▎ type RuntimeEvent = RuntimeEvent;
2 ▎ type Balance = Balance;
3 ▎ type AssetId = xcm::v5::Location;
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.
tasked Claude with it here #10945
also @franciscoaguirre Location encoding is stable ? if AH use v6 in a couple of month would an existing Location encode to the same value (I assume yes)
| /// Mapping an asset index (derived from the precompile address) to a `ForeignAssetId`. | ||
| #[pallet::storage] | ||
| pub type AssetIndexToForeignAssetId<T: Config> = | ||
| StorageMap<_, Blake2_128Concat, u32, T::ForeignAssetId, OptionQuery>; |
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 would use Identity here instead of Blake2_128Concat since the system control the keys (they are incrementing by one for each insert )
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.
done
| #[pallet::pallet] | ||
| pub struct Pallet<T>(_); | ||
|
|
||
| /// The next available asset index for foreign assets. |
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 don't think we need anything pub in this pallet, all we need is the AssetsCallback and the ForeignAssetIdExtractor that I will keep in that module to keep everything private
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.
Was able to remove a few pubs
|
|
||
| const PRECOMPILE_MAPPINGS_MIGRATION_ID: &[u8; 32] = b"foreign-asset-precompile-mapping"; | ||
|
|
||
| /// Progressive states of the precompile mappings migration. |
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 don't think you need that
you can just do
type Cursor = <T as pallet_assets::Config>::AssetId;
or Location directly if we stop using generic
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.
Not sure what you mean here, can you elaborate?
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.
we don't need the MigrationState we can just use A
| pub use migration::{MigrateForeignAssetPrecompileMappings, MigrationState}; | ||
| pub use weights::{SubstrateWeight, WeightInfo as MigrationWeightInfo}; |
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.
not sure we need all thes extra pub if you have already made the module pub
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.
removed
| /// Mapping a `ForeignAssetId` to an asset index (used for deriving precompile addresses). | ||
| #[pallet::storage] | ||
| pub type ForeignAssetIdToAssetIndex<T: Config> = | ||
| StorageMap<_, Identity, T::ForeignAssetId, u32, OptionQuery>; |
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 one should probably still need a hash though since you could craft Location that make the trie unbalanced
| // Minimum execution time: 8_247_000 picoseconds. | ||
| Weight::from_parts(8_630_000, 0) | ||
| } | ||
| fn migrate_asset_step_skip() -> Weight { |
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.
re-run the bot to get these methods removed
pgherveou
left a comment
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.
After addressing last comments.
Would also prefer to not have this Generic since we know that this is targeting Location
|
/cmd bench --runtime dev --pallet pallet_assets-precompiles |
|
Command "bench --runtime dev --pallet pallet_assets-precompiles" has started 🚀 See logs here |
|
Command "bench --runtime dev --pallet pallet_assets-precompiles" has failed ❌! See logs here |
fixes #8659
Adds ForeignAssetIdExtractor which converts a u32 asset id to an XCM Location type.