MIP 11 #230
MIP 11 #230
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Summary by CodeRabbit
End-users can now organize assets and collections into groups, manage group membership and plugins, and utilize enhanced authority and metadata controls. WalkthroughThis change introduces a comprehensive "group" feature set to the Solana program, including new on-chain account types, instructions, error codes, and supporting logic for hierarchical group management, plugin integration, and metadata updates. It adds extensive Rust integration tests and helper utilities to validate group operations, such as creation, closure, asset/collection membership, plugin management, and authority enforcement. The update also includes new modules for group state, plugin, and utility management, and extends the processor and instruction set to support all group-related workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Program
participant GroupAccount
participant CollectionAccount
participant AssetAccount
participant PluginAccount
%% Group Creation with Initial Links
Client->>Program: CreateGroup (with initial collections/parents)
Program->>GroupAccount: Initialize GroupV1
alt With collections
Program->>CollectionAccount: Validate & Link
Program->>PluginAccount: Upsert CollectionGroupPluginV1
end
alt With parent groups
Program->>GroupAccount: Validate Parent
Program->>GroupAccount: Link Parent/Child
end
Program-->>Client: Success
%% Add Asset to Group
Client->>Program: AddAssetsToGroup (group, assets, authorities)
Program->>GroupAccount: Validate authority & update asset list
Program->>AssetAccount: Validate authority
Program->>PluginAccount: Upsert AssetGroupPluginV1
Program-->>Client: Success
%% Add Plugin to Group
Client->>Program: AddGroupPlugin (group, plugin, authority)
Program->>GroupAccount: Validate authority
Program->>GroupAccount: Add plugin to allowed_plugins
Program-->>Client: Success
%% Close Group
Client->>Program: CloseGroup (group, authority)
Program->>GroupAccount: Validate authority & group is empty
Program->>GroupAccount: Close account, return lamports
Program-->>Client: Success or Error (if not empty)
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
we still need an alt name besides 'group', can do a find and replace once decided |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 51
🔭 Outside diff range comments (1)
idls/mpl_core.json (1)
2801-2865:⚠️ Potential issueCritical: GroupV1 struct missing capacity fields
You acceptmaxCollections,maxAssets,maxGroups, andmaxPluginsinCreateGroupArgsbut do not store them in theGroupV1account. Without persisting these limits, enforcement will be impossible.Add the following fields to
GroupV1:{ "name": "GroupV1", "type": { "kind": "struct", "fields": [ { "name": "key", "type": { "defined": "Key" } }, { "name": "updateAuthority", "type": "publicKey" }, { "name": "delegatedAuthority", "type": { "option": "publicKey" } }, { "name": "name", "type": "string" }, { "name": "uri", "type": "string" }, + { "name": "maxCollections", "type": "u32" }, + { "name": "maxAssets", "type": "u32" }, + { "name": "maxGroups", "type": "u32" }, + { "name": "maxPlugins", "type": "u32" }, { "name": "collections", "type": { "vec": "publicKey" } }, { "name": "assets", "type": { "vec": "publicKey" } }, { "name": "childGroups", "type": { "vec": "publicKey" } },
♻️ Duplicate comments (1)
clients/rust/tests/child_groups.rs (1)
172-179: Same index brittleness in removal pathSee previous comment – consider refactor for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (49)
clients/js/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlclients/js/src/generated/accounts/assetGroupPluginV1.tsis excluded by!**/generated/**clients/js/src/generated/accounts/collectionGroupPluginV1.tsis excluded by!**/generated/**clients/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/addAssetsToGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addCollectionsToGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addGroupPlugin.tsis excluded by!**/generated/**clients/js/src/generated/instructions/addGroupsToGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/approveGroupPlugin.tsis excluded by!**/generated/**clients/js/src/generated/instructions/closeGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/createGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/index.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeAssetsFromGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeCollectionsFromGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeGroupPlugin.tsis excluded by!**/generated/**clients/js/src/generated/instructions/removeGroupsFromGroup.tsis excluded by!**/generated/**clients/js/src/generated/instructions/revokeGroupPlugin.tsis excluded by!**/generated/**clients/js/src/generated/instructions/updateGroupMetadata.tsis excluded by!**/generated/**clients/js/src/generated/instructions/updateGroupPlugin.tsis excluded by!**/generated/**clients/js/src/generated/types/allowedPlugin.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/pluginAuthorityType.tsis excluded by!**/generated/**clients/rust/src/generated/accounts/asset_group_plugin_v1.rsis excluded by!**/generated/**clients/rust/src/generated/accounts/collection_group_plugin_v1.rsis 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.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_collections_to_group.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_group_plugin.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/add_groups_to_group.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/approve_group_plugin.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/close_group.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/create_group.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/mod.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_assets_from_group.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_collections_from_group.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_group_plugin.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/remove_groups_from_group.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/revoke_group_plugin.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/update_group_metadata.rsis excluded by!**/generated/**clients/rust/src/generated/instructions/update_group_plugin.rsis excluded by!**/generated/**clients/rust/src/generated/types/allowed_plugin.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_authority_type.rsis excluded by!**/generated/**
📒 Files selected for processing (31)
Cargo.toml(1 hunks)clients/rust/tests/child_groups.rs(1 hunks)clients/rust/tests/close_group.rs(1 hunks)clients/rust/tests/create_collection_asset.rs(1 hunks)clients/rust/tests/create_group_with_links.rs(1 hunks)clients/rust/tests/group_assets.rs(1 hunks)clients/rust/tests/group_collections.rs(1 hunks)clients/rust/tests/group_plugins.rs(1 hunks)clients/rust/tests/setup/group.rs(1 hunks)clients/rust/tests/setup/mod.rs(1 hunks)clients/rust/tests/update_group_metadata.rs(1 hunks)idls/mpl_core.json(8 hunks)programs/mpl-core/Cargo.toml(1 hunks)programs/mpl-core/src/error.rs(1 hunks)programs/mpl-core/src/instruction.rs(2 hunks)programs/mpl-core/src/plugins/group_plugins.rs(1 hunks)programs/mpl-core/src/plugins/mod.rs(2 hunks)programs/mpl-core/src/processor/group/assets.rs(1 hunks)programs/mpl-core/src/processor/group/child_groups.rs(1 hunks)programs/mpl-core/src/processor/group/close.rs(1 hunks)programs/mpl-core/src/processor/group/collections.rs(1 hunks)programs/mpl-core/src/processor/group/create.rs(1 hunks)programs/mpl-core/src/processor/group/metadata.rs(1 hunks)programs/mpl-core/src/processor/group/mod.rs(1 hunks)programs/mpl-core/src/processor/mod.rs(3 hunks)programs/mpl-core/src/state/asset_group_plugin.rs(1 hunks)programs/mpl-core/src/state/collection_group_plugin.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/group.rs(1 hunks)programs/mpl-core/src/utils/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
clients/rust/tests/setup/mod.rs (1)
clients/rust/tests/setup/group.rs (1)
create_test_group(21-60)
programs/mpl-core/src/state/mod.rs (11)
clients/js/src/generated/accounts/groupV1.ts (1)
GroupV1(41-41)clients/js/src/generated/accounts/collectionGroupPluginV1.ts (1)
CollectionGroupPluginV1(30-31)clients/js/src/generated/accounts/assetGroupPluginV1.ts (1)
AssetGroupPluginV1(30-30)clients/rust/src/generated/instructions/add_collections_to_group.rs (2)
group(105-108)group(300-303)clients/rust/src/generated/instructions/add_assets_to_group.rs (2)
group(103-106)group(296-299)clients/rust/src/generated/instructions/add_group_plugin.rs (2)
group(130-133)group(387-390)clients/rust/src/generated/instructions/approve_group_plugin.rs (2)
group(111-114)group(312-315)clients/rust/src/generated/instructions/close_group.rs (2)
group(89-92)group(254-257)clients/rust/src/generated/instructions/create_group.rs (2)
group(133-136)group(422-425)clients/rust/src/generated/instructions/remove_assets_from_group.rs (2)
group(105-108)group(300-303)clients/rust/src/generated/instructions/remove_collections_from_group.rs (2)
group(105-108)group(300-303)
clients/rust/tests/setup/group.rs (1)
clients/rust/src/generated/instructions/create_group.rs (14)
system_program(149-152)system_program(437-443)group(133-136)group(422-425)update_authority(139-145)update_authority(428-434)max_collections(164-167)max_collections(455-458)max_assets(169-172)max_assets(460-463)max_groups(174-177)max_groups(465-468)max_plugins(179-182)max_plugins(470-473)
programs/mpl-core/src/state/collection_group_plugin.rs (3)
programs/mpl-core/src/state/asset_group_plugin.rs (3)
size(35-37)len(41-43)key(47-49)programs/mpl-core/src/state/group.rs (5)
size(129-150)len(34-36)len(55-57)len(160-169)key(154-156)programs/mpl-core/src/state/mod.rs (2)
len(67-74)len(116-118)
clients/rust/tests/create_collection_asset.rs (5)
clients/js/src/generated/types/freezeDelegate.ts (1)
FreezeDelegate(11-11)clients/js/src/generated/types/plugin.ts (1)
Plugin(68-84)clients/js/src/generated/types/pluginAuthorityPair.ts (1)
PluginAuthorityPair(24-27)clients/rust/tests/setup/mod.rs (6)
program_test(15-17)create_collection(195-224)create_asset(40-73)assert_asset(86-181)assert_collection(238-341)airdrop(343-361)clients/js/test/_setupRaw.ts (2)
CreateCollectionHelperArgs(82-89)CreateAssetHelperArgs(32-43)
programs/mpl-core/src/state/asset_group_plugin.rs (4)
programs/mpl-core/src/state/collection_group_plugin.rs (3)
size(35-37)len(41-43)key(47-49)programs/mpl-core/src/state/group.rs (5)
size(129-150)len(34-36)len(55-57)len(160-169)key(154-156)programs/mpl-core/src/plugins/mod.rs (2)
len(118-144)len(212-214)programs/mpl-core/src/state/mod.rs (2)
len(67-74)len(116-118)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Rust Client / Test
🔇 Additional comments (56)
Cargo.toml (1)
11-11: Looks good - simple formatting change.This is a minor whitespace formatting change with no functional impact.
programs/mpl-core/src/utils/mod.rs (1)
1-4: Looks good - clean module organization.The new group utility module and explanatory comment follow a clear organization pattern, maintaining consistency with the existing codebase structure and documentation style.
clients/rust/tests/setup/mod.rs (1)
392-393: Good test structure for new group functionality.The addition of the group module and re-export of the
create_test_groupfunction provides a clean interface for setting up test groups in the extensive new test suite. This supports proper testing of the new hierarchical group management features.Based on the relevant code snippets, the
create_test_groupfunction implements sensible defaults for customizable group properties, making it flexible for various test scenarios.programs/mpl-core/src/plugins/mod.rs (1)
3-3: Module structure looks good!The addition of the
group_pluginsmodule and its public re-export aligns with the broader group functionality being introduced. This follows the established pattern in the codebase of separating concerns into distinct modules and maintaining a clean public API.Also applies to: 12-12
programs/mpl-core/src/error.rs (1)
216-238: Error handling for group operations looks comprehensive.The new error variants properly cover the different failure modes for group operations:
- Missing groups or collections
- Circular references in group hierarchies
- Groups with remaining linked entities
- Invalid operations
- Plugin restrictions
The error codes follow the sequential numbering pattern and have clear, descriptive messages.
programs/mpl-core/src/state/mod.rs (1)
103-108: Account type discriminators added correctly.The new
Keyenum variants provide proper discriminators for the group-related account types, maintaining the established pattern for account identification within the program.programs/mpl-core/src/processor/group/close.rs (2)
22-30: Thorough emptiness check before closure.The implementation properly verifies all collections that could make a group non-empty:
- Collections
- Child groups
- Parent groups
- Assets
- Allowed plugins
This prevents accidental loss of references when closing groups that still have linked entities.
7-34:❓ Verification inconclusive
Solid implementation of the group closure logic.
The function follows best practices for account validation and authority checking:
- Properly extracts and validates required accounts
- Ensures the authority is a signer
- Checks that the group is empty before allowing closure
- Returns appropriate errors for each failure case
Ensure the group account's program owner is validated before closing it:
🏁 Script executed:
#!/bin/bash # Check if program owner validation is implemented elsewhere # Look for owner checks in imported utility functions cat programs/mpl-core/src/utils/mod.rs | grep -A 10 "close_program_account"Length of output: 75
🏁 Script executed:
#!/bin/bash # Locate the definition of close_program_account and inspect its implementation rg -n "fn close_program_account" -n programs/mpl-core/src # Also search all usages to ensure owner validation isn’t happening elsewhere rg -n "close_program_account" -n programs/mpl-core/srcLength of output: 713
🏁 Script executed:
#!/bin/bash # Inspect the implementation of close_program_account for owner checks rg -n "fn close_program_account" -C 5 programs/mpl-core/src/utils/account.rsLength of output: 380
🏁 Script executed:
#!/bin/bash # Retrieve the full implementation of close_program_account to check for owner validation rg -n "pub(crate) fn close_program_account" -A 100 programs/mpl-core/src/utils/account.rsLength of output: 91
🏁 Script executed:
#!/bin/bash # Retrieve the full implementation of close_program_account for owner validation analysis rg -n "fn close_program_account" -A 200 programs/mpl-core/src/utils/account.rsLength of output: 2693
🏁 Script executed:
#!/bin/bash # Inspect GroupV1::load implementation for owner validation rg -n "impl GroupV1" -A 200 programs/mpl-core/srcLength of output: 6280
Ensure the group account is owned by this program before closing
Before invoking
close_program_account, there’s no explicit check thatgroup_info.owner == &crate::ID, which could allow closing accounts not owned by this program. Please add (or confirm viaGroupV1::load) an owner check:• File: programs/mpl-core/src/processor/group/close.rs
Insert before line 33 (beforeclose_program_account):if group_info.owner != &crate::ID { return Err(MplCoreError::InvalidOwner.into()); }or, if you have an
assert_owned_byhelper:assert_owned_by(group_info, &crate::ID)?;This guarantees only accounts owned by the MPL Core program can be closed.
programs/mpl-core/src/processor/group/metadata.rs (1)
36-43: Authority check looks solidThe signer assertion followed by
validate_authorityprovides a clear 2-step verification and prevents privilege escalation.clients/rust/tests/setup/group.rs (1)
30-44: Nice helper – butupdate_authoritymay need to signWhen the caller passes
Some(pubkey)that ≠context.payer, the transaction will lack the corresponding signature, causing tests to fail unexpectedly.
Either (a) document that the provided authority must becontext.payer, or (b) add it to the signers list when it differs.clients/rust/tests/child_groups.rs (2)
250-285: Great negative test for circular referenceCovers an important edge case; assert looks good.
334-341: Back-pointer check is valuableEnsures bidirectional integrity after removal – nice.
programs/mpl-core/src/processor/mod.rs (3)
11-11: Good addition of the group module.The module declaration follows the existing pattern in the codebase and properly integrates with the rest of the modules.
32-32: Module export looks good.The public re-export of the group module maintains consistency with other module exports.
187-243: Well-structured instruction handlers for group operations.The additions to the instruction processor correctly handle the new group-related instructions following the same pattern as existing code. Each instruction is properly logged and delegated to its specific handler function. The comprehensive set of operations provides a complete API for group management, including creation, closure, metadata management, and various membership operations.
clients/rust/tests/update_group_metadata.rs (3)
16-36: Good helper function for metadata verification.The
assert_group_metadatahelper function is well-designed, keeping the test code DRY by providing a reusable way to verify group metadata fields.
38-80: Thorough success test case.This test covers the happy path for updating group metadata with good test setup, execution, and verification steps. It tests all updatable fields (name, URI, and update authority).
82-130: Good error handling test.The test properly verifies that:
- Using an invalid authority results in the expected error
- The metadata remains unchanged after a failed update attempt
This negative testing is crucial for security and data integrity.
programs/mpl-core/src/state/collection_group_plugin.rs (3)
7-17: Well-structured account data type.The
CollectionGroupPluginV1struct is properly designed with:
- Clear documentation
- Appropriate account discriminator
- Necessary fields for tracking collection-to-group relationships
- Size annotations for serialization
19-38: Good implementation of constructor and size calculation.The implementation:
- Correctly defines the constant base length
- Provides a clean constructor method
- Accurately calculates allocation size based on max groups
The size calculation properly accounts for both fixed-size components and the variable-length vector of group pubkeys.
40-50: Appropriate trait implementations.The implementations of
DataBlobandSolanaAccounttraits are correct:
len()accurately returns the current size based on groups countkey()returns the appropriate discriminatorThese implementations enable proper serialization and account validation.
programs/mpl-core/src/state/asset_group_plugin.rs (3)
7-17: Well-structured asset group plugin account.Similar to the collection group plugin, this account type is properly designed with clear documentation, appropriate fields, and serialization support. The struct handles tracking an asset's membership across multiple groups.
19-38: Good implementation of constructor and size utilities.The implementation correctly:
- Defines the base length constant
- Provides a constructor for new instances
- Calculates account size requirements based on maximum group count
40-50: Solid trait implementations.The trait implementations for
DataBlobandSolanaAccountare correct, providing:
- Accurate current length calculation
- Proper key identification
This ensures the account can be correctly serialized, deserialized, and identified.
programs/mpl-core/src/instruction.rs (1)
364-370: Confirmplugin_programshould really be a required account
plugin_program(idx 3) is currently annotated as a mandatory, read-only account.
In most plugin workflows the CPI target program ID can normally be inferred from thepluginaccount’s data and therefore does not have to be passed explicitly – or it can be passed optionally if some plugins do not require it.For on-chain size and CI-fee reasons you may want to declare this account as
optional(and even remove it when not needed) to keep the account list minimal.If you intentionally enforce an explicit program ID, please document the rationale in the doc-comment above the variant so downstream SDK builders understand why an extra account is required.
clients/rust/tests/create_group_with_links.rs (1)
120-125: Order of remaining accounts must match on-chain validationThe instruction appends
[collection, collection_update_auth, parent_group, parent_update_auth].
If the on-chain processor expects the update-authority directly after each resource account, the ordering is correct.Double-check the processor logic; a mismatch will throw “account out of order” errors which are painful to debug in integration tests.
clients/rust/tests/close_group.rs (1)
84-87: Make the refund check deterministic.
payer_after > payer_beforepasses even if only 1-lamport was refunded.
Capturing the group’s lamports before closing lets you assert the exact delta and removes non-determinism introduced by rent changes.-assert!(payer_after > payer_before, "Payer should receive lamports from the closed account"); +let refunded = payer_after - payer_before; +assert_eq!( + refunded, + group_account_before.lamports, // record this just before issuing the close ix + "Payer should receive the full balance of the closed account" +);[ suggest_nitpick ]
programs/mpl-core/src/processor/group/child_groups.rs (1)
121-134: Vector shrink usesswap_remove– order-agnostic but O(1)Nice touch using
swap_remove; order ofchild_groups/parent_groupsisn’t semantically important and you save an O(n) shift. 👍programs/mpl-core/src/state/group.rs (2)
137-150:sizehelper omits Option worst-case
BASE_LENreserves1 + 32bytes fordelegated_authority, butsize()doesn’t account for the variable nature ofOption.
Callers wanting “worst-case” space (e.g. alloc account upfront) might under-allocate if they later set a delegate.Either document that
size()assumes aNonedelegate or add+ 32to cover theSomecase whenmax_delegate = true.
117-126: 🧹 Nitpick (assertive)
validate_authorityignoresdelegated_authority == update_authorityedge-caseIf someone mistakenly sets
delegated_authorityequal toupdate_authority, the delegated key is effectively duplicated.
Unifying the check prevents silent config errors:-if &self.update_authority != authority { +if authority != &self.update_authority { match self.delegated_authority { - Some(pubkey) if &pubkey == authority => Ok(()), + Some(pubkey) if &pubkey == authority && pubkey != self.update_authority => Ok(()),Likely an incorrect or invalid review comment.
programs/mpl-core/src/plugins/group_plugins.rs (1)
158-161:update_group_plugincurrently performs no state changeAfter authority checks it returns
Ok(()). If this is intentional (e.g. the plugin program handles updates), document that; otherwise this is incomplete functionality.idls/mpl_core.json (25)
2117-2166: New CreateGroup instruction looks correct
Accounts and arguments align with the intended flow for creating a group account (payer, PDA, update authority, system program). The discriminant value (33) follows the previous instruction. Args referenceCreateGroupArgswhich is defined later.
2167-2192: CloseGroup instruction is properly defined
Using theupdateAuthorityas the lamports recipient and closing thegroupPDA is consistent. Discriminant value (34) is sequential.
2193-2225: UpdateGroupMetadata covers name/URI/authority updates
Mapping toUpdateGroupMetadataArgsis correct, and the authority guard is in place. Discriminant (35) is sequential.
2226-2258: AddCollectionsToGroup instruction is well-defined
The authority signer and mutablegroupaccount match expectations. Args typeAddCollectionsToGroupArgsis available. Discriminant (36) is correct.
2259-2291: RemoveCollectionsFromGroup instruction is well-defined
Mirrors the AddCollections instruction with matching args type and correct discriminant (37).
2292-2324: AddAssetsToGroup instruction is well-defined
UsesAddAssetsToGroupArgscorrectly and follows the sequential discriminant (38).
2325-2357: RemoveAssetsFromGroup instruction is well-defined
Args, accounts, and discriminant (39) are consistent with the AddAssets instruction.
2424-2480: AddGroupPlugin instruction integrates plugin support
Accounts includepluginProgramfor on-chain validation, and the argsAddGroupPluginArgsis defined. Discriminant (42) flows correctly.
2481-2514: RemoveGroupPlugin instruction is correct
No args needed, and discriminant (43) is sequential.
2515-2555: UpdateGroupPlugin instruction defined properly
Maps toUpdateGroupPluginArgsand uses the correct authority guard. Discriminant (44) is valid.
2556-2596: ApproveGroupPlugin instruction looks good
Accounts, args (ApproveGroupPluginArgs), and discriminant (45) follow the pattern of plugin management.
2597-2630: RevokeGroupPlugin instruction is correct
Follows RemoveGroupPlugin in structure with discriminant (46).
2719-2742: AssetGroupPluginV1 account type is appropriate
Defines the linkage between an asset and its associated groups. Fields (assetandgroups) are coherent.
2743-2776: CollectionV1 struct extended with metadata fields
Addingname,uri,numMinted, andcurrentSizesupports on-chain group operations and display logic.
2777-2800: CollectionGroupPluginV1 account type is appropriate
Mirrors the asset plugin structure for collections.
3490-3522: Plugin argument types are in place
AddGroupPluginArgs,UpdateGroupPluginArgs, andApproveGroupPluginArgsalign with the new plugin workflow.
4201-4231: Group asset/authority indices types are correct
AddAssetsToGroupArgsandRemoveAssetsFromGroupArgsuse byte arrays for indices; this is consistent with similar IDL patterns.
4233-4263: Group group/authority indices types are correct
AddGroupsToGroupArgsandRemoveGroupsFromGroupArgsfollow the same pattern.
4265-4295: Group collection/authority indices types are correct
AddCollectionsToGroupArgsandRemoveCollectionsFromGroupArgsare consistent with other membership instructions.
4297-4345: CreateGroupArgs struct covers initial state
Fields matchCreateGrouprequirements. Remember to updateGroupV1to store max values (see above).
4347-4371: UpdateGroupMetadataArgs struct is correct
Supports optional update of name, URI, and delegated authority.
4712-4729: AllowedPlugin struct supports plugin lists
TheaddressandauthorityTypefields align withPluginAuthorityType.
5621-5632: Key enum extended with Group types
VariantsGroupV1,CollectionGroupPluginV1, andAssetGroupPluginV1are correctly appended.
5635-5653: PluginAuthorityType enum is defined appropriately
VariantsNone,UpdateAuthority, andSpecificAddresscover all authority models.
5935-5963: New group-specific error codes are added
Codes 51–56 cover all necessary group error conditions consecutively.
| [patch.crates-io] | ||
| ahash = "=0.8.7" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good practice pinning the ahash dependency.
Pinning the ahash version to exactly 0.8.7 ensures consistent hashing behavior across all dependencies in the crate graph, which is crucial for stability when handling complex data structures in the new group functionality.
Consider adding a comment explaining why this specific version was chosen, for future maintainers:
[patch.crates-io]
-ahash = "=0.8.7"
+ahash = "=0.8.7" # Pinned to ensure consistent hashing behavior for group data structures📝 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.
| [patch.crates-io] | |
| ahash = "=0.8.7" | |
| [patch.crates-io] | |
| ahash = "=0.8.7" # Pinned to ensure consistent hashing behavior for group data structures |
🤖 Prompt for AI Agents
In programs/mpl-core/Cargo.toml around lines 27 to 28, add a comment above the
ahash dependency pinning that explains why version 0.8.7 is specifically chosen.
This comment should mention that pinning this exact version ensures consistent
hashing behavior across dependencies, which is important for stability in
handling complex data structures in the new group functionality.
| mod group; | ||
| pub use group::*; | ||
|
|
||
| mod collection_group_plugin; | ||
| pub use collection_group_plugin::*; | ||
|
|
||
| mod asset_group_plugin; | ||
| pub use asset_group_plugin::*; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider reorganizing module declarations.
While the module additions themselves look good, they're placed after the test module rather than grouped with the other module declarations at the top of the file. Consider moving these module declarations to be with the other modules for better code organization and readability.
-mod tests {
- use strum::IntoEnumIterator;
-
- use super::*;
-
- #[test]
- fn test_authority_len() {
- let authorities = vec![
- Authority::None,
- Authority::Owner,
- Authority::UpdateAuthority,
- Authority::Address {
- address: Pubkey::default(),
- },
- ];
- assert_eq!(
- authorities.len(),
- Authority::COUNT,
- "Must test all Authority variants"
- );
- for authority in authorities {
- let serialized = authority.try_to_vec().unwrap();
-
- assert_eq!(serialized.len(), authority.len());
- }
- }
-
- #[test]
- fn test_key_len() {
- for key in Key::iter() {
- let serialized = key.try_to_vec().unwrap();
- assert_eq!(serialized.len(), key.len());
- }
- }
-}
-
-mod group;
-pub use group::*;
-
-mod collection_group_plugin;
-pub use collection_group_plugin::*;
-
-mod asset_group_plugin;
-pub use asset_group_plugin::*;
+mod group;
+pub use group::*;
+
+mod collection_group_plugin;
+pub use collection_group_plugin::*;
+
+mod asset_group_plugin;
+pub use asset_group_plugin::*;
+
+mod tests {
+ use strum::IntoEnumIterator;
+
+ use super::*;
+
+ #[test]
+ fn test_authority_len() {
+ let authorities = vec![
+ Authority::None,
+ Authority::Owner,
+ Authority::UpdateAuthority,
+ Authority::Address {
+ address: Pubkey::default(),
+ },
+ ];
+ assert_eq!(
+ authorities.len(),
+ Authority::COUNT,
+ "Must test all Authority variants"
+ );
+ for authority in authorities {
+ let serialized = authority.try_to_vec().unwrap();
+
+ assert_eq!(serialized.len(), authority.len());
+ }
+ }
+
+ #[test]
+ fn test_key_len() {
+ for key in Key::iter() {
+ let serialized = key.try_to_vec().unwrap();
+ assert_eq!(serialized.len(), key.len());
+ }
+ }
+}📝 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.
| mod group; | |
| pub use group::*; | |
| mod collection_group_plugin; | |
| pub use collection_group_plugin::*; | |
| mod asset_group_plugin; | |
| pub use asset_group_plugin::*; | |
| mod group; | |
| pub use group::*; | |
| mod collection_group_plugin; | |
| pub use collection_group_plugin::*; | |
| mod asset_group_plugin; | |
| pub use asset_group_plugin::*; | |
| mod tests { | |
| use strum::IntoEnumIterator; | |
| use super::*; | |
| #[test] | |
| fn test_authority_len() { | |
| let authorities = vec![ | |
| Authority::None, | |
| Authority::Owner, | |
| Authority::UpdateAuthority, | |
| Authority::Address { | |
| address: Pubkey::default(), | |
| }, | |
| ]; | |
| assert_eq!( | |
| authorities.len(), | |
| Authority::COUNT, | |
| "Must test all Authority variants" | |
| ); | |
| for authority in authorities { | |
| let serialized = authority.try_to_vec().unwrap(); | |
| assert_eq!(serialized.len(), authority.len()); | |
| } | |
| } | |
| #[test] | |
| fn test_key_len() { | |
| for key in Key::iter() { | |
| let serialized = key.try_to_vec().unwrap(); | |
| assert_eq!(serialized.len(), key.len()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In programs/mpl-core/src/state/mod.rs around lines 157 to 164, the new module
declarations are placed after the test module, which disrupts the logical
grouping of module imports. Move the declarations of group,
collection_group_plugin, and asset_group_plugin modules to be alongside the
other module declarations at the top of the file to improve code organization
and readability.
| if let Some(name) = args.name { | ||
| group.name = name; | ||
| } | ||
| if let Some(uri) = args.uri { | ||
| group.uri = uri; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Bounds check for name / uri length missing
A malicious client could supply a huge string, blowing the account’s size and DoS-ing deserialize.
Consider enforcing a reasonable max length (e.g. 256 chars) consistent with creation logic.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/group/metadata.rs around lines 45 to 50, the
code assigns `name` and `uri` from `args` to `group` without checking their
length, which risks DoS from overly large strings. Add a length check before
assignment to ensure `name` and `uri` do not exceed a maximum length (e.g., 256
characters). Only assign the values if they pass this length validation to
prevent oversized data from causing issues.
| if let Some(name) = args.name { | ||
| group.name = name; | ||
| } | ||
| if let Some(uri) = args.uri { | ||
| group.uri = uri; | ||
| } | ||
| if let Some(new_auth) = args.new_update_authority { | ||
| group.update_authority = new_auth; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Updating update_authority without verifying the new signer
new_update_authority can be set to an arbitrary pubkey that is not a signer of the current tx.
That means the current authority could mistakenly type a wrong key and irreversibly lock the group.
Consider requiring the new authority to be supplied in accounts[2] and asserted with assert_signer, or emit a warning/event when new_auth is not a signer.
+// 2 optional –– new authority must sign if being set
+if let Some(new_auth) = args.new_update_authority {
+ let new_auth_info = accounts.get(2).ok_or(MplCoreError::MissingUpdateAuthority)?;
+ assert_eq!(new_auth, *new_auth_info.key);
+ assert_signer(new_auth_info)?;
+ group.update_authority = new_auth;
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/group/metadata.rs around lines 45 to 53, the
code updates the group's update_authority without verifying that the new
authority is a signer of the transaction, which risks locking the group if an
incorrect key is provided. To fix this, require the new_update_authority to be
passed as a separate account (e.g., accounts[2]) and use assert_signer to verify
it is a signer before assigning it to group.update_authority. Alternatively,
emit a warning or event if the new authority is not a signer to alert about the
potential issue.
| // Save | ||
| let serialized = group.try_to_vec()?; | ||
| sol_memcpy( | ||
| &mut group_info.try_borrow_mut_data()?[0..serialized.len()], | ||
| &serialized, | ||
| serialized.len(), | ||
| ); |
There was a problem hiding this comment.
Partial memcpy may leave stale bytes → future deserialize failures
try_to_vec()’s length can shrink (e.g., shorter name).
sol_memcpy only overwrites the first serialized.len() bytes, leaving trailing bytes from the previous state untouched; Borsh will later error because extra bytes remain.
Zero-fill the whole buffer or assert equal size.
let mut data = group_info.try_borrow_mut_data()?;
-let dst = &mut data[0..serialized.len()];
-sol_memcpy(dst, &serialized, serialized.len());
+// wipe then copy to avoid stale tail bytes
+data.fill(0);
+sol_memcpy(&mut data[..serialized.len()], &serialized, serialized.len());Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In programs/mpl-core/src/processor/group/metadata.rs around lines 55 to 61, the
current code copies only the serialized data length into the buffer, potentially
leaving stale bytes from previous data which can cause deserialization errors.
To fix this, either zero-fill the entire buffer before copying or assert that
the serialized data length matches the buffer length exactly, ensuring no
leftover bytes remain after the memcpy operation.
| sol_memcpy( | ||
| &mut group_info.try_borrow_mut_data()?[..serialized_group.len()], | ||
| &serialized_group, | ||
| serialized_group.len(), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Zero-fill for consistency & safety
add_group_plugin mutates the group but does not clear the leftover bytes, unlike remove_group_plugin, approve_group_plugin, and revoke_group_plugin. Adopt the same pattern here to avoid stale data after a later shrink.
🤖 Prompt for AI Agents
In programs/mpl-core/src/plugins/group_plugins.rs around lines 108 to 113, the
code copies serialized_group into group_info but does not zero-fill the
remaining bytes, which can leave stale data. After the sol_memcpy call, add code
to zero out the rest of the group_info buffer beyond serialized_group.len() to
match the pattern used in remove_group_plugin and others, ensuring leftover
bytes are cleared for consistency and safety.
| if plugin_entry.authority_type == PluginAuthorityType::None { | ||
| if args.plugin_args.len() == 32 { | ||
| let pubkey_bytes: [u8; 32] = args.plugin_args[..32] | ||
| .try_into() | ||
| .map_err(|_| MplCoreError::InvalidPluginOperation)?; | ||
| plugin_entry.authority_type = | ||
| PluginAuthorityType::SpecificAddress(Pubkey::new_from_array(pubkey_bytes)); | ||
| } else { | ||
| plugin_entry.authority_type = PluginAuthorityType::UpdateAuthority; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ambiguous semantics for plugin_args length
Approving with a 32-byte payload implicitly sets SpecificAddress; any other length sets UpdateAuthority. Consider documenting or encoding the variant explicitly to avoid silent misconfiguration when arbitrary data > 32 bytes is supplied.
🤖 Prompt for AI Agents
In programs/mpl-core/src/plugins/group_plugins.rs around lines 216 to 226, the
current logic implicitly sets PluginAuthorityType based on the length of
plugin_args, which can cause silent misconfiguration. To fix this, explicitly
encode the authority type variant in the input data or add clear documentation
explaining this behavior. Modify the code to check for an explicit indicator of
the authority type rather than relying solely on the length of plugin_args,
ensuring that arbitrary data longer than 32 bytes does not unintentionally set
UpdateAuthority.
| if accounts.len() < 5 { | ||
| return Err(MplCoreError::MissingSystemProgram.into()); | ||
| } | ||
|
|
||
| let authority_info = accounts | ||
| .get(0) | ||
| .ok_or::<ProgramError>(MplCoreError::MissingSigner.into())?; | ||
| let group_info = accounts | ||
| .get(1) | ||
| .ok_or::<ProgramError>(MplCoreError::GroupNotFound.into())?; | ||
| let _plugin_account_info = accounts | ||
| .get(2) | ||
| .ok_or::<ProgramError>(MplCoreError::PluginNotFound.into())?; | ||
| let plugin_program_info = accounts | ||
| .get(3) | ||
| .ok_or::<ProgramError>(MplCoreError::InvalidPlugin.into())?; | ||
| let system_program_info = accounts | ||
| .get(4) | ||
| .ok_or::<ProgramError>(MplCoreError::MissingSystemProgram.into())?; | ||
|
|
There was a problem hiding this comment.
Redundant account & potential caller confusion
_plugin_account_info (index 2) is fetched but never used, while the plugin’s program (index 3) is what gets stored in allowed_plugins.
Consequences:
- An extra account is needlessly required, increasing compute & CU cost.
- Subsequent instructions (
remove_*,approve_*, …) look up the plugin by the unused account’s key, leading to a mismatch and inevitablePluginNotFound.
Either drop the unused account or store accounts[2].key in AllowedPlugin.
- let _plugin_account_info = accounts.get(2) ...?;
- let plugin_program_info = accounts.get(3) ...?;
+ let plugin_program_info = accounts.get(2)?; // if you only need the program idCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In programs/mpl-core/src/plugins/group_plugins.rs between lines 56 and 75, the
account at index 2 (_plugin_account_info) is retrieved but never used, causing
unnecessary account requirements and confusion in plugin lookups. To fix this,
either remove the retrieval of the unused account at index 2 entirely or modify
the code to store the key of accounts[2] in AllowedPlugin instead of using the
plugin program at index 3, ensuring consistency in how plugins are referenced
across instructions.
| { | ||
| "name": "RemoveGroupsFromGroup", | ||
| "accounts": [ | ||
| { | ||
| "name": "authority", | ||
| "isMut": false, | ||
| "isSigner": true, | ||
| "docs": [ | ||
| "The parent group update authority or delegated authority" | ||
| ] | ||
| }, | ||
| { | ||
| "name": "parentGroup", | ||
| "isMut": true, | ||
| "isSigner": false, | ||
| "docs": [ | ||
| "The parent group account" | ||
| ] | ||
| } | ||
| ], | ||
| "args": [ | ||
| { | ||
| "name": "removeGroupsFromGroupArgs", | ||
| "type": { | ||
| "defined": "RemoveGroupsFromGroupArgs" | ||
| } | ||
| } | ||
| ], | ||
| "discriminant": { | ||
| "type": "u8", | ||
| "value": 41 | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
RemoveGroupsFromGroup instruction mirrors AddGroups
Accounts, args, and discriminant (41) align correctly.
Same as above: consider standardizing on group for the account name.
🤖 Prompt for AI Agents
In idls/mpl_core.json between lines 2391 and 2423, the RemoveGroupsFromGroup
instruction uses the account name "parentGroup" which should be standardized to
"group" to maintain consistency with similar instructions like AddGroups. Rename
the "parentGroup" account to "group" in the accounts array, update its
documentation accordingly to reflect it as the group account, and ensure all
references within this instruction match the new name.
| { | ||
| "name": "AddGroupsToGroup", | ||
| "accounts": [ | ||
| { | ||
| "name": "authority", | ||
| "isMut": false, | ||
| "isSigner": true, | ||
| "docs": [ | ||
| "The parent group update authority or delegated authority" | ||
| ] | ||
| }, | ||
| { | ||
| "name": "parentGroup", | ||
| "isMut": true, | ||
| "isSigner": false, | ||
| "docs": [ | ||
| "The parent group account" | ||
| ] | ||
| } | ||
| ], | ||
| "args": [ | ||
| { | ||
| "name": "addGroupsToGroupArgs", | ||
| "type": { | ||
| "defined": "AddGroupsToGroupArgs" | ||
| } | ||
| } | ||
| ], | ||
| "discriminant": { | ||
| "type": "u8", | ||
| "value": 40 | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
AddGroupsToGroup instruction is logically correct
Using parentGroup to denote the parent PDA is clear, and the args type AddGroupsToGroupArgs matches. Discriminant (40) is correct.
Consider renaming the account name parentGroup to group for naming consistency with other group-related instructions, then disambiguating in client code if needed.
🤖 Prompt for AI Agents
In idls/mpl_core.json between lines 2358 and 2390, rename the account named
"parentGroup" to "group" in the AddGroupsToGroup instruction to maintain naming
consistency with other group-related instructions. After renaming, ensure any
client code referencing this account is updated to disambiguate and correctly
use the new "group" name.
No description provided.