feat: add (un)/installation expected behavior for both validation and execution #168
Conversation
standard/ERCs/erc-6900.md
Outdated
| #### Responsibilties of `StandardExecutor` and `ModuleExecutor` | ||
|
|
||
| `StandardExecutor` functions are used for open-ended calls to external addresses. | ||
|
|
||
| `ModuleExecutor` functions are specifically used by modules to request the account to execute with account's context. Explicit permissions are required for modules to use `ModuleExecutor`. | ||
|
|
||
| The following behavior MUST be followed: | ||
|
|
||
| - `StandardExecutor` can NOT call module execution functions and/or `ModuleExecutor`. This is guaranteed by checking whether the call's target implements the `IModule` interface via ERC-165 as required. | ||
| - `StandardExecutor` can NOT be called by module execution functions and/or `ModuleExecutor`. | ||
| - Module execution functions MUST NOT request access to `StandardExecutor`, they MAY request access to `ModuleExecutor`. | ||
|
|
There was a problem hiding this comment.
this is not deleted or changed. Github just moved it to the end of the newly added sections.
There was a problem hiding this comment.
Looks good! Some small comments.
Also, couldn't commit this on the sections, but what do you think of renaming the headers:
- "Validations and their installation /uninstallation" -> "Installation and Uninstallation of Validation Functions"
- "Execution and their installation /uninstallation" -> "Installation and Uninstallation of Execution Functions"
standard/ERCs/erc-6900.md
Outdated
| - Revert if the module does not implement ERC-165 or does not support the `IModule` interface. | ||
| - Revert if `manifestHash` does not match the computed Keccak-256 hash of the module's returned manifest. This prevents installation of modules that attempt to install a different module configuration than the one that was approved by the client. | ||
| - Revert if any address in `dependencies` does not support the interface at its matching index in the manifest's `dependencyInterfaceIds`, or if the two array lengths do not match, or if any of the dependencies are not already installed on the modular account. | ||
| - the account MUST install all pre validation hooks required by the user and call `onInstall` with the user-provided data on the hook module to initialize the states. |
There was a problem hiding this comment.
A bit of a meta-note for all locations in the spec where we describe the requirement to call onInstall: Technically, our account currently doesn't perform this call if the data is empty. With respect to the specificiation, I think we could change the requirement to either be:
- If data is provided to <parameter_name>, the account MUST call
onInstallwith the data. - The account SHOULD call
onInstallwith the data in <parameter_name>.
This pattern appears in each install/uninstall for validation/execution.
Wdyt?
There was a problem hiding this comment.
good point. Updated wording to user intent rather than specific var/param names.
There was a problem hiding this comment.
I think the 2nd is nicer, since it implies more clearly that hooks may not always be initialized with onInstall, as it's a user choice.
I like what we have better because these section don't just talk about installation and uninstallations. They also cover other general higher level things about validations and executions. |
fangting-alchemy
force-pushed
the
installation
branch
from
41e9cdd to
6d3fae8
Compare
standard/ERCs/erc-6900.md
Outdated
| - the account MUST install all pre validation hooks required by the user and call `onInstall` with the user-provided data on the hook module to initialize the states. | ||
| - the account MUST install all permission hooks required by the user and call `onInstall` with the user-provided data on the hook module to initialize the states. | ||
| - the account MUST add all selectors required by the user that the validation can validate. | ||
| - the account MUST set `isGlobal` and `isSignatureValidation` flags as required. |
There was a problem hiding this comment.
I think we're missing the new isUserOpValidation flag too
There was a problem hiding this comment.
Do we need to list all of them here? We say "MUST correctly set flags and other fields" above, so I think we should either:
- Avoid listing individual fields, and just use the generic "all flags and fields" wording
- Avoid stating the generic grouping, and only list individual fields.
There was a problem hiding this comment.
Maybe all flags is enough. Save headaches to update later if more flags are added. Probably dont need fields since all other fields (like hooks and selectors etc.) are mentioned below for clarity and some guidance.
There was a problem hiding this comment.
Hmm, I think it might be nice to list them too to just be clear what flag means. maybe in a way to not indicate the list is exhaustive, but as examples.
There was a problem hiding this comment.
Updated wording, wdyt?
There was a problem hiding this comment.
I think it's good, one nit is that when it says "flags, like isGlobal ...", using "like" immplies there may be more flags beyond those, so maybe we just do "flags: isGloval, ...". But that's just a tiny nit.
There was a problem hiding this comment.
That's a good point, do we want to specify exactly which flags, or do we want to give some examples? Are there implications if accounts add other flags later?
fangting-alchemy
force-pushed
the
installation
branch
from
50357ee to
97371eb
Compare
fangting-alchemy
force-pushed
the
installation
branch
from
97371eb to
0e7874a
Compare
fangting-alchemy
force-pushed
the
installation
branch
from
fbe6fe9 to
e525cb8
Compare
FYI; nothing is changed for
#### Responsibilties ofStandardExecutorandModuleExecutor``