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

set correct dependentCount #9

Conversation

doublespending
Copy link
Contributor

Motivation

dependentCount in pluginData of plugin A is to record how many other plugins depends on plugin A.
If we want to uninstall plugin A, we have to make sure its dependentCount equals to 0.
When installing plugin B, we should also increase the dependentCount of the installed plugins responsible for permittedExecutionSelectors. Otherwise, after uninstalling those plugins, the plugin B will be broken.

Solution

Increase dependentCount in _enableExecFromPlugin and decrease dependentCount in _disableExecFromPlugin.

@adam-alchemy
Copy link
Contributor

Good catch. This is something we considered and was actually a requirement in an older version of the spec, but we dropped it due to a nuanced reason. If a call is performed via executeFromPlugin to a function that has permissions granted for it but is not installed, then it will fail. However, this is not the only reason for which that call can fail. Another circumstances where this can happen is if a pre/post hook is applied, either as a permitted call hook for the calling plugin or generally as an execution hook. If the hook performs a check that fails, then the hook will revert and the call itself via executeFromPlugin will also revert.

Because these are intentional cases where executeFromPlugin can revert, plugins using it must expect that the call will not always succeed. They can then opt to also revert, or depending on the action, may choose to catch the failure and continue.

Since a lack of installed execution function is a similar case where the call reverts, we thought it was unnecessary to strictly enforce this check on the contract level during uninstallation. Frontends and wallets may still choose to warn if a function the plugin intends to call is missing. This also makes plugin upgrade operations somewhat easier, when dealing with certain combinations of installed plugins.

For additional context, the primary purpose of the dependency tracking counter is to prevent the account from ever performing a function call to a plugin that is no longer installed.

That said, this is a nuanced issue, and I understand where you're coming from and why it seems like this should be fixed. If you have any other feedback please feel free to leave it here in the repo or in eth-magicians, we'd really appreciate it.

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