-
Notifications
You must be signed in to change notification settings - Fork 45
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
Updatable Metadata #3
Updatable Metadata #3
Conversation
wen merge 🪄 |
04ce25b
to
c8864df
Compare
Reviewing now |
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.
I've mostly looked at the contract changes, but haven't looked at the generated code or tests in detail yet. But I wanted to post my initial comments.
The code is looking great, I like the design and implementation, and I like the extra protection for items already in a verified collection. I'll post again if I have any more comments after looking at the rest.
let no_new_creators_were_verified = creators | ||
.iter() | ||
.filter(|c| c.verified) // select only creators that are verified | ||
.all(|c| { | ||
old_creators | ||
.iter() | ||
.any(|old| old.address == c.address && old.verified) | ||
}); | ||
require!( | ||
no_new_creators_were_verified, | ||
BubblegumError::CreatorDidNotVerify | ||
); |
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.
I know it says we don't want to verify new creators, but we also need to make it check that we don't unverify previously verified creators. I need to study the logic more but I'm not sure if it does the second case.
Either way, could you create some tests specifically to make sure those two conditions will fail? The test is also important to make sure it doesn't regress in the future.
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.
Yeah this check doesn't prevent the issuer of the update from un-verifying some creators. I can change the check to do that. Originally, I made the choice intentionally, since the tree_delegate
has to sign-off on mints, I figured they could arbitrarily control creators
for an NFT as desired, and then those creators
can opt to sign and verify themselves or not. Under that premise, I thought it may be reasonable that the updater could unverify or even completely remove creators, if they desired (that is also permitted here).
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.
It does seem possible though that two parties could agree for a specific address to be a creator for some period of time, and then want to remove them afterwards. I.e. two parties agree that A will be a creator (and receive royalties) for 3 months, after which they will be removed. I suppose their share
could also be updated to 0, but they may want to replace that creator slot.
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.
Let me know what you think about the above. I'll leave the check as-is for now, and only add a test that a new creator cannot be verified through this ix. But if after reading the thread above, you feel we still want to enforce the old creators cannot be removed/unverified I can add additional tests for that
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 current behaviour for NFTs is that Update
does not remove verified creators nor unverify them. This prevents the authority to unilaterally remove a verified creator from the creator array, although as you pointed out, they can set the share to 0
. The only exception is when the authority itself is a creator.
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.
I think we should follow the current NFT behavior, including not allowing removing verified creators unless the signing authority is the verified creator being removed. In general that is what people will expect from us.
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.
Added code that prevents removing existing verified creators during metadata update.
Note: I added the exception cases for the transaction signer as well (tree delegate): the tree delegate can add itself as a new verified creator, or remove/unverify itself even if it is a currently verified creator.
Also added 4 tests to test these cases:
✓ Cannot verify currently unverified creator if not signer (14148 ms)
✓ Can verify currently unverified creator if signer (14143 ms)
✓ Cannot unverify currently verified creator if not signer (14547 ms)
✓ Can unverify currently verified creator if signer (14561 ms)
Note initial design discussion was done here: metaplex-foundation/metaplex-program-library#1121 |
@samwise2 I will have a look at the program in detail now, but I have a question regarding the This is a bit different to how NFTs currently work, in the sense that you have the |
} | ||
|
||
#[derive(Accounts)] | ||
pub struct UpdateMetadataCollectionNFT<'info> { |
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.
I am not sure we need 2 separate Update
instructions.
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.
Will remove ``update_metadata_collection_nft` from our repo/branch as follow-on PR.
Yeah, I think the tricky bit here is that there is no One other thing to consider here is compress/decompress. I'm not super well versed in that part of the code, but from what I can tell, the way Super open to discussion here, but that was the initial thinking for creating two ixs. Technically, all of the logic here could be combined into one ix, with lots of optional accounts, etc. but that just makes the interface clumsier imo. With two ixs it is clear what has to be provided by the client depending on the kind of NFT they have. |
Yes during decompress the token-metadata |
Chatted with @febo and for right now we would like to skip the |
Exception: allow verification/unverification during metadata update for the signer of the tx.
I'm going to merge this PR with |
@@ -102,3 +102,14 @@ pub struct MetadataArgs { | |||
pub token_program_version: TokenProgramVersion, | |||
pub creators: Vec<Creator>, | |||
} | |||
|
|||
#[derive(AnchorSerialize, AnchorDeserialize, PartialEq, Eq, Debug, Clone)] |
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 should derive Default
here, so we could have:
UpdateArgs {
name: Some("New name"),
..Default::default()
}
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.
done
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.
Only a super minor comment.
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.
BPF tests pass.
Umi tests pass.
js-solita tests pass except for decompression one which needs the recent changes from main, which we will get in the update-metadata
branch.
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
* Fix binary name * Set decompressible on tree creation
* Removing deploy capabilities * Adding back program deploy. * Update collection verification checks * Add Umi tests around collection verification * Add deprecated log message (#2) * Set decompressible on tree creation (#3) * Fix binary name * Set decompressible on tree creation * Fix binary name * Revert "Removing deploy capabilities" This reverts commit c203069. --------- Co-authored-by: blockiosaurus <[email protected]> Co-authored-by: Fernando Otero <[email protected]>
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]>
* 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]>
This PR adds two new instructions
update_metadata
andupdate_metadata_collection_nft
to Bubblegum which enable updates to cNFT metadata after mint.High level overview of
update_metadata
:name
,symbol
,uri
,creators
,seller_fee_basis_points
,primary_sale_happened
andis_mutable
can all be updated.update_metadata
must be signed by thetree_delegate
.update_metadata
can only be invoked for NFTs which are not linked to verified collections.update_metadata
CPIs toreplace_leaf
. Thus it requires the old hash of the leaf's metadata. Unfortunately, the ix cannot simply takeold_data_hash
as an argument because it needs to check theis_mutable
flag to see if the metadata update is valid. For this reason, the instruction needs to intake the un-hashed form of the oldMetadataArgs
. SinceMetadataArgs
is a large struct, we've provided the optionality to pass it directly as an argument, or through data in an account, allowing the caller to make a decision regarding the tradeoffs between tx space and rent allocation.High level overview of
update_metadata_collection_nft
:collection_authority
must sign to authorize the metadata changes.update_metadata
This PR adds two
js-solita
client tests to show the correctness of the ix's and demonstrate how they can be invoked.