feat: more consise pass on mip 11 #231
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
WalkthroughIntroduces “Group” as a first-class on-chain entity with full lifecycle (create, update, close), relationship management (assets, collections, child/parent groups), and plugin support (internal and external). Extends instruction set, errors, state, utilities, processors, JS/Rust client bindings, IDL, and tests. Adds safety guards and memory-move fixes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Program as mpl-core Program
participant Sys as System Program
participant Group as Group Account
participant Related as Related Accounts
Client->>Program: CreateGroupV1(name, uri, relationships)
Program->>Sys: create_account(Group)
Program->>Group: init GroupV1{ua, name, uri}
alt relationships provided
loop for each entry
Program->>Related: load & authority checks
alt Collection/Asset
Program->>Related: ensure Groups plugin includes Group
else Child/Parent Group
Program->>Group: add child/parent
Program->>Related: add parent/child
end
end
end
Program-->>Client: ok
sequenceDiagram
autonumber
actor Client
participant Program
participant Group
participant Coll as Collection(s)
Client->>Program: AddCollectionsToGroupV1(...)
Program->>Group: load, auth check
loop each collection acct
Program->>Coll: load, auth check
Program->>Group: add collection key (if absent)
Program->>Coll: ensure Groups plugin contains Group key
end
Program-->>Client: ok
sequenceDiagram
autonumber
actor Client
participant Program
participant Group
participant Plugins as Plugin Meta/Registry
Client->>Program: ApproveGroupPluginAuthorityV1(pluginType, newAuthority)
Program->>Group: load, auth check
Program->>Plugins: fetch plugin header/registry
Program->>Plugins: approve_authority(pluginType, newAuthority)
Program-->>Client: ok
Client->>Program: RevokeGroupPluginAuthorityV1(pluginType)
Program->>Group: load, auth check
Program->>Plugins: revoke_authority(pluginType)
Program-->>Client: ok
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 47
🔭 Outside diff range comments (2)
programs/mpl-core/src/plugins/mod.rs (1)
120-145: 🧹 Nitpick (assertive)Minor maintainability idea: collapse repetitive
len()matchThe 20-line match inside
Plugin::len()grows each time we add a variant.
Consider letting each plugin exposefn padded_len(&self) -> usizeviaDataBlobdefault impl or a macro to cut repetition.Not blocking, just food for thought.
programs/mpl-core/src/processor/mod.rs (1)
80-288: 🧹 Nitpick (assertive)Processor match-arm explosion – maintainability concern
process_instructionnow has >90 match arms. This is already hitting the
BPF verifier instruction limit and becomes error-prone.Consider generating the dispatch table with a macro:
macro_rules! dispatch { ($( $variant:path => $handler:ident ),* $(,)?) => { match instruction { $( $variant(args) => { msg!(concat!("Instruction: ", stringify!($handler))); $handler(accounts, args) } ),* } } } dispatch!( MplAssetInstruction::CreateV1 => create_v1, MplAssetInstruction::CreateCollectionV1 => create_collection_v1, // … MplAssetInstruction::RemoveAssetsFromGroupV1 => remove_assets_from_group_v1, );This reduces churn when adding/removing instructions and keeps the file readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (31)
programs/mpl-core/src/error.rs(1 hunks)programs/mpl-core/src/instruction.rs(3 hunks)programs/mpl-core/src/plugins/internal/authority_managed/groups.rs(1 hunks)programs/mpl-core/src/plugins/internal/authority_managed/mod.rs(2 hunks)programs/mpl-core/src/plugins/mod.rs(8 hunks)programs/mpl-core/src/processor/add_assets_to_group.rs(1 hunks)programs/mpl-core/src/processor/add_collections_to_group.rs(1 hunks)programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs(1 hunks)programs/mpl-core/src/processor/add_group_plugin.rs(1 hunks)programs/mpl-core/src/processor/add_groups_to_group.rs(1 hunks)programs/mpl-core/src/processor/add_plugin.rs(1 hunks)programs/mpl-core/src/processor/approve_group_plugin_authority.rs(1 hunks)programs/mpl-core/src/processor/approve_plugin_authority.rs(1 hunks)programs/mpl-core/src/processor/close_group.rs(1 hunks)programs/mpl-core/src/processor/create_group.rs(1 hunks)programs/mpl-core/src/processor/mod.rs(2 hunks)programs/mpl-core/src/processor/remove_assets_from_group.rs(1 hunks)programs/mpl-core/src/processor/remove_collections_from_group.rs(1 hunks)programs/mpl-core/src/processor/remove_group_external_plugin_adapter.rs(1 hunks)programs/mpl-core/src/processor/remove_group_plugin.rs(1 hunks)programs/mpl-core/src/processor/remove_groups_from_group.rs(1 hunks)programs/mpl-core/src/processor/remove_plugin.rs(1 hunks)programs/mpl-core/src/processor/revoke_group_plugin_authority.rs(1 hunks)programs/mpl-core/src/processor/revoke_plugin_authority.rs(1 hunks)programs/mpl-core/src/processor/update_group.rs(1 hunks)programs/mpl-core/src/processor/update_group_plugin.rs(1 hunks)programs/mpl-core/src/processor/update_plugin.rs(1 hunks)programs/mpl-core/src/processor/write_group_external_plugin_adapter_data.rs(1 hunks)programs/mpl-core/src/state/group.rs(1 hunks)programs/mpl-core/src/state/mod.rs(2 hunks)programs/mpl-core/src/utils/mod.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
programs/mpl-core/src/processor/add_plugin.rs (2)
programs/mpl-core/src/error.rs (1)
from(228-230)programs/mpl-core/src/plugins/mod.rs (1)
from(222-242)
programs/mpl-core/src/processor/update_plugin.rs (2)
programs/mpl-core/src/error.rs (1)
from(228-230)programs/mpl-core/src/plugins/mod.rs (1)
from(222-242)
programs/mpl-core/src/utils/mod.rs (3)
programs/mpl-core/src/plugins/utils.rs (1)
fetch_wrapped_plugin(156-189)programs/mpl-core/src/plugins/mod.rs (1)
load(76-82)clients/js/src/generated/types/updateDelegate.ts (1)
UpdateDelegate(17-17)
programs/mpl-core/src/processor/approve_group_plugin_authority.rs (3)
programs/mpl-core/src/processor/approve_plugin_authority.rs (4)
approve_authority_on_plugin(168-168)fetch_wrapped_plugin(51-51)fetch_wrapped_plugin(120-120)fetch_core_data(156-156)programs/mpl-core/src/utils/mod.rs (8)
fetch_wrapped_plugin(594-594)fetch_wrapped_plugin(622-622)fetch_core_data(88-102)fetch_core_data(150-150)fetch_core_data(181-181)fetch_core_data(400-400)is_valid_group_authority(582-608)resolve_authority(567-578)programs/mpl-core/src/plugins/mod.rs (1)
load(76-82)
programs/mpl-core/src/processor/close_group.rs (4)
programs/mpl-core/src/entrypoint.rs (1)
error(18-18)programs/mpl-core/src/utils/account.rs (1)
close_program_account(9-34)programs/mpl-core/src/utils/mod.rs (2)
is_valid_group_authority(582-608)resolve_authority(567-578)programs/mpl-core/src/plugins/mod.rs (1)
load(76-82)
programs/mpl-core/src/processor/add_assets_to_group.rs (4)
programs/mpl-core/src/processor/remove_assets_from_group.rs (3)
create_meta_idempotent(136-136)is_valid_asset_authority(26-55)fetch_wrapped_plugin(42-42)programs/mpl-core/src/processor/create_group.rs (7)
create_meta_idempotent(335-335)create_meta_idempotent(437-437)initialize_plugin(351-351)initialize_plugin(453-453)is_valid_asset_authority(53-85)fetch_wrapped_plugin(72-72)process_asset_groups_plugin_add(429-528)programs/mpl-core/src/utils/mod.rs (4)
is_valid_group_authority(582-608)resolve_authority(567-578)fetch_wrapped_plugin(594-594)fetch_wrapped_plugin(622-622)programs/mpl-core/src/utils/account.rs (1)
resize_or_reallocate_account(37-74)
programs/mpl-core/src/processor/create_group.rs (6)
programs/mpl-core/src/processor/add_assets_to_group.rs (4)
create_meta_idempotent(159-159)initialize_plugin(172-172)fetch_wrapped_plugin(50-50)process_asset_groups_plugin_add(152-230)programs/mpl-core/src/processor/add_collections_to_group.rs (3)
create_meta_idempotent(158-158)initialize_plugin(174-174)process_collection_groups_plugin_add(150-250)programs/mpl-core/src/utils/mod.rs (5)
is_valid_collection_authority(612-638)is_valid_group_authority(582-608)resolve_authority(567-578)fetch_wrapped_plugin(594-594)fetch_wrapped_plugin(622-622)programs/mpl-core/src/utils/account.rs (1)
resize_or_reallocate_account(37-74)programs/mpl-core/src/plugins/mod.rs (4)
load(76-82)len(119-146)len(216-218)inner(93-113)programs/mpl-core/src/state/group.rs (2)
new(46-65)len(96-104)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Programs / Test (mpl-core)
- GitHub Check: Programs / Build
🔇 Additional comments (19)
programs/mpl-core/src/error.rs (1)
215-218: LGTM! Error variant follows established patterns.The new
GroupMustBeEmptyerror variant is properly implemented with:
- Correct sequential error code (51)
- Clear, descriptive error message
- Proper documentation and formatting
programs/mpl-core/src/processor/remove_plugin.rs (1)
118-121: LGTM! Architectural guard properly implemented.This guard correctly enforces that Groups plugins can only be managed through specialized group instructions, not via generic collection plugin operations. The implementation is efficient (early return) and uses appropriate error handling.
programs/mpl-core/src/plugins/internal/authority_managed/mod.rs (1)
3-3: LGTM! Module addition follows established patterns.The new
groupsmodule is properly declared and re-exported following the exact pattern used by other modules in this file.Also applies to: 12-12
programs/mpl-core/src/processor/approve_plugin_authority.rs (1)
114-117: LGTM! Consistent architectural guard implementation.This guard properly enforces the architectural decision that Groups plugins must be managed exclusively through group-specific instructions. The implementation is consistent with similar guards in other processor functions and includes a clear explanatory comment.
programs/mpl-core/src/processor/update_plugin.rs (1)
115-118: Excellent guard implementation for Groups plugin protection.The guard is well-positioned and properly prevents Groups plugins from being updated through the generic collection plugin pathway. The early return pattern is efficient, and the explanatory comment clearly documents the architectural decision to manage Groups plugins exclusively through dedicated instructions.
programs/mpl-core/src/state/mod.rs (2)
29-30: Clean module integration for group functionality.The group module addition follows the established pattern in the codebase and provides the necessary foundation for group-related state management.
106-107: Proper account discriminator for Groups.The GroupV1 variant addition to the Key enum enables proper account type identification for group accounts, aligning with the existing discriminator pattern for other entity types.
programs/mpl-core/src/processor/revoke_plugin_authority.rs (1)
128-131: Consistent guard implementation for Groups plugin authority management.The guard properly prevents revoking plugin authority for Groups plugins through generic collection operations. The implementation is consistent with other processor files and maintains architectural separation between generic and group-specific plugin management.
programs/mpl-core/src/processor/add_plugin.rs (1)
141-146: Excellent guard implementation with comprehensive documentation.The guard effectively prevents Groups plugins from being added through the generic collection plugin pathway. The multi-line comment provides clear explanation of both the restriction and the architectural reasoning, setting a good documentation standard for the codebase.
programs/mpl-core/src/plugins/internal/authority_managed/groups.rs (1)
1-13: Struct & trait wiring looks solidDerives, layout (
repr(C)), andDataBlobimplementation are all correct; nothing blocking here.
Nice job keeping the plugin self-contained with zero validation overhead.programs/mpl-core/src/processor/close_group.rs (1)
41-45: Should we assert that the plugin registry is empty?A group can still carry arbitrary plugins even when
collections,groups, andparent_groupsare empty.
Closing such an account silently discards plugin metadata and any delegated authorities bound to it, which may surprise integrators.Please double-check the intended semantics; if plugins must be absent, consider adding:
if plugin_header_opt.is_some() { return Err(MplCoreError::GroupMustBeEmpty.into()); }(or a dedicated error).
Let me know if you’d like a follow-up patch.programs/mpl-core/src/plugins/mod.rs (1)
61-63: Variant added correctly – nothing to flag
Groups(Groups)is wired through every required match arm (inner,len,From,manager).
Unit tests were expanded, andPluginType::COUNTstill balances. Good coverage!programs/mpl-core/src/processor/approve_group_plugin_authority.rs (1)
71-80: Confirm persistence of the mutated registry
approve_authority_on_pluginreceives&mut plugin_registry, but this file doesn’t re-serialize the registry afterward.
If the helper doesn’t write back internally, changes will be lost.Please verify that
approve_authority_on_plugincallstry_to_vec()/save()internally; otherwise you must persistplugin_registryhere.programs/mpl-core/src/processor/update_group_plugin.rs (1)
91-99:❌ Incorrect review comment
Offset adjustment assumes plugin is stored before registry
plugin_header.plugin_registry_offsetis blindly incremented bysize_diff.
If the plugin being updated lies after the registry, this will corrupt
offsets.
Verify the relative ordering before shifting, or compute the new offset
based on absolute positions.Example safeguard:
if registry_record.offset < plugin_header.plugin_registry_offset as isize { plugin_header.plugin_registry_offset = (plugin_header.plugin_registry_offset as isize + size_diff) .try_into() .map_err(|_| MplCoreError::NumericalOverflow)?; }
I’ll pull in the surrounding code to confirm the semantics of these offsets before finalizing the recommendation.
🏁 Script executed:
#!/bin/bash echo "bump_offsets context:" rg -n -C5 "bump_offsets" programs/mpl-core/src/processor/update_group_plugin.rs echo "" echo "plugin_registry_offset context:" rg -n -C5 "plugin_registry_offset" programs/mpl-core/src/processor/update_group_plugin.rsLength of output: 1896
I’ll locate where
registry_recordcomes from to confirm its meaning and ordering:
🏁 Script executed:
#!/bin/bash echo "registry_record declaration and context:" rg -n -C5 "registry_record" programs/mpl-core/src/processor/update_group_plugin.rsLength of output: 1713
I've gathered evidence that in the
create_groupflow, the registry is serialized after all plugin data, and not before it. This matches howupdate_group_pluginthen updatesplugin_registry_offsetbysize_diff: since the registry block lives after the plugin data, growing or shrinking a plugin moves the registry data.Thus, the registry offset adjustment is correct as written, and the original comment’s assumption (that the registry comes before plugins) is inverted. The registry always sits at the end, and plugins are inserted before it.
Conclusion: The offset adjustment logic is correct and does not need a guard on relative ordering.
Likely an incorrect or invalid review comment.
programs/mpl-core/src/processor/add_assets_to_group.rs (1)
114-123: Authority check ignores asset owner
is_valid_asset_authorityonly accepts the update-authority or its delegates.
If the program semantics allow the owner of an asset to add it to a group, the current logic will reject valid signers.Double-check the desired permission model; if owners should be allowed, extend the helper accordingly.
programs/mpl-core/src/processor/remove_assets_from_group.rs (1)
87-103: Inconsistent state when asset not in groupIf an asset is not listed in
group.assets, the loopcontinues and skips plugin clean-up.
This leaves the asset’s Groups plugin still referencing the group, creating dangling links.Either:
- Remove the plugin entry unconditionally (even if the asset was not found in the group vector), or
- Fail the instruction with
MplCoreError::InvalidGroupto force the caller to provide coherent data.programs/mpl-core/src/processor/add_collections_to_group.rs (1)
112-117:⚠️ Potential issueCompilation failure – wrong reference passed to
Vec::contains
containsexpects&Pubkey, but the current call passes&Pubkeyby value (i.e.&Pubkeyvs& &Pubkey).-if group.collections.contains(collection_info.key) { +if group.collections.contains(&collection_info.key) { // Already present, skip to next. continue; }This is a hard compile-time error that will prevent the program from building.
Likely an incorrect or invalid review comment.
programs/mpl-core/src/processor/create_group.rs (2)
96-100:assert_signer(group)may block PDA accountsRequiring
ctx.accounts.groupto be a signer means the keypair’s private key must sign the transaction.
If you ever want the group account to be a program-derived address (PDA) you’ll need to:
- Drop this assertion, and
- Switch the
create_accountCPI toinvoke_signedwith the PDA seeds.Please verify that relying on a raw keypair is intentional; otherwise group creation by other programs or clients without the secret key will fail.
131-142:✅ Verification successful
Future growth not budgeted in rent-exemption lamports
lamportsis calculated fromserialized_data.len(), i.e. the exact size of the freshly-serializedGroupV1.
However,GroupV1can later grow via:
- its own vectors (
name,uri, …) if you expose update instructions,- plugin metadata (header + registry) if you attach plugins directly to a group.
Without an explicit realloc/lamport‐top-up path, the account risks dropping below rent-exempt once it is resized. Two mitigations:
- let lamports = rent.minimum_balance(serialized_data.len()); + // Pre-allocate a small safety margin (e.g. 256 bytes) to absorb minor updates. + let preallocated_size = serialized_data.len() + 256; + let lamports = rent.minimum_balance(preallocated_size);or document a mandatory “resize & top-up” flow for future mutations.
🏁 Script executed:
#!/bin/bash # Locate struct GroupV1 definition and its fields rg "struct GroupV1" -A10 -n # Find any processor instructions for updating or modifying a group rg "update_group" -n programs/mpl-core/src/processor rg "modify_group" -n programs/mpl-core/src/processor # Find Account context definitions involving a group for potential mutations rg "derive(Accounts)" -n programs/mpl-core/src/processor | grep -i -B1 -A2 group # Search for any CPI calls to allocate/transfer lamports for existing group accounts rg "system_instruction::allocate" -n programs/mpl-core rg "system_instruction::assign" -n programs/mpl-core rg "system_instruction::transfer" -n programs/mpl-coreLength of output: 2120
🏁 Script executed:
#!/bin/bash # 1. Search for any realloc or resize usage in the codebase rg "realloc" -n programs/mpl-core rg "resize" -n programs/mpl-core # 2. Inspect update_group_v1 to see if it handles account resizing or lamport top-up rg "pub(crate) fn update_group_v1" -A30 -n programs/mpl-core/src/processor/update_group.rs # 3. Inspect update_group_plugin for any resizing logic rg "pub(crate) fn update_group_plugin" -A30 -n programs/mpl-core/src/processor/update_group_plugin.rs # 4. Review account utility functions for any resize/realloc helpers sed -n '1,200p' programs/mpl-core/src/utils/account.rs # 5. Check the execute processor for any CPI calls related to resizing sed -n '1,200p' programs/mpl-core/src/processor/execute.rsLength of output: 18866
GroupV1 rent‐exemption is maintained by existing resize/top-up logic
The account is created with exact size in create_group.rs, and all subsequent mutations (update_group_v1, update_group_plugin, add_/remove_collections|groups|assets, etc.) invoke the
resize_or_reallocate_accounthelper, which automatically tops up lamports to keep the account rent-exempt. No code changes are strictly required unless you prefer to pre-allocate a small safety margin to avoid extra CPIs.Key locations:
programs/mpl-core/src/utils/account.rs–resize_or_reallocate_accounttransfers lamports as needed.programs/mpl-core/src/processor/update_group.rs:76– update_group_v1 callsresize_or_reallocate_account.programs/mpl-core/src/processor/update_group_plugin.rs:104– update_group_plugin callsresize_or_reallocate_account.- All other add/remove instructions under
programs/mpl-core/src/processor/*likewise use the helper.Optional margin diff in create_group.rs:
- let lamports = rent.minimum_balance(serialized_data.len()); + let lamports = rent.minimum_balance(serialized_data.len() + 256);Please ensure your developer documentation highlights the “resize & top-up” flow via
resize_or_reallocate_account.
| let group = GroupV1::load(ctx.accounts.group, 0)?; | ||
|
|
||
| // Ensure the signer is the update authority or delegate of the group. | ||
| if !is_valid_group_authority(ctx.accounts.group, authority)? { | ||
| return Err(MplCoreError::InvalidAuthority.into()); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Eliminate redundant deserialization of the group account
GroupV1::load is executed twice: once here (group) and again inside is_valid_group_authority.
This is harmless but costs ~1 μs and 32 B heap each time on-chain.
If you pass the already-deserialized group to the helper (or extend the helper to accept an optional &GroupV1), you avoid the duplicate Borsh read.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/close_group.rs around lines 34 to 38, the
group account is deserialized twice: once with GroupV1::load and again inside
is_valid_group_authority, causing unnecessary performance overhead. To fix this,
modify is_valid_group_authority to accept a reference to the already
deserialized GroupV1 instance and pass the group variable to it, eliminating the
redundant deserialization.
| assert_signer(payer_info)?; | ||
| let authority_info = resolve_authority(payer_info, authority_info_opt)?; | ||
|
|
||
| if system_program_info.key != &solana_program::system_program::ID { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant group deserialize
GroupV1::load is invoked at line 55 but its result is never used thereafter.
If the sole purpose is the authority check, you can drop this variable completely—is_valid_group_authority loads the account again.
- let group = GroupV1::load(group_info, 0)?;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/approve_group_plugin_authority.rs around
lines 47 to 50, the GroupV1::load call at line 55 is redundant because its
result is not used and the authority check is already performed by
is_valid_group_authority which loads the account again. Remove the GroupV1::load
call and the associated variable to eliminate unnecessary deserialization and
simplify the code.
| assert_signer(ctx.accounts.payer)?; | ||
| let authority = resolve_authority(ctx.accounts.payer, ctx.accounts.authority)?; | ||
|
|
There was a problem hiding this comment.
Signer check is bypassed when a distinct authority is supplied
assert_signer is executed only for payer.
When authority is different from payer, it may be non-signing, yet is_valid_group_authority will still accept it as long as the pubkey matches the stored update-authority or delegate.
An attacker could therefore:
- Sign with any funded
payeraccount. - Pass the legitimate authority account in
accounts[?]without a signature.
and gain full update capabilities.
@@
assert_signer(ctx.accounts.payer)?;
-let authority = resolve_authority(ctx.accounts.payer, ctx.accounts.authority)?;
+let authority = resolve_authority(ctx.accounts.payer, ctx.accounts.authority)?;
+
+// If the effective authority is NOT the payer, it must also be a signer.
+if authority.key != ctx.accounts.payer.key {
+ assert_signer(authority)?;
+}🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/update_group.rs around lines 34 to 36, the
code only asserts the signer status of the payer but not the authority, allowing
a non-signing authority to bypass checks. To fix this, add an assert_signer
check for the authority account when it differs from the payer to ensure the
authority has signed the transaction before proceeding with update operations.
| if let Some(new_update_authority) = ctx.accounts.new_update_authority { | ||
| group.update_authority = *new_update_authority.key; | ||
| dirty = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Changing update-authority without verification
Before assigning a new update authority, we should (a) ensure the supplied account is a signer to prevent silent hijacking, and (b) forbid setting it to the zero-pubkey.
@@
if let Some(new_update_authority) = ctx.accounts.new_update_authority {
- group.update_authority = *new_update_authority.key;
+ // Safety: new authority must explicitly sign the tx and be non-default.
+ assert_signer(new_update_authority)?;
+ if new_update_authority.key == &solana_program::pubkey::Pubkey::default() {
+ return Err(MplCoreError::InvalidAuthority.into());
+ }
+ group.update_authority = *new_update_authority.key;
dirty = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(new_update_authority) = ctx.accounts.new_update_authority { | |
| group.update_authority = *new_update_authority.key; | |
| dirty = true; | |
| } | |
| if let Some(new_update_authority) = ctx.accounts.new_update_authority { | |
| // Safety: new authority must explicitly sign the tx and be non-default. | |
| assert_signer(new_update_authority)?; | |
| if new_update_authority.key == &solana_program::pubkey::Pubkey::default() { | |
| return Err(MplCoreError::InvalidAuthority.into()); | |
| } | |
| group.update_authority = *new_update_authority.key; | |
| dirty = true; | |
| } |
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/update_group.rs around lines 54 to 57, before
assigning the new update authority, add a check to ensure the
new_update_authority account is a signer to prevent unauthorized changes, and
also verify that the new_update_authority key is not the zero public key to
avoid invalid assignments. If either condition fails, return an appropriate
error instead of updating the group.
| if accounts.len() < 4 { | ||
| return Err(ProgramError::NotEnoughAccountKeys); | ||
| } | ||
|
|
||
| let group_info = &accounts[0]; | ||
| let payer_info = &accounts[1]; | ||
| let authority_info_opt = accounts.get(2); | ||
| let system_program_info = &accounts[3]; | ||
|
|
||
| let collection_accounts = &accounts[4..]; | ||
|
|
There was a problem hiding this comment.
Same optional-authority indexing issue as in add_group_external_plugin_adapter
system_program is fixed at index 3, so omitting the authority breaks the processor.
Apply the same dynamic index logic as suggested for the previous file to keep the account layout consistent with the instruction docs.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/remove_collections_from_group.rs around lines
46 to 56, the system_program_info is incorrectly fixed at index 3, which breaks
the processor when the optional authority account is omitted. To fix this,
implement dynamic indexing for system_program_info based on whether
authority_info_opt is Some or None, similar to the approach used in
add_group_external_plugin_adapter, ensuring the account layout matches the
instruction documentation and handles the optional authority correctly.
| if size_diff != 0 { | ||
| plugin_registry.bump_offsets(record_offset, size_diff)?; | ||
| let new_registry_offset = (plugin_header.plugin_registry_offset as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)?; | ||
| plugin_header.plugin_registry_offset = new_registry_offset as usize; | ||
|
|
||
| let new_size = (collection_info.data_len() as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)? as usize; | ||
| resize_or_reallocate_account( | ||
| collection_info, | ||
| payer_info, | ||
| system_program_info, | ||
| new_size, | ||
| )?; | ||
|
|
||
| let next_plugin_offset = (record_offset + old_plugin_data.len()) as isize; | ||
| let new_next_plugin_offset = next_plugin_offset + size_diff; | ||
|
|
||
| unsafe { | ||
| let base_ptr = collection_info.data.borrow_mut().as_mut_ptr(); | ||
| sol_memmove( | ||
| base_ptr.add(new_next_plugin_offset as usize), | ||
| base_ptr.add(next_plugin_offset as usize), | ||
| plugin_header.plugin_registry_offset - next_plugin_offset as usize, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Possible unsigned-underflow & memory corruption when shrinking the plugin
When size_diff is negative (plugin shrinks), plugin_header.plugin_registry_offset is already mutated before the sol_memmove length is computed:
let len = plugin_header.plugin_registry_offset - next_plugin_offset as usize;
If plugin_header.plugin_registry_offset < next_plugin_offset, the subtraction underflows (usize wrap-around) causing an unbounded copy and memory corruption.
Recommended fix:
- sol_memmove(
- base_ptr.add(new_next_plugin_offset as usize),
- base_ptr.add(next_plugin_offset as usize),
- plugin_header.plugin_registry_offset - next_plugin_offset as usize,
- );
+ // Bytes to move = total_account_data_len - next_plugin_offset
+ let bytes_to_move = collection_info.data_len()
+ .checked_sub(next_plugin_offset as usize)
+ .ok_or(MplCoreError::NumericalOverflow)?;
+
+ sol_memmove(
+ base_ptr.add(new_next_plugin_offset as usize),
+ base_ptr.add(next_plugin_offset as usize),
+ bytes_to_move,
+ );Also consider delaying the plugin_header offset update until after the mem-move, or hold the old offset in a temporary to avoid such ordering hazards.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if size_diff != 0 { | |
| plugin_registry.bump_offsets(record_offset, size_diff)?; | |
| let new_registry_offset = (plugin_header.plugin_registry_offset as isize) | |
| .checked_add(size_diff) | |
| .ok_or(MplCoreError::NumericalOverflow)?; | |
| plugin_header.plugin_registry_offset = new_registry_offset as usize; | |
| let new_size = (collection_info.data_len() as isize) | |
| .checked_add(size_diff) | |
| .ok_or(MplCoreError::NumericalOverflow)? as usize; | |
| resize_or_reallocate_account( | |
| collection_info, | |
| payer_info, | |
| system_program_info, | |
| new_size, | |
| )?; | |
| let next_plugin_offset = (record_offset + old_plugin_data.len()) as isize; | |
| let new_next_plugin_offset = next_plugin_offset + size_diff; | |
| unsafe { | |
| let base_ptr = collection_info.data.borrow_mut().as_mut_ptr(); | |
| sol_memmove( | |
| base_ptr.add(new_next_plugin_offset as usize), | |
| base_ptr.add(next_plugin_offset as usize), | |
| plugin_header.plugin_registry_offset - next_plugin_offset as usize, | |
| ); | |
| } | |
| } | |
| if size_diff != 0 { | |
| plugin_registry.bump_offsets(record_offset, size_diff)?; | |
| let new_registry_offset = (plugin_header.plugin_registry_offset as isize) | |
| .checked_add(size_diff) | |
| .ok_or(MplCoreError::NumericalOverflow)?; | |
| plugin_header.plugin_registry_offset = new_registry_offset as usize; | |
| let new_size = (collection_info.data_len() as isize) | |
| .checked_add(size_diff) | |
| .ok_or(MplCoreError::NumericalOverflow)? as usize; | |
| resize_or_reallocate_account( | |
| collection_info, | |
| payer_info, | |
| system_program_info, | |
| new_size, | |
| )?; | |
| let next_plugin_offset = (record_offset + old_plugin_data.len()) as isize; | |
| let new_next_plugin_offset = next_plugin_offset + size_diff; | |
| unsafe { | |
| let base_ptr = collection_info.data.borrow_mut().as_mut_ptr(); | |
| // Bytes to move = total account data length − start of plugin registry | |
| let bytes_to_move = collection_info | |
| .data_len() | |
| .checked_sub(next_plugin_offset as usize) | |
| .ok_or(MplCoreError::NumericalOverflow)?; | |
| sol_memmove( | |
| base_ptr.add(new_next_plugin_offset as usize), | |
| base_ptr.add(next_plugin_offset as usize), | |
| bytes_to_move, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/remove_collections_from_group.rs around lines
182 to 210, the subtraction for the memmove length uses the updated
plugin_header.plugin_registry_offset, which can cause unsigned underflow and
memory corruption if the offset is smaller than next_plugin_offset. To fix this,
store the old plugin_registry_offset in a temporary variable before updating it,
use that old value to calculate the memmove length, and only update
plugin_header.plugin_registry_offset after the memmove operation to avoid
ordering hazards.
| let old_plugin_data = | ||
| Plugin::deserialize(&mut &collection_info.data.borrow()[record.offset..])? | ||
| .try_to_vec()?; | ||
| let new_plugin_data = plugin.try_to_vec()?; | ||
| let size_diff = (new_plugin_data.len() as isize) | ||
| .checked_sub(old_plugin_data.len() as isize) | ||
| .ok_or(MplCoreError::NumericalOverflow)?; | ||
|
|
||
| if size_diff != 0 { | ||
| // Bump offsets for subsequent registry entries and header. | ||
| plugin_registry.bump_offsets(record.offset, size_diff)?; | ||
| let new_registry_offset = (plugin_header.plugin_registry_offset as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)?; | ||
| plugin_header.plugin_registry_offset = new_registry_offset as usize; | ||
|
|
||
| // Resize account. | ||
| let new_size = (collection_info.data_len() as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)? | ||
| as usize; | ||
| resize_or_reallocate_account( | ||
| collection_info, | ||
| payer_info, | ||
| system_program_info, | ||
| new_size, | ||
| )?; | ||
|
|
||
| // Move trailing data to accommodate new plugin size. | ||
| let next_plugin_offset = (record.offset + old_plugin_data.len()) as isize; | ||
| let new_next_plugin_offset = next_plugin_offset + size_diff; | ||
|
|
||
| unsafe { | ||
| let base_ptr = collection_info.data.borrow_mut().as_mut_ptr(); | ||
| sol_memmove( | ||
| base_ptr.add(new_next_plugin_offset as usize), | ||
| base_ptr.add(next_plugin_offset as usize), | ||
| plugin_header.plugin_registry_offset - next_plugin_offset as usize, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential underflow / overflow when shrinking a plugin
When size_diff is negative (plugin shrinks), the expression:
plugin_header.plugin_registry_offset - next_plugin_offset as usizewill underflow because plugin_registry_offset < next_plugin_offset, yielding a huge
usize and causing sol_memmove to copy an absurd amount of memory.
Consider computing the trailing length safely:
- let next_plugin_offset = (record.offset + old_plugin_data.len()) as isize;
- let new_next_plugin_offset = next_plugin_offset + size_diff;
+ let next_plugin_offset = record.offset + old_plugin_data.len();
+ let new_next_plugin_offset = (next_plugin_offset as isize + size_diff) as usize;
+
+ // bytes after the plugin up to the registry
+ let trailing_len = plugin_header.plugin_registry_offset
+ .checked_sub(next_plugin_offset)
+ .ok_or(MplCoreError::NumericalOverflow)?;and then invoke sol_memmove(base_ptr.add(new_next_plugin_offset), …, trailing_len).
Failing to guard this path can corrupt the account data and brick the account.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/add_collections_to_group.rs between lines 200
and 240, the calculation for the length of trailing data to move when shrinking
a plugin can underflow because it subtracts next_plugin_offset from
plugin_header.plugin_registry_offset without checking their order. To fix this,
compute the trailing length safely by subtracting new_next_plugin_offset from
plugin_header.plugin_registry_offset after adjusting for size_diff, ensuring no
underflow occurs. Then pass this safe trailing length to sol_memmove instead of
the current subtraction expression.
| let authority_info = resolve_authority(payer_info, authority_info_opt)?; | ||
|
|
||
| if system_program_info.key != &solana_program::system_program::ID { | ||
| return Err(MplCoreError::InvalidSystemProgram.into()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nit: redundant signer check
assert_signer(payer_info)? is done, but later you resolve authority_info.
If the caller provides a delegate authority, it is that signer you care about –
so the signer assertion should be on authority_info (after resolution) rather
than always on the payer.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/add_collections_to_group.rs around lines 60
to 64, the code currently asserts the signer on payer_info before resolving
authority_info, which is redundant and incorrect if a delegate authority is
provided. Remove the signer assertion on payer_info and instead perform the
signer check on the resolved authority_info after calling resolve_authority.
This ensures the correct signer is validated based on the actual authority used.
| /// Returns true if the `authority_info` represents either the update authority of the asset | ||
| /// or a valid update delegate of the asset. | ||
| fn is_valid_asset_authority( | ||
| asset_info: &AccountInfo, | ||
| authority_info: &AccountInfo, | ||
| ) -> Result<bool, ProgramError> { | ||
| use crate::plugins::PluginType; | ||
| use crate::state::UpdateAuthority; | ||
|
|
||
| // Load asset core data | ||
| let asset_core = AssetV1::load(asset_info, 0)?; | ||
|
|
||
| // Fast path: signer is the update authority address (when address type) | ||
| if let UpdateAuthority::Address(addr) = asset_core.update_authority.clone() { | ||
| if addr == *authority_info.key { | ||
| return Ok(true); | ||
| } | ||
| } | ||
|
|
||
| // Attempt to locate an UpdateDelegate plugin on the asset. | ||
| if let Ok((_plugin_authority, plugin)) = | ||
| fetch_wrapped_plugin::<AssetV1>(asset_info, Some(&asset_core), PluginType::UpdateDelegate) | ||
| { | ||
| if let crate::plugins::Plugin::UpdateDelegate(update_delegate) = plugin { | ||
| if update_delegate | ||
| .additional_delegates | ||
| .contains(authority_info.key) | ||
| { | ||
| return Ok(true); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicate authority-validation logic – move to shared utils
is_valid_asset_authority repeats almost the same pattern that already exists for groups and collections (is_valid_group_authority, is_valid_collection_authority). Consider moving this function to utils/mod.rs (or extending an existing helper with a polymorphic check) to:
- keep all authority rules in one place,
- avoid future drift between implementations,
- shrink the on-chain binary a bit.
This is purely a refactor, but it will reduce maintenance overhead as more asset-level instructions arrive.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/create_group.rs around lines 51 to 85, the
function is_valid_asset_authority duplicates authority-validation logic found in
is_valid_group_authority and is_valid_collection_authority. Refactor by moving
this function to utils/mod.rs or by extending an existing helper function to
handle polymorphic authority checks. This consolidation will centralize
authority validation logic, reduce code duplication, prevent future
inconsistencies, and slightly reduce the on-chain binary size.
| /// Add the parent group pubkey to the collection's Groups plugin, creating the plugin if necessary. | ||
| fn process_collection_groups_plugin_add<'a>( | ||
| collection_info: &AccountInfo<'a>, | ||
| parent_group: Pubkey, | ||
| payer_info: &AccountInfo<'a>, | ||
| system_program_info: &AccountInfo<'a>, | ||
| ) -> ProgramResult { | ||
| // Ensure plugin metadata exists or create. | ||
| let (_collection_core, header_offset, mut plugin_header, mut plugin_registry) = | ||
| create_meta_idempotent::<CollectionV1>(collection_info, payer_info, system_program_info)?; | ||
|
|
||
| // Attempt to fetch existing Groups plugin. | ||
| let plugin_record_opt = plugin_registry | ||
| .registry | ||
| .iter() | ||
| .find(|r| r.plugin_type == PluginType::Groups) | ||
| .cloned(); | ||
|
|
||
| match plugin_record_opt { | ||
| None => { | ||
| // Plugin does not exist; create it. | ||
| let groups_plugin = Groups { | ||
| groups: vec![parent_group], | ||
| }; | ||
| let plugin = Plugin::Groups(groups_plugin); | ||
| initialize_plugin::<CollectionV1>( | ||
| &plugin, | ||
| &plugin.manager(), | ||
| header_offset, | ||
| &mut plugin_header, | ||
| &mut plugin_registry, | ||
| collection_info, | ||
| payer_info, | ||
| system_program_info, | ||
| )? | ||
| } | ||
| Some(record) => { | ||
| // Plugin exists, load, modify, and update. | ||
| let mut plugin = Plugin::load(collection_info, record.offset)?; | ||
| if let Plugin::Groups(inner) = &mut plugin { | ||
| if inner.groups.contains(&parent_group) { | ||
| // Already present, nothing to do. | ||
| return Ok(()); | ||
| } | ||
| inner.groups.push(parent_group); | ||
| } else { | ||
| return Err(MplCoreError::InvalidPlugin.into()); | ||
| } | ||
|
|
||
| // Serialize old and new plugin to compute size diff. | ||
| let old_plugin_data = | ||
| Plugin::deserialize(&mut &collection_info.data.borrow()[record.offset..])? | ||
| .try_to_vec()?; | ||
| let new_plugin_data = plugin.try_to_vec()?; | ||
| let size_diff = (new_plugin_data.len() as isize) | ||
| .checked_sub(old_plugin_data.len() as isize) | ||
| .ok_or(MplCoreError::NumericalOverflow)?; | ||
|
|
||
| if size_diff != 0 { | ||
| // Bump offsets for subsequent registry entries and header. | ||
| plugin_registry.bump_offsets(record.offset, size_diff)?; | ||
| let new_registry_offset = (plugin_header.plugin_registry_offset as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)?; | ||
| plugin_header.plugin_registry_offset = new_registry_offset as usize; | ||
|
|
||
| // Resize account. | ||
| let new_size = (collection_info.data_len() as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)? | ||
| as usize; | ||
| resize_or_reallocate_account( | ||
| collection_info, | ||
| payer_info, | ||
| system_program_info, | ||
| new_size, | ||
| )?; | ||
|
|
||
| // Move trailing data to accommodate new plugin size. | ||
| let next_plugin_offset = (record.offset + old_plugin_data.len()) as isize; | ||
| let new_next_plugin_offset = next_plugin_offset + size_diff; | ||
|
|
||
| unsafe { | ||
| let base_ptr = collection_info.data.borrow_mut().as_mut_ptr(); | ||
| sol_memmove( | ||
| base_ptr.add(new_next_plugin_offset as usize), | ||
| base_ptr.add(next_plugin_offset as usize), | ||
| plugin_header.plugin_registry_offset - next_plugin_offset as usize, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Save header, registry and new plugin. | ||
| plugin_header.save(collection_info, header_offset)?; | ||
| plugin_registry.save(collection_info, plugin_header.plugin_registry_offset)?; | ||
| plugin.save(collection_info, record.offset)?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Add the asset pubkey to the group's Assets plugin, creating the plugin if necessary. | ||
| fn process_asset_groups_plugin_add<'a>( | ||
| asset_info: &AccountInfo<'a>, | ||
| group: Pubkey, | ||
| payer_info: &AccountInfo<'a>, | ||
| system_program_info: &AccountInfo<'a>, | ||
| ) -> ProgramResult { | ||
| // Ensure plugin metadata exists or create. | ||
| let (_asset_core, header_offset, mut plugin_header, mut plugin_registry) = | ||
| create_meta_idempotent::<AssetV1>(asset_info, payer_info, system_program_info)?; | ||
|
|
||
| // Attempt to fetch existing Assets plugin. | ||
| let plugin_record_opt = plugin_registry | ||
| .registry | ||
| .iter() | ||
| .find(|r| r.plugin_type == PluginType::Groups) | ||
| .cloned(); | ||
|
|
||
| match plugin_record_opt { | ||
| None => { | ||
| // Plugin does not exist; create it. | ||
| let assets_plugin = Groups { | ||
| groups: vec![group], | ||
| }; | ||
| let plugin = Plugin::Groups(assets_plugin); | ||
| initialize_plugin::<AssetV1>( | ||
| &plugin, | ||
| &plugin.manager(), | ||
| header_offset, | ||
| &mut plugin_header, | ||
| &mut plugin_registry, | ||
| asset_info, | ||
| payer_info, | ||
| system_program_info, | ||
| )? | ||
| } | ||
| Some(record) => { | ||
| // Plugin exists, load, modify, and update. | ||
| let mut plugin = Plugin::load(asset_info, record.offset)?; | ||
| if let Plugin::Groups(inner) = &mut plugin { | ||
| if inner.groups.contains(&group) { | ||
| // Already present, nothing to do. | ||
| return Ok(()); | ||
| } | ||
| inner.groups.push(group); | ||
| } else { | ||
| return Err(MplCoreError::InvalidPlugin.into()); | ||
| } | ||
|
|
||
| // Serialize old and new plugin to compute size diff. | ||
| let old_plugin_data = | ||
| Plugin::deserialize(&mut &asset_info.data.borrow()[record.offset..])? | ||
| .try_to_vec()?; | ||
| let new_plugin_data = plugin.try_to_vec()?; | ||
| let size_diff = (new_plugin_data.len() as isize) | ||
| .checked_sub(old_plugin_data.len() as isize) | ||
| .ok_or(MplCoreError::NumericalOverflow)?; | ||
|
|
||
| if size_diff != 0 { | ||
| // Bump offsets for subsequent registry entries and header. | ||
| plugin_registry.bump_offsets(record.offset, size_diff)?; | ||
| let new_registry_offset = (plugin_header.plugin_registry_offset as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)?; | ||
| plugin_header.plugin_registry_offset = new_registry_offset as usize; | ||
|
|
||
| // Resize account. | ||
| let new_size = (asset_info.data_len() as isize) | ||
| .checked_add(size_diff) | ||
| .ok_or(MplCoreError::NumericalOverflow)? | ||
| as usize; | ||
| resize_or_reallocate_account( | ||
| asset_info, | ||
| payer_info, | ||
| system_program_info, | ||
| new_size, | ||
| )?; | ||
|
|
||
| // Move trailing data to accommodate new plugin size. | ||
| let next_plugin_offset = (record.offset + old_plugin_data.len()) as isize; | ||
| let new_next_plugin_offset = next_plugin_offset + size_diff; | ||
|
|
||
| unsafe { | ||
| let base_ptr = asset_info.data.borrow_mut().as_mut_ptr(); | ||
| sol_memmove( | ||
| base_ptr.add(new_next_plugin_offset as usize), | ||
| base_ptr.add(next_plugin_offset as usize), | ||
| plugin_header.plugin_registry_offset - next_plugin_offset as usize, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Save header, registry and new plugin. | ||
| plugin_header.save(asset_info, header_offset)?; | ||
| plugin_registry.save(asset_info, plugin_header.plugin_registry_offset)?; | ||
| plugin.save(asset_info, record.offset)?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consolidate nearly identical process_*_groups_plugin_add helpers
process_collection_groups_plugin_add and process_asset_groups_plugin_add are ~95 % identical. Duplicated unsafe sol_memmove logic is especially error-prone.
You can DRY this up with a generic helper:
fn process_groups_plugin_add<'a, C: SolanaAccount>(
target_info: &AccountInfo<'a>,
group: Pubkey,
payer_info: &AccountInfo<'a>,
system_program_info: &AccountInfo<'a>,
) -> ProgramResult {
// generic body that was duplicated …
}- process_collection_groups_plugin_add(...)
+ process_groups_plugin_add::<CollectionV1>(...)
- process_asset_groups_plugin_add(...)
+ process_groups_plugin_add::<AssetV1>(...)Benefits: smaller binary, one place to fix future bugs, and centralised unsafe memory juggling.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/create_group.rs from lines 326 to 528, the
functions process_collection_groups_plugin_add and
process_asset_groups_plugin_add contain nearly identical code including unsafe
sol_memmove logic. Refactor by creating a single generic function
process_groups_plugin_add that takes a generic type parameter for the account
type (e.g., CollectionV1 or AssetV1) and consolidates the shared logic. Replace
the two existing functions with calls to this generic helper, passing the
appropriate type and parameters. This will reduce code duplication, centralize
unsafe memory operations, and simplify future maintenance.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
programs/mpl-core/src/processor/add_collections_to_group.rs (2)
49-52: Redundant signer assertion (duplicate of earlier feedback)Same issue seen in previous PR reviews: signer should be enforced on the resolved authority, not always on
payer_info.
Tagging as duplicate for traceability.
200-211: Underflow when shrinking plugin (duplicate of earlier feedback)
plugin_header.plugin_registry_offset - next_plugin_offsetwill wrap ifsize_diffis negative.
Computetrailing_lenwithchecked_subas suggested in prior review to prevent memory corruption.programs/mpl-core/src/instruction.rs (1)
317-444:⚠️ Potential issueEnum-variant insertion risks ABI break
MplAssetInstructionis Borsh-encoded by ordinal. Inserting group-related variants afterUpdateCollectionInfoV1shifts every subsequent index.
Unless this position was originally the end of the enum, existing clients will fail to decode instructions.Options:
- Append all new variants after the previous last variant.
- Assign explicit discriminants via
#[repr(u16)]to freeze indices.- Version the enum.
Please confirm and adjust before main-net deployment.
programs/mpl-core/src/processor/create_group.rs (2)
47-81: 🧹 Nitpick (assertive)Duplicate authority-validation logic – move to shared utils
is_valid_asset_authorityrepeats almost the same pattern that already exists for groups and collections (is_valid_group_authority,is_valid_collection_authority). Consider moving this function toutils/mod.rsto keep all authority rules in one place and avoid future drift between implementations.
338-540: 🛠️ Refactor suggestionConsolidate nearly identical
process_*_groups_plugin_addhelpers
process_collection_groups_plugin_addandprocess_asset_groups_plugin_addare ~95% identical. Duplicated unsafesol_memmovelogic is especially error-prone.You can DRY this up with a generic helper that takes a type parameter for the account type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
programs/mpl-core/src/error.rs(1 hunks)programs/mpl-core/src/instruction.rs(2 hunks)programs/mpl-core/src/plugins/mod.rs(8 hunks)programs/mpl-core/src/processor/add_assets_to_group.rs(1 hunks)programs/mpl-core/src/processor/add_collections_to_group.rs(1 hunks)programs/mpl-core/src/processor/create_group.rs(1 hunks)programs/mpl-core/src/state/group.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
programs/mpl-core/src/state/group.rs (2)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
programs/mpl-core/src/instruction.rs (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
🧬 Code Graph Analysis (2)
programs/mpl-core/src/processor/add_assets_to_group.rs (4)
programs/mpl-core/src/processor/create_group.rs (7)
create_meta_idempotent(347-347)create_meta_idempotent(449-449)initialize_plugin(363-363)initialize_plugin(465-465)is_valid_asset_authority(49-81)fetch_wrapped_plugin(68-68)process_asset_groups_plugin_add(441-540)programs/mpl-core/src/processor/remove_assets_from_group.rs (3)
create_meta_idempotent(136-136)is_valid_asset_authority(26-55)fetch_wrapped_plugin(42-42)programs/mpl-core/src/utils/mod.rs (4)
is_valid_group_authority(582-608)resolve_authority(567-578)fetch_wrapped_plugin(594-594)fetch_wrapped_plugin(622-622)programs/mpl-core/src/utils/account.rs (1)
resize_or_reallocate_account(37-74)
programs/mpl-core/src/processor/create_group.rs (5)
programs/mpl-core/src/processor/add_assets_to_group.rs (5)
create_meta_idempotent(149-149)initialize_plugin(162-162)fetch_wrapped_plugin(49-49)is_valid_asset_authority(30-62)process_asset_groups_plugin_add(142-220)programs/mpl-core/src/processor/add_collections_to_group.rs (3)
create_meta_idempotent(130-130)initialize_plugin(146-146)process_collection_groups_plugin_add(122-222)programs/mpl-core/src/utils/mod.rs (5)
is_valid_collection_authority(612-638)is_valid_group_authority(582-608)resolve_authority(567-578)fetch_wrapped_plugin(594-594)fetch_wrapped_plugin(622-622)programs/mpl-core/src/utils/account.rs (1)
resize_or_reallocate_account(37-74)programs/mpl-core/src/state/group.rs (2)
new(46-65)len(96-104)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Programs / Test (mpl-core)
- GitHub Check: Programs / Build
🔇 Additional comments (5)
programs/mpl-core/src/error.rs (1)
215-222: Confirm error‐code continuityCodes 51 & 52 look fine, but we now have three “Numerical-overflow” style variants (32, 76, and 78). Double-check that external consumers rely on the numeric value, not the variant name, and ensure there are no duplicate numeric codes elsewhere.
programs/mpl-core/src/plugins/mod.rs (1)
111-112:GroupsimplementsPluginValidation?
inner()andlen()dispatch toGroups, which must implementPluginValidationandDataBlob. If either impl is missing, the program will panic at runtime when loading the plugin. Confirm the impls exist ininternal/authority_managed/groups.rs.Also applies to: 144-145
programs/mpl-core/src/state/group.rs (1)
154-181: Test coverage looks good!The test properly validates that the calculated length matches the serialized length for different group configurations.
programs/mpl-core/src/processor/create_group.rs (2)
117-123: Good duplicate detection implementation!The use of HashSet to prevent duplicate entries across all relationship categories effectively addresses potential issues with duplicate pubkeys.
176-183: Comprehensive error handling and validationThe implementation includes thorough validation for each relationship type with clear error messages, proper authority checks, and account verification.
Also applies to: 189-333
| // serialize adjustments | ||
| let old_data = Plugin::deserialize(&mut &asset_info.data.borrow()[record.offset..])? | ||
| .try_to_vec()?; | ||
| let new_data = plugin.try_to_vec()?; | ||
| let size_diff = new_data.len() as isize - old_data.len() as isize; | ||
| if size_diff != 0 { | ||
| plugin_registry.bump_offsets(record.offset, size_diff)?; | ||
| plugin_header.plugin_registry_offset = | ||
| (plugin_header.plugin_registry_offset as isize + size_diff) as usize; | ||
|
|
||
| let new_size = (asset_info.data_len() as isize + size_diff) as usize; | ||
| resize_or_reallocate_account( | ||
| asset_info, | ||
| payer_info, | ||
| system_program_info, | ||
| new_size, | ||
| )?; | ||
|
|
||
| let next_offset = (record.offset + old_data.len()) as isize; | ||
| let new_next_offset = next_offset + size_diff; | ||
| unsafe { | ||
| let base_ptr = asset_info.data.borrow_mut().as_mut_ptr(); | ||
| sol_memmove( | ||
| base_ptr.add(new_next_offset as usize), | ||
| base_ptr.add(next_offset as usize), | ||
| plugin_header.plugin_registry_offset - next_offset as usize, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unchecked arithmetic & potential under/overflow when moving trailing data
plugin_header.plugin_registry_offset and new_size are updated with raw casts:
plugin_header.plugin_registry_offset =
(plugin_header.plugin_registry_offset as isize + size_diff) as usize;
let new_size = (asset_info.data_len() as isize + size_diff) as usize;and sol_memmove copies:
plugin_header.plugin_registry_offset - next_offset as usizeIf size_diff is negative these subtractions can underflow, corrupting account data.
Reuse the checked pattern already present in create_group.rs (checked_add, safe trailing_len calc) to eliminate UB.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/add_assets_to_group.rs between lines 185 and
213, the code performs unchecked arithmetic with raw casts from isize to usize
when updating plugin_header.plugin_registry_offset and calculating new_size, and
also in the sol_memmove length calculation, which can cause underflow or
overflow if size_diff is negative. To fix this, replace all raw casts with
checked_add or checked_sub methods to safely handle potential negative size_diff
values, and compute the length for sol_memmove using a safe checked calculation
pattern similar to the one used in create_group.rs, ensuring no unsigned
underflow or overflow occurs.
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (12)
programs/mpl-core/src/processor/close_group.rs (1)
34-38: Eliminate redundant deserialization of the group account
GroupV1::loadis executed twice: once here (group) and again insideis_valid_group_authority. This is harmless but costs ~1 μs and 32 B heap each time on-chain.If you pass the already-deserialized
groupto the helper (or extend the helper to accept an optional&GroupV1), you avoid the duplicate Borsh read.programs/mpl-core/src/processor/add_assets_to_group.rs (3)
194-201: Potential integer overflow when adjusting offsets & size
size_diffis added directly to bothplugin_header.plugin_registry_offsetandasset_info.data_len()without bounds checking. The reference implementation increate_group.rsuseschecked_addand returnsNumericalOverflowon failure. Current code can silently wrap on large values leading to memory corruption.- plugin_header.plugin_registry_offset = - (plugin_header.plugin_registry_offset as isize + size_diff) as usize; + plugin_header.plugin_registry_offset = (plugin_header + .plugin_registry_offset as isize) + .checked_add(size_diff) + .ok_or(MplCoreError::NumericalOverflow)? as usize; - let new_size = (asset_info.data_len() as isize + size_diff) as usize; + let new_size = (asset_info.data_len() as isize) + .checked_add(size_diff) + .ok_or(MplCoreError::NumericalOverflow)? as usize;
147-230: Extract duplicated plugin management logic
process_asset_groups_plugin_addduplicates logic found increate_group.rs. Extract this into a shared utility function to reduce maintenance burden and prevent inconsistencies.Consider extracting the common plugin upsert logic into a utility function like
utils::plugins::upsert_groups_plugin_on_assetthat can be reused across processors.
103-111: Add owner check for asset accountsThe processor should verify that each asset account is owned by the mpl-core program to prevent passing arbitrary accounts. This validation is missing and could lead to security issues.
for asset_info in remaining_accounts.iter() { + // Verify account is owned by this program + if asset_info.owner != &crate::ID { + msg!("Error: Asset account not owned by mpl-core program"); + return Err(ProgramError::InvalidAccountData); + } + if !asset_info.is_writable { msg!("Error: Asset account must be writable"); return Err(ProgramError::InvalidAccountData); }programs/mpl-core/src/processor/remove_assets_from_group.rs (2)
160-165: Unchecked arithmetic operations
plugin_header.plugin_registry_offsetandnew_sizeare updated with raw casts without overflow checks.- plugin_header.plugin_registry_offset = - (plugin_header.plugin_registry_offset as isize + size_diff) as usize; - let new_size = (asset_info.data_len() as isize + size_diff) as usize; + plugin_header.plugin_registry_offset = (plugin_header + .plugin_registry_offset as isize) + .checked_add(size_diff) + .ok_or(MplCoreError::NumericalOverflow)? as usize; + let new_size = (asset_info.data_len() as isize) + .checked_add(size_diff) + .ok_or(MplCoreError::NumericalOverflow)? as usize;
147-152: Remove empty Groups plugin to reclaim spaceAfter removing the group from
inner.groups, the plugin may become empty. Consider removing the entire plugin to reclaim account space and avoid unnecessary registry entries.if let Some(pos) = inner.groups.iter().position(|pk| pk == &parent_group) { inner.groups.remove(pos); + // If groups list is now empty, remove the plugin entirely + if inner.groups.is_empty() { + // Remove the registry entry and adjust offsets + plugin_registry.registry.remove(index); + // Adjust all subsequent offsets + let removed_size = old_data.len(); + for record in plugin_registry.registry.iter_mut().skip(index) { + record.offset = record.offset.saturating_sub(removed_size); + } + // Update header offset + plugin_header.plugin_registry_offset = + plugin_header.plugin_registry_offset.saturating_sub(removed_size); + // Save and resize account... + return Ok(()); + } } else { return Ok(()); }programs/mpl-core/src/processor/add_collections_to_group.rs (2)
50-51: Redundant signer check on payerThe signer check on
payer_infois redundant sinceresolve_authoritywill use the payer as the default authority if no explicit authority is provided. The critical signer check should be on the resolvedauthority_info.
200-217: Potential underflow when shrinking a pluginWhen
size_diffis negative (plugin shrinks), the calculation at line 206:let copy_len = plugin_header .plugin_registry_offset .saturating_sub(next_plugin_offset as usize);While
saturating_subprevents panic, it might silently produce incorrect results. When shrinking,plugin_header.plugin_registry_offsethas already been updated at line 186, so this calculation uses the new offset instead of the old one.Consider storing the old registry offset before updating it:
+ let old_registry_offset = plugin_header.plugin_registry_offset; let new_registry_offset = (plugin_header.plugin_registry_offset as isize) .checked_add(size_diff) .ok_or(MplCoreError::NumericalOverflow)?; plugin_header.plugin_registry_offset = new_registry_offset as usize; // ... later in the memmove section ... - let copy_len = plugin_header - .plugin_registry_offset - .saturating_sub(next_plugin_offset as usize); + let copy_len = old_registry_offset + .saturating_sub(next_plugin_offset as usize);programs/mpl-core/src/processor/remove_collections_from_group.rs (2)
46-56: Optional authority indexing breaks when authority is omittedThe system_program is hardcoded at index 3, which assumes the optional authority is always present. When the authority is truly omitted, this will cause an out-of-bounds access.
182-216: Memory corruption risk when shrinking pluginThe
plugin_header.plugin_registry_offsetis already updated at line 187 before calculating the memmove length at line 203. This can cause underflow and memory corruption when the plugin shrinks.programs/mpl-core/src/processor/create_group.rs (2)
47-81: Move authority validation to shared utilitiesThe
is_valid_asset_authorityfunction duplicates the same pattern already implemented for groups and collections. This should be moved toutils/mod.rsto maintain consistency and reduce code duplication.
338-552: Consolidate duplicate plugin helper functionsThe
process_collection_groups_plugin_addandprocess_asset_groups_plugin_addfunctions are nearly identical with ~95% code duplication. This includes critical unsafe memory operations that should be centralized.Create a generic helper to reduce duplication and centralize the unsafe memory handling:
fn process_groups_plugin_add<'a, T: SolanaAccount>( account_info: &AccountInfo<'a>, group: Pubkey, payer_info: &AccountInfo<'a>, system_program_info: &AccountInfo<'a>, ) -> ProgramResult { // unified implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (57)
Cargo.lockis excluded by!**/*.lockclients/js/src/generated/accounts/groupV1.tsis excluded by!**/generated/**clients/js/src/generated/accounts/index.tsis excluded by!**/generated/**clients/js/src/generated/errors/mplCore.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addAssetsToGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addCollectionsToGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addGroupExternalPluginAdapterV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addGroupPluginV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addGroupsToGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/approveGroupPluginAuthorityV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/closeGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/createGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/index.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeAssetsFromGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeCollectionsFromGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeGroupExternalPluginAdapterV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeGroupPluginV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeGroupsFromGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/revokeGroupPluginAuthorityV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/updateGroupPluginV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/updateGroupV1.tsis excluded by!**/generated/**clients/js/src/generated/instructions/writeGroupExternalPluginAdapterDataV1.tsis excluded by!**/generated/**clients/js/src/generated/types/groups.tsis excluded by!**/generated/**clients/js/src/generated/types/index.tsis excluded by!**/generated/**clients/js/src/generated/types/key.tsis excluded by!**/generated/**clients/js/src/generated/types/plugin.tsis excluded by!**/generated/**clients/js/src/generated/types/pluginType.tsis excluded by!**/generated/**clients/js/src/generated/types/relationshipEntry.tsis excluded by!**/generated/**clients/js/src/generated/types/relationshipKind.tsis excluded by!**/generated/**clients/rust/src/generated/accounts/group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/accounts/mod.rsis excluded by!**/generated/**clients/rust/src/generated/errors/mpl_core.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_assets_to_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_collections_to_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_group_external_plugin_adapter_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_group_plugin_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_groups_to_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/approve_group_plugin_authority_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/close_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/create_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/mod.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_assets_from_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_collections_from_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_group_external_plugin_adapter_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_group_plugin_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_groups_from_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/revoke_group_plugin_authority_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/update_group_plugin_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/update_group_v1.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/write_group_external_plugin_adapter_data_v1.rsis excluded by!**/generated/**clients/rust/src/generated/types/groups.rsis excluded by!**/generated/**clients/rust/src/generated/types/key.rsis excluded by!**/generated/**clients/rust/src/generated/types/mod.rsis excluded by!**/generated/**clients/rust/src/generated/types/plugin.rsis excluded by!**/generated/**clients/rust/src/generated/types/plugin_type.rsis excluded by!**/generated/**clients/rust/src/generated/types/relationship_entry.rsis excluded by!**/generated/**clients/rust/src/generated/types/relationship_kind.rsis excluded by!**/generated/**
📒 Files selected for processing (50)
clients/js/package.json(1 hunks)clients/js/src/hooked/groupAccountData.ts(1 hunks)clients/js/src/hooked/index.ts(1 hunks)clients/js/src/instructions/collection/index.ts(1 hunks)clients/js/src/instructions/collection/writeCollectionData.ts(1 hunks)clients/js/src/instructions/group/addAssetsToGroup.ts(1 hunks)clients/js/src/instructions/group/addCollectionsToGroup.ts(1 hunks)clients/js/src/instructions/group/addGroupPlugin.ts(1 hunks)clients/js/src/instructions/group/addGroupsToGroup.ts(1 hunks)clients/js/src/instructions/group/approveGroupPluginAuthority.ts(1 hunks)clients/js/src/instructions/group/closeGroup.ts(1 hunks)clients/js/src/instructions/group/createGroup.ts(1 hunks)clients/js/src/instructions/group/index.ts(1 hunks)clients/js/src/instructions/group/removeAssetsFromGroup.ts(1 hunks)clients/js/src/instructions/group/removeCollectionsFromGroup.ts(1 hunks)clients/js/src/instructions/group/removeGroupPlugin.ts(1 hunks)clients/js/src/instructions/group/removeGroupsFromGroup.ts(1 hunks)clients/js/src/instructions/group/revokeGroupPluginAuthority.ts(1 hunks)clients/js/src/instructions/group/updateGroup.ts(1 hunks)clients/js/src/instructions/group/updateGroupPlugin.ts(1 hunks)clients/js/src/instructions/group/writeGroupData.ts(1 hunks)clients/js/src/instructions/index.ts(1 hunks)clients/js/src/plugins/lib.ts(2 hunks)clients/js/src/plugins/types.ts(2 hunks)clients/js/test/_setupRaw.ts(4 hunks)clients/js/test/approveGroupPluginAuthority.test.ts(1 hunks)clients/js/test/closeGroup.test.ts(1 hunks)clients/js/test/compute.test.ts(1 hunks)clients/js/test/createGroup.test.ts(1 hunks)clients/js/test/group.test.ts(1 hunks)clients/js/test/groupAdditionalPlugins.test.ts(1 hunks)clients/js/test/groupExternalPluginAdapters.test.ts(1 hunks)clients/js/test/groupPlugins.test.ts(1 hunks)clients/js/test/groupRelations.test.ts(1 hunks)clients/js/test/updateGroupAuthority.test.ts(1 hunks)clients/rust/Cargo.toml(1 hunks)clients/rust/src/hooked/mod.rs(1 hunks)clients/rust/src/hooked/plugin.rs(1 hunks)configs/kinobi.cjs(1 hunks)idls/mpl_core.json(19 hunks)package.json(1 hunks)programs/mpl-core/src/processor/add_assets_to_group.rs(1 hunks)programs/mpl-core/src/processor/add_collections_to_group.rs(1 hunks)programs/mpl-core/src/processor/close_group.rs(1 hunks)programs/mpl-core/src/processor/create_group.rs(1 hunks)programs/mpl-core/src/processor/remove_assets_from_group.rs(1 hunks)programs/mpl-core/src/processor/remove_collections_from_group.rs(1 hunks)programs/mpl-core/src/processor/update.rs(1 hunks)programs/mpl-core/src/processor/update_external_plugin_adapter.rs(1 hunks)programs/mpl-core/src/processor/update_plugin.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts have inconsistent documentation - sometimes described as "The collection to which the asset belongs" and sometimes as "The address of the asset" or "The address of the new asset". The team has chosen to maintain existing patterns for consistency rather than correcting individual instances.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
clients/rust/Cargo.toml (1)
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: programs/mpl-core/src/plugins/internal/permanent/bubblegum_v2.rs:20-31
Timestamp: 2025-05-08T00:27:45.972Z
Learning: In Rust code, use `pub(crate)` visibility modifier for constants, structs, and functions that only need to be accessed within the crate, rather than using `pub` to keep the public API surface minimal and prevent unintended coupling.
clients/js/test/updateGroupAuthority.test.ts (2)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
programs/mpl-core/src/processor/update_plugin.rs (3)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
clients/js/src/plugins/lib.ts (2)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: clients/js/test/plugins/asset/bubblegumV2.test.ts:8-16
Timestamp: 2025-05-07T18:45:46.317Z
Learning: The `createPlugin` function in mpl-core is deprecated. The simple object literal approach (e.g., `{ type: 'BubblegumV2' }`) is appropriate for testing plugin creation in mpl-core tests.
clients/js/src/instructions/group/addAssetsToGroup.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
clients/js/test/group.test.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
programs/mpl-core/src/processor/update_external_plugin_adapter.rs (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
programs/mpl-core/src/processor/close_group.rs (2)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
clients/js/src/instructions/group/addGroupsToGroup.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
clients/js/test/groupAdditionalPlugins.test.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
clients/js/src/instructions/group/removeAssetsFromGroup.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
programs/mpl-core/src/processor/update.rs (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
clients/js/src/instructions/group/updateGroupPlugin.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
clients/js/src/instructions/group/approveGroupPluginAuthority.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
clients/js/test/groupPlugins.test.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
programs/mpl-core/src/processor/add_assets_to_group.rs (9)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts have inconsistent documentation - sometimes described as "The collection to which the asset belongs" and sometimes as "The address of the asset" or "The address of the new asset". The team has chosen to maintain existing patterns for consistency rather than correcting individual instances.
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts are consistently documented as "The address of the asset" even though they are collection accounts, and this pattern is intentionally maintained for consistency.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs:38-47
Timestamp: 2025-06-05T12:01:38.260Z
Learning: In the mpl-core codebase, "optional" accounts use a pattern where None/absent accounts are represented by the program ID as a placeholder in the accounts array. This means the accounts array length remains constant and predictable, avoiding variable-length account arrays and off-by-one indexing errors.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:99-103
Timestamp: 2025-06-24T20:50:30.534Z
Learning: In the mpl-core codebase, writable checks are generally ignored because the Solana runtime will automatically throw an error if a readonly account is written to, making explicit writable checks redundant.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: programs/mpl-core/src/plugins/internal/permanent/bubblegum_v2.rs:40-61
Timestamp: 2025-05-08T00:51:20.026Z
Learning: In the mpl-core codebase, danenbm prefers to maintain separate conditional branches with distinct explanatory comments for better readability, even when the outcome (like using `reject!()`) is the same.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
clients/js/src/instructions/group/revokeGroupPluginAuthority.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
clients/js/test/approveGroupPluginAuthority.test.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
programs/mpl-core/src/processor/remove_assets_from_group.rs (4)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:99-103
Timestamp: 2025-06-24T20:50:30.534Z
Learning: In the mpl-core codebase, writable checks are generally ignored because the Solana runtime will automatically throw an error if a readonly account is written to, making explicit writable checks redundant.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
programs/mpl-core/src/processor/remove_collections_from_group.rs (3)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs:38-47
Timestamp: 2025-06-05T12:01:38.260Z
Learning: In the mpl-core codebase, "optional" accounts use a pattern where None/absent accounts are represented by the program ID as a placeholder in the accounts array. This means the accounts array length remains constant and predictable, avoiding variable-length account arrays and off-by-one indexing errors.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
programs/mpl-core/src/processor/add_collections_to_group.rs (7)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts have inconsistent documentation - sometimes described as "The collection to which the asset belongs" and sometimes as "The address of the asset" or "The address of the new asset". The team has chosen to maintain existing patterns for consistency rather than correcting individual instances.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs:38-47
Timestamp: 2025-06-05T12:01:38.260Z
Learning: In the mpl-core codebase, "optional" accounts use a pattern where None/absent accounts are represented by the program ID as a placeholder in the accounts array. This means the accounts array length remains constant and predictable, avoiding variable-length account arrays and off-by-one indexing errors.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:99-103
Timestamp: 2025-06-24T20:50:30.534Z
Learning: In the mpl-core codebase, writable checks are generally ignored because the Solana runtime will automatically throw an error if a readonly account is written to, making explicit writable checks redundant.
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts are consistently documented as "The address of the asset" even though they are collection accounts, and this pattern is intentionally maintained for consistency.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
clients/js/test/_setupRaw.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
clients/js/src/plugins/types.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
programs/mpl-core/src/processor/create_group.rs (5)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs:38-47
Timestamp: 2025-06-05T12:01:38.260Z
Learning: In the mpl-core codebase, "optional" accounts use a pattern where None/absent accounts are represented by the program ID as a placeholder in the accounts array. This means the accounts array length remains constant and predictable, avoiding variable-length account arrays and off-by-one indexing errors.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/plugins/internal/authority_managed/groups.rs:19-22
Timestamp: 2025-06-05T12:02:41.859Z
Learning: In the mpl-core codebase, Pubkey sizes are consistently represented using the literal value `32` rather than a constant like `Pubkey::LEN`. This is the established pattern throughout the codebase, seen in structs like GroupV1, Authority, UpdateDelegate, and various plugin implementations.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
idls/mpl_core.json (6)
undefined
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
</retrieved_learning>
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
</retrieved_learning>
<retrieved_learning>
Learnt from: danenbm
PR: #225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts have inconsistent documentation - sometimes described as "The collection to which the asset belongs" and sometimes as "The address of the asset" or "The address of the new asset". The team has chosen to maintain existing patterns for consistency rather than correcting individual instances.
</retrieved_learning>
<retrieved_learning>
Learnt from: danenbm
PR: #225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts are consistently documented as "The address of the asset" even though they are collection accounts, and this pattern is intentionally maintained for consistency.
</retrieved_learning>
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs:38-47
Timestamp: 2025-06-05T12:01:38.260Z
Learning: In the mpl-core codebase, "optional" accounts use a pattern where None/absent accounts are represented by the program ID as a placeholder in the accounts array. This means the accounts array length remains constant and predictable, avoiding variable-length account arrays and off-by-one indexing errors.
</retrieved_learning>
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
</retrieved_learning>
🧬 Code Graph Analysis (23)
clients/rust/src/hooked/plugin.rs (2)
clients/js/src/generated/types/plugin.ts (1)
Plugin(71-88)clients/js/src/generated/types/groups.ts (1)
Groups(17-17)
clients/js/test/updateGroupAuthority.test.ts (2)
clients/js/src/instructions/group/updateGroup.ts (1)
updateGroup(6-9)clients/js/test/_setupRaw.ts (2)
assertGroup(306-345)DEFAULT_GROUP(59-62)
clients/rust/src/hooked/mod.rs (2)
clients/js/src/generated/types/plugin.ts (1)
Plugin(71-88)clients/js/src/generated/types/groups.ts (1)
Groups(17-17)
clients/js/src/plugins/lib.ts (2)
clients/js/src/plugins/types.ts (5)
GroupAllPluginArgsV2(231-231)BasePlugin(37-40)AssetAllPluginArgsV2(158-160)GroupAddablePluginAuthorityPairArgsV2(235-236)AssetPluginAuthorityPairArgsV2(161-162)clients/js/src/generated/types/pluginAuthorityPair.ts (1)
PluginAuthorityPair(24-27)
clients/js/test/closeGroup.test.ts (2)
clients/js/src/instructions/group/closeGroup.ts (1)
closeGroup(6-9)clients/js/test/_setupRaw.ts (1)
assertBurned(347-358)
clients/js/test/createGroup.test.ts (1)
clients/js/test/_setupRaw.ts (2)
assertGroup(306-345)DEFAULT_GROUP(59-62)
clients/js/src/instructions/group/addGroupsToGroup.ts (1)
clients/js/src/generated/instructions/addGroupsToGroupV1.ts (1)
addGroupsToGroupV1(81-154)
clients/js/test/groupExternalPluginAdapters.test.ts (5)
clients/js/src/plugins/externalPluginAdapters.ts (1)
createExternalPluginAdapterInitInfo(263-272)clients/js/src/generated/instructions/addGroupExternalPluginAdapterV1.ts (1)
addGroupExternalPluginAdapterV1(86-167)clients/js/src/generated/instructions/removeGroupExternalPluginAdapterV1.ts (1)
removeGroupExternalPluginAdapterV1(86-167)clients/js/src/plugins/externalPluginAdapterKey.ts (1)
externalPluginAdapterKeyToBase(29-57)clients/js/test/_setupRaw.ts (2)
assertGroup(306-345)DEFAULT_GROUP(59-62)
clients/js/test/groupAdditionalPlugins.test.ts (4)
clients/js/src/instructions/group/addGroupPlugin.ts (1)
addGroupPlugin(27-48)clients/js/test/_setupRaw.ts (2)
assertGroup(306-345)DEFAULT_GROUP(59-62)clients/js/src/instructions/group/updateGroupPlugin.ts (1)
updateGroupPlugin(14-21)clients/js/src/instructions/group/removeGroupPlugin.ts (1)
removeGroupPlugin(26-41)
clients/js/test/compute.test.ts (3)
clients/js/test/_setupRaw.ts (1)
createAsset(64-89)clients/js/src/instructions/group/addAssetsToGroup.ts (1)
addAssetsToGroup(6-9)clients/js/src/instructions/group/removeAssetsFromGroup.ts (1)
removeAssetsFromGroup(8-11)
clients/js/src/hooked/groupAccountData.ts (4)
clients/js/src/plugins/types.ts (1)
GroupPluginsList(238-242)clients/js/src/plugins/externalPluginAdapters.ts (2)
ExternalPluginAdaptersList(79-86)externalRegistryRecordsToExternalPluginAdapterList(139-247)clients/js/src/generated/accounts/pluginHeaderV1.ts (3)
PluginHeaderV1(25-25)PluginHeaderV1AccountData(27-30)getPluginHeaderV1AccountDataSerializer(37-48)clients/js/src/plugins/lib.ts (1)
registryRecordsToPluginsList(198-220)
clients/js/src/instructions/group/removeAssetsFromGroup.ts (1)
clients/js/src/generated/instructions/removeAssetsFromGroupV1.ts (1)
removeAssetsFromGroupV1(81-154)
clients/js/src/instructions/group/addCollectionsToGroup.ts (1)
clients/js/src/generated/instructions/addCollectionsToGroupV1.ts (1)
addCollectionsToGroupV1(66-135)
clients/js/src/instructions/group/updateGroup.ts (1)
clients/js/src/generated/instructions/updateGroupV1.ts (1)
updateGroupV1(87-164)
clients/js/src/instructions/group/createGroup.ts (1)
clients/js/src/generated/instructions/createGroupV1.ts (1)
createGroupV1(91-163)
clients/js/src/instructions/group/updateGroupPlugin.ts (3)
clients/js/src/plugins/types.ts (1)
GroupAllPluginArgsV2(231-231)clients/js/src/generated/instructions/updateGroupPluginV1.ts (1)
updateGroupPluginV1(80-158)clients/js/src/plugins/lib.ts (1)
createGroupPluginV2(134-138)
clients/js/src/instructions/group/approveGroupPluginAuthority.ts (2)
clients/js/src/generated/instructions/approveGroupPluginAuthorityV1.ts (1)
approveGroupPluginAuthorityV1(92-173)clients/js/src/plugins/pluginAuthority.ts (2)
PluginAuthority(4-7)pluginAuthorityToBase(11-21)
clients/js/src/instructions/collection/writeCollectionData.ts (2)
clients/js/src/generated/instructions/writeCollectionExternalPluginAdapterDataV1.ts (3)
WriteCollectionExternalPluginAdapterDataV1InstructionArgs(95-96)WriteCollectionExternalPluginAdapterDataV1InstructionAccounts(40-53)writeCollectionExternalPluginAdapterDataV1(99-184)clients/js/src/plugins/externalPluginAdapterKey.ts (2)
ExternalPluginAdapterKey(6-27)externalPluginAdapterKeyToBase(29-57)
clients/js/test/approveGroupPluginAuthority.test.ts (5)
clients/js/src/instructions/group/addGroupPlugin.ts (1)
addGroupPlugin(27-48)clients/js/src/instructions/group/approveGroupPluginAuthority.ts (1)
approveGroupPluginAuthority(17-25)clients/js/src/instructions/group/updateGroupPlugin.ts (1)
updateGroupPlugin(14-21)clients/js/test/_setupRaw.ts (2)
assertGroup(306-345)DEFAULT_GROUP(59-62)clients/js/src/instructions/group/revokeGroupPluginAuthority.ts (1)
revokeGroupPluginAuthority(15-22)
clients/js/src/instructions/group/removeGroupsFromGroup.ts (1)
clients/js/src/generated/instructions/removeGroupsFromGroupV1.ts (1)
removeGroupsFromGroupV1(81-154)
clients/js/src/instructions/group/removeGroupPlugin.ts (4)
clients/js/src/plugins/externalPluginAdapterKey.ts (2)
ExternalPluginAdapterKey(6-27)externalPluginAdapterKeyToBase(29-57)clients/js/src/generated/instructions/removeGroupPluginV1.ts (1)
removeGroupPluginV1(82-160)clients/js/src/plugins/externalPluginAdapters.ts (1)
isExternalPluginAdapterType(249-261)clients/js/src/generated/instructions/removeGroupExternalPluginAdapterV1.ts (1)
removeGroupExternalPluginAdapterV1(86-167)
clients/js/test/groupRelations.test.ts (7)
clients/js/test/_setupRaw.ts (3)
createAsset(64-89)assertGroup(306-345)createCollection(100-117)clients/js/src/instructions/group/addAssetsToGroup.ts (1)
addAssetsToGroup(6-9)clients/js/src/instructions/group/removeAssetsFromGroup.ts (1)
removeAssetsFromGroup(8-11)clients/js/src/instructions/group/addCollectionsToGroup.ts (1)
addCollectionsToGroup(8-11)clients/js/src/instructions/group/removeCollectionsFromGroup.ts (1)
removeCollectionsFromGroup(8-11)clients/js/src/instructions/group/addGroupsToGroup.ts (1)
addGroupsToGroup(6-9)clients/js/src/instructions/group/removeGroupsFromGroup.ts (1)
removeGroupsFromGroup(8-11)
clients/js/src/plugins/types.ts (3)
clients/js/src/generated/types/attributes.ts (1)
AttributesArgs(18-18)clients/js/src/generated/types/autograph.ts (1)
AutographArgs(22-22)clients/js/src/generated/types/verifiedCreators.ts (1)
VerifiedCreatorsArgs(22-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (51)
clients/rust/Cargo.toml (1)
33-33: No functional impact – formatting onlyLooks good.
package.json (1)
2-30: Pure re-formattingIndentation change only; nothing to review.
clients/js/package.json (1)
69-69: Bump to pnpm 8 .9 .0 – check CI imageCI runners or Docker images need the matching pnpm version (8.9). Confirm they are updated; otherwise installs may fall back to a different version and cause lock-file drift.
clients/rust/src/hooked/plugin.rs (1)
351-352: Discarded Groups plugin – please confirm intentIt looks like the
Plugin::Groups(_)arm in
clients/rust/src/hooked/plugin.rs (around line 351)
never surfaces any data becausePluginsList(in clients/rust/src/hooked/advanced_types.rs around line 171) has nogroupsfield.• If dropping the Groups plugin here is deliberate (e.g. you expose group data via a separate hook), add a comment to explain where and how to access it.
• Otherwise, wire it throughPluginsListjust like the other plugins:
- Add
pub groups: Option<GroupsPlugin>,toPluginsList.- Update the match arm in plugin.rs:
- Plugin::Groups(_) => {}
- // TODO: populate group membership data
- Plugin::Groups(groups) => acc.groups = Some(GroupsPlugin { base, groups }),
clients/js/src/hooked/index.ts (1)
3-3: LGTM – export addedMakes
groupAccountDataavailable to consumers.clients/rust/src/hooked/mod.rs (1)
53-53: LGTM! Clean implementation following established patterns.The addition of the
Groupsplugin variant mapping is correctly implemented and follows the same pattern as other plugin mappings in this match expression.clients/js/src/instructions/collection/index.ts (1)
9-9: LGTM! Proper export addition following established patterns.The new export for
writeCollectionDatais correctly added and aligns with the expanded plugin management capabilities being introduced in this PR.clients/js/src/instructions/index.ts (1)
1-16: LGTM! Proper integration of group functionality exports.The addition of the
./groupexport correctly integrates the new group-related instruction wrappers into the public API. The reordering of exports appears to maintain logical organization while preserving all existing functionality.clients/js/test/closeGroup.test.ts (1)
1-15: LGTM! Well-structured test for group lifecycle management.The test properly validates the
closeGroupfunctionality by:
- Creating a group using the established helper function
- Calling the closeGroup instruction
- Verifying the account is properly burned using the assertBurned helper
This follows the established testing patterns and provides good coverage for the group closing functionality.
programs/mpl-core/src/processor/update.rs (1)
388-399: LGTM! Important safety improvement for memory operations.The modification enhances memory safety by:
- Using
saturating_sub()to prevent underflow when calculatingcopy_len- Adding a conditional check to only perform the memory move when
copy_len > 0This prevents invalid memory operations that could occur with zero-length or underflow scenarios during account data resizing and plugin data rearrangement.
programs/mpl-core/src/processor/update_plugin.rs (2)
115-118: LGTM: Proper separation of concerns enforced.The guard correctly prevents Groups plugin updates through the generic collection plugin pathway, ensuring that group plugins are managed exclusively through dedicated group instructions. This aligns with the architectural design where group-related operations have their own specialized processors.
208-218: LGTM: Improved memory movement safety.The changes enhance safety by:
- Using
saturating_subto prevent potential underflow when calculating copy length- Conditionally executing the memory move only when
copy_len > 0This replaces the previous unconditional unsafe memory move with a safer, more robust implementation.
clients/js/src/instructions/group/index.ts (1)
1-15: LGTM: Well-organized instruction exports.The index file provides a clean, comprehensive entry point for all group-related instructions. The exports are logically organized and cover the full spectrum of group operations including lifecycle management (create, update, close), relationship management (add/remove assets, collections, groups), and plugin management.
configs/kinobi.cjs (1)
13-365: LGTM: Consistent formatting improvements.The changes are purely stylistic, involving consistent indentation and spacing throughout the configuration file. No functional modifications were made, maintaining all existing program updates, transformations, and rendering configurations while improving code readability.
clients/js/test/createGroup.test.ts (1)
1-23: LGTM: Well-structured group creation test.The test properly validates the group creation functionality by:
- Creating a group with custom name
- Asserting the group exists with expected properties (name, public key, update authority)
- Using the established testing utilities for consistency
The test coverage is appropriate for verifying the core group creation flow.
clients/js/test/group.test.ts (1)
1-32: LGTM: Comprehensive GPA filtering test.The test thoroughly validates the GPA (Get Program Accounts) functionality for groups by:
- Creating multiple groups with different update authorities
- Using the GPA builder to filter by specific criteria (
updateAuthorityandkey)- Verifying the correct count and properties of returned groups
The test design effectively validates the query functionality and ensures proper filtering behavior.
clients/js/src/plugins/lib.ts (2)
134-138: LGTM: Clean delegation pattern with proper type safety.The function correctly delegates to the existing
createPluginV2while maintaining type safety through proper casting. The comment clearly explains the delegation rationale.
140-149: LGTM: Consistent delegation pattern with proper parameter handling.The function correctly spreads parameters and delegates to
pluginAuthorityPairV2, maintaining consistency with the pattern established increateGroupPluginV2.clients/js/src/instructions/group/closeGroup.ts (1)
1-9: LGTM: Clean wrapper implementation following established patterns.The function correctly wraps the generated
closeGroupV1instruction with proper type extraction and parameter forwarding. The implementation is consistent with other instruction wrappers in the codebase.programs/mpl-core/src/processor/update_external_plugin_adapter.rs (1)
250-263: LGTM: Good safety improvement with saturating subtraction and conditional move.The change correctly uses
saturating_subto prevent underflow and adds a conditional check to avoid unnecessary zero-length memory moves. This aligns with defensive programming practices and is consistent with similar safety improvements mentioned in the AI summary.clients/js/src/instructions/group/addGroupsToGroup.ts (1)
1-9: LGTM: Clean wrapper implementation following established patterns.The function correctly wraps the generated
addGroupsToGroupV1instruction with proper type extraction and parameter forwarding. The implementation is consistent with other instruction wrappers in the codebase.clients/js/test/updateGroupAuthority.test.ts (1)
15-64: LGTM: Comprehensive test covering authority transfer scenarios.The test effectively verifies the complete authority transfer workflow:
- Successfully transfers update authority to a new signer
- Validates the new authority can perform updates
- Confirms the old authority loses access (throws error)
The test structure is well-organized with clear comments and uses appropriate helper functions. The assertions are comprehensive and the error handling verification is correct.
clients/js/src/instructions/group/removeAssetsFromGroup.ts (1)
1-11: LGTM! Consistent wrapper implementation.The wrapper follows the established pattern for group instruction wrappers - extracting the args type correctly and providing a clean passthrough to the generated function.
clients/js/src/instructions/group/updateGroup.ts (1)
1-9: LGTM! Consistent with other group instruction wrappers.The implementation follows the established pattern and provides a clean interface to the generated
updateGroupV1instruction.clients/js/src/instructions/group/removeGroupsFromGroup.ts (1)
1-11: LGTM! Consistent wrapper implementation.The wrapper follows the established pattern for group instruction wrappers and provides a clean interface to the generated
removeGroupsFromGroupV1instruction.clients/js/src/instructions/group/addCollectionsToGroup.ts (1)
1-12: LGTM! Clean and consistent wrapper implementation.The wrapper function follows the established pattern perfectly:
- Proper type extraction using
Parameters<typeof>utility type- Consistent context typing with
Pick<Context, 'payer' | 'programs'>- Simple forwarding to the generated instruction function
This provides a clean, type-safe API that will automatically stay in sync with the generated function signature.
clients/js/src/instructions/group/addAssetsToGroup.ts (1)
1-10: LGTM! Consistent wrapper pattern.The implementation follows the same clean pattern as other group instruction wrappers. The type extraction and context handling are correct.
clients/js/src/instructions/group/removeCollectionsFromGroup.ts (1)
1-12: LGTM! Consistent with the established wrapper pattern.The implementation maintains consistency with other group instruction wrappers:
- Proper type extraction using
Parameters<typeof>utility type- Consistent context typing
- Clean forwarding to the generated function
The naming follows the expected pattern for removal operations.
clients/js/test/compute.test.ts (2)
10-16: LGTM! Clean utility function for compute unit measurement.The utility function properly fetches transaction metadata and extracts compute units consumed. The error handling with nullish coalescing (
?? 0) is appropriate for cases where the transaction might not have compute unit data.
23-67: LGTM! Comprehensive compute unit test.The test properly validates that removing assets from groups stays within Solana's 1.4M compute unit limit:
- Creates the necessary test setup (group and asset)
- Adds the asset to the group first to ensure it's a valid removal operation
- Properly measures compute units from the removal transaction
- Uses appropriate assertion with descriptive error message
The test design is sound and tests an important operational constraint.
clients/js/src/instructions/group/createGroup.ts (1)
1-10: LGTM! Consistent wrapper implementation.The
createGroupfunction follows the established pattern perfectly:
- Proper type extraction using
Parameters<typeof>utility type- Consistent context typing with
Pick<Context, 'payer' | 'programs'>- Clean forwarding to the generated
createGroupV1functionThis maintains consistency with all other group instruction wrappers in the codebase.
clients/js/test/groupPlugins.test.ts (4)
22-44: Test structure is well-organized and follows good practices.The test properly creates a fresh group, adds an Attributes plugin, and validates the group state. The use of
DEFAULT_GROUPandassertGrouphelper provides good test consistency.
46-80: Comprehensive plugin lifecycle testing.The test effectively validates the update operation by first adding a plugin, then updating it with new attributes, and finally asserting the group state. The progression from single to multiple attributes demonstrates proper plugin update functionality.
82-111: Complete plugin removal testing.The test properly validates plugin removal by adding a plugin first, then removing it, and confirming the group state is clean. This ensures the remove operation works correctly and doesn't leave orphaned data.
117-138: Group metadata update testing is appropriate.The test validates the core group update functionality by changing both name and URI fields and verifying the changes are persisted. The test structure is clean and follows the established patterns.
programs/mpl-core/src/processor/close_group.rs (2)
41-45: Comprehensive emptiness validation is correct.The check ensures the group has no child collections, child groups, or parent groups before allowing closure. This prevents orphaned relationships and maintains data integrity.
47-49: Proper account closure implementation.The function correctly transfers the rent-exempt lamports back to the payer and closes the account using the utility function.
clients/js/test/groupAdditionalPlugins.test.ts (3)
18-86: Comprehensive Autograph plugin lifecycle testing.The test properly validates the complete lifecycle of the Autograph plugin - adding with initial signature, updating with multiple signatures, and removing the plugin entirely. The structured approach with clear section comments makes the test easy to follow.
92-131: VerifiedCreators plugin testing is adequate.The test covers the essential operations for the VerifiedCreators plugin. The plugin data structure with
verifiedfield is correctly used. While there's no update test, this may be intentional if VerifiedCreators doesn't commonly require updates.
40-42: Helpful comment about test scope.The comment clarifies that these tests focus on instruction execution success rather than detailed plugin data validation, which is appropriate for integration tests and avoids duplication with unit tests.
clients/js/src/instructions/collection/writeCollectionData.ts (2)
12-17: Type definition provides good type safety.The
WriteCollectionDataArgstype properly replaces the rawkeyfield with the strongly typedExternalPluginAdapterKey, providing better type safety and developer experience.
19-29: Wrapper implementation is clean and correct.The function properly extracts the key, converts it to base representation using
externalPluginAdapterKeyToBase, and delegates to the generated function. The destructuring pattern keeps the code clean and follows established conventions.clients/js/src/instructions/group/writeGroupData.ts (1)
1-29: LGTM!The wrapper function provides a clean, type-safe interface for writing group external plugin adapter data. The key conversion pattern is consistent with other plugin-related wrappers in the codebase.
clients/js/src/instructions/group/revokeGroupPluginAuthority.ts (1)
1-22: LGTM!The implementation correctly wraps the underlying instruction with type-safe plugin type handling. The pattern is consistent with the
approveGroupPluginAuthoritywrapper.clients/js/src/instructions/group/addGroupPlugin.ts (1)
27-48: Excellent unified interface design!The function elegantly handles both internal and external plugin types through type guards. The type assertions on lines 35 and 41 are safe due to the branching logic based on
isExternalPluginAdapterType.clients/js/src/hooked/groupAccountData.ts (1)
37-99: Well-structured deserialization implementation!The serializer correctly:
- Validates the account key type
- Handles optional plugin data based on buffer length
- Properly composes the final data structure with plugins and external adapters
The decision to throw on serialize is appropriate for a read-only deserializer.
clients/js/src/instructions/group/approveGroupPluginAuthority.ts (1)
1-25: LGTM!The wrapper correctly handles plugin type mapping and authority conversion. The implementation is consistent with the revoke counterpart and follows the established pattern for plugin authority management.
clients/js/src/plugins/types.ts (1)
220-242: LGTM!The new group plugin types are well-structured and follow the established patterns for asset and collection plugins. The types correctly represent the plugins that groups can have (Attributes, Autograph, VerifiedCreators).
clients/js/test/groupRelations.test.ts (1)
1-163: Well-structured test coverageThe test suite provides comprehensive coverage for group relationship operations:
- Tests both add and remove operations for assets, collections, and child groups
- Properly uses
addRemainingAccountsfor dynamic account lists- Includes appropriate assertions to verify state changes
- Follows consistent testing patterns
programs/mpl-core/src/processor/create_group.rs (1)
114-131: Effective duplicate prevention across relationship typesGood use of
HashSetto prevent duplicate entries across all relationship categories. The pattern of checking the return value ofinsert()is efficient and prevents the same account from being referenced multiple times.idls/mpl_core.json (1)
4213-4218: No change required:AddAssetsToGroupV1Argsis intentionally emptyThe
AddAssetsToGroupV1instruction reads its list of assets fromctx.remaining_accounts, not from its args struct. DefiningAddAssetsToGroupV1Argsas an empty payload is deliberate and matches the processor implementation:
- The handler signature is
fn add_assets_to_group_v1(..., _args: AddAssetsToGroupV1Args), and_argsis unused.- Assets are iterated over in the loop
for asset_info in remaining_accounts.iter().You can safely ignore adding an
assetsfield to the IDL.Likely an incorrect or invalid review comment.
| // Group External Plugin Adapters – AppData | ||
| // ----------------------------------------------------------------------------- | ||
|
|
||
| test('it can add, write, and remove an AppData external plugin adapter on a group', async (t) => { |
There was a problem hiding this comment.
Test name doesn't match implementation - missing "write" step.
The test name claims to test "add, write, and remove" but the implementation only covers adding and removing the AppData adapter. The "write" step (step 3) is missing from the test flow.
Consider either:
- Adding the missing write step to match the test name
- Updating the test name to reflect what it actually tests: "add and remove"
🤖 Prompt for AI Agents
In clients/js/test/groupExternalPluginAdapters.test.ts at line 26, the test name
mentions "add, write, and remove" but the implementation only adds and removes
the AppData external plugin adapter without performing a write operation. To fix
this, either add the missing write step to the test implementation to match the
name or update the test name to accurately reflect the current behavior by
removing "write" from it.
| // --------------------------------------------------------------------------- | ||
| // 4. Remove the adapter and ensure it no longer exists. | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Comment numbering inconsistency.
The comments jump from step 2 to step 4, which aligns with the missing "write" step mentioned in the test name.
🤖 Prompt for AI Agents
In clients/js/test/groupExternalPluginAdapters.test.ts around lines 50 to 52,
the comment numbering skips step 3, causing inconsistency with the test steps.
Insert a comment for step 3 that corresponds to the "write" step mentioned in
the test name to maintain sequential and clear numbering of the test steps.
| // The new authority should now be able to update the plugin. | ||
| await updateGroupPlugin(umi, { | ||
| group: group.publicKey, | ||
| payer: umi.identity, | ||
| authority: umi.identity, | ||
| logWrapper: LOG_WRAPPER, | ||
| plugin: { | ||
| type: 'Attributes', | ||
| attributeList: [{ key: 'k1', value: 'v1' }], | ||
| }, | ||
| }).sendAndConfirm(umi); |
There was a problem hiding this comment.
Test doesn't verify that the approved authority can actually perform updates.
After approving newAuthority as the plugin authority, the test should verify that this new authority can perform updates. However, the updateGroupPlugin call still uses umi.identity as the authority instead of the newly approved newAuthority.
Change the authority in the update call to test the approved authority:
await updateGroupPlugin(umi, {
group: group.publicKey,
payer: umi.identity,
- authority: umi.identity,
+ authority: newAuthority,
logWrapper: LOG_WRAPPER,
plugin: {
type: 'Attributes',
attributeList: [{ key: 'k1', value: 'v1' }],
},
}).sendAndConfirm(umi);🤖 Prompt for AI Agents
In clients/js/test/approveGroupPluginAuthority.test.ts between lines 54 and 64,
the test updates the plugin using umi.identity as the authority even after
approving newAuthority. To properly verify that the approved newAuthority can
perform updates, change the authority field in the updateGroupPlugin call from
umi.identity to newAuthority so the test confirms the new authority's
permissions.
| import { updateGroupPluginV1 } from '../../generated'; | ||
| import { createGroupPluginV2, GroupAllPluginArgsV2 } from '../../plugins'; | ||
|
|
||
| export type UpdateGroupPluginArgsPlugin = GroupAllPluginArgsV2; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Type alias is unnecessary.
The UpdateGroupPluginArgsPlugin type is just an alias for GroupAllPluginArgsV2 and doesn't add any value. Consider using GroupAllPluginArgsV2 directly in the UpdateGroupPluginArgs type definition.
-export type UpdateGroupPluginArgsPlugin = GroupAllPluginArgsV2;
-
export type UpdateGroupPluginArgs = Omit<
Parameters<typeof updateGroupPluginV1>[1],
'plugin'
> & {
- plugin: UpdateGroupPluginArgsPlugin;
+ plugin: GroupAllPluginArgsV2;
};🤖 Prompt for AI Agents
In clients/js/src/instructions/group/updateGroupPlugin.ts at line 5, the type
alias UpdateGroupPluginArgsPlugin is redundant as it only duplicates
GroupAllPluginArgsV2. Remove the UpdateGroupPluginArgsPlugin alias and replace
its usage directly with GroupAllPluginArgsV2 in the UpdateGroupPluginArgs type
definition to simplify the code.
| export const updateGroupPlugin = ( | ||
| context: Pick<Context, 'payer' | 'programs'>, | ||
| { plugin, ...args }: UpdateGroupPluginArgs | ||
| ) => | ||
| updateGroupPluginV1(context, { | ||
| ...args, | ||
| plugin: createGroupPluginV2(plugin as GroupAllPluginArgsV2), | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Wrapper implementation is correct with redundant type cast.
The wrapper properly provides type safety and delegates to the generated function. However, the type cast as GroupAllPluginArgsV2 is redundant since the plugin parameter is already typed as GroupAllPluginArgsV2.
export const updateGroupPlugin = (
context: Pick<Context, 'payer' | 'programs'>,
{ plugin, ...args }: UpdateGroupPluginArgs
) =>
updateGroupPluginV1(context, {
...args,
- plugin: createGroupPluginV2(plugin as GroupAllPluginArgsV2),
+ plugin: createGroupPluginV2(plugin),
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In clients/js/src/instructions/group/updateGroupPlugin.ts around lines 14 to 21,
remove the redundant type cast 'as GroupAllPluginArgsV2' from the 'plugin'
parameter in the call to createGroupPluginV2, since 'plugin' is already typed as
GroupAllPluginArgsV2. This will clean up the code without affecting type safety.
| }; | ||
| if (updateAuthoritySigner) { | ||
| // Account field type now allows Signer. | ||
| (createGroupArgs as any).updateAuthority = updateAuthoritySigner; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unsafe type assertion
Using as any to bypass TypeScript's type checking is unsafe and should be avoided.
- (createGroupArgs as any).updateAuthority = updateAuthoritySigner;
+ createGroupArgs.updateAuthority = updateAuthoritySigner;If this causes a type error, the proper fix would be to update the type definition of createGroupArgs to accept a Signer for the updateAuthority field.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (createGroupArgs as any).updateAuthority = updateAuthoritySigner; | |
| createGroupArgs.updateAuthority = updateAuthoritySigner; |
🤖 Prompt for AI Agents
In clients/js/test/_setupRaw.ts at line 161, avoid using the unsafe type
assertion 'as any' on createGroupArgs. Instead, update the type definition of
createGroupArgs to include the updateAuthority field with the correct Signer
type. This will ensure type safety and eliminate the need for bypassing
TypeScript's checks.
| const { updateGroup } = await import('../src'); | ||
| await updateGroup(umi, { | ||
| group: group.publicKey, | ||
| payer, | ||
| authority: payer, | ||
| newUpdateAuthority: updateAuthorityPubkey, | ||
| newName: null, | ||
| newUri: null, | ||
| }).sendAndConfirm(umi); | ||
| } | ||
|
|
||
| // If we have a signer for the update authority, delegate permissions to the payer | ||
| if (updateAuthoritySigner) { | ||
| const { plugin } = await import('../src/generated/types/plugin'); | ||
| const { addGroupPluginV1 } = await import('../src/generated'); | ||
|
|
||
| await addGroupPluginV1(umi, { | ||
| group: group.publicKey, | ||
| payer, | ||
| authority: updateAuthoritySigner, | ||
| plugin: plugin('UpdateDelegate', [ | ||
| { | ||
| additionalDelegates: [publicKey(payer)], | ||
| }, | ||
| ] as any), | ||
| initAuthority: null, | ||
| }).sendAndConfirm(umi); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Move imports to module level and avoid type assertions
Dynamic imports inside functions can impact performance and the use of as any bypasses TypeScript's type safety.
+import { updateGroup } from '../src';
+import { plugin } from '../src/generated/types/plugin';
+import { addGroupPluginV1 } from '../src/generated';
// ... inside createGroup function ...
- const { updateGroup } = await import('../src');
await updateGroup(umi, {
// ... params
}).sendAndConfirm(umi);
// ... later ...
- const { plugin } = await import('../src/generated/types/plugin');
- const { addGroupPluginV1 } = await import('../src/generated');
-
await addGroupPluginV1(umi, {
group: group.publicKey,
payer,
authority: updateAuthoritySigner,
plugin: plugin('UpdateDelegate', [
{
additionalDelegates: [publicKey(payer)],
},
- ] as any),
+ ]),
initAuthority: null,
}).sendAndConfirm(umi);🤖 Prompt for AI Agents
In clients/js/test/_setupRaw.ts between lines 169 and 196, move the dynamic
imports of updateGroup, plugin, and addGroupPluginV1 to the top of the module to
improve performance by avoiding repeated imports during function execution.
Replace the 'as any' type assertion by properly typing the plugin argument
according to its expected type to maintain TypeScript type safety.
| "name": "CreateGroupV1Args", | ||
| "type": { | ||
| "kind": "struct", | ||
| "fields": [ | ||
| { | ||
| "name": "name", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "name": "uri", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "name": "relationships", | ||
| "type": { | ||
| "vec": { | ||
| "defined": "crate" | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
Invalid "defined": "crate" breaks IDL type resolution
CreateGroupV1Args.relationships references a non-existent type called crate. Anchor/IDL generators will fail to parse this and downstream clients will not be able to serialize the instruction.
- {
- "name": "relationships",
- "type": {
- "vec": {
- "defined": "crate"
- }
- }
- }
+ {
+ "name": "relationships",
+ "type": {
+ "vec": {
+ "defined": "RelationshipEntry"
+ }
+ }
+ }Please correct the type name (most likely RelationshipEntry) before publishing the IDL.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "CreateGroupV1Args", | |
| "type": { | |
| "kind": "struct", | |
| "fields": [ | |
| { | |
| "name": "name", | |
| "type": "string" | |
| }, | |
| { | |
| "name": "uri", | |
| "type": "string" | |
| }, | |
| { | |
| "name": "relationships", | |
| "type": { | |
| "vec": { | |
| "defined": "crate" | |
| } | |
| } | |
| } | |
| ] | |
| } | |
| }, | |
| "name": "CreateGroupV1Args", | |
| "type": { | |
| "kind": "struct", | |
| "fields": [ | |
| { | |
| "name": "name", | |
| "type": "string" | |
| }, | |
| { | |
| "name": "uri", | |
| "type": "string" | |
| }, | |
| { | |
| "name": "relationships", | |
| "type": { | |
| "vec": { | |
| "defined": "RelationshipEntry" | |
| } | |
| } | |
| } | |
| ] | |
| } | |
| }, |
🤖 Prompt for AI Agents
In idls/mpl_core.json around lines 4591 to 4613, the field "relationships" in
"CreateGroupV1Args" incorrectly references a type named "crate" which does not
exist. Replace the "defined" value from "crate" to the correct type name
"RelationshipEntry" to fix the IDL type resolution and ensure proper
serialization by downstream clients.
| "name": "AddCollectionsToGroupV1Args", | ||
| "type": { | ||
| "kind": "struct", | ||
| "fields": [] | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
AddCollectionsToGroupV1Args also empty – mirror the removal variant
Similar to the previous comment, the remove counterpart carries a collections vector but the add variant is empty. The API surface will be confusing and may not match processor expectations.
Consider adding:
{
"name": "collections",
"type": { "vec": "publicKey" }
}or document clearly why the list is omitted.
🏁 Script executed:
#!/bin/bash
# Locate the remove counterpart to inspect its fields
rg '"name": "RemoveCollections' -n idls/mpl_core.json -A7Length of output: 582
🏁 Script executed:
#!/bin/bash
# Verify the AddCollectionsToGroupV1Args fields are indeed empty
rg '"name": "AddCollectionsToGroupV1Args"' -n idls/mpl_core.json -A7Length of output: 287
Add missing collections field to AddCollectionsToGroupV1Args to match removal variant
The RemoveCollectionsFromGroupV1Args struct expects a collections: Vec<PublicKey>, but AddCollectionsToGroupV1Args currently has an empty fields array. This asymmetry will lead to a confusing and inconsistent IDL/API. Either include the same collections field in the “add” args or document why additions require no explicit list.
• File: idls/mpl_core.json
Location: around lines 4220–4225
Suggested diff:
{
"name": "AddCollectionsToGroupV1Args",
"type": {
"kind": "struct",
- "fields": []
+ "fields": [
+ {
+ "name": "collections",
+ "type": { "vec": "publicKey" }
+ }
+ ]
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "AddCollectionsToGroupV1Args", | |
| "type": { | |
| "kind": "struct", | |
| "fields": [] | |
| } | |
| }, | |
| { | |
| "name": "AddCollectionsToGroupV1Args", | |
| "type": { | |
| "kind": "struct", | |
| "fields": [ | |
| { | |
| "name": "collections", | |
| "type": { "vec": "publicKey" } | |
| } | |
| ] | |
| } | |
| }, |
🤖 Prompt for AI Agents
In idls/mpl_core.json around lines 4220 to 4225, the AddCollectionsToGroupV1Args
struct is missing the collections field that is present in
RemoveCollectionsFromGroupV1Args. To fix this, add a collections field of type
Vec<PublicKey> to the fields array of AddCollectionsToGroupV1Args to ensure
consistency between the add and remove argument structures.
| }, | ||
| { | ||
| "name": "AddCollectionsToGroupV1", | ||
| "accounts": [ | ||
| { | ||
| "name": "group", | ||
| "isMut": true, | ||
| "isSigner": false, | ||
| "docs": [ | ||
| "The address of the group to modify" | ||
| ] | ||
| }, | ||
| { | ||
| "name": "payer", | ||
| "isMut": true, | ||
| "isSigner": true, | ||
| "docs": [ | ||
| "The account paying for storage fees" | ||
| ] | ||
| }, | ||
| { | ||
| "name": "authority", | ||
| "isMut": false, | ||
| "isSigner": true, | ||
| "isOptional": true, | ||
| "docs": [ | ||
| "The update authority or delegate of the group/collections" | ||
| ] | ||
| }, | ||
| { | ||
| "name": "systemProgram", | ||
| "isMut": false, | ||
| "isSigner": false, | ||
| "docs": [ | ||
| "The system program" | ||
| ] | ||
| } | ||
| ], | ||
| "args": [ | ||
| { | ||
| "name": "addCollectionsToGroupV1Args", | ||
| "type": { | ||
| "defined": "AddCollectionsToGroupV1Args" | ||
| } | ||
| } | ||
| ], | ||
| "discriminant": { | ||
| "type": "u8", | ||
| "value": 33 | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure new instruction discriminants are appended, not reordered
AddCollectionsToGroupV1 starts with discriminant 33, following UpdateCollectionInfoV1 (32). That looks correct, but please verify in programs/mpl-core/src/instruction.rs that the new variants were appended to the MplAssetInstruction enum, never inserted in the middle—otherwise on-chain Borsh ordinals will shift and break existing clients.
🏁 Script executed:
#!/bin/bash
# Confirm new variants are appended at the tail of the enum.
ast-grep --pattern $'enum MplAssetInstruction { $$$ }' programs/mpl-core/src/instruction.rs -A3Length of output: 96
🏁 Script executed:
#!/bin/bash
set -eux
# Locate the instruction.rs file(s) in the repo
fd --type f instruction.rs
# Assuming the file at programs/mpl-core/src/instruction.rs, print the enum block
file="programs/mpl-core/src/instruction.rs"
if [[ -f "$file" ]]; then
rg -n 'enum MplAssetInstruction' -A 50 "$file"
else
echo "programs/mpl-core/src/instruction.rs not found; please verify the correct path."
fiLength of output: 3957
🏁 Script executed:
#!/bin/bash
set -eux
# Print the full MplAssetInstruction enum to verify the order of variants
rg -n 'enum MplAssetInstruction' -A200 programs/mpl-core/src/instruction.rsLength of output: 14614
#!/bin/bash
set -eux
# Show UpdateCollectionInfoV1 and following variants to see its position
rg -n 'UpdateCollectionInfoV1' -A5 programs/mpl-core/src/instruction.rs
# Show AddCollectionsToGroupV1 context to confirm its placement
rg -n 'AddCollectionsToGroupV1' -B5 -A5 programs/mpl-core/src/instruction.rs🏁 Script executed:
#!/bin/bash
set -eux
# Locate AddCollectionsToGroupV1 in the enum to verify it's the final variant
rg -n 'AddCollectionsToGroupV1' -B5 -A5 programs/mpl-core/src/instruction.rs
# Show the last 30 lines of the file to confirm the enum closing brace follows immediately
tail -n 30 programs/mpl-core/src/instruction.rsLength of output: 3847
Append AddCollectionsToGroupV1 to the end of the enum for ABI compatibility
The new variant
/// Add collections to a group.
#[account(0, writable, name="group", desc = "The address of the group to modify")]
…
AddCollectionsToGroupV1(AddCollectionsToGroupV1Args),is currently inserted before other group instructions, shifting all following Borsh ordinals. To preserve on-chain ABI:
• Move it after the last enum variant (UpdateGroupV1(UpdateGroupV1Args),)
• Ensure it sits immediately before the closing } of pub(crate) enum MplAssetInstruction
File: programs/mpl-core/src/instruction.rs
Current: line 317
Desired: just above the final } of the enum
🤖 Prompt for AI Agents
In idls/mpl_core.json lines 2117 to 2167, the AddCollectionsToGroupV1 variant is
inserted before other group instructions, causing a shift in Borsh ordinals and
breaking ABI compatibility. To fix this, move the AddCollectionsToGroupV1
variant to the end of the enum, placing it immediately before the closing brace
of the MplAssetInstruction enum in programs/mpl-core/src/instruction.rs around
line 317. This preserves the on-chain ABI by keeping the original order of enum
variants.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
programs/mpl-core/src/plugins/mod.rs (1)
65-66: Consider clarifying internal-management natureWhile the documentation is helpful, consider adding a brief note that this plugin is internally-managed only and cannot be added/updated via generic plugin instructions to prevent misuse by downstream contributors.
idls/mpl_core.json (2)
4616-4622: Invalid"defined": "crate"is still present
Typecratedoes not exist in the IDL, causing generator failures. Replace with the correct type (likelyRelationshipEntry).
4232-4236:AddCollectionsToGroupV1Argsstill empty
The companion remove args carries acollections: Vec<Pubkey>field. Maintaining this asymmetry continues to confuse consumers and likely mis-documents the processor’s expectation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (7)
clients/js/src/generated/instructions/writeGroupExternalPluginAdapterDataV1.tsis excluded by!**/generated/**clients/js/src/generated/types/index.tsis excluded by!**/generated/**clients/js/src/generated/types/plugin.tsis excluded by!**/generated/**clients/js/src/generated/types/pluginType.tsis excluded by!**/generated/**clients/rust/src/generated/types/mod.rsis excluded by!**/generated/**clients/rust/src/generated/types/plugin.rsis excluded by!**/generated/**clients/rust/src/generated/types/plugin_type.rsis excluded by!**/generated/**
📒 Files selected for processing (6)
clients/js/package.json(1 hunks)clients/js/src/plugins/types.ts(1 hunks)clients/rust/src/hooked/mod.rs(1 hunks)clients/rust/src/hooked/plugin.rs(1 hunks)idls/mpl_core.json(19 hunks)programs/mpl-core/src/plugins/mod.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
programs/mpl-core/src/plugins/mod.rs (2)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/plugins/internal/authority_managed/groups.rs:19-22
Timestamp: 2025-06-05T12:02:41.859Z
Learning: In the mpl-core codebase, Pubkey sizes are consistently represented using the literal value `32` rather than a constant like `Pubkey::LEN`. This is the established pattern throughout the codebase, seen in structs like GroupV1, Authority, UpdateDelegate, and various plugin implementations.
clients/js/src/plugins/types.ts (1)
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
idls/mpl_core.json (7)
undefined
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
</retrieved_learning>
<retrieved_learning>
Learnt from: danenbm
PR: #225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts have inconsistent documentation - sometimes described as "The collection to which the asset belongs" and sometimes as "The address of the asset" or "The address of the new asset". The team has chosen to maintain existing patterns for consistency rather than correcting individual instances.
</retrieved_learning>
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
</retrieved_learning>
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
</retrieved_learning>
<retrieved_learning>
Learnt from: danenbm
PR: #225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts are consistently documented as "The address of the asset" even though they are collection accounts, and this pattern is intentionally maintained for consistency.
</retrieved_learning>
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs:38-47
Timestamp: 2025-06-05T12:01:38.260Z
Learning: In the mpl-core codebase, "optional" accounts use a pattern where None/absent accounts are represented by the program ID as a placeholder in the accounts array. This means the accounts array length remains constant and predictable, avoiding variable-length account arrays and off-by-one indexing errors.
</retrieved_learning>
<retrieved_learning>
Learnt from: blockiosaurus
PR: #231
File: programs/mpl-core/src/plugins/internal/authority_managed/groups.rs:19-22
Timestamp: 2025-06-05T12:02:41.859Z
Learning: In the mpl-core codebase, Pubkey sizes are consistently represented using the literal value 32 rather than a constant like Pubkey::LEN. This is the established pattern throughout the codebase, seen in structs like GroupV1, Authority, UpdateDelegate, and various plugin implementations.
</retrieved_learning>
🧬 Code Graph Analysis (4)
clients/rust/src/hooked/mod.rs (2)
clients/js/src/generated/types/plugin.ts (1)
Plugin(74-92)clients/js/src/generated/types/groups.ts (1)
Groups(17-17)
clients/rust/src/hooked/plugin.rs (2)
clients/js/src/generated/types/plugin.ts (1)
Plugin(74-92)clients/js/src/generated/types/groups.ts (1)
Groups(17-17)
programs/mpl-core/src/plugins/mod.rs (2)
clients/js/src/generated/types/groups.ts (1)
Groups(17-17)clients/js/src/generated/types/plugin.ts (1)
Plugin(74-92)
clients/js/src/plugins/types.ts (3)
clients/js/src/generated/types/attributes.ts (1)
AttributesArgs(18-18)clients/js/src/generated/types/autograph.ts (1)
AutographArgs(22-22)clients/js/src/generated/types/verifiedCreators.ts (1)
VerifiedCreatorsArgs(22-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Programs / Test (mpl-core)
🔇 Additional comments (14)
clients/js/package.json (1)
70-70: LGTM: Package manager version updateThe update from [email protected] to [email protected] is a routine maintenance update that should include bug fixes and improvements while maintaining backward compatibility.
clients/rust/src/hooked/plugin.rs (1)
351-351: LGTM: Groups plugin filtering is intentionalThe empty block correctly filters out Groups plugins from the standard plugin list conversion, which is appropriate since Groups plugins are used for internal taxonomy purposes and parent group memberships rather than being exposed in the standard plugin listing.
clients/rust/src/hooked/mod.rs (1)
53-53: LGTM: Consistent plugin type conversionThe Groups plugin variant mapping follows the established pattern for all other plugin variants in the
From<&Plugin> for PluginTypeimplementation.programs/mpl-core/src/plugins/mod.rs (7)
65-66: LGTM: Groups plugin integration follows established patternsThe Groups plugin variant is correctly integrated into the Plugin enum with appropriate documentation. The implementation follows the established pattern for adding new plugin variants.
113-113: LGTM: Consistent inner() method implementationThe Groups plugin case is correctly added to the inner() method following the same pattern as all other plugin variants.
147-147: LGTM: Consistent len() method implementationThe Groups plugin case is correctly added to the len() method following the same pattern as all other plugin variants.
203-204: LGTM: Groups plugin type correctly positionedThe Groups variant is correctly added to the PluginType enum at the end, maintaining consistency with the established pattern.
246-246: LGTM: Consistent plugin type conversionThe Groups plugin conversion follows the established pattern in the
From<&Plugin> for PluginTypeimplementation.
274-274: LGTM: Appropriate authority assignmentThe Groups plugin correctly returns
Authority::UpdateAuthorityin the manager() method, which is appropriate for an internally-managed plugin.
327-327: LGTM: Comprehensive test coverageThe test cases appropriately include both empty and non-empty Groups plugin instances to verify serialization size correctness.
Also applies to: 445-447
clients/js/src/plugins/types.ts (3)
231-240: LGTM: Well-defined group plugin argumentsThe GroupAddablePluginArgsV2 union type appropriately restricts group plugins to Attributes, Autograph, and VerifiedCreators, which is a logical subset for group entities.
242-247: LGTM: Consistent type alias patternThe GroupAllPluginArgsV2 and authority pair types follow the established pattern used for Asset and Collection plugin types.
249-253: LGTM: Comprehensive plugin list interfaceThe GroupPluginsList type provides a clean interface for group plugins, maintaining consistency with the optional property pattern used in other plugin list types.
idls/mpl_core.json (1)
4225-4230:AddAssetsToGroupV1Argshas no fields – verify intentUnlike its
RemoveAssetsFromGroupV1Argscounterpart, this struct is empty.
If the processor expects anassets: Vec<Pubkey>list, the absence here will desync the IDL and runtime behavior.
Confirm whether assets are supplied solely via remaining accounts; if not, add the field for clarity.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark
| Benchmark suite | Current: a1afb1f | Previous: 4917f62 | Ratio |
|---|---|---|---|
CU: create a new, empty asset |
7643 Compute Units |
7628 Compute Units |
1.00 |
Space: create a new, empty asset |
91 Bytes |
91 Bytes |
1 |
CU: create a new, empty asset with empty collection |
15730 Compute Units |
15706 Compute Units |
1.00 |
Space: create a new, empty asset with empty collection |
91 Bytes |
91 Bytes |
1 |
CU: create a new asset with plugins |
25940 Compute Units |
25922 Compute Units |
1.00 |
Space: create a new asset with plugins |
194 Bytes |
194 Bytes |
1 |
CU: create a new asset with plugins and empty collection |
30859 Compute Units |
30830 Compute Units |
1.00 |
Space: create a new asset with plugins and empty collection |
194 Bytes |
194 Bytes |
1 |
CU: list an asset |
19064 Compute Units |
19019 Compute Units |
1.00 |
CU: sell an asset |
24223 Compute Units |
24206 Compute Units |
1.00 |
CU: list an asset with empty collection |
23557 Compute Units |
23516 Compute Units |
1.00 |
CU: sell an asset with empty collection |
31604 Compute Units |
31593 Compute Units |
1.00 |
CU: list an asset with collection royalties |
22983 Compute Units |
22906 Compute Units |
1.00 |
CU: sell an asset with collection royalties |
34692 Compute Units |
34644 Compute Units |
1.00 |
CU: transfer an empty asset |
3646 Compute Units |
3611 Compute Units |
1.01 |
CU: transfer an empty asset with empty collection |
5204 Compute Units |
5171 Compute Units |
1.01 |
CU: transfer an asset with plugins |
7131 Compute Units |
7048 Compute Units |
1.01 |
CU: transfer an asset with plugins and empty collection |
8689 Compute Units |
8608 Compute Units |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
This is looking good! So far the only big protocol thing I see is the git merge put the Group plugin before freeze execute which will break things. But otherwise looks good. I think we just need to make sure all the permutations are tested in JS tests and then good for Devnet.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
programs/mpl-core/src/plugins/mod.rs (2)
67-68: Add documentation clarifying Groups plugin is internally-managed only.As noted in previous reviews, please add a brief comment clarifying that this plugin is internally-managed only and cannot be added/updated via generic plugin instructions.
- /// Groups plugin stores parent group memberships of a collection for taxonomy purposes + /// Groups plugin stores parent group memberships of a collection for taxonomy purposes. + /// This plugin is internally-managed only and cannot be added/updated via generic plugin instructions. Groups(Groups),
145-149: Move Groups match arm to the end for consistency.The Groups match arm should be placed at the end of the match statement, after FreezeExecute, to maintain consistency with the pattern of adding new variants at the end.
- Plugin::Autograph(autograph) => autograph.len(), - Plugin::BubblegumV2(bubblegum_v2) => bubblegum_v2.len(), - Plugin::FreezeExecute(freeze_execute) => freeze_execute.len(), - Plugin::Groups(groups) => groups.len(), + Plugin::Autograph(autograph) => autograph.len(), + Plugin::BubblegumV2(bubblegum_v2) => bubblegum_v2.len(), + Plugin::FreezeExecute(freeze_execute) => freeze_execute.len(), + Plugin::Groups(groups) => groups.len(),programs/mpl-core/src/processor/add_group_plugin.rs (2)
13-13: Remove unusedSolanaAccountimport.The
SolanaAccountimport is not used in this file and should be removed to avoid compiler warnings.- state::{Authority, GroupV1, SolanaAccount}, + state::{Authority, GroupV1},
62-64: Remove redundantGroupV1::loadcall.The
groupvariable is assigned but never used, andGroupV1::loadis called again increate_meta_idempotentbelow. This causes unnecessary compute overhead.- // Deserialize group and authority check - let group = GroupV1::load(group_info, 0)?; - + // Authority check if !is_valid_group_authority(group_info, authority_info)? {programs/mpl-core/src/processor/add_assets_to_group.rs (2)
142-226: Extract duplicated plugin management logic into shared utility.The
process_asset_groups_plugin_addfunction duplicates similar logic found increate_group.rs. Consider extracting this into a shared utility function to reduce code duplication and prevent future divergence.Would you like me to help create a shared utility function for managing Groups plugin updates that can be reused across both files?
192-195: Use checked arithmetic to prevent overflow/underflow.Direct casts between
isizeandusizecan cause overflow or underflow whensize_diffis negative. Use checked arithmetic as done in similar code elsewhere in the codebase.- plugin_header.plugin_registry_offset = - (plugin_header.plugin_registry_offset as isize + size_diff) as usize; + plugin_header.plugin_registry_offset = (plugin_header + .plugin_registry_offset as isize) + .checked_add(size_diff) + .ok_or(MplCoreError::NumericalOverflow)? as usize; - let new_size = (asset_info.data_len() as isize + size_diff) as usize; + let new_size = (asset_info.data_len() as isize) + .checked_add(size_diff) + .ok_or(MplCoreError::NumericalOverflow)? as usize;idls/mpl_core.json (2)
4232-4236:AddCollectionsToGroupV1Argsstill missingcollectionsvector
Same problem as above and flagged in the previous review – the struct’sfieldsarray is empty, so nothing can be added.See the diff in the earlier comment; the fix is identical but with
collectionsinstead ofassets.
4616-4622: Invalid type aliascratebreaks IDL generation
Therelationshipsfield still references a non-existent type named"crate". Replace it with the newly-addedRelationshipEntry.- "vec": { "defined": "crate" } + "vec": { "defined": "RelationshipEntry" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockclients/js/src/generated/types/plugin.tsis excluded by!**/generated/**clients/js/src/generated/types/pluginType.tsis excluded by!**/generated/**clients/rust/src/generated/types/plugin.rsis excluded by!**/generated/**clients/rust/src/generated/types/plugin_type.rsis excluded by!**/generated/**
📒 Files selected for processing (8)
clients/js/test/groupComplexRelations.test.ts(1 hunks)clients/js/test/groupErrors.test.ts(1 hunks)clients/js/test/groupWriteData.test.ts(1 hunks)clients/rust/src/hooked/mod.rs(1 hunks)idls/mpl_core.json(19 hunks)programs/mpl-core/src/plugins/mod.rs(8 hunks)programs/mpl-core/src/processor/add_assets_to_group.rs(1 hunks)programs/mpl-core/src/processor/add_group_plugin.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
📚 Learning: in the mpl-core codebase, delegate authority validation for groups is handled at the plugin level th...
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:86-92
Timestamp: 2025-06-05T12:01:30.659Z
Learning: In the mpl-core codebase, delegate authority validation for groups is handled at the plugin level through each plugin's validate_update() implementation, not at the core GroupV1::validate_update level. The GroupV1::validate_update method intentionally only checks direct update_authority matches, while plugins handle delegate authorization scenarios.
Applied to files:
programs/mpl-core/src/plugins/mod.rsprograms/mpl-core/src/processor/add_assets_to_group.rsprograms/mpl-core/src/processor/add_group_plugin.rsidls/mpl_core.json
📚 Learning: in the mpl-core codebase, pubkey sizes are consistently represented using the literal value `32` rat...
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/plugins/internal/authority_managed/groups.rs:19-22
Timestamp: 2025-06-05T12:02:41.859Z
Learning: In the mpl-core codebase, Pubkey sizes are consistently represented using the literal value `32` rather than a constant like `Pubkey::LEN`. This is the established pattern throughout the codebase, seen in structs like GroupV1, Authority, UpdateDelegate, and various plugin implementations.
Applied to files:
programs/mpl-core/src/plugins/mod.rsidls/mpl_core.json
📚 Learning: in the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), th...
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:81-83
Timestamp: 2025-06-24T20:46:47.342Z
Learning: In the add_assets_to_group_v1 processor (programs/mpl-core/src/processor/add_assets_to_group.rs), the payer must always be a signer even when an explicit authority is provided, because the payer is responsible for paying rent for new data space when assets are added to groups.
Applied to files:
programs/mpl-core/src/plugins/mod.rsprograms/mpl-core/src/processor/add_assets_to_group.rsprograms/mpl-core/src/processor/add_group_plugin.rsidls/mpl_core.json
📚 Learning: in the mpl-core program, new variants must always be appended to the end of the mplassetinstruction ...
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
Applied to files:
programs/mpl-core/src/processor/add_assets_to_group.rsidls/mpl_core.json
📚 Learning: in the mpl-core idl, collection accounts have inconsistent documentation - sometimes described as "t...
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts have inconsistent documentation - sometimes described as "The collection to which the asset belongs" and sometimes as "The address of the asset" or "The address of the new asset". The team has chosen to maintain existing patterns for consistency rather than correcting individual instances.
Applied to files:
programs/mpl-core/src/processor/add_assets_to_group.rsidls/mpl_core.json
📚 Learning: in the mpl-core idl, collection accounts are consistently documented as "the address of the asset" e...
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: idls/mpl_core.json:2089-2094
Timestamp: 2025-05-07T19:09:34.569Z
Learning: In the mpl-core IDL, collection accounts are consistently documented as "The address of the asset" even though they are collection accounts, and this pattern is intentionally maintained for consistency.
Applied to files:
programs/mpl-core/src/processor/add_assets_to_group.rsidls/mpl_core.json
📚 Learning: in the mpl-core codebase, "optional" accounts use a pattern where none/absent accounts are represent...
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_group_external_plugin_adapter.rs:38-47
Timestamp: 2025-06-05T12:01:38.260Z
Learning: In the mpl-core codebase, "optional" accounts use a pattern where None/absent accounts are represented by the program ID as a placeholder in the accounts array. This means the accounts array length remains constant and predictable, avoiding variable-length account arrays and off-by-one indexing errors.
Applied to files:
programs/mpl-core/src/processor/add_assets_to_group.rsidls/mpl_core.json
📚 Learning: in the mpl-core codebase, writable checks are generally ignored because the solana runtime will auto...
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/processor/add_assets_to_group.rs:99-103
Timestamp: 2025-06-24T20:50:30.534Z
Learning: In the mpl-core codebase, writable checks are generally ignored because the Solana runtime will automatically throw an error if a readonly account is written to, making explicit writable checks redundant.
Applied to files:
programs/mpl-core/src/processor/add_assets_to_group.rsprograms/mpl-core/src/processor/add_group_plugin.rs
📚 Learning: solana accounts are currently limited to 10mib (10,485,760 bytes), and u32::max-sized accounts (~4.3...
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/state/group.rs:96-105
Timestamp: 2025-06-05T12:01:33.707Z
Learning: Solana accounts are currently limited to 10MiB (10,485,760 bytes), and u32::MAX-sized accounts (~4.3GB) would cost approximately 30K SOL in rent, making them financially infeasible. Therefore, integer overflow protection in length calculations is generally unnecessary for Solana program account size computations due to these practical runtime constraints.
Applied to files:
programs/mpl-core/src/processor/add_assets_to_group.rs
📚 Learning: in the mpl-core codebase, danenbm prefers to maintain separate conditional branches with distinct ex...
Learnt from: danenbm
PR: metaplex-foundation/mpl-core#225
File: programs/mpl-core/src/plugins/internal/permanent/bubblegum_v2.rs:40-61
Timestamp: 2025-05-08T00:51:20.026Z
Learning: In the mpl-core codebase, danenbm prefers to maintain separate conditional branches with distinct explanatory comments for better readability, even when the outcome (like using `reject!()`) is the same.
Applied to files:
programs/mpl-core/src/processor/add_assets_to_group.rs
🧬 Code Graph Analysis (2)
clients/rust/src/hooked/mod.rs (2)
clients/js/src/generated/types/plugin.ts (1)
Plugin(74-92)clients/js/src/generated/types/groups.ts (1)
Groups(17-17)
programs/mpl-core/src/plugins/mod.rs (2)
clients/js/src/generated/types/groups.ts (1)
Groups(17-17)clients/js/src/generated/types/plugin.ts (1)
Plugin(74-92)
🔇 Additional comments (15)
clients/js/test/groupComplexRelations.test.ts (5)
1-3: LGTM!Clean imports with appropriate test framework and utility functions.
5-15: LGTM!Excellent test structure with clear naming and well-organized sections. The test name accurately describes the bidirectional synchronization behavior being tested.
20-41: LGTM!The add relationship logic correctly implements bidirectional synchronization:
- Properly includes child group in remaining accounts as writable for parentGroups update
- Verifies both parent's
groupslist and child'sparentGroupslist are updated- Uses appropriate authority for the operation
46-67: LGTM!The remove relationship logic properly mirrors the add operation:
- Symmetric API usage for removing relationships
- Correctly verifies both sides are cleaned up with empty arrays
- Maintains the same remaining accounts pattern for bidirectional updates
9-68: Well-designed test for bidirectional group relationship synchronization.This test effectively validates the core requirement that parent-child group relationships are properly synchronized on both sides during add and remove operations. The test structure is clear and the assertions comprehensively verify the expected behavior.
clients/js/test/groupWriteData.test.ts (4)
1-20: LGTM! Clean imports and consistent constant usage.The imports are well-organized and the LOG_WRAPPER constant reuse maintains consistency with other test files in the suite.
25-44: Well-structured test setup.The test setup correctly creates the necessary group and AppData external plugin adapter with appropriate configuration for the JSON data writing scenario.
46-70: Excellent coverage of buffer account data writing approach.The test correctly demonstrates writing data via a buffer account, including proper JSON encoding, buffer creation with appropriate space allocation, and correct usage of the
writeGroupDatafunction with buffer parameter.
72-83: Thorough verification of written data.The verification correctly checks both the presence of the appDatas entry and validates that the dataLen matches the actual data length, providing comprehensive test coverage.
clients/js/test/groupErrors.test.ts (3)
18-31: Good coverage of unsupported adapter type error handling.The test correctly verifies that
writeGroupDatarejects unsupported adapter types like Oracle, ensuring proper input validation.
33-62: Comprehensive test for missing data source validation.The test correctly sets up a valid adapter first, then verifies that
writeGroupDataproperly rejects attempts to write without providing either inline data or a buffer account.
64-101: Excellent authorization failure test coverage.The test properly validates that
writeGroupDataenforces data authority requirements by setting up an adapter with a specific data authority and verifying that attempts to write with incorrect authority are rejected.clients/rust/src/hooked/mod.rs (1)
54-54: LGTM!The addition of the
Plugin::Groups(_)match arm correctly maps toPluginType::Groupsand is properly placed at the end of the match expression.idls/mpl_core.json (2)
5341-5348: Variant appended correctly – ABI preserved
Plugin::Groupsis appended to the end of the enum afterFreezeExecute, preserving ordinal order.
LGTM.
5406-5410:PluginType::Groupsplacement is correctThe new discriminant is appended, so no ABI shift occurs.
| "name": "AddAssetsToGroupV1Args", | ||
| "type": { | ||
| "kind": "struct", | ||
| "fields": [] | ||
| } | ||
| }, |
There was a problem hiding this comment.
AddAssetsToGroupV1Args has no payload – cannot specify which assets to add
The struct is completely empty, so the processor has no way to know which asset public-keys should be inserted into the group.
Mirror the pattern used for the remove variant and include a assets: Vec<PublicKey> field.
- {
- "name": "AddAssetsToGroupV1Args",
- "type": { "kind": "struct", "fields": [] }
- },
+ {
+ "name": "AddAssetsToGroupV1Args",
+ "type": {
+ "kind": "struct",
+ "fields": [
+ {
+ "name": "assets",
+ "type": { "vec": "publicKey" }
+ }
+ ]
+ }
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "AddAssetsToGroupV1Args", | |
| "type": { | |
| "kind": "struct", | |
| "fields": [] | |
| } | |
| }, | |
| { | |
| "name": "AddAssetsToGroupV1Args", | |
| "type": { | |
| "kind": "struct", | |
| "fields": [ | |
| { | |
| "name": "assets", | |
| "type": { "vec": "publicKey" } | |
| } | |
| ] | |
| } | |
| }, |
🤖 Prompt for AI Agents
In idls/mpl_core.json around lines 4225 to 4230, the AddAssetsToGroupV1Args
struct is empty, so it lacks the necessary field to specify which assets to add.
To fix this, add a field named "assets" of type "Vec<PublicKey>" to the struct's
fields array, following the pattern used in the corresponding remove variant.
This will allow the processor to know which asset public keys to insert into the
group.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
idls/mpl_core.json (3)
4615-4634: FixCreateGroupV1Args.relationshipstype name
"defined": "crate"is not a valid IDL type reference, so clients cannot serialize this field. Point it at the actual struct (RelationshipEntry) instead.
4237-4249: PopulateAddCollectionsToGroupV1Argswith the target listThe “add collections” args struct is still empty, so processors/clients have no way to specify which collection pubkeys to add. Mirror the removal variant and include a
collections: Vec<PublicKey>.
4237-4242: PopulateAddAssetsToGroupV1Argswith the target listLikewise, the “add assets” args struct is empty, preventing callers from naming the assets to insert. Add an
assets: Vec<PublicKey>field to match the removal counterpart.programs/mpl-core/src/plugins/mod.rs (1)
69-70: Clarify thatGroupsis internally managed only.Per the earlier review request, please extend this doc comment to state that
Groupscannot be added or updated via the generic plugin instruction set and is managed internally. This guardrail helps downstream integrators avoid accidental misuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockclients/js/src/generated/types/index.tsis excluded by!**/generated/**clients/js/src/generated/types/plugin.tsis excluded by!**/generated/**clients/js/src/generated/types/pluginType.tsis excluded by!**/generated/**clients/rust/src/generated/types/mod.rsis excluded by!**/generated/**clients/rust/src/generated/types/plugin.rsis excluded by!**/generated/**clients/rust/src/generated/types/plugin_type.rsis excluded by!**/generated/**
📒 Files selected for processing (8)
clients/js/src/plugins/types.ts(1 hunks)clients/rust/Cargo.toml(2 hunks)clients/rust/src/hooked/advanced_types.rs(3 hunks)clients/rust/src/hooked/mod.rs(1 hunks)clients/rust/src/hooked/plugin.rs(2 hunks)idls/mpl_core.json(19 hunks)programs/mpl-core/src/plugins/mod.rs(8 hunks)rust-toolchain.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
clients/rust/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the Rust client library in clients/rust/
Files:
clients/rust/Cargo.tomlclients/rust/src/hooked/mod.rsclients/rust/src/hooked/advanced_types.rsclients/rust/src/hooked/plugin.rs
clients/js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the JavaScript client in clients/js/ (hand-written APIs under clients/js/src/)
Files:
clients/js/src/plugins/types.ts
programs/mpl-core/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep the on-chain Solana program under programs/mpl-core/
Files:
programs/mpl-core/src/plugins/mod.rs
🧠 Learnings (1)
📚 Learning: 2025-06-05T12:01:30.794Z
Learnt from: blockiosaurus
PR: metaplex-foundation/mpl-core#231
File: programs/mpl-core/src/instruction.rs:48-68
Timestamp: 2025-06-05T12:01:30.794Z
Learning: In the mpl-core program, new variants must always be appended to the end of the MplAssetInstruction enum to maintain on-chain ABI compatibility. Inserting variants in the middle breaks Borsh serialization ordinals and would render existing clients unable to decode instructions correctly.
Applied to files:
idls/mpl_core.json
🧬 Code graph analysis (5)
clients/js/src/plugins/types.ts (3)
clients/js/src/generated/types/attributes.ts (1)
AttributesArgs(18-18)clients/js/src/generated/types/autograph.ts (1)
AutographArgs(22-22)clients/js/src/generated/types/verifiedCreators.ts (1)
VerifiedCreatorsArgs(22-24)
programs/mpl-core/src/plugins/mod.rs (2)
clients/js/src/generated/types/groups.ts (1)
Groups(17-17)clients/js/src/generated/types/plugin.ts (1)
Plugin(77-96)
clients/rust/src/hooked/mod.rs (2)
clients/js/src/generated/types/plugin.ts (1)
Plugin(77-96)clients/js/src/generated/types/groups.ts (1)
Groups(17-17)
clients/rust/src/hooked/advanced_types.rs (2)
clients/js/src/generated/types/groups.ts (1)
Groups(17-17)clients/js/src/plugins/types.ts (1)
BasePlugin(41-44)
clients/rust/src/hooked/plugin.rs (2)
clients/js/src/generated/types/plugin.ts (1)
Plugin(77-96)clients/js/src/generated/types/groups.ts (1)
Groups(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (actions)
🔇 Additional comments (4)
rust-toolchain.toml (1)
1-3: Toolchain pin looks good.Locking to stable 1.83.0 with rustfmt and clippy keeps local/dev tooling aligned. 👍
clients/js/src/plugins/types.ts (3)
242-251: LGTM! Well-structured group plugin types with focused plugin support.The
GroupAddablePluginArgsV2union type follows the established discriminated union pattern and correctly intersects each plugin type with its corresponding Args type. The intentional limitation to three plugins (Attributes, Autograph, VerifiedCreators) suggests groups have focused functionality compared to Assets and Collections, which is appropriate for a new entity type.
253-258: Consistent with existing plugin type patterns.The group plugin authority pair types follow the same structure as Asset and Collection plugin types:
GroupAllPluginArgsV2correctly aliasesGroupAddablePluginArgsV2(groups don't support create-only plugins)- Intersection types properly combine plugin args with
AuthorityArgsV2This maintains API consistency across entity types while reflecting the intentional design that groups don't require create-only plugins.
260-264: Plugin list interface correctly mirrors the supported plugin types.The
GroupPluginsListinterface properly defines optional properties for the three supported plugins, with each typed as the corresponding Plugin type (which extendsBasePlugin). This structure is consistent withAssetPluginsListandCollectionPluginsListpatterns.
| readme = "README.md" | ||
| repository = "https://github.com/metaplex-foundation/mpl-core" | ||
| version = "0.11.0" | ||
| version = "0.10.1-alpha.2" |
There was a problem hiding this comment.
Restore monotonically increasing crate version.
This drops the crate from 0.11.0 to 0.10.1-alpha.2. Cargo/crates.io require monotonically increasing versions; this regression blocks publishing and can confuse downstream consumers that already depend on 0.11.0. Please bump forward instead of down.
🤖 Prompt for AI Agents
In clients/rust/Cargo.toml around line 8, the crate version was decreased from
0.11.0 to 0.10.1-alpha.2 which breaks Cargo/crates.io monotonic versioning;
update the version field to a forward bump (e.g., version = "0.11.1") so it is
greater than the previously published 0.11.0 and regenerate any lock or release
metadata if applicable.
No description provided.