-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix/migrate nft ownership #921
base: main
Are you sure you want to change the base?
Conversation
…ship # Conflicts: # pallet/nft/src/tests.rs # runtime/src/migrations/mod.rs
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 👍
Just need CI to pass.
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 👍
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. left some comments. also we might need some bench run for nft pallet now the storage structures are different.
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 check other affected pallets also need CI bench runs.
…ship # Conflicts: # pallet/common/src/test_utils.rs # pallet/nfi/src/mock.rs # pallet/nft/src/impls.rs # pallet/nft/src/lib.rs # pallet/nft/src/mock.rs # pallet/nft/src/types.rs # pallet/nft/src/weights.rs # pallet/xls20/src/mock.rs # runtime/src/lib.rs # runtime/src/weights/pallet_nft.rs
…ship # Conflicts: # pallet/nft/src/types.rs
// For simplicity, we will only migrate max 50 tokens from 1 account at a time | ||
if let Some(mut ownership) = old.owned_tokens.pop() { | ||
let mut serial_numbers = ownership.owned_serials.clone(); | ||
// take at max 10 from serial_numbers |
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.
// take at max MAX_TOKENS_PER_STEP from serial_numbers
} | ||
|
||
// Update OwnedTokens with the migrated serials | ||
let _ = pallet_nft::OwnedTokens::<T>::try_mutate( |
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 ignored the error in owned tokens update
if let Err(e) = pallet_nft::OwnedTokens::::try_mutate(
&ownership.owner,
collection_id,
|owned_serials| -> DispatchResult {
// ...
Ok(())
},
) {
log::error!(target: LOG_TARGET, "🦆 Error updating owned tokens: {:?}", e);
// Consider how to handle this error - maybe mark the migration as failed?
}
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 try_mutate will never error as we always return Ok(())
In both places where it could have errored, we are using force_push and truncate_from which do not return a Result. These are both safe as the BoundedVec upper bound will not change between the old and new datasets
so it is safe to ignore the result altogether
@@ -134,18 +135,20 @@ pub mod pallet { | |||
|
|||
// Map from a collection id to a collection's pending issuances | |||
#[pallet::storage] | |||
pub type PendingIssuances<T: Config> = StorageMap< | |||
pub type PendingIssuances<T: Config> = StorageNMap< |
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.
Interesing use of new storage data structure :+1
Description
Context & Background
Notes & Additional Information
Related Issues
Release Notes
Key Changes
Type of Change
API Changes
Storage Changes
Added
Changed
Removed
Storage Migrations
Added