-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add ClassAccount storage to unique pallet #9940
Conversation
this requires migration |
Not against this going in but it will need the migration. |
Migration code was missing. Also we would need to run benchmark to update the weights. Will add the storage version and migration logic to the pallet and will run the benchmark to fix. |
2021-11-02 16:26:04 |
if on_chain_storage_version < 1 { | ||
let mut count = 0; | ||
for (class, detail) in Class::<T, I>::iter() { | ||
ClassAccount::<T, I>::insert(&detail.owner, &class, ()); |
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.
Note that this migration might be very big and cause issues on a parachain.
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.
What is the best practice to bound the migrations. do we just set an upper limit?! Currently there are only 5-6 classes on statemine that this will run for. I assume any other parachain which launches after this will start from storage version 1, hence won't run this 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.
Looks overall good to me. To test the migration properly with try-runtime you need to integrate it in cumulus, but I think the logic of the migration is pretty trivial. I am just worried about the size of migration.
run this with |
I think there are very few classes actually, so this should be fine? |
Co-authored-by: Kian Paimani <[email protected]>
currently there are 7 classes. so it will be a light migration. |
/benchmark runtime pallet pallet_uniques |
Benchmark Runtime Pallet for branch "hamidra/class-account-storage" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_uniques --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/uniques/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
… hamidra/class-account-storage
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_uniques --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/uniques/src/weights.rs --template=./.maintain/frame-weight-template.hbs
Now that try-runtime is added to cumulus I also tried the runtime upgrade on cumulus and it was successful against the current statemine state: 2021-12-06 13:17:07 ✅ no migration for Proxy |
bot merge |
* add ClassAccount to uniques storage * add tests for Class and ClassAccount storage * fix format * fix description * add migration * remove extra iteration * Update frame/uniques/src/migration.rs Co-authored-by: Kian Paimani <[email protected]> * cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_uniques --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/uniques/src/weights.rs --template=./.maintain/frame-weight-template.hbs * fix format Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: Parity Bot <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]>
* add ClassAccount to uniques storage * add tests for Class and ClassAccount storage * fix format * fix description * add migration * remove extra iteration * Update frame/uniques/src/migration.rs Co-authored-by: Kian Paimani <[email protected]> * cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_uniques --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/uniques/src/weights.rs --template=./.maintain/frame-weight-template.hbs * fix format Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: Parity Bot <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]>
The changes addresses #9777 by adding a storage to unique pallet to keep classes that are owned by an account. The current Account storage keeps the instances that are owned by an account but currently the only way to query the classes that an account owns is by iterating through all asset classes. Having the account classes indexed seems to be very useful for NFT UXs which the user creates a collection of NFTs, and then needs to make changes to those collections in future.
[fixes #9777 ]