Moving ME checks to the plugin. #222
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes update error handling for the master edition plugin. Test cases now expect a new error name, "PluginNotAllowedOnAsset," and corresponding error definitions have been added to the JSON file and the error enum. New validation methods have been introduced to the master edition plugin for creation and plugin addition, while lifecycle handling now explicitly considers the MasterEdition type. Additionally, processor functions have been modified to remove checks against the MasterEdition type and update the validation context assignment. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Processor
participant ValidationCtx as ValidationContext
participant MasterEdition as "MasterEdition Plugin"
Client->>Processor: Request to add or create plugin
Processor->>ValidationCtx: Populate target plugin
Processor->>MasterEdition: Call validate_add_plugin / validate_create
alt Invalid Configuration
MasterEdition-->>Processor: Return error "PluginNotAllowedOnAsset"
else Valid Configuration
MasterEdition-->>Processor: Return success
end
Processor->>Client: Return result
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
There was a problem hiding this comment.
PR Overview
This PR moves the MasterEdition checks from the top-level processor into the plugin validations of MasterEdition, centralizing the logic and updating error handling accordingly.
- Implements plugin-specific validation functions (validate_create and validate_add_plugin) for MasterEdition
- Updates lifecycle checks to include PluginType::MasterEdition
- Refactors tests and error handling by replacing InvalidPlugin with PluginNotAllowedOnAsset and updating processor creation and plugin addition flows
Reviewed Changes
| File | Description |
|---|---|
| programs/mpl-core/src/plugins/internal/authority_managed/master_edition.rs | Implements MasterEdition plugin validations for create and add_plugin scenarios |
| programs/mpl-core/src/plugins/lifecycle.rs | Updates PluginType lifecycle checks for MasterEdition support |
| clients/js/test/plugins/collection/masterEdition.test.ts | Updates error name expectations in tests to reflect the new PluginNotAllowedOnAsset error |
| programs/mpl-core/src/error.rs | Adds the PluginNotAllowedOnAsset error to clearly indicate disallowed plugin usage |
| programs/mpl-core/src/processor/create.rs | Removes direct MasterEdition check and assigns target_plugin for proper plugin validation |
| programs/mpl-core/src/processor/add_plugin.rs | Removes direct MasterEdition check to rely on plugin validation instead |
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
programs/mpl-core/src/plugins/internal/authority_managed/master_edition.rs (1)
47-62: Consider refactoring duplicated logic invalidate_add_plugin.Both
validate_createandvalidate_add_pluginenforce the MasterEdition prohibition on assets. While this duplication is minor, extracting a shared helper to check for MasterEdition-on-asset combinations could make the code more concise and consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clients/js/src/generated/errors/mplCore.tsis excluded by!**/generated/**clients/rust/src/generated/errors/mpl_core.rsis excluded by!**/generated/**
📒 Files selected for processing (7)
clients/js/test/plugins/collection/masterEdition.test.ts(2 hunks)idls/mpl_core.json(1 hunks)programs/mpl-core/src/error.rs(1 hunks)programs/mpl-core/src/plugins/internal/authority_managed/master_edition.rs(2 hunks)programs/mpl-core/src/plugins/lifecycle.rs(2 hunks)programs/mpl-core/src/processor/add_plugin.rs(0 hunks)programs/mpl-core/src/processor/create.rs(1 hunks)
💤 Files with no reviewable changes (1)
- programs/mpl-core/src/processor/add_plugin.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Programs / Test (mpl-core)
- GitHub Check: Programs / Build
🔇 Additional comments (10)
idls/mpl_core.json (1)
4953-4957: New error code for plugin validation looks good.This new error code (50) with name "PluginNotAllowedOnAsset" provides a more specific error message when a plugin, particularly MasterEdition, is not allowed to be added to an Asset. This is in line with the PR objective of moving ME checks to the plugin validators.
programs/mpl-core/src/error.rs (1)
212-214: Error definition is consistent with IDL changes.The new error variant
PluginNotAllowedOnAssetwith error code 50 correctly matches the corresponding error in the IDL file. The error message clearly communicates that the plugin is not allowed to be added to an Asset.clients/js/test/plugins/collection/masterEdition.test.ts (2)
130-132: Test updated to expect the new error type.Test case for "it cannot add masterEdition to asset" has been updated to expect the more specific
PluginNotAllowedOnAsseterror instead of the genericInvalidPluginerror. This correctly reflects the changes in error handling.
152-154: Test updated to expect the new error type.Test case for "it cannot create asset with masterEdition" has been updated to expect the more specific
PluginNotAllowedOnAsseterror. This properly tests the validation logic that has been moved to the plugin validators.programs/mpl-core/src/plugins/lifecycle.rs (3)
94-94: Added MasterEdition to check_add_plugin with CanReject permission.MasterEdition plugin type has been added to the
check_add_pluginmethod withCheckResult::CanReject, allowing it to reject attempts to add the plugin. This is part of moving the validation logic from processors to plugin validators.
146-146: Added MasterEdition to check_create with CanReject permission.MasterEdition plugin type has been added to the
check_createmethod withCheckResult::CanReject, enabling it to reject asset creation attempts that include a MasterEdition plugin. This change moves the validation from processors to the plugin system.
88-96:✅ Verification successful
Verify that MasterEdition validation is implemented correctly.
Now that the MasterEdition plugin can reject during lifecycle events, ensure that the validation method in the MasterEdition plugin implementation correctly prevents it from being added to an asset (as opposed to a collection).
🏁 Script executed:
#!/bin/bash # Let's find and check the MasterEdition plugin implementation to ensure it has proper validation logic # Search for MasterEdition validator implementation rg "MasterEdition" --type rust -A 10 -B 10 "validate_" programs/Length of output: 27634
MasterEdition Validation Confirmed
- The MasterEdition plugin’s validation methods in
programs/mpl-core/src/plugins/internal/authority_managed/master_edition.rs
(bothvalidate_createandvalidate_add_plugin) correctly enforce that the MasterEdition plugin cannot be added to an asset. They check for the presence of an asset (viactx.asset_info.is_some()) and return the appropriate error (PluginNotAllowedOnAsset) when a plugin addition is attempted on an asset.- Additionally, the lifecycle configuration in
programs/mpl-core/src/plugins/lifecycle.rscorrectly mapsPluginType::MasterEditiontoCheckResult::CanReject, ensuring rejection during lifecycle events.No further changes are required here.
programs/mpl-core/src/processor/create.rs (1)
199-199: Ensure plugin context remains consistent after relocating checks.By setting
target_plugintoSome(&plugin.plugin), you effectively shift the plugin-specific checks to the plugin’s validation methods. This is aligned with the goal of moving MasterEdition checks out of the top-level processor logic. However, ensure that no unintended side effects occur for other plugin types. A quick global check of all plugin usage sites will confirm there aren’t any unforeseen conflicts.programs/mpl-core/src/plugins/internal/authority_managed/master_edition.rs (2)
2-2: Imports are consistent with the new validation logic.Adding
ProgramErrorand referencingMplCoreError,PluginValidationContext, andValidationResultensures that the plugin’s validation can produce clear, typed error conditions. This appears well-integrated with the rest of the crate’s error handling.Also applies to: 4-7
29-45: Good introduction ofvalidate_createmethod.Shifting the MasterEdition checks to the plugin level is a more modular approach, allowing the plugin to reject creation when an asset context is detected. This aligns with the PR objective of relocating MasterEdition checks from top-level logic. If you want deeper test coverage, consider verifying that
validate_createindeed prevents MasterEdition initialization on assets for various scenarios.
![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: cd72401 | Previous: 661d5e0 | Ratio |
|---|---|---|---|
CU: create a new, empty asset |
11208 Compute Units |
11206 Compute Units |
1.00 |
Space: create a new, empty asset |
91 Bytes |
91 Bytes |
1 |
CU: create a new, empty asset with empty collection |
22683 Compute Units |
22681 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 |
35121 Compute Units |
35182 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 |
41141 Compute Units |
41202 Compute Units |
1.00 |
Space: create a new asset with plugins and empty collection |
194 Bytes |
194 Bytes |
1 |
CU: list an asset |
28072 Compute Units |
28101 Compute Units |
1.00 |
CU: sell an asset |
44473 Compute Units |
44473 Compute Units |
1 |
CU: list an asset with empty collection |
35634 Compute Units |
35663 Compute Units |
1.00 |
CU: sell an asset with empty collection |
56825 Compute Units |
56825 Compute Units |
1 |
CU: list an asset with collection royalties |
36405 Compute Units |
36434 Compute Units |
1.00 |
CU: sell an asset with collection royalties |
61909 Compute Units |
61909 Compute Units |
1 |
CU: transfer an empty asset |
6444 Compute Units |
6444 Compute Units |
1 |
CU: transfer an empty asset with empty collection |
9043 Compute Units |
9043 Compute Units |
1 |
CU: transfer an asset with plugins |
14769 Compute Units |
14769 Compute Units |
1 |
CU: transfer an asset with plugins and empty collection |
17368 Compute Units |
17368 Compute Units |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
11-25: Path Instructions Configuration Uses Valid Flow StyleThe
path_instructionsarray is provided in flow style with inline objects. Each object definespathandinstructionsappropriately. Although using block style may enhance readability for longer lists, the current concise format is acceptable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.coderabbit.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Programs / Test (mpl-core)
- GitHub Check: Programs / Build
🔇 Additional comments (5)
.coderabbit.yaml (5)
1-1: Schema Header Declaration is CorrectThe header on line 1 properly points to the CodeRabbit YAML schema, ensuring the file will be validated against the expected configuration.
2-3: Language and Early Access Settings are ValidThe
languageis set to"en-US", which is an allowed enum value as per the schema, andearly_accessis correctly set tofalse.
4-10: Review Configuration Block is Well-StructuredThe keys under
reviews(such asprofile,request_changes_workflow,high_level_summary,poem,review_status, andcollapse_walkthrough) are defined appropriately and align with the configuration schema.
26-28: Auto Review Settings are Configured CorrectlyThe
auto_reviewsettings withenabled: trueanddrafts: falseare correctly specified according to the schema.
29-31: Chat Settings are Defined AppropriatelyThe
chatblock includesauto_reply: true, which is consistent with the schema. There is no issue with the current setup.
Moving the ME checks preventing creation to the plugin validators instead of the top level processors.
I had to add the plugin as the target plugin in the create validator but that shouldn't effect anything.
Summary by CodeRabbit
New Features
Bug Fixes
Tests