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

[3/n] Enable validation composability and add composable single singer validation and tests #96

Merged
merged 7 commits into from
Jul 17, 2024
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

Reference implementation for [ERC-6900](https://eips.ethereum.org/EIPS/eip-6900). It is an early draft implementation.

The implementation includes an upgradable modular account with two plugins (`SingleOwnerPlugin` and `TokenReceiverPlugin`). It is compliant with ERC-6900 with the latest updates.
The implementation includes an upgradable modular account with two plugins (`SingleSignerValidation` and `TokenReceiverPlugin`). It is compliant with ERC-6900 with the latest updates.

## Important Callouts

Expand Down
10 changes: 7 additions & 3 deletions src/account/UpgradeableModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,10 @@ contract UpgradeableModularAccount is
revert SignatureValidationInvalid(plugin, entityId);
}

if (IValidation(plugin).validateSignature(entityId, msg.sender, hash, signature[24:]) == _1271_MAGIC_VALUE)
{
if (
IValidation(plugin).validateSignature(address(this), entityId, msg.sender, hash, signature[24:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if this should be addressed in this PR, but if the return values are _1271_MAGIC_VALUE or _1271_INVALID, would prefer to just remove the if/else part and do return IValidation(plugin).validateSignature(address(this), entityId, msg.sender, hash, signature[24:])

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 preferable to do the equivalence check before returning, because then we can absorb any bad values that are valid bytes4 types but aren't _1271_MAGIC_VALUE or _1271_INVALID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm from the spec, returning anything thats not _1271_MAGIC_VALUE should be considered invalid. Don't feel super strongly about this, just a LOC reduction maxi

== _1271_MAGIC_VALUE
) {
return _1271_MAGIC_VALUE;
}
return _1271_INVALID;
Expand Down Expand Up @@ -517,7 +519,9 @@ contract UpgradeableModularAccount is

(address plugin, uint32 entityId) = runtimeValidationFunction.unpack();

try IValidation(plugin).validateRuntime(entityId, msg.sender, msg.value, callData, authSegment.getBody())
try IValidation(plugin).validateRuntime(
address(this), entityId, msg.sender, msg.value, callData, authSegment.getBody()
)
// forgefmt: disable-start
// solhint-disable-next-line no-empty-blocks
{} catch (bytes memory revertReason) {
Expand Down
14 changes: 10 additions & 4 deletions src/interfaces/IValidation.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ interface IValidation is IPlugin {

/// @notice Run the runtime validationFunction specified by the `entityId`.
/// @dev To indicate the entire call should revert, the function MUST revert.
/// @param account the account to validate for.
/// @param entityId An identifier that routes the call to different internal implementations, should there
/// be more than one.
/// @param sender The caller address.
/// @param value The call value.
/// @param data The calldata sent.
/// @param authorization Additional data for the validation function to use.
function validateRuntime(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating these other two validation functions - with this, we should have full composability across all of the different types of validation functions.

address account,
uint32 entityId,
address sender,
uint256 value,
Expand All @@ -34,14 +36,18 @@ interface IValidation is IPlugin {

/// @notice Validates a signature using ERC-1271.
/// @dev To indicate the entire call should revert, the function MUST revert.
/// @param account the account to validate for.
/// @param entityId An identifier that routes the call to different internal implementations, should there
/// be more than one.
/// @param sender the address that sent the ERC-1271 request to the smart account
/// @param hash the hash of the ERC-1271 request
/// @param signature the signature of the ERC-1271 request
/// @return the ERC-1271 `MAGIC_VALUE` if the signature is valid, or 0xFFFFFFFF if invalid.
function validateSignature(uint32 entityId, address sender, bytes32 hash, bytes calldata signature)
external
view
returns (bytes4);
function validateSignature(
address account,
uint32 entityId,
address sender,
bytes32 hash,
bytes calldata signature
) external view returns (bytes4);
}
188 changes: 0 additions & 188 deletions src/plugins/owner/SingleOwnerPlugin.sol

This file was deleted.

28 changes: 28 additions & 0 deletions src/plugins/validation/ISingleSignerValidation.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.25;

import {IValidation} from "../../interfaces/IValidation.sol";

interface ISingleSignerValidation is IValidation {
/// @notice This event is emitted when Signer of the account's validation changes.
/// @param account The account whose validation Signer changed.
/// @param entityId The entityId for the account and the signer.
/// @param previousSigner The address of the previous signer.
/// @param newSigner The address of the new signer.
event SignerTransferred(
address indexed account, uint32 indexed entityId, address previousSigner, address newSigner
);

error NotAuthorized();

/// @notice Transfer Signer of the account's validation to `newSigner`.
/// @param entityId The entityId for the account and the signer.
/// @param newSigner The address of the new signer.
function transferSigner(uint32 entityId, address newSigner) external;

/// @notice Get the signer of the `account`'s validation.
/// @param entityId The entityId for the account and the signer.
/// @param account The account to get the signer of.
/// @return The address of the signer.
function signerOf(uint32 entityId, address account) external view returns (address);
}
Loading
Loading