search

rmrk-team / rmrk-substrate

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

Mutable properties #269

Closed arrudagates closed 11 months ago

arrudagates commented 1 year ago

This PR implements the ability to allow the current owner of an NFT to mutate properties that were set as mutable by the collection issuer prior to being sent to the new owner. Closes #198

Please do let me know if this implementation should be done differently, I just wanted to give a head start to this feature and am flexible to refactoring this PR entirely.

Regarding the arguments to the set_property call including the mutable field, this is handled by ignoring the value of that field if the user is not the collection issuer (which means they can't modify the mutability of the property), but I believe it would be a lot cleaner if we broke it down into 2 distinct calls: set_property and set_property_mutability, the former would be reverted to how it's arguments were previously, and the latter would be responsible for changing the value of the mutable field by the collection issuer (if they're also the current owner of the NFT).

Szegoo commented 1 year ago

I believe it would be a lot cleaner if we broke it down into 2 distinct calls: set_property and set_property_mutability

Yes, this would make sense, but I don't think it is bad the way it is now. I am interested to hear the opinions of others here.

HashWarlock commented 1 year ago

What would migration for storage look like? This will have a big impact for our PhalaWorld & Stakepool V2 implementation, so we need to prepare for this.

We should also take a look at how Attributes (Uniques v2 NFTs pallet) handles the ability to edit an Attribute for an Item since the future RMRK pallet will be ported to be tightly coupled with the Uniques v2 Pallet. We can see the implementation here https://github.com/paritytech/substrate/blob/master/frame/nfts/src/features/attributes.rs#L22 with the types settings here https://github.com/paritytech/substrate/blob/248fdf0d4b5e3758cfdadb283b5eca5f0731e466/frame/nfts/src/types.rs#L360

@h4x3rotab may have some input as well on how to best implement this for future compatibility.

h4x3rotab commented 1 year ago

Currently Khala has a blockchain storage of around 300MB. Among them, 200MB are RMRK NFT properties. Not sure if we migrate the full properties, we need to suspend the blockchain to do multi-bock migrations. If so, we would like to avoid it as much as possible, because a multi-block migration can cause tremendous chaos in our live parachain.

arrudagates commented 1 year ago

It is possible to separate property mutability into it's own new storage, which we can then just use as ValueQuery where default is false, but I'm not sure how that affects performance and if such a compromise is desirable.

On Wed, Feb 1, 2023, 04:04 h4x3rotab @.***> wrote:

Currently Khala has a blockchain storage of around 300MB. Among them, 200MB are RMRK NFT properties. Not sure if we migrate the full properties, we need to suspend the blockchain to do multi-bock migrations. If so, we would like to avoid it as much as possible, because a multi-block migration can cause tremendous chaos in our live parachain.

— Reply to this email directly, view it on GitHub https://github.com/rmrk-team/rmrk-substrate/pull/269#issuecomment-1411559087, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHGXNIWCZJWAJN2L5HQPHXTWVIDHVANCNFSM6AAAAAAUD7VAUE . You are receiving this because you authored the thread.Message ID: @.***>