search

rmrk-team / rmrk-substrate

[WIP] RMRK Substrate pallets
https://rmrk.app
Other
74 stars 36 forks source link

Accept NFT allows for NFT owner to accept NFT to another NFT that is not current owner #229

Closed HashWarlock closed 1 year ago

HashWarlock commented 1 year ago

Currently the accept_nft function has a parameter named new_owner this new_owner can be defined by the user that owns a pending NFT. So this opens up for the following example described in this excalidraw:

image

Since the lookup_root_owner already proves that the accept_nft sender owns the NFT & we do not require the need to define the new_owner since the new_owner will already possess the NFT in Storage. All that is needed is to mutate the pending field from true to false & we also do not require another call to uniques pallet's do_transfer function. Also, calling the do_transfer would cause the Storage between uniques and rmrk to be inconsistent.

I have also found that the Nfts field owner is never updated when sending an NFT to another NFT in the nft_send function. This causes the NftInfo owner field to be inconsistent with the Uniques owner() function for an NFT. This is not caught due to the current ownership checks only using the Uniques pallet functions & does not check if the Nfts owner field matches the value of Uniques owner(col_id, nft_id). The cause of this is due to this change here where updating the owner field was commented out. https://github.com/rmrk-team/rmrk-substrate/blob/6485ffde093c7e1def7a177285ed61c4b17cc317/pallets/rmrk-core/src/functions.rs#L621

HashWarlock commented 1 year ago

When un-comment the code for updating Nfts storage, tests start to fail.

ilionic commented 1 year ago

https://github.com/rmrk-team/rmrk-substrate/pull/230