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

fix: improve comments in IPluginExecutor interface #37

Merged

Conversation

jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Jan 25, 2024

#36 erroneously added strict restrictions on what a plugin can call.

Currently in v0.6.0:

interface IPluginExecutor {
    /// @notice Method from calls made from plugins.
    /// @param data The call data for the call.
    /// @return The return data from the call.
    function executeFromPlugin(bytes calldata data) external payable returns (bytes memory);

    /// @notice Method from cals made from plugins.
    /// @dev If the target is a plugin, the call SHOULD revert.
    /// @param target The target of the external contract to be called.
    /// @param value The value to pass.
    /// @param data The data to pass.
    function executeFromPluginExternal(address target, uint256 value, bytes calldata data) external payable;
}

@jaypaik jaypaik requested a review from adam-alchemy January 25, 2024 00:35
@jaypaik jaypaik merged commit 8b9ba71 into main Jan 25, 2024
3 checks passed
@jaypaik jaypaik deleted the 01-24-fix_improve_comments_in_IPluginExecutor_interface branch January 25, 2024 00:39
/// @dev Plugins are not allowed to call native functions on the account. Permissions must be granted to the
/// calling plugin for the call to go through.
/// @param data The calldata to send to the plugin.
/// @notice Execute a call from a plugin through the account.
Copy link
Contributor

Choose a reason for hiding this comment

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

If executeFromPluginExternal is responsible for non-plugin address, can this method call into non-plugin address (e.g. wallet native functions)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we don't explicitly prohibit this in the standard (hence the correction made here from the erroneous change from prior PRs: #32, #36). The reference implementation has chosen to disallow calls to native functions, but it can be done. Perhaps worthwhile to define in the standard in a future update though, since it impacts how plugins are written.

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.

3 participants