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

feat(spec): misc fixes #171

Merged
merged 1 commit into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/interfaces/IExecutionModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ pragma solidity ^0.8.25;
import {IModule} from "./IModule.sol";

struct ManifestExecutionFunction {
// TODO(erc6900 spec): These fields can be packed into a single word
// The selector to install
bytes4 executionSelector;
// If true, the function won't need runtime validation, and can be called by anyone.
Expand All @@ -14,7 +13,6 @@ struct ManifestExecutionFunction {
}

struct ManifestExecutionHook {
// TODO(erc6900 spec): These fields can be packed into a single word
bytes4 executionSelector;
uint32 entityId;
bool isPreHook;
Expand Down
5 changes: 2 additions & 3 deletions src/modules/validation/SingleSignerValidationModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,8 @@ contract SingleSignerValidationModule is ISingleSignerValidationModule, ReplaySa

/// @inheritdoc IModule
function onUninstall(bytes calldata data) external override {
// ToDo: what does it mean in the world of composable validation world to uninstall one type of validation
// We can either get rid of all SingleSigner signers. What about the nested ones?
_transferSigner(abi.decode(data, (uint32)), address(0));
uint32 entityId = abi.decode(data, (uint32));
_transferSigner(entityId, address(0));
}

/// @inheritdoc IValidationModule
Expand Down
14 changes: 7 additions & 7 deletions standard/ERCs/erc-6900.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ Each step is modular, supporting different implementations, that allows for open

**Modular Smart Contract Accounts** **MUST** implement

- `IAccount.sol` from [ERC-4337](./eip-4337.md).
- `IAccountExecute.sol` from [ERC-4337](./eip-4337.md).
- `IAccount.sol` and `IAccountExecute.sol` from [ERC-4337](./eip-4337.md).
- `IModularAccount.sol` to support module management and usage, and account identification.
- The function `isValidSignature` from [ERC-1271](./eip-1271.md)

Expand All @@ -78,7 +77,7 @@ Each step is modular, supporting different implementations, that allows for open

- `IModule.sol` described below and implement ERC-165 for `IModule`.

**Modules** **May** implement one of the following module types
**Modules** **MAY** implement any of the following module types

- `IValidationModule` to support validations for account.
- `IValidationHookModule` to support hooks for validations.
Expand Down Expand Up @@ -411,8 +410,7 @@ interface IValidationHookModule is IModule {
/// @param signature The signature of the message.
function preSignatureValidationHook(uint32 entityId, address sender, bytes32 hash, bytes calldata signature)
external
view
returns (bytes4);
view;
}
```

Expand Down Expand Up @@ -532,7 +530,7 @@ During execution uninstallation, the account MUST correctly clear flags and othe

Modular accounts support three different calls flows for validation: user op validation, runtime validation, and signature validation. User op validation happens within the account's implementation of the function `validateUserOp`, defined in the ERC-4337 interface `IAccount`. Runtime validation happens through the dispatcher function `executeWithAuthorization`, or when using direct call validation. Signature validation happens within the account's implementation of the function `isValidSignature`, defined in ERC-1271.

For each of these validation types, an account implementation may specify its own format for selecting which validation function to use, as well as any per-hook data for validation hooks.
For each of these validation types, an account implementation MAY specify its own format for selecting which validation function to use, as well as any per-hook data for validation hooks.

Within the implementation of each type of validation function, the modular account MUST check that the provided validation function applies to the given function selector intended to be run. Then, the account MUST execute all validation hooks of the corresponding type associated with the validation function in use. These hooks MUST be executed in the order specified during installation. After the execution of validation hooks, the account MUST invoke the validation function of the corresponding type. If any validation hooks or the validation function revert, the account MUST revert. It SHOULD include the module's revert data within its revert data.

Expand Down Expand Up @@ -588,7 +586,9 @@ This proposal includes several interfaces that build on ERC-4337. First, we stan

## Backwards Compatibility

TODO
Existing accounts that are deployed as proxies may have the ability to upgrade account implementations to one that supports this standard for modularity. Depending on implementation logic, existing modules may be wrapped in an adapter contract to adhere to the standard.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should mention anything related to storage re: upgrades?

Technically speaking, the RI namespaces storage, but is that expected of other implementations? If not, that could cause issues with upgrades. Do you think we ought to mention a little warning about storage collisions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The focus of this section is on backwards compatibility, and because the standard is proxy-agnostic (whether or not you use one at all), I'm not sure we want to introduce it here. If we explicitly call out namedspaced storage (ERC-7201) then we also have to add it to the dependencies list, and I think it's probably not worth introducing the dependency just for this section.

Maybe we just add a sentence like "Users should exercise caution when doing proxy upgrades, and verify that no issues, such as storage collisions, occur in the process." ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine as is then-- better not bleed into proxies in that case!


The standard also allows for flexibility in account implementations, including accounts that have certain features implemented without modules, so usage of modules may be gradually introduced.

## Reference Implementation

Expand Down
Loading