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

Bubblegum: Updatable Metadata #1121

Conversation

samwise2
Copy link
Contributor

This PR adds a new instruction to Bubblegum to enable the Tree Delegate / Collection Authority of a compressed NFT to update its metadata (if mutable).

Implemented Solution

update_metadata takes in the MetadataArgs of the compressed NFT to be updated. It checks that these MetadataArgs set is_mutable. If the former MetadataArgs specified a verified collection, then update_metadata checks that a CollectionAuthority signed the tx. update_metadata then updates a few select fields in MetadataArgs and CPIs to account_compression via replace_leaf.

TODO (WAITING ON HIGH-LEVEL DESIGN APPROVAL BEFORE COMPLETING)

  • Small refactor to minimize duplicated code
  • Add tests

Important Design Discussion

The primary challenge with this instruction is dealing with tx size constraints. The most naïve solution would be for the instruction to take in both the old MetadataArgs and new MetadataArgs as arguments. The old MetadataArgs would enable us to check is_mutable, and the new MetadataArgs would contain the updated metadata. The issue with this is the max size of MetadataArgs is > 457 bytes. Two MetadataArgs = 2 * 457 = 914 bytes. With at least 10 accounts needed to implement the ix, we are at 10*32 + 914 = 1234 bytes, without any other arguments, signatures, or the proof required to replace_leaf.

There's a few different approaches that aim to reduce the size the ix requires:

  1. Factor metadata updates into two txs. In the first tx, force the user to create an account(s) storing the old and new MetadataArgs. In the second tx, we pass these accounts to update_metadata, and then destroy the accounts after the update is complete.

The issue with this approach is the need for 2 txs, the capital for temporary rent + overhead to do account cleanup.

  1. Change how data_hash is computed. Last summer we re-hashed MetadataArgs with seller_fee_basis_points for this exact reason. We could change data_hash to be h(h(metadata_args), seller_fee_basis_points, is_mutable). I think this would only break decompress_v1. If we changed redeem to take in seller_fee_basis_points and is_mutable, then this breaks nothing (I think), and update_metadata would only need one MetadataArgs argument (the new one).

This approach would not support metadata updates for existing collections. It would also make life challenging for marketplace smart contracts which may be relying on data_hash being computed as-is. (They would need to trustfully select seller_fee_basis_points, or use off-chain params to apply different logic on their smart contracts depending on how the hash of the compressed NFT was computed).

  1. The approach implemented. Only allow updates to a few fields which are individually passed as optional arguments. This approach saves the least space, but is also the least invasive to the protocol (although I suppose the first option also isn't very invasive). The advantage here is updates only take 1 tx. The disadvantage is, practically speaking, the user may not have enough space in their tx to update all of the fields which could be updated simultaneously via update_metadata. This instruction also won't work for proofs of significant length. But for reasonably sized MetadataArgs and proof lengths <= 10 this should do the job (see math below).

13 Accounts * 32 bytes + ("reasonable" 277 byte MetadataArgs) + 32 bytes (root) + (100 byte "reasonable" uri) + 5 None + 8 (nonce) + 4 (index) = 842 bytes. 1232 - 842 = 390. 390/32 = 12.1... After signatures, tx headers, etc. we should be able to fit proofs of length 10. If the tx payer is willing to allocate an ALT, they could probably stretch that further.

@samwise2
Copy link
Contributor Author

samwise2 commented Jun 17, 2023

@danenbm and I chatted a bit about this approach, but given there's a few options for how to deal with space constraints we wanted to wait for others to review before polishing

@danenbm
Copy link
Contributor

danenbm commented Jun 21, 2023

From @lorisleiva:

Option 3 makes sense IMO although I like the idea of a hybrid solution between 1 and 3. Meaning you could either pass a “buffer” account containing the old MetadataArgs (optional account) XOR pass the old MetadataArgs directly as data (optional arg). Then we could have a MetadataArgsChanges struct that uses options/toggles for each attributes. That way if you want to update all the attributes in one go, it’s more efficient to use the buffer account but if you just want to update one field you don’t need to. Wdyt?

@danenbm response:

That seems easy enough although maybe its a nice-to-have that could be added later as well?

@danenbm
Copy link
Contributor

danenbm commented Jun 21, 2023

From @blockiosaurus

Why pick certain fields instead of including options for them all?

@danenbm response:

This is a good question, my assumption is for the ones omitted:

  • edition nonce doesn't do anything, doesn't decompress into anything either.
  • token_standard doesn't do anything and no matter what they pick it would decompress into an NFT.
  • collection can be updated with set_and_verify.
  • uses doesn't do anything although its copied over during decompression.
  • token_program_version has to be Original or it won't decompress.
  • The one that seems like it could be useful is that creators could probably be updated. But creator array could get bigger in size and then you have to add more code to check things like what token-metadata has such as "don't unverify an existing verified creator".

@samwise2
Copy link
Contributor Author

samwise2 commented Jun 21, 2023

From @lorisleiva:

Option 3 makes sense IMO although I like the idea of a hybrid solution between 1 and 3. Meaning you could either pass a “buffer” account containing the old MetadataArgs (optional account) XOR pass the old MetadataArgs directly as data (optional arg). Then we could have a MetadataArgsChanges struct that uses options/toggles for each attributes. That way if you want to update all the attributes in one go, it’s more efficient to use the buffer account but if you just want to update one field you don’t need to. Wdyt?

@danenbm response:

That seems easy enough although maybe its a nice-to-have that could be added later as well?

I think this is a good idea. If the tree has an especially long proof length, then this could give the tree owner some extra flexibility in terms of their update tx size, so that they could fit in a larger proof. I suppose the downside is: if this account is not frequently used then we waste a pubkey which shortens the lengths of proofs that users who do not wish to allocate a temporary account can fit into their tx's (although we can circle back around here and say that if you are space constrained in your tx size then you should just alloc on account). Allocate this account would also add complexity for users of the protocol to manage the two tx process and deal with failures between tx's etc.

I think this is generally useful, but maybe is more of an upgrade if we find that (3) by itself doesn't solve the tx space problem sufficiently?

@samwise2
Copy link
Contributor Author

samwise2 commented Jun 21, 2023

From @blockiosaurus

Why pick certain fields instead of including options for them all?

@danenbm response:

This is a good question, my assumption is for the ones omitted:

  • edition nonce doesn't do anything, doesn't decompress into anything either.
  • token_standard doesn't do anything and no matter what they pick it would decompress into an NFT.
  • collection can be updated with set_and_verify.
  • uses doesn't do anything although its copied over during decompression.
  • token_program_version has to be Original or it won't decompress.
  • The one that seems like it could be useful is that creators could probably be updated. But creator array could get bigger in size and then you have to add more code to check things like what token-metadata has such as "don't unverify an existing verified creator".

Yep, @danenbm your reasoning here matches what I was thinking. I thought a bit about including creators, but it seemed like it would bloat the ix a bit to do the validation you mentioned (array is the correct size, didn't verify an unverified creator, etc.). But I can see how being able to update the creators could be a useful thing, and so I would be open to adding it here.

There might be a bit of a philosophical debate regarding updates to creator arrays for NFTs which are not parts of collections. I suppose if a cNFT is not part of a collection then the tree_delegate is like the de-facto update_authority and should be able to modify the creator arrays in any of its cNFTs at will; but I'm not sure if this is formally true or has just been subtly adopted as a convention in Bubblegum. I can see a case for keeping creators immutable as well.

@lorisleiva
Copy link
Contributor

From @samwise2

if this account is not frequently used then we waste a pubkey which shortens the lengths of proofs

That would be an optional account though right? So when not needed it would only take:

  • zero extra bytes if we just don't pass it in.
  • one extra byte if we use the Anchor convention that passes the program ID for optional accounts in order to keep the order of accounts predictable and improve maintainability of the program.

@danenbm
Copy link
Contributor

danenbm commented Jun 22, 2023

From @samwise2

if this account is not frequently used then we waste a pubkey which shortens the lengths of proofs

That would be an optional account though right? So when not needed it would only take:

  • zero extra bytes if we just don't pass it in.
  • one extra byte if we use the Anchor convention that passes the program ID for optional accounts in order to keep the order of accounts predictable and improve maintainability of the program.

Maybe one concern not related to the convenience of approach 1 but the effect is that it enables sending ALL metadata fields so then it drives changes to like make sure creator array updates are valid, which the current limited approach 3 bypasses that by not including it as an updatable field.

@samwise2
Copy link
Contributor Author

samwise2 commented Jun 22, 2023

From @samwise2

if this account is not frequently used then we waste a pubkey which shortens the lengths of proofs

That would be an optional account though right? So when not needed it would only take:

  • zero extra bytes if we just don't pass it in.
  • one extra byte if we use the Anchor convention that passes the program ID for optional accounts in order to keep the order of accounts predictable and improve maintainability of the program.

Ohh cool! I had previously been working with an old version of Anchor and didn't realize we could do things like Option<Account<'info, T>>. In that case, there's probably no concerns about space overhead

@samwise2
Copy link
Contributor Author

From @samwise2

if this account is not frequently used then we waste a pubkey which shortens the lengths of proofs

That would be an optional account though right? So when not needed it would only take:

  • zero extra bytes if we just don't pass it in.
  • one extra byte if we use the Anchor convention that passes the program ID for optional accounts in order to keep the order of accounts predictable and improve maintainability of the program.

Maybe one concern not related to the convenience of approach 1 but the effect is that it enables sending ALL metadata fields so then it drives changes to like make sure creator array updates are valid, which the current limited approach 3 bypasses that by not including it as an updatable field.

Yep agree. Although, I think in the hybrid approach suggested here, we would not pass in the new, updated MetadataArgs struct. I think the user would manually specify each change they wanted to make in some optional argument. This way we could still pick the fields which are updatable. I.e. we wouldn't include collection so that we don't have to worry about collection verification. The utility would be that if a user was concerned about their tx size, and is willing to allocate an account for their old MetadataArgs, they could simply supply it as an account rather than as an arg. Please correct me if I misinterpreted @lorisleiva

I think allowing updates to creators would be nice, but I don't think we should allow the verification of any creators through this instruction. Imo the only reason to not allow creator updates would be if there are decentralization concerns about the tree delegate being able to arbitrarily remove creators as they wish

@samwise2
Copy link
Contributor Author

As a loose proposal from these feedback items I would suggest:

  • We allow updates to creators but we do not allow any verifications of previously un-verified creators.
  • We give the optionality @lorisleiva suggested to pass in the old MetadataArgs in an optional anchor account so that trees with longer proofs still have a means of updating their metadata.

@danenbm
Copy link
Contributor

danenbm commented Jun 22, 2023

As a loose proposal from these feedback items I would suggest:

  • We allow updates to creators but we do not allow any verifications of previously un-verified creators.
  • We give the optionality @lorisleiva suggested to pass in the old MetadataArgs in an optional anchor account so that trees with longer proofs still have a means of updating their metadata.

I think this will work and resolves all the issues. I think we could even skip the creator stuff if you wanted to for now if it adds too much complexity, but nice to have if possible.

@samwise2
Copy link
Contributor Author

As a loose proposal from these feedback items I would suggest:

  • We allow updates to creators but we do not allow any verifications of previously un-verified creators.
  • We give the optionality @lorisleiva suggested to pass in the old MetadataArgs in an optional anchor account so that trees with longer proofs still have a means of updating their metadata.

I think this will work and resolves all the issues. I think we could even skip the creator stuff if you wanted to for now if it adds too much complexity, but nice to have if possible.

Awesome, will proceed with this. I'll open a PR in the new, separate bubblegum repo, and I'll make it clear there whether or not creator updates are included or not. Thanks to everyone for their feedback!

danenbm added a commit to metaplex-foundation/mpl-bubblegum that referenced this pull request Oct 4, 2023
Original design discussion PR: metaplex-foundation/metaplex-program-library#1121

Original merged PR: #3

---------

* fix protocol conflicts

* allow updating creators without new verifications

* cleanup

* conflicts

* conflicts

* all tests included

* regen solita and fix bugs

* small fixes

* nit

* address code review, without additional test

* added creator verification test

* Regenerate js client

* Rename old_metadata to current_metadata

* Use UpdateArgs to group new metadata params

* Prevent existing verified creators from being removed

Exception: allow verification/unverification during metadata
update for the signer of the tx.

* Derive Default on UpdateArgs

* Fix typo

* Regenerate IDL, Rust client and Umi client

* Regenerate js-solita

* Add canopy to JS Solita test since it uses 2 nodes

* Run CI on any PR

* Removing SetDecompressableState from pub enum InstructionName

* Remove `update_metadata_collection_nft` instruction (#47)

* Remove update_metadata_collection_nft ix

* Remove unneeded checks and errors

* Regenerate IDL and clients

* Update Solita test

---------

Co-authored-by: Sam Orend <[email protected]>
Copy link

@aUsABuisnessman aUsABuisnessman left a comment

Choose a reason for hiding this comment

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

here we are

Copy link

@aUsABuisnessman aUsABuisnessman left a comment

Choose a reason for hiding this comment

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

ok

@danenbm
Copy link
Contributor

danenbm commented Jan 9, 2024

Work was done in new repo at:
metaplex-foundation/mpl-bubblegum#3

There's been some other PR's along the way such as:
metaplex-foundation/mpl-bubblegum#44

The final version that we are planning to deploy on devnet is at:
https://github.com/metaplex-foundation/mpl-bubblegum/tree/update-metadata-staging

PR for indexing in DAS API is at:
metaplex-foundation/digital-asset-rpc-infrastructure#134

We expect RPCs will be indexing update_metadata on devnet very soon, then we will deploy Bubblegum to devnet.

@danenbm danenbm closed this Jan 9, 2024
danenbm added a commit to metaplex-foundation/mpl-bubblegum that referenced this pull request Jan 12, 2024
* Update metadata (#44)

Original design discussion PR: metaplex-foundation/metaplex-program-library#1121

Original merged PR: #3

---------

* fix protocol conflicts

* allow updating creators without new verifications

* cleanup

* conflicts

* conflicts

* all tests included

* regen solita and fix bugs

* small fixes

* nit

* address code review, without additional test

* added creator verification test

* Regenerate js client

* Rename old_metadata to current_metadata

* Use UpdateArgs to group new metadata params

* Prevent existing verified creators from being removed

Exception: allow verification/unverification during metadata
update for the signer of the tx.

* Derive Default on UpdateArgs

* Fix typo

* Regenerate IDL, Rust client and Umi client

* Regenerate js-solita

* Add canopy to JS Solita test since it uses 2 nodes

* Run CI on any PR

* Removing SetDecompressableState from pub enum InstructionName

* Remove `update_metadata_collection_nft` instruction (#47)

* Remove update_metadata_collection_nft ix

* Remove unneeded checks and errors

* Regenerate IDL and clients

* Update Solita test

---------

Co-authored-by: Sam Orend <[email protected]>

* Avoid inlining UpdateArgs (#50)

* Adding LeafSchemaEvent struct to Rust client

* Add leaf schema accessors to program and Rust client

* Add InstructionName::UpdateMetadata to Rust client

* chore: Release mpl-bubblegum version 1.0.1-beta.1

* Add borsh version range (#54)

* Add borsh version range

* Update lock file

* chore: Release mpl-bubblegum version 1.0.1-beta.2

* Remove optional metadata buffer account from update_metadata

* Update Rust and Umi JS clients, update IDL

* Update js-solita client

* Fixing rust client cargo lock after release downgrades it

* chore: Release mpl-bubblegum version 1.0.1-beta.3

* Delete and regenerate rust client Cargo.lock after release downgrades borsh 0.10.3

* Add update_metadata_collection_nft

* Regenerate clients

* Regenerate Solita JS client

* update Solita tests

* Fix path to js test script

* Remove signer req for tree delegate on update_metadata_collection_nft

* Regenerate Solita JS client

* Add tests for using correct ix based on if item is in collection

* Combine update_metadata and update_metadata_collection_nft

* Regenerate JS and Rust clients

* Regenerate Solita JS client

* Update Solita tests

* Removed unneeded instruction name

* Regenerate IDL

* Clean up README and add update_metadata

* Add updateArgs and fix links

* Remove redundancies and clarify

* chore: Release mpl-bubblegum version 1.0.1-beta.4

---------

Co-authored-by: Sam Orend <[email protected]>
Co-authored-by: Fernando Otero <[email protected]>
Co-authored-by: febo <[email protected]>
Co-authored-by: danenbm <[email protected]>
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.

4 participants