Replace function pointer params with LifecycleEvent trait for plugin validation - #257
Replace function pointer params with LifecycleEvent trait for plugin validation#257blockiosaurus wants to merge 2 commits into
Conversation
…validation Instead of passing 6-8 function pointers (check_fp, validate_fp, etc.) to validate_asset_permissions and validate_collection_permissions, callers now specify the lifecycle event as a type parameter using a LifecycleEvent trait. This eliminates the function pointer plumbing at every call site, replacing e.g. `validate_asset_permissions(..., AssetV1::check_transfer, CollectionV1::check_transfer, PluginType::check_transfer, AssetV1::validate_transfer, CollectionV1::validate_transfer, Plugin::validate_transfer, Some(ExternalPluginAdapter::validate_transfer), Some(HookableLifecycleEvent::Transfer))` with just `validate_asset_permissions::<TransferLifecycle>(...)`. A define_lifecycle! macro generates zero-sized types implementing the trait for all 14 lifecycle events, keeping the boilerplate minimal. The generics are monomorphized at compile time so there is zero runtime cost. https://claude.ai/code/session_01PvtubR12q5JC5MjWHHZdx9
…efault Replaces 6-8 positional None parameters at each call site with a LifecycleContext struct that defaults all fields to None. Callers now only specify the fields relevant to their lifecycle event using struct initialization with ..Default::default(). This reduces validate_asset_permissions from 10 params to 5 and validate_collection_permissions from 8 params to 4, while also simplifying the internal validate_plugin_checks (14 -> 8 params) and validate_external_plugin_adapter_checks (14 -> 8 params). https://claude.ai/code/session_01PvtubR12q5JC5MjWHHZdx9
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbitRelease Notes
WalkthroughThe PR refactors lifecycle validation across the mpl-core plugin system by introducing a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The refactoring spans 16 files with new trait-based abstractions requiring careful verification of dispatch logic consistency. While processor changes follow a repetitive pattern, the core lifecycle trait implementation and utility function generics demand attention to ensure validation semantics are preserved across all event types. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/mpl-core/src/processor/add_external_plugin_adapter.rs`:
- Line 196: Remove the duplicate assignment to the variable
external_plugin_adapter (the second call to
ExternalPluginAdapter::from(&args.init_info)); keep the original variable
created earlier and ensure all subsequent uses reference that existing
external_plugin_adapter rather than reassigning or shadowing it.
| @@ -206,21 +196,15 @@ pub(crate) fn add_collection_external_plugin_adapter<'a>( | |||
| let external_plugin_adapter = ExternalPluginAdapter::from(&args.init_info); | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant variable assignment.
external_plugin_adapter is already computed at line 158. This duplicate assignment at line 196 is unnecessary and can be removed.
♻️ Remove redundant assignment
- let external_plugin_adapter = ExternalPluginAdapter::from(&args.init_info);
-
// Validate collection permissions.
let _ = validate_collection_permissions::<AddExternalPluginAdapterLifecycle>(📝 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.
| let external_plugin_adapter = ExternalPluginAdapter::from(&args.init_info); | |
| // Validate collection permissions. | |
| let _ = validate_collection_permissions::<AddExternalPluginAdapterLifecycle>( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/mpl-core/src/processor/add_external_plugin_adapter.rs` at line 196,
Remove the duplicate assignment to the variable external_plugin_adapter (the
second call to ExternalPluginAdapter::from(&args.init_info)); keep the original
variable created earlier and ensure all subsequent uses reference that existing
external_plugin_adapter rather than reassigning or shadowing it.
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 55c9fed | Previous: e021ca4 | Ratio |
|---|---|---|---|
CU: create a new, empty asset |
7499 Compute Units |
7628 Compute Units |
0.98 |
Space: create a new, empty asset |
91 Bytes |
91 Bytes |
1 |
CU: create a new, empty asset with empty collection |
15569 Compute Units |
15706 Compute Units |
0.99 |
Space: create a new, empty asset with empty collection |
91 Bytes |
91 Bytes |
1 |
CU: create a new asset with plugins |
25793 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 |
30701 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 |
18815 Compute Units |
19019 Compute Units |
0.99 |
CU: sell an asset |
23913 Compute Units |
24206 Compute Units |
0.99 |
CU: list an asset with empty collection |
23294 Compute Units |
23516 Compute Units |
0.99 |
CU: sell an asset with empty collection |
31292 Compute Units |
31593 Compute Units |
0.99 |
CU: list an asset with collection royalties |
22680 Compute Units |
22906 Compute Units |
0.99 |
CU: sell an asset with collection royalties |
34299 Compute Units |
34644 Compute Units |
0.99 |
CU: transfer an empty asset |
3480 Compute Units |
3611 Compute Units |
0.96 |
CU: transfer an empty asset with empty collection |
5031 Compute Units |
5171 Compute Units |
0.97 |
CU: transfer an asset with plugins |
6889 Compute Units |
7048 Compute Units |
0.98 |
CU: transfer an asset with plugins and empty collection |
8440 Compute Units |
8608 Compute Units |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
Instead of passing 6-8 function pointers (check_fp, validate_fp, etc.) to
validate_asset_permissions and validate_collection_permissions, callers now
specify the lifecycle event as a type parameter using a LifecycleEvent trait.
This eliminates the function pointer plumbing at every call site, replacing
e.g.
validate_asset_permissions(..., AssetV1::check_transfer, CollectionV1::check_transfer, PluginType::check_transfer, AssetV1::validate_transfer, CollectionV1::validate_transfer, Plugin::validate_transfer, Some(ExternalPluginAdapter::validate_transfer), Some(HookableLifecycleEvent::Transfer))with justvalidate_asset_permissions::<TransferLifecycle>(...).A define_lifecycle! macro generates zero-sized types implementing the trait
for all 14 lifecycle events, keeping the boilerplate minimal. The generics
are monomorphized at compile time so there is zero runtime cost.
https://claude.ai/code/session_01PvtubR12q5JC5MjWHHZdx9