feat: [v0.8-develop] remove plugin dependencies #86
Conversation
adamegyed
force-pushed
the
adam/refactor-validation-mapping
branch
from
364e06b to
b78c45b
Compare
adamegyed
force-pushed
the
adam/axe-dependencies
branch
from
b98930a to
e6d9980
Compare
adamegyed
force-pushed
the
adam/refactor-validation-mapping
branch
from
b78c45b to
05cea78
Compare
adamegyed
force-pushed
the
adam/axe-dependencies
branch
from
e6d9980 to
eb68744
Compare
| // Plugin metadata storage | ||
| EnumerableSet.AddressSet plugins; | ||
| mapping(address => PluginData) pluginData; | ||
| EnumerableMap.AddressToUintMap pluginManifestHashes; |
There was a problem hiding this comment.
Why not AddressToBytes32Map?
There was a problem hiding this comment.
Unfortunately, it doesn't exist: https://docs.openzeppelin.com/contracts/5.x/api/utils#EnumerableMap
This is the only map that has address as a key type.
There was a problem hiding this comment.
We need to use a newer version!
There was a problem hiding this comment.
I think it is the newest version, haha
There was a problem hiding this comment.
Ah that's not released yet, check under the releases tab. I'd prefer to avoid using pre-release content, but we should update this when 5.1 is released.
| if (mv.isDefault) { | ||
| _storage.validationData[validationFunction].isDefault = true; | ||
| } | ||
|
|
||
| if (mv.isSignatureValidation) { |
There was a problem hiding this comment.
Do we want to keep validation installation in plugin installation? If not, not worth adding new code in.
We talked about only keeping it in one place. While experimenting with validationId, validation installation within plugin installation is almost impossible.
- It require unique onInstall data (an extra validationId) that is not required by other plugin installation.
- We might generate false positive validations due to unable to match the validationIds for selectors actually just using the same validation.
There was a problem hiding this comment.
I think it's worth considering removing validation install via the manifest completely - this PR doesn't do that yet though, it just removes dependencies.
There was a problem hiding this comment.
I meant that we don't need to add more code in here like checking isSignatureValidation since we plan to remove validation install here altogether. No big deal either way.
adamegyed
force-pushed
the
adam/refactor-validation-mapping
branch
from
05cea78 to
4cfa5bf
Compare
adamegyed
force-pushed
the
adam/axe-dependencies
branch
from
eb68744 to
1a3809a
Compare
adamegyed
force-pushed
the
adam/refactor-validation-mapping
branch
from
4cfa5bf to
509ba03
Compare
adamegyed
force-pushed
the
adam/axe-dependencies
branch
from
1a3809a to
7e5fc4f
Compare
adamegyed
force-pushed
the
adam/refactor-validation-mapping
branch
from
509ba03 to
1bfd893
Compare
adamegyed
force-pushed
the
adam/axe-dependencies
branch
from
7e5fc4f to
eaf3a53
Compare
Motivation
Dependencies were used only to assign validation functions to execution functions defined in a plugin. Seeing how we intend to allow more user-controlled configuration of validation functions, we no longer have a good use for dependencies. Dependencies also add a lot of complexity to plugin install, uninstall, and the manifest.
Note that we don't yet have a finalized plan for what to do with the discrepancy between
installValidationandinstallPlugin- this PR is just a simplification step towards finding the actual solution.Solution
Remove dependencies as a parameter to
installPlugin, and the corresponding storage fields.After this change, the
PluginDatastruct would only hold one parametermanifestHash. To simplify, combine thepluginDatamapping and the enumerable set of plugins into an enumerable map, where the key is the plugin address and the value is the manifest hash.Change the structs in the plugin manifest to hold a simplified representation of validation functions - only defining a function id, flags for global use + signature validation, and an optional list of selectors.
Update all usage of the
installPluginfunction and definitions of validation functions in manifests.Future work
This PR still allows plugins to define validation functions. It is not clear to me whether we should allow manifests to define validation functions, given how we intend to treat the "function id" parameter as more of an "entity id" parameter, which interferes with the static nature of the manifest. We could consider only allowing manifests to define "runtime call" permissions, similar to the "permitted calls" list, if we decide to keep the manifest in a similar form to this.
It also doesn't merge
installValidationandinstallPlugin. It would be nice to de-duplicate work here, but so far the manifest-based installation of plugins conflicts with the user-defined installation of validation functions and validation-associated hooks.