Skip to content

Commit

Permalink
refactor: rename ECDSAValidationModule (#251)
Browse files Browse the repository at this point in the history
  • Loading branch information
jaypaik authored Oct 15, 2024
1 parent 30cd7f5 commit 2485d72
Show file tree
Hide file tree
Showing 16 changed files with 85 additions and 71 deletions.
8 changes: 4 additions & 4 deletions gas/modular-account/ModularAccount.gas.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {Vm} from "forge-std/src/Vm.sol";

import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol";
import {AccountFactory} from "../../src/factory/AccountFactory.sol";
import {ECDSAValidationModule} from "../../src/modules/validation/ECDSAValidationModule.sol";
import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol";

import {ModularAccountBenchmarkBase} from "./ModularAccountBenchmarkBase.sol";

Expand All @@ -38,7 +38,7 @@ contract ModularAccountGasTest is ModularAccountBenchmarkBase("ModularAccount")

assertEq(logs.length, 4);
// Logs:
// 0: ECDSAValidationModule `SignerTransferred` (anonymous)
// 0: SingleSignerValidationModule `SignerTransferred` (anonymous)
// 1: ModularAccount `ValidationInstalled`
// 2: ModularAccount `Initialized`
// 3: AccountFactory `ModularAccountDeployed`
Expand Down Expand Up @@ -243,7 +243,7 @@ contract ModularAccountGasTest is ModularAccountBenchmarkBase("ModularAccount")

vm.deal(address(account1), 1 ether);

ECDSAValidationModule newValidationModule = _deployECDSAValidationModule();
SingleSignerValidationModule newValidationModule = _deploySingleSignerValidationModule();
uint32 newEntityId = 0;
(address owner2, uint256 owner2Key) = makeAddrAndKey("owner2");

Expand Down Expand Up @@ -281,7 +281,7 @@ contract ModularAccountGasTest is ModularAccountBenchmarkBase("ModularAccount")
owner1Key,
_getECDSAReplaySafeHash(
account1,
ecdsaValidationModule,
singleSignerValidationModule,
_getDeferredInstallHash(
account1,
deferredInstallNonce,
Expand Down
16 changes: 8 additions & 8 deletions gas/modular-account/ModularAccountBenchmarkBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {AccountFactory} from "../../src/factory/AccountFactory.sol";
import {FALLBACK_VALIDATION} from "../../src/helpers/Constants.sol";
import {AllowlistModule} from "../../src/modules/permissions/AllowlistModule.sol";
import {TimeRangeModule} from "../../src/modules/permissions/TimeRangeModule.sol";
import {ECDSAValidationModule} from "../../src/modules/validation/ECDSAValidationModule.sol";
import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol";

import {Counter} from "../../test/mocks/Counter.sol";
import {ModuleSignatureUtils} from "../../test/utils/ModuleSignatureUtils.sol";
Expand All @@ -32,7 +32,7 @@ abstract contract ModularAccountBenchmarkBase is BenchmarkBase, ModuleSignatureU
AccountFactory public factory;
ModularAccount public accountImpl;
SemiModularAccountBytecode public semiModularImpl;
ECDSAValidationModule public ecdsaValidationModule;
SingleSignerValidationModule public singleSignerValidationModule;

AllowlistModule public allowlistModule;
TimeRangeModule public timeRangeModule;
Expand All @@ -50,10 +50,10 @@ abstract contract ModularAccountBenchmarkBase is BenchmarkBase, ModuleSignatureU

accountImpl = _deployModularAccount(IEntryPoint(entryPoint));
semiModularImpl = _deploySemiModularAccountBytecode(IEntryPoint(entryPoint));
ecdsaValidationModule = _deployECDSAValidationModule();
singleSignerValidationModule = _deploySingleSignerValidationModule();

factory = new AccountFactory(
entryPoint, accountImpl, semiModularImpl, address(ecdsaValidationModule), address(this)
entryPoint, accountImpl, semiModularImpl, address(singleSignerValidationModule), address(this)
);

allowlistModule = new AllowlistModule();
Expand All @@ -65,7 +65,7 @@ abstract contract ModularAccountBenchmarkBase is BenchmarkBase, ModuleSignatureU

function _deployAccount1() internal {
account1 = factory.createAccount(owner1, 0, 0);
signerValidation = ModuleEntityLib.pack(address(ecdsaValidationModule), 0);
signerValidation = ModuleEntityLib.pack(address(singleSignerValidationModule), 0);
}

function _deploySemiModularAccountBytecode1() internal {
Expand All @@ -84,7 +84,7 @@ abstract contract ModularAccountBenchmarkBase is BenchmarkBase, ModuleSignatureU
uint32 sessionKeyEntityId = 1;

ValidationConfig validationConfig = ValidationConfigLib.pack({
_module: address(ecdsaValidationModule),
_module: address(singleSignerValidationModule),
_entityId: sessionKeyEntityId,
_isGlobal: false,
_isSignatureValidation: false,
Expand Down Expand Up @@ -153,14 +153,14 @@ abstract contract ModularAccountBenchmarkBase is BenchmarkBase, ModuleSignatureU
(bool success,) = address(account1).call(_getInstallDataSessionKeyCase1());
require(success, "Install Session key 1 failed");

return ModuleEntityLib.pack(address(ecdsaValidationModule), 1);
return ModuleEntityLib.pack(address(singleSignerValidationModule), 1);
}

function _verifySessionKeyCase1InstallState() internal view {
// Assert account state is correctly set up

ValidationDataView memory validationData =
account1.getValidationData(ModuleEntityLib.pack(address(ecdsaValidationModule), 1));
account1.getValidationData(ModuleEntityLib.pack(address(singleSignerValidationModule), 1));

// Flags
assertFalse(validationData.isGlobal);
Expand Down
5 changes: 2 additions & 3 deletions gas/modular-account/SemiModularAccount.gas.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ import {Vm} from "forge-std/src/Vm.sol";

import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol";
import {AccountFactory} from "../../src/factory/AccountFactory.sol";

import {ECDSAValidationModule} from "../../src/modules/validation/ECDSAValidationModule.sol";
import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol";

import {ModularAccountBenchmarkBase} from "./ModularAccountBenchmarkBase.sol";

Expand Down Expand Up @@ -238,7 +237,7 @@ contract ModularAccountGasTest is ModularAccountBenchmarkBase("SemiModularAccoun

vm.deal(address(account1), 1 ether);

ECDSAValidationModule newValidationModule = _deployECDSAValidationModule();
SingleSignerValidationModule newValidationModule = _deploySingleSignerValidationModule();
uint32 newEntityId = 0;
(address owner2, uint256 owner2Key) = makeAddrAndKey("owner2");

Expand Down
4 changes: 2 additions & 2 deletions src/factory/AccountFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ contract AccountFactory is Ownable {
IEntryPoint _entryPoint,
ModularAccount _accountImpl,
SemiModularAccountBytecode _semiModularImpl,
address _ecdsaValidationModule,
address _singleSignerValidationModule,
address owner
) Ownable(owner) {
ENTRY_POINT = _entryPoint;
ACCOUNT_IMPL = _accountImpl;
SEMI_MODULAR_ACCOUNT_IMPL = _semiModularImpl;
SINGLE_SIGNER_VALIDATION_MODULE = _ecdsaValidationModule;
SINGLE_SIGNER_VALIDATION_MODULE = _singleSignerValidationModule;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import {BaseModule} from "../BaseModule.sol";
/// - This validation supports ERC-1271. The signature is valid if it is signed by the owner's private key.
/// - This validation supports composition that other validation can relay on entities in this validation
/// to validate partially or fully.
contract ECDSAValidationModule is IValidationModule, ReplaySafeWrapper, BaseModule {
contract SingleSignerValidationModule is IValidationModule, ReplaySafeWrapper, BaseModule {
using MessageHashUtils for bytes32;

uint256 internal constant _SIG_VALIDATION_PASSED = 0;
Expand Down
2 changes: 1 addition & 1 deletion test/account/DeferredValidation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ contract DeferredValidationTest is AccountTestBase {

function setUp() public override {
_encodedCall = abi.encodeCall(ModularAccountBase.execute, (makeAddr("dead"), 0, ""));
_deferredValidation = ModuleEntityLib.pack(address(_deployECDSAValidationModule()), 0);
_deferredValidation = ModuleEntityLib.pack(address(_deploySingleSignerValidationModule()), 0);
uint32 entityId = 0;

(address newSigner, uint256 newSignerKey) = makeAddrAndKey("newSigner");
Expand Down
3 changes: 2 additions & 1 deletion test/account/GlobalValidationTest.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ contract GlobalValidationTest is AccountTestBase {
account2 = ModularAccount(payable(factory.getAddress(owner2, 0, TEST_DEFAULT_VALIDATION_ENTITY_ID)));
vm.deal(address(account2), 100 ether);

_signerValidation = ModuleEntityLib.pack(address(ecdsaValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID);
_signerValidation =
ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID);

ethRecipient = makeAddr("ethRecipient");
vm.deal(ethRecipient, 1 wei);
Expand Down
49 changes: 29 additions & 20 deletions test/account/ModularAccount.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {ModularAccount} from "../../src/account/ModularAccount.sol";
import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol";
import {ModuleManagerInternals} from "../../src/account/ModuleManagerInternals.sol";
import {SemiModularAccountBytecode} from "../../src/account/SemiModularAccountBytecode.sol";
import {ECDSAValidationModule} from "../../src/modules/validation/ECDSAValidationModule.sol";
import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol";

import {Counter} from "../mocks/Counter.sol";
import {ComprehensiveModule} from "../mocks/modules/ComprehensiveModule.sol";
Expand Down Expand Up @@ -104,9 +104,11 @@ contract ModularAccountTest is AccountTestBase {
: abi.encodeCall(
ModularAccountBase.execute,
(
address(ecdsaValidationModule),
address(singleSignerValidationModule),
0,
abi.encodeCall(ECDSAValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2))
abi.encodeCall(
SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)
)
)
);

Expand Down Expand Up @@ -398,34 +400,41 @@ contract ModularAccountTest is AccountTestBase {
vm.stopPrank();
}

// TODO: Consider if this test belongs here or in the tests specific to the ECDSAValidationModule
// TODO: Consider if this test belongs here or in the tests specific to the SingleSignerValidationModule
function test_transferOwnership() public withSMATest {
if (_isSMATest) {
// Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the
// test to pass by ensuring the signer can be set in the validation.
assertEq(
ecdsaValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0)
singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)),
address(0)
);
} else {
assertEq(ecdsaValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1);
assertEq(
singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1
);
}

vm.prank(address(entryPoint));
account1.execute(
address(ecdsaValidationModule),
address(singleSignerValidationModule),
0,
abi.encodeCall(ECDSAValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2))
abi.encodeCall(
SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)
)
);

assertEq(ecdsaValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner2);
assertEq(
singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner2
);
}

function test_isValidSignature() public withSMATest {
bytes32 message = keccak256("hello world");

bytes32 replaySafeHash = _isSMATest
? SemiModularAccountBytecode(payable(account1)).replaySafeHash(message)
: ecdsaValidationModule.replaySafeHash(address(account1), message);
: singleSignerValidationModule.replaySafeHash(address(account1), message);

(uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, replaySafeHash);

Expand All @@ -439,11 +448,11 @@ contract ModularAccountTest is AccountTestBase {

// Only need a test case for the negative case, as the positive case is covered by the isValidSignature test
function test_signatureValidationFlag_enforce() public withSMATest {
// Install a new copy of ECDSAValidationModule with the signature validation flag set to false
// Install a new copy of SingleSignerValidationModule with the signature validation flag set to false
uint32 newEntityId = 2;
vm.prank(address(entryPoint));
account1.installValidation(
ValidationConfigLib.pack(address(ecdsaValidationModule), newEntityId, false, false, true),
ValidationConfigLib.pack(address(singleSignerValidationModule), newEntityId, false, false, true),
new bytes4[](0),
abi.encode(newEntityId, owner1),
new bytes[](0)
Expand All @@ -453,28 +462,28 @@ contract ModularAccountTest is AccountTestBase {

bytes32 replaySafeHash = _isSMATest
? SemiModularAccountBytecode(payable(account1)).replaySafeHash(message)
: ecdsaValidationModule.replaySafeHash(address(account1), message);
: singleSignerValidationModule.replaySafeHash(address(account1), message);

(uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, replaySafeHash);

bytes memory signature = _encode1271Signature(
ModuleEntityLib.pack(address(ecdsaValidationModule), newEntityId), abi.encodePacked(r, s, v)
ModuleEntityLib.pack(address(singleSignerValidationModule), newEntityId), abi.encodePacked(r, s, v)
);

vm.expectRevert(
abi.encodeWithSelector(
ModularAccountBase.SignatureValidationInvalid.selector, ecdsaValidationModule, newEntityId
ModularAccountBase.SignatureValidationInvalid.selector, singleSignerValidationModule, newEntityId
)
);
IERC1271(address(account1)).isValidSignature(message, signature);
}

function test_userOpValidationFlag_enforce() public withSMATest {
// Install a new copy of ECDSAValidationModule with the userOp validation flag set to false
// Install a new copy of SingleSignerValidationModule with the userOp validation flag set to false
uint32 newEntityId = 2;
vm.prank(address(entryPoint));
account1.installValidation(
ValidationConfigLib.pack(address(ecdsaValidationModule), newEntityId, true, false, false),
ValidationConfigLib.pack(address(singleSignerValidationModule), newEntityId, true, false, false),
new bytes4[](0),
abi.encode(newEntityId, owner1),
new bytes[](0)
Expand All @@ -496,7 +505,7 @@ contract ModularAccountTest is AccountTestBase {
bytes32 userOpHash = entryPoint.getUserOpHash(userOp);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash());
userOp.signature = _encodeSignature(
ModuleEntityLib.pack(address(ecdsaValidationModule), newEntityId),
ModuleEntityLib.pack(address(singleSignerValidationModule), newEntityId),
GLOBAL_VALIDATION,
abi.encodePacked(r, s, v)
);
Expand All @@ -510,7 +519,7 @@ contract ModularAccountTest is AccountTestBase {
0,
"AA23 reverted",
abi.encodeWithSelector(
ModularAccountBase.UserOpValidationInvalid.selector, ecdsaValidationModule, newEntityId
ModularAccountBase.UserOpValidationInvalid.selector, singleSignerValidationModule, newEntityId
)
)
);
Expand All @@ -521,7 +530,7 @@ contract ModularAccountTest is AccountTestBase {
account1.executeWithRuntimeValidation(
abi.encodeCall(ModularAccountBase.execute, (ethRecipient, 1 wei, "")),
_encodeSignature(
ModuleEntityLib.pack(address(ecdsaValidationModule), newEntityId), GLOBAL_VALIDATION, ""
ModuleEntityLib.pack(address(singleSignerValidationModule), newEntityId), GLOBAL_VALIDATION, ""
)
);

Expand Down
6 changes: 3 additions & 3 deletions test/account/MultiValidation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";

import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol";
import {ECDSAValidationModule} from "../../src/modules/validation/ECDSAValidationModule.sol";
import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol";

import {AccountTestBase} from "../utils/AccountTestBase.sol";
import {CODELESS_ADDRESS, TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol";
Expand All @@ -19,13 +19,13 @@ contract MultiValidationTest is AccountTestBase {
using ECDSA for bytes32;
using MessageHashUtils for bytes32;

ECDSAValidationModule public validator2;
SingleSignerValidationModule public validator2;

address public owner2;
uint256 public owner2Key;

function setUp() public override {
validator2 = new ECDSAValidationModule();
validator2 = new SingleSignerValidationModule();

(owner2, owner2Key) = makeAddrAndKey("owner2");
}
Expand Down
4 changes: 2 additions & 2 deletions test/account/PerHookData.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ contract PerHookDataTest is CustomValidationTestBase {

bytes32 messageHash = keccak256(abi.encodePacked(message));

bytes32 replaySafeHash = ecdsaValidationModule.replaySafeHash(address(account1), messageHash);
bytes32 replaySafeHash = singleSignerValidationModule.replaySafeHash(address(account1), messageHash);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, replaySafeHash);

PreValidationHookData[] memory preValidationHookData = new PreValidationHookData[](1);
Expand Down Expand Up @@ -500,7 +500,7 @@ contract PerHookDataTest is CustomValidationTestBase {
abi.encode(_PRE_HOOK_ENTITY_ID_1, _counter)
);
// patched to work during SMA tests by enforcing that the new validation is not the fallback validation.
_signerValidation = ModuleEntityLib.pack(address(ecdsaValidationModule), _VALIDATION_ENTITY_ID);
_signerValidation = ModuleEntityLib.pack(address(singleSignerValidationModule), _VALIDATION_ENTITY_ID);
return (
_signerValidation, true, true, true, new bytes4[](0), abi.encode(_VALIDATION_ENTITY_ID, owner1), hooks
);
Expand Down
6 changes: 3 additions & 3 deletions test/account/ReplaceModule.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {ModuleEntityLib} from "@erc6900/reference-implementation/libraries/Modul
import {ValidationConfigLib} from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol";

import {ModularAccountBase} from "../../src/account/ModularAccountBase.sol";
import {ECDSAValidationModule} from "../../src/modules/validation/ECDSAValidationModule.sol";
import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol";

import {MockModule} from "../mocks/modules/MockModule.sol";
import {AccountTestBase} from "../utils/AccountTestBase.sol";
Expand Down Expand Up @@ -99,8 +99,8 @@ contract UpgradeModuleTest is AccountTestBase {

function test_upgradeModuleValidationFunction() public withSMATest {
// Setup new validaiton with pre validation and execution hooks associated with a validator
ECDSAValidationModule validation1 = new ECDSAValidationModule();
ECDSAValidationModule validation2 = new ECDSAValidationModule();
SingleSignerValidationModule validation1 = new SingleSignerValidationModule();
SingleSignerValidationModule validation2 = new SingleSignerValidationModule();
uint32 validationEntityId1 = 10;
uint32 validationEntityId2 = 11;

Expand Down
Loading

0 comments on commit 2485d72

Please sign in to comment.