Skip to content

Conversation

@adamegyed
Copy link
Contributor

@adamegyed adamegyed commented May 17, 2024

Motivation

#52 identified an issue, where if a validation plugin defined any execution functions, it could not be installed multiple times. This is a goal we're working towards with composable validation, so we need a way to address the issue.

A simple candidate change to address this is to remove the call restriction on plugins, and to handle the logic of guarded functions like transferOwnership through the execute/executeBatch workflows.

Solution

Remove the restriction on _exec to only call non-plugin contracts.

Remove the installation of execution functions owner() and transferOwnership(...) from SingleOwnerPlugin. We should eventually provide this as a non-mandatory option during install, but that is not addressed here.

Future work

This PR does not address the security ramifications of this - it breaks the segmentation of validation functions provided by the 1:1 association of selectors and validation. We will need to implement some ability to filter calls based on the type of validation used in the future.

@adamegyed adamegyed changed the title feat: [v0.8-develop, experimental]: remove plugin call restriction feat: [v0.8-develop, experimental]: remove plugin call restriction [3/N] May 17, 2024
@adamegyed adamegyed marked this pull request as ready for review May 21, 2024 18:46
@adamegyed adamegyed requested a review from a team May 21, 2024 23:30
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

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

I generally like this direction. Could you leave a TODO somewhere appropriate to track validation-execution association?

@huaweigu
Copy link
Contributor

Removing the installation of execution functions on plugins is an interesting idea. Would it be accurate to say this approach essentially recommends native exec functions such as execute/executeBatch, and simplifies the enforcement of global validation?

@adamegyed
Copy link
Contributor Author

Yes, I think that's accurate @huaweigu. It simplifies the enforcement, but we may need to provide ways to lock it down more in the future. It could be necessary for cases when a plugin wants to add protections to specific functions, like transferring ownership.

@adamegyed adamegyed force-pushed the adam/sig-validator-interface branch from c25af9c to 7f62cb9 Compare May 29, 2024 20:50
@adamegyed adamegyed force-pushed the adam/remove-plugin-call-restriction branch from c179b20 to f2ebe06 Compare May 29, 2024 20:56
@adamegyed adamegyed force-pushed the adam/sig-validator-interface branch from 7f62cb9 to 3d38b61 Compare May 31, 2024 18:29
@adamegyed adamegyed force-pushed the adam/remove-plugin-call-restriction branch from f2ebe06 to 34ccb35 Compare May 31, 2024 18:30
Base automatically changed from adam/sig-validator-interface to v0.8-develop May 31, 2024 18:32
@adamegyed adamegyed force-pushed the adam/remove-plugin-call-restriction branch from 34ccb35 to 7d9b2f1 Compare May 31, 2024 18:34
@adamegyed adamegyed merged commit 7ac0f6b into v0.8-develop May 31, 2024
@adamegyed adamegyed deleted the adam/remove-plugin-call-restriction branch May 31, 2024 18:44
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.

4 participants