feature: Update Tree Config Instruction - #156
Open
KartikSoneji wants to merge 9 commits into
Open
Conversation
|
@KartikSoneji is attempting to deploy a commit to the Metaplex Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Summary by CodeRabbit
WalkthroughAdds a new on-chain instruction Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (off-chain)
participant Program as Bubblegum Program
participant TreeConfig as TreeConfig PDA (on-chain)
participant Merkle as MerkleTree Account (on-chain)
Client->>Program: send updateTreeConfig(UpdateTreeConfigArgs), signer=authority
Program->>TreeConfig: derive & load PDA (check version & authority)
Program->>Merkle: read merkle tree account (unchecked PDA reference)
alt Version == V2 and is_decompressible == Enabled
Program-->>Client: UnsupportedUpdateOperation error
else Valid update
Program->>TreeConfig: selectively update provided fields
Program-->>Client: success / tx confirmed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/js/test/updateTreeConfig.test.ts`:
- Around line 32-36: The test must explicitly assert that treeDelegate remains
unchanged when only rotating the creator: after calling fetchTreeConfigFromSeeds
and storing treeConfig, add an assertion comparing the previous treeDelegate to
the fetched one using string (base58) equality (use t.is rather than t.like) —
convert both treeDelegate values to strings if they are PublicKey objects; do
the same in the other similar assertion block (the one around the second check).
Reference fetchTreeConfigFromSeeds, treeConfig, treeCreator, treeDelegate, and
updateTreeConfig to locate and update the assertions.
In `@programs/bubblegum/program/src/processor/update_tree_config.rs`:
- Around line 19-20: The account constraint on authority in
update_tree_config.rs uses BubblegumError::TreeAuthorityIncorrect but this
instruction is creator-only; replace that error with a creator-specific variant
(e.g., BubblegumError::InvalidAuthority or add a new variant like
BubblegumError::TreeCreatorOnly) and update all references: add the new enum
variant to the BubblegumError definition (or reuse InvalidAuthority if already
appropriate) and change the attribute on the authority field (the
account(address = tree_authority.tree_creator @ ... ) constraint) to reference
the creator-specific error so delegate-signed calls produce the correct error
message.
- Around line 21-23: The merkle_tree account in the UpdateTreeConfig instruction
is marked mut but is only used to derive tree_authority and not read or mutated;
remove the mut annotation on the merkle_tree UncheckedAccount<'info> (the field
named merkle_tree) so it is declared without #[account(mut)] and update the
CHECK comment to reflect it is only used for PDA derivation (remove the
"modified in the downstream program" assertion); ensure any reference in the
handler or tests still derives tree_authority from merkle_tree and that client
account metadata will now mark merkle_tree as read-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dde55116-8c80-4258-9835-d08ba6de37a3
⛔ Files ignored due to path filters (6)
clients/js/src/generated/errors/mplBubblegum.tsis excluded by!**/generated/**clients/js/src/generated/instructions/index.tsis excluded by!**/generated/**clients/js/src/generated/instructions/updateTreeConfig.tsis excluded by!**/generated/**clients/rust/src/generated/errors/mpl_bubblegum.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/mod.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/update_tree_config.rsis excluded by!**/generated/**
📒 Files selected for processing (8)
clients/js/test/updateTreeConfig.test.tsconfigs/kinobi.cjsidls/bubblegum.jsonprograms/bubblegum/program/src/error.rsprograms/bubblegum/program/src/lib.rsprograms/bubblegum/program/src/processor/mod.rsprograms/bubblegum/program/src/processor/update_tree_config.rsprograms/bubblegum/program/src/state/metaplex_adapter.rs
Comment on lines
+32
to
+36
| // Then the tree config account was updated accordingly. | ||
| treeConfig = await fetchTreeConfigFromSeeds(umi, { merkleTree }); | ||
| t.like(treeConfig, <TreeConfig>{ | ||
| treeCreator, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert that treeDelegate stays unchanged in the creator-only cases.
These assertions still pass if updateTreeConfig accidentally rewrites treeDelegate while rotating treeCreator. Since the contract here is partial updates, make the untouched field explicit.
Suggested assertion update
- t.like(treeConfig, <TreeConfig>{
- treeCreator,
- });
+ t.is(treeConfig.treeCreator, treeCreator);
+ t.is(treeConfig.treeDelegate, umi.identity.publicKey);
...
- t.like(treeConfig, <TreeConfig>{
- treeCreator,
- });
+ t.is(treeConfig.treeCreator, treeCreator);
+ t.is(treeConfig.treeDelegate, umi.identity.publicKey);Based on learnings, in tests involving PublicKey values from the Umi framework, treat PublicKey as a string (base58) and use t.is() for equality checks.
Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/js/test/updateTreeConfig.test.ts` around lines 32 - 36, The test must
explicitly assert that treeDelegate remains unchanged when only rotating the
creator: after calling fetchTreeConfigFromSeeds and storing treeConfig, add an
assertion comparing the previous treeDelegate to the fetched one using string
(base58) equality (use t.is rather than t.like) — convert both treeDelegate
values to strings if they are PublicKey objects; do the same in the other
similar assertion block (the one around the second check). Reference
fetchTreeConfigFromSeeds, treeConfig, treeCreator, treeDelegate, and
updateTreeConfig to locate and update the assertions.
Comment on lines
+19
to
+20
| #[account(address = tree_authority.tree_creator @ BubblegumError::TreeAuthorityIncorrect)] | ||
| pub authority: Signer<'info>, |
Contributor
There was a problem hiding this comment.
Use a creator-specific auth error here.
A delegate-signed call will fail this address constraint but return TreeAuthorityIncorrect, whose message says the creator or delegate may sign. That is misleading for a creator-only instruction; use InvalidAuthority or add a dedicated creator-only error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/bubblegum/program/src/processor/update_tree_config.rs` around lines
19 - 20, The account constraint on authority in update_tree_config.rs uses
BubblegumError::TreeAuthorityIncorrect but this instruction is creator-only;
replace that error with a creator-specific variant (e.g.,
BubblegumError::InvalidAuthority or add a new variant like
BubblegumError::TreeCreatorOnly) and update all references: add the new enum
variant to the BubblegumError definition (or reuse InvalidAuthority if already
appropriate) and change the attribute on the authority field (the
account(address = tree_authority.tree_creator @ ... ) constraint) to reference
the creator-specific error so delegate-signed calls produce the correct error
message.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/bubblegum/program/src/processor/update_tree_config.rs`:
- Line 54: The error message in update_tree_config.rs uses the wrong field name;
change the msg! invocation that currently says "is_decompressed must be None or
Disabled, got {:?}" to use the correct field name "is_decompressible" so it
reads "is_decompressible must be None or Disabled, got {:?}", updating the
string in the msg! call in the update_tree_config logic (referencing
UpdateTreeConfigArgs / TreeConfig and the msg! call that logs the
is_decompressible value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8bc9963-b3eb-4ff2-b9f6-b6cdfed84479
📒 Files selected for processing (1)
programs/bubblegum/program/src/processor/update_tree_config.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/bubblegum/program/src/processor/update_tree_config.rs`:
- Around line 60-62: The code assigns tree_creator/tree_delegate directly into
tree_authority allowing Pubkey::default() (zero address); add explicit checks
before assignment to reject Pubkey::default() and return an error. In
update_tree_config (where tree_creator/tree_delegate are optional and assigned
to tree_authority), check if let Some(p) and if p != Pubkey::default() then
assign, otherwise return a clear Program error (e.g., InvalidArgument or a
Bubblegum-specific error) so zero-address values cannot be set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c6fe458-2ced-4890-b6bd-147ca0ef58cb
📒 Files selected for processing (1)
programs/bubblegum/program/src/processor/update_tree_config.rs
Comment on lines
+60
to
+62
| if let Some(tree_creator) = tree_creator { | ||
| tree_authority.tree_creator = tree_creator; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for existing Pubkey::default() validation patterns in the codebase
# Check for existing patterns preventing zero pubkey assignment
rg -n 'Pubkey::default\(\)' --type rust -C 2Repository: metaplex-foundation/mpl-bubblegum
Length of output: 59
🏁 Script executed:
cat -n programs/bubblegum/program/src/processor/update_tree_config.rs | head -80Repository: metaplex-foundation/mpl-bubblegum
Length of output: 2880
🏁 Script executed:
# Search for validation patterns with Pubkey in the codebase
rg -n 'require!' --type rust -C 2 | head -100Repository: metaplex-foundation/mpl-bubblegum
Length of output: 8311
🏁 Script executed:
# Check error definitions
fd -e rs -path "*/error*" | head -20Repository: metaplex-foundation/mpl-bubblegum
Length of output: 245
🏁 Script executed:
fd -type f -name "*.rs" | xargs rg -l "BubblegumError" | head -5Repository: metaplex-foundation/mpl-bubblegum
Length of output: 537
🏁 Script executed:
# Find and read error definitions
rg -A 2 "InvalidAuthority" --type rustRepository: metaplex-foundation/mpl-bubblegum
Length of output: 2041
🏁 Script executed:
cat programs/bubblegum/program/src/error.rs | head -50Repository: metaplex-foundation/mpl-bubblegum
Length of output: 1896
Consider validating non-zero tree_creator and tree_delegate.
Setting tree_creator or tree_delegate to Pubkey::default() (the zero address) creates an edge case where signature validation becomes unreliable. The zero address cannot sign transactions, and while the account constraint on line 19 would catch an invalid tree_creator, explicit validation prevents this footgun entirely.
🛡️ Optional validation to prevent zero-address assignment
if let Some(tree_creator) = tree_creator {
+ require!(
+ tree_creator != Pubkey::default(),
+ BubblegumError::InvalidAuthority
+ );
tree_authority.tree_creator = tree_creator;
}
if let Some(tree_delegate) = tree_delegate {
+ require!(
+ tree_delegate != Pubkey::default(),
+ BubblegumError::InvalidAuthority
+ );
tree_authority.tree_delegate = tree_delegate;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/bubblegum/program/src/processor/update_tree_config.rs` around lines
60 - 62, The code assigns tree_creator/tree_delegate directly into
tree_authority allowing Pubkey::default() (zero address); add explicit checks
before assignment to reject Pubkey::default() and return an error. In
update_tree_config (where tree_creator/tree_delegate are optional and assigned
to tree_authority), check if let Some(p) and if p != Pubkey::default() then
assign, otherwise return a clear Program error (e.g., InvalidArgument or a
Bubblegum-specific error) so zero-address values cannot be set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an
updateTreeConfiginstruction that allows updating:in a single operation.
Additionally, this is the only instruction that allows updating the tree creator after creation.
Supports both V1 and V2 trees.