feat: [v0.8-develop, experimental]: move runtime validation always allow to execution function definition [4/N] #61
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamegyed
changed the title
jaypaik
approved these changes
May 28, 2024
Collaborator
jaypaik
left a comment
jaypaik
left a comment
There was a problem hiding this comment.
Generally really like this simplification too. Would be great to get rid of the last magic value if possible too...
| // The selector to install | ||
| bytes4 executionSelector; | ||
| // If true, the function won't need runtime validaiton, and can be called by anyone. | ||
| bool isPublic; |
Collaborator
There was a problem hiding this comment.
Perhaps isUnprotected or isUnguarded (or something else similar) is clearer?
Or maybe even isRuntimeUnprotected?
Contributor
Author
There was a problem hiding this comment.
Yeah good ideas. I also thought of needsValidation, but I thought the intuition around isPublic might be simpler.
Contributor
There was a problem hiding this comment.
Using a name that includes runtime might help reduce user errors
huaweigu
approved these changes
May 29, 2024
| // The selector to install | ||
| bytes4 executionSelector; | ||
| // If true, the function won't need runtime validaiton, and can be called by anyone. | ||
| bool isPublic; |
Contributor
There was a problem hiding this comment.
Using a name that includes runtime might help reduce user errors
adamegyed
force-pushed
the
adam/remove-plugin-call-restriction
branch
from
c179b20 to
f2ebe06
Compare
adamegyed
force-pushed
the
adam/simplify-rt-allowed-calls
branch
from
c83ac72 to
e871784
Compare
adamegyed
force-pushed
the
adam/remove-plugin-call-restriction
branch
from
f2ebe06 to
34ccb35
Compare
adamegyed
force-pushed
the
adam/simplify-rt-allowed-calls
branch
from
96934cc to
77ce9ff
Compare
adamegyed
force-pushed
the
adam/remove-plugin-call-restriction
branch
from
34ccb35 to
7d9b2f1
Compare
Base automatically changed from
adam/remove-plugin-call-restriction
to
v0.8-develop
May 31, 2024 18:44
adamegyed
force-pushed
the
adam/simplify-rt-allowed-calls
branch
from
77ce9ff to
f48ccd7
Compare
adamegyed
added a commit
that referenced
this pull request
Jul 26, 2024
…on function definition (#61)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
To allow for multiple validation functions per selector, we need a way to deal with overlapping values. The "always allow in runtime" magic value for validation poses a problem, where it could create additional overlapping values, similar to the "always deny" validation hook.
Solution
Instead of doing another special-casing in the validation storage, we can add a property to the definition of an execution function to indicate whether or not the function requires validation.
This is a more logical organization, because the implementer of a function should know whether it is a sensitive function or not. It avoids having to define validation functions for each of these, which simplifies most plugin manifests.
Although this will most often be applied to view functions, this PR avoids restricting it to only view functions and to using
staticcall- there may be legitimate non-view functions that can be called by anyone, and allowing them comes at no incremental cost.Future work
This introduces a new struct to the manifest, even though the fields within it would easily fit within a single word. The spec should pack these in the future, once we are more certain of its format.