Skip to content
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

Lazy ownership tree based on pallet-unique's owner #31

Merged
merged 13 commits into from
Jan 11, 2022

Conversation

HashWarlock
Copy link
Contributor

@HashWarlock HashWarlock commented Dec 22, 2021

Targets

  • Update mint to use uniques pallet's mint
  • Ownership check
  • Create virtual account for NFT
  • Update transfer to check ownership & create virtual account if transferred to an NFT (Do we need to handle pending logic, yet?)
  • Update logic to send_nft to utilize lazy ownership tree
  • Handle children logic after do_transfer
  • Rebase and fix conflicts

Procedure

  • User mints an NFT with pallet_unique::Pallet::<T>::mint(origin, owner, col, nft)?;
  • User transfer(origin: OriginFor<T>, col: CollectionId, nft: NftId, dest: AccountIdOrNftTuple) NFT to another NFT
  • Check ownership with lookup_root_owner(col, nft)
    • match dest_account to AccountId(id) if Account owner
    • else if match on NFTTuple(c,n) then call lookup_root_owner(c, n) & if no circle detection triggered then convert to virtual account with nft_to_account_id(c, n)
  • check children
    • If origin does transfer then auto-accept NFT & set pending === false
    • else set NFT property pending === true
  • Emit Event NFTSent

RMRK Specs

HashWarlock and others added 5 commits December 22, 2021 13:13
…r burning an NFT from a non root_owner to trigger expected failure
…orrect bounds to allow for Collection ID & NFT ID to be mapped to Class ID & Instance ID of Uniques pallet
…ns to generate virtual accounts & retrieve collection_id, nft_id. Add CircleDetected error when sending nft to self nft and add test
@HashWarlock
Copy link
Contributor Author

Updates now allow for creating virtual accounts for NFTs while also decoding the AccountId to determine if NFT owner is an actual account vs a NFT's virtual account. Enabled to call uniques do_transfer. More detail info by @h4x3rotab on issue #27.

@HashWarlock HashWarlock marked this pull request as ready for review January 3, 2022 15:58
pallets/rmrk-core/src/functions.rs Outdated Show resolved Hide resolved
pub fn lookup_root_owner(collection_id: CollectionId, nft_id: NftId) -> Result<(T::AccountId, (CollectionId, NftId)), Error<T>> {
let parent =
pallet_uniques::Pallet::<T>::owner(collection_id, nft_id);
// Check if parent returns None which indicates the NFT is not available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the cases a NFT can have no parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had thought of a couple cases, but the second case would be if we did the "rootless" children and tried to perform a lookup_root_owner

  1. NFT is already burned (Added a test for this one)
  2. A case if an ancestor NFT were burned leaving the

pallets/rmrk-core/src/functions.rs Show resolved Hide resolved
pallets/rmrk-core/src/lib.rs Outdated Show resolved Hide resolved
pallets/rmrk-core/src/lib.rs Outdated Show resolved Hide resolved
pallets/rmrk-core/src/lib.rs Outdated Show resolved Hide resolved
pallets/rmrk-core/src/lib.rs Show resolved Hide resolved
…le of collection_id & nft_id as key. Add additional tests to ensure functionality works as expected.
pallets/rmrk-core/src/functions.rs Outdated Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Outdated Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Outdated Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Outdated Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Show resolved Hide resolved
pallets/uniques/src/functions.rs Show resolved Hide resolved
pallets/rmrk-core/src/lib.rs Show resolved Hide resolved
@ilionic ilionic merged commit b10240c into rmrk-team:main Jan 11, 2022
@ilionic ilionic mentioned this pull request Jan 11, 2022
pallets/rmrk-core/src/functions.rs Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Show resolved Hide resolved
pallets/rmrk-core/src/functions.rs Show resolved Hide resolved
}
Ok(())
}

pub fn recursive_burn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callers must be transactional to avoid stop in the middle problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for recursive_burn or for is_x_descendent_of_y?

pallets/rmrk-core/src/lib.rs Show resolved Hide resolved
pallets/rmrk-core/src/lib.rs Show resolved Hide resolved
// Handle Children StorageMap for NFTs
let current_cid_nid = Pallet::<T>::decode_nft_account_id::<T::AccountId>(current_owner.clone());
if current_cid_nid.is_some() {
// remove child from parent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the new owner is not a NFT? Should we still keep tracking the children? It may be on very large for a big player

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we need to change the Children StorageMap or would this be tracked in NftsByOwner StorageMap? Right now children are tracked for NFTs that own NFTs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being tracked in Issue #48

pallets/rmrk-core/src/lib.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants