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

[draft] feat: [v0.8-develop] Only allow installing direct call validation via manifest 3/N Option 1 #103

Closed

Conversation

adamegyed
Copy link
Contributor

@adamegyed adamegyed commented Jul 18, 2024

Motivation

Since the introduction of installValidation as a fully user-controlled install function, there have been two different ways to install validation funcitons from plugins:

  • The caller can specify the validation function, flags, and any pre-validation / permission hooks via installValidation
  • The caller can install a plugin that declares a validation function in its manifest. In this setup, there is no opportunity to apply pre-validation or permission hooks.

In practice, most uses of validation functions in manifests were for allowing "direct call validation", aka recreating the old executeFromPlugin workflow.

Solution

This PR explores one possible solution to removing the redundancy, by making the only possible validation function to install via a manifest the direct call validation.

Future work

If we choose this path, we will need to add a way to specify pre-validation + permission hooks for the direct call validation functions installed via the manifest.

@adamegyed adamegyed changed the title [draft] feat: [v0.8-develop] Only allow installing direct call validation via manifest [draft] feat: [v0.8-develop] Only allow installing direct call validation via manifest 3/N Option 1 Jul 18, 2024
@adamegyed adamegyed force-pushed the adam/merge-plugin-manager branch from 220497a to 2a5de9d Compare July 19, 2024 18:38
revert PluginAlreadyInstalled(plugin);
}

// TODO: do we need this check? Or switch to a non-165 checking function?
// Check that the plugin supports the IPlugin interface.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i like the check as a safety mechanism, because installation of non plugin contracts can be dangerous (installing ERC20 tokens/ERC20.transfer.selector as an execution selector to transfer contract tokens out)

Are you thinking of moving it to the SDK to reduce pollution in the tx traces? That's something I'll support. I think we can say with high confidence that most accounts are used with their paired SDKs today, and will be the case in the future, so safety checks in the SDK are equally effective as on chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of adding a view function to IModule that would do something similar to the ERC-165 check, basically just validating that the provided address does report itself to be a module of the appropriate type. But for now, I think it's ok to leave it as the 165 check.

@adamegyed
Copy link
Contributor Author

Closing this to prioritize #104

@adamegyed adamegyed closed this Jul 22, 2024
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