Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin side effects with example. #221

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

blockiosaurus
Copy link
Contributor

@blockiosaurus blockiosaurus commented Mar 3, 2025

This is a WIP PR that adds a side effect processor function after validation is complete. The plugins that indicate they have side effects process the context and return their updated Self value. This is then written to the account struct.

An example TransferCount plugin has been added to show how the concept should work.

Summary by CodeRabbit

  • New Features

    • Introduced an asset transfer counter that tracks the number of times an asset is transferred, enhancing auditability.
    • Improved asset lifecycle operations with new validation checks for plugin side effects during transfers, updates, and related actions.
  • Tests

    • Added comprehensive tests to verify the transfer counter functionality and overall asset operations.

Copy link

vercel bot commented Mar 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mpl-core-js-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2025 8:30pm

Copy link

coderabbitai bot commented Mar 3, 2025

Walkthrough

This pull request introduces a new "TransferCount" functionality across multiple components. TypeScript types and tests have been updated for tracking transfer counts. The JSON schema is expanded with a new TransferCount structure, and Rust modules have been enhanced to include a new TransferCount plugin. Several processor functions throughout the Rust code now integrate additional validation methods for plugin side effects, and static permission checks are refactored into dynamic instances via a new CheckResult struct.

Changes

Files Changes Summary
clients/js/src/plugins/types.ts
clients/js/test/plugins/asset/transferCount.test.ts
Adds new TransferCount TypeScript types, updates plugin creation types, and introduces tests that verify asset transfer count functionality.
idls/mpl_core.json Introduces a new TransferCount structure with a count: u64 field and updates related enums to reference this type.
programs/mpl-core/src/plugins/internal/authority_managed/mod.rs
…/transfer_count.rs
Adds a new transfer_count module defining the TransferCount plugin, including its struct, a new() method, implementations for DataBlob and PluginValidation, and accompanying tests.
programs/mpl-core/src/plugins/lifecycle.rs Converts static CheckResult values to dynamic instances via CheckResult::new() and adds multiple side effects methods for plugin lifecycle events.
programs/mpl-core/src/plugins/mod.rs Introduces a new TransferCount variant in both the Plugin and PluginType enums and updates associated methods accordingly.
Multiple processor files in programs/mpl-core/src/processor/ Integrates new validation functions (e.g., Plugin::side_effects_add_plugin, Plugin::side_effects_transfer, etc.) into existing permission checks for various operations like add, approve, burn, compress, create, decompress, execute, remove, revoke, transfer, update, and update_plugin.
programs/mpl-core/src/state/asset.rs
programs/mpl-core/src/state/collection.rs
Refactors permission-checking methods by replacing static CheckResult returns with dynamic instantiations using CheckResult::new().with_<flag>(true).
programs/mpl-core/src/utils/mod.rs Updates validation functions by adding a new plugin side effect function pointer parameter and refines conditional checks for asset and collection validations.
.coderabbit.yaml Introduces a new configuration file defining settings for a YAML language server, including options for reviews and path instructions.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant P as Processor
    participant V as Validator
    participant S as Plugin SideEffects
    U->>P: Initiate action (e.g., transfer, add)
    P->>V: Call validate_asset_permissions()
    V->>S: Run Plugin::side_effects_* check
    S-->>V: Return side effect result
    V-->>P: Provide validation result
    P-->>U: Complete action
Loading

Poem

I'm a bunny coding with a joyful hop,
Adding TransferCount, I never stop.
Plugins and tests now dance in line,
Validations hopping, oh so fine!
With twitching ears, I cheer our code divine! 🐰✨

Tip

⚡🧪 Multi-step agentic review comment chat (experimental)
  • We're introducing multi-step agentic chat in review comments. This experimental feature enhances review discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments.
    - To enable this feature, set early_access to true under in the settings.
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@blockiosaurus
Copy link
Contributor Author

@coderabbitai review

Copy link

coderabbitai bot commented Mar 3, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR adds a new TransferCount plugin to track asset transfers and integrates side effect processing into several asset lifecycle operations (create, burn, transfer, update, etc.). Additionally, TypeScript typings and tests for the new plugin are introduced, and plugin processing functions now invoke corresponding side effect callbacks.

Reviewed Changes

File Description
clients/js/test/plugins/asset/transferCount.test.ts Adds a test harness for the TransferCount plugin demonstrating its side effect behavior.
programs/mpl-core/src/plugins/internal/authority_managed/transfer_count.rs Implements the new TransferCount plugin in Rust.
programs/mpl-core/src/processor/(create burn
programs/mpl-core/src/plugins/mod.rs Adds the TransferCount variant to plugin enums and associated functions.
clients/js/src/plugins/types.ts Incorporates TypeScript types for the new TransferCount plugin.
programs/mpl-core/src/processor/create_collection.rs Minor refactor to use a more idiomatic check for plugin creation.
programs/mpl-core/src/plugins/internal/authority_managed/mod.rs Exposes the new TransferCount plugin module.

Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

clients/js/test/plugins/asset/transferCount.test.ts:32

  • [nitpick] Consider replacing the console.log with assertions that verify expected log output to improve test coverage and validation of side effects.
console.log((await umi.rpc.getTransaction(tx.signature))?.meta?.logs);
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark

Benchmark suite Current: c97a62c Previous: 661d5e0 Ratio
CU: create a new, empty asset 11237 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 22707 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 35204 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 41224 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 28194 Compute Units 28101 Compute Units 1.00
CU: sell an asset 44765 Compute Units 44473 Compute Units 1.01
CU: list an asset with empty collection 35744 Compute Units 35663 Compute Units 1.00
CU: sell an asset with empty collection 57108 Compute Units 56825 Compute Units 1.00
CU: list an asset with collection royalties 36580 Compute Units 36434 Compute Units 1.00
CU: sell an asset with collection royalties 62250 Compute Units 61909 Compute Units 1.01
CU: transfer an empty asset 6475 Compute Units 6444 Compute Units 1.00
CU: transfer an empty asset with empty collection 9071 Compute Units 9043 Compute Units 1.00
CU: transfer an asset with plugins 14896 Compute Units 14769 Compute Units 1.01
CU: transfer an asset with plugins and empty collection 17492 Compute Units 17368 Compute Units 1.01

This comment was automatically generated by workflow using github-action-benchmark.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (2)
programs/mpl-core/src/processor/add_plugin.rs (2)

58-60: 💡 Verification agent

🧩 Analysis chain

Consider addressing TODOs in validation context

There are TODO comments about seeding with Rejected and refactoring to allow add_plugin to approve additions. These comments may be relevant to the side effects implementation. Consider addressing them or clarifying their relationship to this PR.


🏁 Script executed:

#!/bin/bash
# Check the implementation of PluginValidationContext to understand what "Seed with Rejected" means
rg "PluginValidationContext" --type rust -A 10 -B 2

# Look for other occurrences of similar TODOs
rg "TODO: Seed with Rejected" --type rust
rg "TODO: refactor to allow" --type rust

Length of output: 71888


Action Required: Clarify and Address the TODOs in the Validation Context

The file still contains the two TODO comments at lines 58–60 regarding:

  • Seeding the validation context with a “Rejected” result, and
  • Refactoring to allow the add_plugin call to approve additions.

Based on the search results, these comments remain unaddressed. Please either implement the intended behavior (if critical for the side effects and validation flow) or update the comments with additional context explaining why they’re deferred or how they integrate with the overall design. This will help ensure clarity and maintain consistency with the similar implementations found in other parts of the codebase.


167-182: 💡 Verification agent

🧩 Analysis chain

Consider adding side effects for collection plugins

While side effects are added for asset plugins in the add_plugin function, they aren't added to the validate_collection_permissions call in the add_collection_plugin function. For consistency, consider adding the appropriate side effects function here as well.


🏁 Script executed:

#!/bin/bash
# Check if there's a side_effects function for collection plugins
rg "side_effects_add_collection_plugin|side_effects_add_plugin" --type rust

# Look at the validate_collection_permissions function to see if it supports side effects
rg "validate_collection_permissions" --type rust -A 10 -B 2

Length of output: 18931


Action Required: Add Missing Side Effects Function for Collection Plugins

The current implementation in programs/mpl-core/src/processor/add_plugin.rs (lines 167–182) calls validate_collection_permissions without providing functions for handling side effects (the last two parameters are both None). In contrast, asset plugins use a side effects function (e.g., via Plugin::side_effects_add_plugin as defined in programs/mpl-core/src/plugins/lifecycle.rs). For consistency and to ensure that any necessary state updates or events are triggered when adding collection plugins, please verify if a corresponding side effects function exists (perhaps within a collection-specific implementation like CollectionV1) and update the call to include it.

  • Location to review:

    • programs/mpl-core/src/processor/add_plugin.rs (lines 167–182)
  • Recommended change:

    • Replace the trailing None, None arguments in the validate_collection_permissions call with the appropriate side effects function reference, similar to the asset plugins’ implementation.
🧹 Nitpick comments (15)
programs/mpl-core/src/processor/update_plugin.rs (1)

151-225: Consider validating for plugin data size constraints.

The process_update_plugin function correctly handles account resizing when the plugin data size changes, but it might be beneficial to add a validation check that ensures the new plugin data doesn't exceed a maximum size limit to prevent excessive consumption of account space.

fn process_update_plugin<'a, T: DataBlob + SolanaAccount>(
    core: T,
    new_plugin: Plugin,
    plugin_header: Option<PluginHeaderV1>,
    plugin_registry: Option<PluginRegistryV1>,
    account: &AccountInfo<'a>,
    payer: &AccountInfo<'a>,
    system_program: &AccountInfo<'a>,
) -> ProgramResult {
    let mut plugin_registry = plugin_registry.ok_or(MplCoreError::PluginsNotInitialized)?;
    let mut plugin_header = plugin_header.ok_or(MplCoreError::PluginsNotInitialized)?;

    let plugin_registry_clone = plugin_registry.clone();
    let plugin_type: PluginType = (&new_plugin).into();
    let registry_record = plugin_registry_clone
        .registry
        .iter()
        .find(|record| record.plugin_type == plugin_type)
        .ok_or(MplCoreError::PluginNotFound)?;

    let plugin = Plugin::load(account, registry_record.offset)?;
    let plugin_data = plugin.try_to_vec()?;
    let new_plugin_data = new_plugin.try_to_vec()?;

+   // Ensure new plugin data doesn't exceed reasonable size constraints
+   const MAX_PLUGIN_SIZE: usize = 1024; // Example limit, adjust as needed
+   if new_plugin_data.len() > MAX_PLUGIN_SIZE {
+       return Err(MplCoreError::InvalidPluginData.into());
+   }

    // The difference in size between the new and old account which is used to calculate the new size of the account.
    let plugin_size = plugin_data.len() as isize;
    let size_diff = (new_plugin_data.len() as isize)
        .checked_sub(plugin_size)
        .ok_or(MplCoreError::NumericalOverflow)?;
programs/mpl-core/src/processor/remove_plugin.rs (1)

160-160: Consider addressing the TODO comment

There's a TODO comment on line 160 without additional context. It would be helpful to either implement the suggested improvement or add more details about what needs to be addressed in this function.

programs/mpl-core/src/processor/add_plugin.rs (1)

50-54: Consider moving validation into plugin validation

There's a TODO comment indicating that some validation logic should be moved into the plugin validation when asset/collection is part of the validation context. Since this PR adds side effects processing, it might be a good opportunity to address this TODO and ensure proper validation.

clients/js/test/plugins/asset/transferCount.test.ts (2)

32-32: Remove or replace console.log statement

This console.log statement appears to be debugging code. Consider removing it or replacing it with specific test assertions if you need to verify something in the transaction logs.

- console.log((await umi.rpc.getTransaction(tx.signature))?.meta?.logs);

1-40: Consider adding more comprehensive test cases

While this test effectively verifies the basic functionality, consider adding additional test cases to ensure robust behavior:

  1. Multiple transfers to verify consecutive increments
  2. Attempted transfers by non-owners (should fail)
  3. Verification that the transfer count persists across other operations

These additional tests would help ensure the TransferCount plugin behaves correctly in all scenarios.

programs/mpl-core/src/processor/add_external_plugin_adapter.rs (1)

104-105: Remove duplicate comment line

There's a duplicate line "// Validate asset permissions." that should be removed.

- // Validate asset permissions.
  // Validate asset permissions.
+ // Validate asset permissions.
programs/mpl-core/src/utils/mod.rs (2)

215-215: Clarify none() usage
if !asset_check.none() might read less clearly than checking explicit fields like can_approve or can_reject. A more explicit condition could improve readability.


430-434: Refine conditional logic
Evaluating multiple conditions within a single if can be overwhelming. Consider extracting or simplifying these checks for better readability.

programs/mpl-core/src/plugins/internal/authority_managed/transfer_count.rs (2)

15-22: Document the default behavior
TransferCount::new() returns a default struct. Consider adding a brief doc comment explaining the initialization rationale (count = 0).


41-58: Unit tests
Tests for serialization length are good. Consider adding a test verifying that calling side_effects_transfer actually increments the count field.

programs/mpl-core/src/plugins/lifecycle.rs (3)

438-572: Added side_effects_ methods*
These new methods unify side-effect handling across multiple lifecycle events. Consider ensuring you have tests covering each scenario to confirm expected plugin modifications.


676-679: Empty PluginSideEffectContext
This context struct is currently empty. If side-effect logic expands, consider embedding relevant metadata or references.


811-928: Unreachable default side-effect methods
Each method calls unreachable!(), which may cause unexpected panics if a plugin forgets to override them. Consider returning harmless no-ops instead.

programs/mpl-core/src/state/collection.rs (1)

58-58: Refactoring from enum to builder pattern enhances extensibility.

The changes replacing static enum returns (CheckResult::CanApprove or CheckResult::None) with the builder pattern (CheckResult::new().with_can_approve(true) or CheckResult::new().with_none(true)) provide better extensibility. This modification facilitates the plugin side effects feature mentioned in the PR description by allowing a more flexible CheckResult structure.

This refactoring to a builder pattern will make it easier to extend the permission checking system in the future, especially for implementing side effects as described in the PR objectives. It transforms what was likely an enum into a more flexible struct with boolean flags, which can be more easily expanded without breaking existing code.

Also applies to: 63-63, 68-68, 73-73, 78-78, 83-83, 88-88, 93-93, 98-98, 103-103, 108-108, 113-113, 118-118, 123-123, 128-128

programs/mpl-core/src/state/asset.rs (1)

72-72: Consistent refactoring to builder pattern aligns with collection implementation.

These modifications follow the same pattern as in the collection implementation, replacing static enum returns with a builder pattern for CheckResult. This maintains consistency across the codebase and enables the plugin side effects feature mentioned in the PR objectives.

The consistent application of this pattern across different asset types ensures that the permission system behaves uniformly. This will make it easier to implement the side effect processor function that executes after validation is completed, as mentioned in the PR description.

Also applies to: 77-77, 82-82, 87-87, 92-92, 97-97, 102-102, 107-107, 112-112, 117-117, 122-122, 127-127, 132-132, 137-137, 142-142

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 661d5e0 and 636566d.

⛔ Files ignored due to path filters (8)
  • clients/js/src/generated/types/index.ts is excluded by !**/generated/**
  • clients/js/src/generated/types/plugin.ts is excluded by !**/generated/**
  • clients/js/src/generated/types/pluginType.ts is excluded by !**/generated/**
  • clients/js/src/generated/types/transferCount.ts is excluded by !**/generated/**
  • clients/rust/src/generated/types/mod.rs is excluded by !**/generated/**
  • clients/rust/src/generated/types/plugin.rs is excluded by !**/generated/**
  • clients/rust/src/generated/types/plugin_type.rs is excluded by !**/generated/**
  • clients/rust/src/generated/types/transfer_count.rs is excluded by !**/generated/**
📒 Files selected for processing (25)
  • clients/js/src/plugins/types.ts (5 hunks)
  • clients/js/test/plugins/asset/transferCount.test.ts (1 hunks)
  • idls/mpl_core.json (3 hunks)
  • programs/mpl-core/src/plugins/internal/authority_managed/mod.rs (2 hunks)
  • programs/mpl-core/src/plugins/internal/authority_managed/transfer_count.rs (1 hunks)
  • programs/mpl-core/src/plugins/lifecycle.rs (9 hunks)
  • programs/mpl-core/src/plugins/mod.rs (8 hunks)
  • programs/mpl-core/src/processor/add_external_plugin_adapter.rs (1 hunks)
  • programs/mpl-core/src/processor/add_plugin.rs (1 hunks)
  • programs/mpl-core/src/processor/approve_plugin_authority.rs (1 hunks)
  • programs/mpl-core/src/processor/burn.rs (1 hunks)
  • programs/mpl-core/src/processor/compress.rs (1 hunks)
  • programs/mpl-core/src/processor/create.rs (3 hunks)
  • programs/mpl-core/src/processor/create_collection.rs (2 hunks)
  • programs/mpl-core/src/processor/decompress.rs (1 hunks)
  • programs/mpl-core/src/processor/execute.rs (1 hunks)
  • programs/mpl-core/src/processor/remove_external_plugin_adapter.rs (1 hunks)
  • programs/mpl-core/src/processor/remove_plugin.rs (1 hunks)
  • programs/mpl-core/src/processor/revoke_plugin_authority.rs (1 hunks)
  • programs/mpl-core/src/processor/transfer.rs (1 hunks)
  • programs/mpl-core/src/processor/update.rs (1 hunks)
  • programs/mpl-core/src/processor/update_plugin.rs (1 hunks)
  • programs/mpl-core/src/state/asset.rs (1 hunks)
  • programs/mpl-core/src/state/collection.rs (1 hunks)
  • programs/mpl-core/src/utils/mod.rs (7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Programs / Test (mpl-core)
  • GitHub Check: Programs / Build
🔇 Additional comments (70)
programs/mpl-core/src/processor/revoke_plugin_authority.rs (1)

77-77: Addition of side effects handler is correctly implemented.

The new side effects handler for revoking plugin authority is seamlessly integrated into the existing validation chain. This is part of a broader enhancement allowing plugins to register and execute side effects during operations.

programs/mpl-core/src/processor/remove_external_plugin_adapter.rs (1)

84-84: Side effects implementation for plugin adapter removal looks good.

This addition follows the same pattern as other operations, adding side effects handling to the validation pipeline for removing external plugin adapters.

programs/mpl-core/src/processor/compress.rs (1)

63-63:

✅ Verification successful

The side effects handler is properly integrated into compress operation.

The addition is correctly implemented, but note that this is currently within code that returns MplCoreError::NotAvailable (see line 82), meaning these side effects won't be executed until compression is fully enabled.


🏁 Script executed:

#!/bin/bash
# Check if compression functionality is enabled elsewhere in the codebase
# or if there are plans to enable it soon

# Look for TODOs related to compression
rg -A 3 -B 3 "TODO.*compression" --glob "*.rs"

# Check if there are any open issues or PRs related to compression
echo "Note: You may want to search GitHub issues for 'compression' to see if there are plans to enable this functionality soon"

Length of output: 1423


Side Effects Integration Verified

The side effects handler is correctly integrated into the compression operation. The additional shell script output confirms that the compression functionality remains gated by the MplCoreError::NotAvailable return (with the corresponding // TODO Enable compression comments in both compress.rs and decompress.rs). This means the side effects will not execute until compression is fully enabled. No changes are needed at this time; however, please revisit this area once compression is implemented.

programs/mpl-core/src/processor/update_plugin.rs (1)

70-70: Side effects handler correctly integrated for plugin updates.

The addition of Plugin::side_effects_update_plugin follows the consistent pattern used throughout the codebase for enabling plugin side effects during validation, and is correctly positioned in the validation function call.

programs/mpl-core/src/processor/approve_plugin_authority.rs (1)

71-71: Feature addition for plugin side effects processing

This line adds the Plugin::side_effects_approve_plugin_authority function to the validation pipeline, enabling plugins to execute side effects during the approve plugin authority operation. This is part of the new side effects processor functionality being implemented across the codebase.

programs/mpl-core/src/processor/remove_plugin.rs (1)

76-76: Feature addition for plugin side effects processing

This line adds support for plugin side effects during the removal process, matching the pattern implemented across other processor functions. The side effects function is properly positioned after the validation functions in the argument list.

programs/mpl-core/src/processor/burn.rs (2)

103-103: Feature addition for plugin side effects processing

This line adds the Plugin::side_effects_burn function, enabling plugins to execute side effects during the burn operation. This is particularly important for plugins that need to perform cleanup or record state changes before an asset is destroyed.


77-79:

✅ Verification successful

Verify the TODO comment status

There's a TODO comment about enabling compressed burn functionality, with a corresponding error message indicating this feature is not available. Consider whether this TODO should be addressed in this PR or if it's intentionally left for future work.


🏁 Script executed:

#!/bin/bash
# Check if there are any open issues or PRs related to compressed burn functionality
gh issue list --repo metaplex-foundation/mpl-core --search "compressed burn"
gh pr list --repo metaplex-foundation/mpl-core --search "compressed burn"

# Also check for relevant comments in code
rg "compressed burn" --type rust -A 3 -B 3

Length of output: 780


Compressed Burn Functionality TODO Clarification

After verifying the repository, it's clear that the TODO regarding enabling compressed burn functionality (in programs/mpl-core/src/processor/burn.rs at lines 77–79) remains intentional. The error message explicitly indicates that this feature is not available, and there are no active issues or PRs suggesting that compressed burn is being worked on as part of this PR.

  • File: programs/mpl-core/src/processor/burn.rs (lines 77–79)
  • Observation: The TODO and error message consistently signal that compressed burn is deferred to future work.

Please confirm if it’s acceptable to leave this functionality unimplemented in the current PR or if additional work is expected to address it.

programs/mpl-core/src/processor/add_plugin.rs (1)

97-97: Feature addition for plugin side effects processing

This line adds the Plugin::side_effects_add_plugin function to the validation pipeline, enabling plugins to execute side effects during the add plugin operation. This aligns with the side effects implementation across other processor functions.

programs/mpl-core/src/processor/execute.rs (1)

67-67:

✅ Verification successful

Side effects processing added to asset validation pipeline

The addition of Plugin::side_effects_execute into the validation chain enriches the plugin ecosystem by allowing plugins to process side effects after validation completes.


🏁 Script executed:

#!/bin/bash
# Check for other side_effects implementations to understand the pattern
rg -A 1 -B 1 "side_effects_" --type rust

Length of output: 15857


Side Effects Processing in Validation Pipeline Verified

The addition of Plugin::side_effects_execute in the validation chain of programs/mpl-core/src/processor/execute.rs is consistent with our established side effects pattern. As seen in other lifecycle actions (e.g., side effects for create, update, transfer, etc. in plugins/lifecycle.rs), this change properly integrates side effects processing into the asset validation pipeline.

programs/mpl-core/src/processor/create_collection.rs (2)

12-15: API refactoring - CheckResult import removed

The imports have been refactored to remove the CheckResult enum dependency, which aligns with the logic change on line 135.


135-135:

❓ Verification inconclusive

Logic updated from equality check to method call

The condition has been updated from a direct comparison with CheckResult::None to using the .none() method, which improves code readability and follows a more fluent API design pattern.


🏁 Script executed:

#!/bin/bash
# Check other places that may still be using the old CheckResult comparison pattern
rg "CheckResult::None" --type rust

Length of output: 34


Update Confirmation – Fluent API Usage Verified
The condition in programs/mpl-core/src/processor/create_collection.rs at line 135 now employs the .none() method instead of directly comparing against CheckResult::None. A repository-wide search for "CheckResult::None" returned no results, indicating that legacy comparisons have been removed. However, please manually verify that the search output aligns with the expected behavior in your environment.

programs/mpl-core/src/plugins/internal/authority_managed/mod.rs (2)

6-6: New TransferCount plugin module added

This new module implements the TransferCount functionality mentioned in the PR, which serves as an example for the new side effects processing feature.


15-15:

✅ Verification successful

Exposing TransferCount plugin API

All public items from the TransferCount module are now accessible through the authority_managed module, making the new plugin available for use throughout the codebase.


🏁 Script executed:

#!/bin/bash
# Check how the TransferCount plugin is defined and used
rg -A 10 "TransferCount" --type rust | grep -v "test"

Length of output: 12852


Exposing TransferCount Plugin API Verified

The public re-export (pub use transfer_count::*;) in programs/mpl-core/src/plugins/internal/authority_managed/mod.rs correctly exposes all public items from the transfer_count module. The grep output confirms that the TransferCount plugin is defined in transfer_count.rs and is referenced appropriately across the codebase (e.g., in plugins/mod.rs and generated types). No further changes are needed.

programs/mpl-core/src/processor/update.rs (1)

127-127:

❓ Verification inconclusive

Side effects processing for update operation

The addition of Plugin::side_effects_update enables plugins to process side effects during asset updates, consistent with the pattern applied to other operations like execute.


🏁 Script executed:

#!/bin/bash
# Check for side_effects implementation in the Plugin struct
ast-grep --pattern $'impl Plugin {
  $$$
  pub fn side_effects_update($$$) $$$
  $$$
}'

Length of output: 88


Attention: Verify the existence of the Plugin::side_effects_update implementation

The update operation now includes a call to Plugin::side_effects_update to handle side effects during asset updates—mirroring the pattern used in operations such as execute. However, our automated search (using AST-based grep) did not return any evidence of an explicit implementation of side_effects_update in the Plugin struct.

  • Please manually verify that side_effects_update is implemented with the correct signature and intended functionality.
  • If the method is generated or conditionally compiled, confirm that it is appropriately defined in those contexts.
idls/mpl_core.json (3)

2945-2956: New TransferCount plugin structure looks good

This correctly defines the structure for the TransferCount plugin with a single counter field, adhering to the project's type definition patterns.


4000-4007: Properly integrated TransferCount into Plugin enum

The TransferCount variant is appropriately added to the Plugin enum, allowing it to be used throughout the system.


4061-4063: TransferCount correctly added to PluginType enum

The addition of TransferCount to the PluginType enum ensures it can be properly referenced in the plugin type system.

programs/mpl-core/src/processor/transfer.rs (1)

96-96: Side effects processor integration looks good

The addition of Plugin::side_effects_transfer to the validation parameters correctly enables plugins to implement side effects during transfer operations. This supports the new TransferCount plugin functionality.

clients/js/test/plugins/asset/transferCount.test.ts (3)

7-26: Test setup and initial assertions are well structured

The test correctly sets up an asset with the TransferCount plugin initialized to 0 and verifies the initial state.


27-31: Asset transfer operation is implemented correctly

The code properly uses the transferV1 function to transfer the asset to a new owner.


34-39: Final assertions correctly verify transfer count increment

The test properly verifies that the asset was transferred to the new owner and that the transfer count was incremented from 0 to 1.

programs/mpl-core/src/processor/add_external_plugin_adapter.rs (2)

122-122: Side effects processor correctly integrated

The addition of Plugin::side_effects_add_external_plugin_adapter to the validation parameters properly extends the side effects functionality to external plugin adapter operations.


210-225:

❓ Verification inconclusive

Consider adding side effects processing to collection adapters

The side_effects function has been added to the asset-level external plugin adapter function but not to the collection-level function. If side effects are also relevant for collection-level plugin adapters, consider adding the appropriate side effects processor here as well.


🏁 Script executed:

#!/bin/bash
# Check if collection plugins also implement side effects functionality

# Search for any side effects implementations for collections
echo "Searching for collection side effects implementations..."
rg -A 5 "side_effects.*collection" --type rust

Length of output: 670


Action Required: Confirm/Integrate Side Effects Processing for Collection Adapters

The current implementation in programs/mpl-core/src/processor/add_external_plugin_adapter.rs (lines 210–225) calls the collection permissions validator without any side effects processing. However, our search in programs/mpl-core/src/utils/mod.rs reveals that collection side effects are handled via a call like:

  • run_plugin_side_effects(&checks, Some(asset), collection, plugin_side_effect_fp)?;

Please review whether collection-level plugin adapters should incorporate side effects processing similar to asset-level adapters. If so, update the collection adapter (or the surrounding logic) to integrate the appropriate side effects processor. Otherwise, document clearly why side effects are not applicable in this case.

programs/mpl-core/src/processor/decompress.rs (1)

79-79: Side effect handler integration looks good.

The addition of Plugin::side_effects_decompress to the validation chain follows the established pattern used for other plugin validations. This aligns with the PR's objective of adding side effect processing capabilities.

programs/mpl-core/src/processor/create.rs (3)

13-13: Import update correctly reflects implementation change.

The import now includes ExternalCheckResultBits instead of CheckResult, which aligns with the pattern of refactoring static permission checks into dynamic instances through the PR.


168-168: Side effect handler integration looks good.

The addition of Plugin::side_effects_create to the validation chain follows the established pattern used for other plugin validations. This aligns with the PR's objective of adding side effect processing capabilities.


192-192: Improved condition check syntax.

Changed from comparison with CheckResult::None to using the .none() method, which improves readability and follows a more idiomatic approach.

clients/js/src/plugins/types.ts (5)

28-29: New TransferCount types imported correctly.

The imports for TransferCount and TransferCountArgs are correctly added to support the new plugin functionality.


93-96: TransferCount plugin type added to CreatePluginArgs.

The new plugin type is correctly integrated into the type definition structure following the established pattern of other plugins.


152-154: TransferCount added to AuthorityManagedPluginArgsV2.

The TransferCount plugin is correctly classified as an authority managed plugin, which matches its definition in the Rust code where it uses Authority::UpdateAuthority.


192-192: TransferCountPlugin type definition looks good.

The new exported type correctly combines the base plugin interface with the TransferCount specific functionality.


204-204: TransferCount added to CommonPluginsList.

The TransferCount plugin is correctly added to the CommonPluginsList, making it available for both assets and collections.

programs/mpl-core/src/plugins/mod.rs (9)

63-64: New TransferCount plugin variant added with clear documentation.

The TransferCount variant is added to the Plugin enum with appropriate documentation explaining its purpose to track the number of times an asset has been transferred.


108-108: TransferCount variant correctly added to inner() method.

The inner() method implementation is properly updated to handle the new TransferCount variant.


140-140: DataBlob implementation updated for TransferCount.

The len() method in the DataBlob implementation for Plugin correctly includes the new TransferCount variant.


193-194: PluginType enum updated with TransferCount variant.

The TransferCount variant is properly added to the PluginType enum with consistent documentation.


233-233: From<&Plugin> implementation updated for TransferCount.

The conversion from Plugin to PluginType correctly handles the new TransferCount variant.


257-257: Manager method updated for TransferCount authority.

The manager() method correctly defines the TransferCount plugin as being managed by Authority::UpdateAuthority, which is consistent with its classification in the TypeScript types.


308-308: Test coverage added for TransferCount empty size case.

The test_plugin_empty_size test is updated to include a test case for the TransferCount plugin, ensuring comprehensive test coverage.


424-424: Test coverage added for TransferCount different size case.

The test_plugin_different_size test is also updated to include a test case for the TransferCount plugin with a non-default count value, ensuring the serialization and length calculation are correct for different states.


427-431:

✅ Verification successful

Verify test coverage completeness across all plugin types.

This assertion is important to ensure all plugin types are tested. With the addition of TransferCount, make sure that any new plugin types added in the future are also added to the test suite.

Run the following script to check if all plugin types are covered in tests:


🏁 Script executed:

#!/bin/bash
# Description: Verify that all plugin types are covered in the test suite

# Find all plugin type declarations in the codebase
echo "All plugin types defined in PluginType enum:"
rg -n "^\s+[A-Za-z]+," "programs/mpl-core/src/plugins/mod.rs" | grep -A30 "pub enum PluginType" | grep -v "pub enum PluginType"

echo -e "\nPlugin types tested in test_plugin_empty_size:"
rg -n "Plugin::[A-Za-z]+" "programs/mpl-core/src/plugins/mod.rs" | grep "test_plugin_empty_size" | sort

echo -e "\nPlugin types tested in test_plugin_different_size:"
rg -n "vec!\[Plugin::[A-Za-z]+" "programs/mpl-core/src/plugins/mod.rs" | grep "test_plugin_different_size" | sort

echo -e "\nChecking if all plugin types have corresponding implementations:"
rg -n "Plugin::[A-Za-z]+\(" "programs/mpl-core/src/plugins/internal" --glob "*.rs" | sort

Length of output: 2534


Ensure Complete Test Coverage for All Plugin Types

The assertion

assert_eq!(plugins.len(), PluginType::COUNT, "All plugins should be tested");

in programs/mpl-core/src/plugins/mod.rs verifies that every plugin type defined in the PluginType enum is being exercised in the test suite. The recent addition of the TransferCount plugin is reflected in its corresponding implementation (as seen in programs/mpl-core/src/plugins/internal/authority_managed/transfer_count.rs), which confirms that the test suite continues to cover all plugin types.

Please continue to ensure that when new plugin types are added in the future, they are also incorporated into the test suite. The provided shell script is a helpful utility to verify that:

  • All plugin types declared in the enum are accounted for.
  • Each plugin type has a corresponding implementation and is covered by tests.

This assertion serves as an important safeguard for preventing regressions in test coverage as plugins evolve.

programs/mpl-core/src/utils/mod.rs (5)

10-14: New imports for plugin side effects
The addition of run_plugin_side_effects, along with other plugin-related imports, cleanly integrates the side-effect processing logic.


136-136: Confirm function pointer usage
plugin_side_effect_fp extends the validate_asset_permissions logic to allow custom side-effect handling. Make sure all callers now supply the correct side-effect function.


177-177: Switch to struct-based approach
Using CheckResult::new().with_none(true) instead of an enum variant aligns with the new bitfield design for plugin checks.


231-231: Consistent naming
As with the asset check, if !collection_check.none() could be clearer if renamed to is_none() or replaced with explicit field checks.


297-298: Validate asset availability
Calling run_plugin_side_effects with Some(asset) assumes the asset is always present. Confirm that this won't lead to runtime errors in edge cases where the asset might be missing.

programs/mpl-core/src/plugins/internal/authority_managed/transfer_count.rs (4)

1-6: Imports and crate references
The usage of DataBlob, Plugin, and PluginValidation is cleanly organized.


7-13: Struct definition
Defining TransferCount with a clear doc comment and Borsh serialization provides good clarity for how this plugin will be persisted.


24-28: Implementing DataBlob
len() returning 8 (the size of u64) is correct and aligns with Borsh serialization.


30-39: Transfer side effect
side_effects_transfer increments the count safely. However, in extreme usage scenarios, consider whether u64 overflow should be handled.

programs/mpl-core/src/plugins/lifecycle.rs (19)

2-4: Utilize bitfield for checks
Importing modular_bitfield to define permission flags streamlines the CheckResult structure.


21-37: New CheckResult bitfield
Defining discrete boolean fields (e.g., can_approve, can_reject, side_effect) is a flexible approach for plugin lifecycle permissions.


38-41: Default state verification
impl Default for CheckResult calls Self::new(), leaving all bits false. Verify that this is indeed the intended default for plugin checks.


99-108: Permissions for add_plugin
Each plugin type has a distinct response here. Ensure these logic branches align with your intended plugin add behavior.


116-119: Permissions for remove_plugin
FreezeDelegate and PermanentFreezeDelegate can reject removal. Double-check that this doesn't block valid removals.

Also applies to: 121-121


129-129: Generic approval
Everything else defaults to can_approve(true). Verify that no plugin type should instead abstain or reject.


137-137: Double-check default
Setting _ => can_approve(true) for check_approve_plugin_authority might be too permissive.


147-147: Broad approval
_ => CheckResult::new().with_can_approve(true) in check_revoke_plugin_authority can allow unexpected revokes.


155-159: Create lifecycle checks
Royalties, Autograph, and VerifiedCreators can reject, UpdateDelegate can approve, and the rest do none. This appears consistent with typical usage.


167-169: Immutable metadata
Confirm that allowing ImmutableMetadata to reject updates fully matches your requirements.


176-180: Burn lifecycle
Check logic for FreezeDelegate or PermanentFreezeDelegate to reject while BurnDelegate can approve.


187-193: Check for side_effect
PluginType::TransferCount sets the side_effect bit. Good approach. Confirm that no other plugin also needs side effects for transfers.


201-201: Compress default
All plugin types default to no permission for compress.


209-209: Decompress default
Similarly, no plugin type may approve or reject the decompress lifecycle.


217-217: Execute default
Check that no plugin type should override the default none for execute.


225-225: Add external plugin adapter
All plugin types default to none, meaning no direct approval or rejection.


233-233: Remove external plugin adapter
Same pattern as add: none.


241-241: Update external plugin adapter
Again, defaulting to none.


1087-1123: run_plugin_side_effects
Applying side effects in a loop is sensible. Pay attention to error handling if one side effect fails mid-way, as partial updates may occur.

Copy link
Contributor

@nhanphan nhanphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts/questions:

  • there may need to be guard rails on what a plugin is allowed to mutate as there is no post side effect validation and it seems a plugin can change any plugin's data
  • is there a better name than "side effect"?
  • random idea: log plugin, write specific values (e.g. regex/token builder) to the tx log for custom indexers to pick up.

@blockiosaurus
Copy link
Contributor Author

Some thoughts/questions:

  • there may need to be guard rails on what a plugin is allowed to mutate as there is no post side effect validation and it seems a plugin can change any plugin's data
  • is there a better name than "side effect"?
  • random idea: log plugin, write specific values (e.g. regex/token builder) to the tx log for custom indexers to pick up.
  1. Yeah I'm a bit on the fence about this. Validation is pretty much our biggest performance cost so I'm worried about adding this (which is nonzero impact) plus a second round of validations. In the short term we can probably just be careful but I agree we'll need some rails at some point.
  2. I'm open to whatever. Maybe post-action? Result?
  3. Oh this would be nice. Not sure if there's a hit for people who don't include the log program by default but definitely an option.

@blockiosaurus blockiosaurus marked this pull request as ready for review March 14, 2025 20:30
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
programs/mpl-core/src/plugins/lifecycle.rs (4)

2-6: Consider removing unused imports.
It appears sol_memcpy might not be used in this file. If so, removing unused imports can help keep the code clean and reduce confusion.


38-41: Clarify default behavior.
By returning just CheckResult::new(), all flags default to false. If the expected “no permissions” default is truly equivalent to setting .with_none(true), consider explicitly calling .with_none(true) for extra clarity.


99-109: Refactor repetitive match arms.
Each “check” function enumerates plugin types and sets can_approve, can_reject, or none. This pattern repeats and may benefit from a single consolidated function or table-driven logic. It would simplify maintainability if future plugin types are added.

Also applies to: 116-121, 129-129, 137-137, 147-147, 155-159, 167-169, 176-180, 187-193, 201-201, 217-218, 225-226, 233-234


676-678: Enrich the side effect context.
PluginSideEffectContext is currently empty. If more advanced or defensive checks are needed (like restricting data mutations or logging suspicious actions), adding relevant fields—e.g., references to the plugin’s account data or the processed instruction—could enable more robust guard rails.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 636566d and c97a62c.

📒 Files selected for processing (2)
  • .coderabbit.yaml (1 hunks)
  • programs/mpl-core/src/plugins/lifecycle.rs (9 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .coderabbit.yaml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Make duplicated logic between the rust client and...

**/*.rs: Make duplicated logic between the rust client and program are consistent

  • programs/mpl-core/src/plugins/lifecycle.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Programs / Build
  • GitHub Check: Programs / Test (mpl-core)
🔇 Additional comments (2)
programs/mpl-core/src/plugins/lifecycle.rs (2)

21-36: Validate bitfield usage and naming.
The #[bitfield(bits = 8)] annotation for multiple flags and the naming of fields (none, can_approve, etc.) may cause confusion or off-by-one bit errors if not carefully maintained. Consider verifying the correctness of the bit ordering, ensuring that each boolean reflects the intended meaning.


1087-1117: Implement post-write validation or sanity checks.
run_plugin_side_effects loads the plugin, executes the side effect, then saves the updated plugin without further validation. This design allows each plugin to modify its data with no final check. Consider adding either guard checks or a final validate_self step after saving, especially if future plugin additions may interact in unsafe ways.

Comment on lines +439 to +475
/// Process side effects for the add plugin lifecycle action.
/// This gets called on all existing plugins when a new plugin is added.
pub(crate) fn side_effects_add_plugin(
plugin: &Plugin,
ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
plugin.inner().side_effects_add_plugin(ctx)
}

/// Process side effects for the remove plugin lifecycle action.
/// This gets called on all existing plugins when the target plugin is removed.
pub(crate) fn side_effects_remove_plugin(
plugin: &Plugin,
ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
plugin.inner().side_effects_remove_plugin(ctx)
}

/// Process side effects for the add plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is added.
pub(crate) fn side_effects_add_external_plugin_adapter(
plugin: &Plugin,
ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
plugin.inner().side_effects_add_external_plugin_adapter(ctx)
}

/// Process side effects for the remove plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is removed.
pub(crate) fn side_effects_remove_external_plugin_adapter(
plugin: &Plugin,
ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
plugin
.inner()
.side_effects_remove_external_plugin_adapter(ctx)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add guard rails or final validations after side effects.
These newly introduced side_effects_* methods allow each plugin to mutate data after validation. However, there's currently no post-side-effect validation step. Consider adding a guard or final pass to ensure that a plugin cannot tamper with unrelated data, especially where multiple plugins coexist.

Also applies to: 477-491, 493-527, 529-552, 554-561, 562-575

Comment on lines +811 to 928
/// Process side effects for the add plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is added.
fn side_effects_add_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the remove plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is removed.
fn side_effects_remove_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the approve plugin authority lifecycle action.
fn side_effects_approve_plugin_authority(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the revoke plugin authority lifecycle action.
fn side_effects_revoke_plugin_authority(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the create lifecycle action.
/// This ONLY gets called to side_effects the self plugin
fn side_effects_create(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the update lifecycle action.
/// This gets called on all existing plugins when an asset or collection is updated.
fn side_effects_update(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the update_plugin lifecycle action.
/// This gets called on all existing plugins when a plugin is updated.
fn side_effects_update_plugin(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the burn lifecycle action.
/// This gets called on all existing plugins when an asset is burned.
fn side_effects_burn(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the transfer lifecycle action.
/// This gets called on all existing plugins when an asset is transferred.
fn side_effects_transfer(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the compress lifecycle action.
fn side_effects_compress(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the decompress lifecycle action.
fn side_effects_decompress(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the execute lifecycle action.
fn side_effects_execute(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}

/// Process side effects for the update_plugin lifecycle action.
#[allow(unused)]
fn side_effects_update_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Rethink using unreachable!() for default side-effect methods.
If a plugin accidentally triggers these methods (e.g., a new plugin type incorrectly routes a call), the program will panic. Consider returning a safe default (e.g., a no-op plugin return) rather than panicking to reduce possible runtime failures.

Example fix:

-fn side_effects_add_plugin(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
-    unreachable!()
+fn side_effects_add_plugin(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
+    solana_program::msg!("Default no-op for side_effects_add_plugin");
+    // Indicate no transformations needed
+    Ok(Plugin::from(self))
}
📝 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.

Suggested change
/// Process side effects for the add plugin lifecycle action.
/// This gets called on all existing plugins when a new plugin is added.
fn side_effects_add_plugin(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the remove plugin lifecycle action.
/// This gets called on all existing plugins when the target plugin is removed.
fn side_effects_remove_plugin(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the add plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is added.
fn side_effects_add_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the remove plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is removed.
fn side_effects_remove_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the approve plugin authority lifecycle action.
fn side_effects_approve_plugin_authority(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the revoke plugin authority lifecycle action.
fn side_effects_revoke_plugin_authority(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the create lifecycle action.
/// This ONLY gets called to side_effects the self plugin
fn side_effects_create(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the update lifecycle action.
/// This gets called on all existing plugins when an asset or collection is updated.
fn side_effects_update(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the update_plugin lifecycle action.
/// This gets called on all existing plugins when a plugin is updated.
fn side_effects_update_plugin(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the burn lifecycle action.
/// This gets called on all existing plugins when an asset is burned.
fn side_effects_burn(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the transfer lifecycle action.
/// This gets called on all existing plugins when an asset is transferred.
fn side_effects_transfer(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the compress lifecycle action.
fn side_effects_compress(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the decompress lifecycle action.
fn side_effects_decompress(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the execute lifecycle action.
fn side_effects_execute(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the update_plugin lifecycle action.
#[allow(unused)]
fn side_effects_update_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
}
/// Process side effects for the add plugin lifecycle action.
/// This gets called on all existing plugins when a new plugin is added.
fn side_effects_add_plugin(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
solana_program::msg!("Default no-op for side_effects_add_plugin");
// Indicate no transformations needed
Ok(Plugin::from(self))
}
/// Process side effects for the remove plugin lifecycle action.
/// This gets called on all existing plugins when the target plugin is removed.
fn side_effects_remove_plugin(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the add plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is added.
fn side_effects_add_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the remove plugin lifecycle action.
/// This gets called on all existing plugins when a new external plugin is removed.
fn side_effects_remove_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the approve plugin authority lifecycle action.
fn side_effects_approve_plugin_authority(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the revoke plugin authority lifecycle action.
fn side_effects_revoke_plugin_authority(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the create lifecycle action.
/// This ONLY gets called to side_effects the self plugin
fn side_effects_create(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the update lifecycle action.
/// This gets called on all existing plugins when an asset or collection is updated.
fn side_effects_update(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the update_plugin lifecycle action.
/// This gets called on all existing plugins when a plugin is updated.
fn side_effects_update_plugin(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the burn lifecycle action.
/// This gets called on all existing plugins when an asset is burned.
fn side_effects_burn(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the transfer lifecycle action.
/// This gets called on all existing plugins when an asset is transferred.
fn side_effects_transfer(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the compress lifecycle action.
fn side_effects_compress(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the decompress lifecycle action.
fn side_effects_decompress(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the execute lifecycle action.
fn side_effects_execute(&self, _ctx: &PluginSideEffectContext) -> Result<Plugin, ProgramError> {
unreachable!()
}
/// Process side effects for the update_plugin lifecycle action.
#[allow(unused)]
fn side_effects_update_external_plugin_adapter(
&self,
_ctx: &PluginSideEffectContext,
) -> Result<Plugin, ProgramError> {
unreachable!()
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants