From a1bb2e6248aca100c00a0aa779cdf244bc41fb44 Mon Sep 17 00:00:00 2001 From: Wilson Cusack Date: Wed, 10 Apr 2024 15:19:59 -0400 Subject: [PATCH] fix tests + include initial implementation in hash --- .gas-snapshot | 18 ++-- src/CoinbaseSmartWalletFactory.sol | 19 +++- .../IsValidSignature.t.sol | 8 +- test/CoinbaseSmartWallet/ValidateUserOp.t.sol | 2 +- test/CoinbaseSmartWalletFactory.t.sol | 4 +- test/ERC1271InputGenerator.t.sol | 95 ++++++++++--------- 6 files changed, 80 insertions(+), 66 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 219fdfa..640b78d 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -10,14 +10,16 @@ AddOwnerPublicKeyTest:testRevertsIfAlreadyOwner() (gas: 116355) AddOwnerPublicKeyTest:testRevertsIfCalledByNonOwner() (gas: 11754) AddOwnerPublicKeyTest:testSetsIsOwner() (gas: 113769) AddOwnerPublicKeyTest:testSetsOwnerAtIndex() (gas: 128616) -CoinbaseSmartWalletFactoryTest:testDeployDeterministicPassValues() (gas: 277752) -CoinbaseSmartWalletFactoryTest:test_CreateAccount_ReturnsPredeterminedAddress_WhenAccountAlreadyExists() (gas: 298114) -CoinbaseSmartWalletFactoryTest:test_RevertsIfLength32ButLargerThanAddress() (gas: 309390) -CoinbaseSmartWalletFactoryTest:test_createAccountDeploysToPredeterminedAddress() (gas: 279250) -CoinbaseSmartWalletFactoryTest:test_createAccountSetsOwnersCorrectly() (gas: 283767) -CoinbaseSmartWalletFactoryTest:test_exitIfAccountIsAlreadyInitialized() (gas: 278647) +CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForDeployedAccount() (gas: 318694) +CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForUndeployedAccount() (gas: 302399) +CoinbaseSmartWalletFactoryTest:testDeployDeterministicPassValues() (gas: 277765) +CoinbaseSmartWalletFactoryTest:test_CreateAccount_ReturnsPredeterminedAddress_WhenAccountAlreadyExists() (gas: 298380) +CoinbaseSmartWalletFactoryTest:test_RevertsIfLength32ButLargerThanAddress() (gas: 309404) +CoinbaseSmartWalletFactoryTest:test_createAccountDeploysToPredeterminedAddress() (gas: 279503) +CoinbaseSmartWalletFactoryTest:test_createAccountSetsOwnersCorrectly() (gas: 283780) +CoinbaseSmartWalletFactoryTest:test_exitIfAccountIsAlreadyInitialized() (gas: 278673) CoinbaseSmartWalletFactoryTest:test_initCodeHash() (gas: 8795) -CoinbaseSmartWalletFactoryTest:test_revertsIfNoOwners() (gas: 31068) +CoinbaseSmartWalletFactoryTest:test_revertsIfNoOwners() (gas: 31029) ERC1271Test:test_returnsExpectedDomainHashWhenProxy() (gas: 29198) ERC1271Test:test_static() (gas: 3243921) MultiOwnableInitializeTest:testRevertsIfLength32ButLargerThanAddress() (gas: 81111) @@ -41,7 +43,7 @@ TestCanSkipChainIdValidation:test_approvedSelectorsReturnTrue() (gas: 15845) TestCanSkipChainIdValidation:test_otherSelectorsReturnFalse() (gas: 12469) TestExecuteWithoutChainIdValidation:testExecute() (gas: 424837) TestExecuteWithoutChainIdValidation:testExecuteBatch() (gas: 728897) -TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3626898, ~: 3472813) +TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3612753, ~: 3472828) TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49960) TestExecuteWithoutChainIdValidation:test__codesize() (gas: 50195) TestExecuteWithoutChainIdValidation:test_revertsWithReservedNonce() (gas: 82347) diff --git a/src/CoinbaseSmartWalletFactory.sol b/src/CoinbaseSmartWalletFactory.sol index e35c6a4..02b220c 100644 --- a/src/CoinbaseSmartWalletFactory.sol +++ b/src/CoinbaseSmartWalletFactory.sol @@ -42,7 +42,7 @@ contract CoinbaseSmartWalletFactory { } (bool alreadyDeployed, address accountAddress) = - LibClone.createDeterministicERC1967(msg.value, emptyImplementation, _getSalt(owners, nonce)); + LibClone.createDeterministicERC1967(msg.value, emptyImplementation, _getSalt(owners, nonce, implementation)); account = CoinbaseSmartWallet(payable(accountAddress)); @@ -58,8 +58,13 @@ contract CoinbaseSmartWalletFactory { /// @param nonce The nonce provided to `createAccount()`. /// /// @return predicted The predicted account deployment address. - function getAddress(bytes[] calldata owners, uint256 nonce) external view returns (address predicted) { - predicted = LibClone.predictDeterministicAddress(initCodeHash(), _getSalt(owners, nonce), address(this)); + function getAddress(bytes[] calldata owners, uint256 nonce, address implementation) + external + view + returns (address predicted) + { + predicted = + LibClone.predictDeterministicAddress(initCodeHash(), _getSalt(owners, nonce, implementation), address(this)); } /// @notice Returns the initialization code hash of the account (a minimal ERC1967 proxy). @@ -75,7 +80,11 @@ contract CoinbaseSmartWalletFactory { /// @param nonce The nonce provided to `createAccount()`. /// /// @return salt The computed salt. - function _getSalt(bytes[] calldata owners, uint256 nonce) internal pure returns (bytes32 salt) { - salt = keccak256(abi.encode(owners, nonce)); + function _getSalt(bytes[] calldata owners, uint256 nonce, address implementation) + internal + pure + returns (bytes32 salt) + { + salt = keccak256(abi.encode(owners, nonce, implementation)); } } diff --git a/test/CoinbaseSmartWallet/IsValidSignature.t.sol b/test/CoinbaseSmartWallet/IsValidSignature.t.sol index a1cdcc5..d92432e 100644 --- a/test/CoinbaseSmartWallet/IsValidSignature.t.sol +++ b/test/CoinbaseSmartWallet/IsValidSignature.t.sol @@ -24,7 +24,7 @@ contract TestIsValidSignature is SmartWalletTestBase { r: uint256(r), s: uint256(s) }) - ) + ) }) ); @@ -78,7 +78,7 @@ contract TestIsValidSignature is SmartWalletTestBase { r: uint256(r), s: uint256(s) }) - ) + ) }) ); @@ -106,7 +106,7 @@ contract TestIsValidSignature is SmartWalletTestBase { r: uint256(r) - 1, s: uint256(s) }) - ) + ) }) ); @@ -153,7 +153,7 @@ contract TestIsValidSignature is SmartWalletTestBase { r: uint256(r) - 1, s: uint256(s) }) - ) + ) }) ); diff --git a/test/CoinbaseSmartWallet/ValidateUserOp.t.sol b/test/CoinbaseSmartWallet/ValidateUserOp.t.sol index b709bd5..446c5cc 100644 --- a/test/CoinbaseSmartWallet/ValidateUserOp.t.sol +++ b/test/CoinbaseSmartWallet/ValidateUserOp.t.sol @@ -65,7 +65,7 @@ contract TestValidateUserOp is SmartWalletTestBase { r: uint256(r), s: uint256(s) }) - ) + ) }) ); diff --git a/test/CoinbaseSmartWalletFactory.t.sol b/test/CoinbaseSmartWalletFactory.t.sol index 2e83e51..d1bf516 100644 --- a/test/CoinbaseSmartWalletFactory.t.sol +++ b/test/CoinbaseSmartWalletFactory.t.sol @@ -47,13 +47,13 @@ contract CoinbaseSmartWalletFactoryTest is Test { } function test_createAccountDeploysToPredeterminedAddress() public { - address p = factory.getAddress(owners, 0); + address p = factory.getAddress(owners, 0, address(account)); CoinbaseSmartWallet a = factory.createAccount{value: 1e18}(owners, 0, address(account)); assertEq(address(a), p); } function test_CreateAccount_ReturnsPredeterminedAddress_WhenAccountAlreadyExists() public { - address p = factory.getAddress(owners, 0); + address p = factory.getAddress(owners, 0, address(account)); CoinbaseSmartWallet a = factory.createAccount{value: 1e18}(owners, 0, address(account)); CoinbaseSmartWallet b = factory.createAccount{value: 1e18}(owners, 0, address(account)); assertEq(address(a), p); diff --git a/test/ERC1271InputGenerator.t.sol b/test/ERC1271InputGenerator.t.sol index 3f75038..5b0503e 100644 --- a/test/ERC1271InputGenerator.t.sol +++ b/test/ERC1271InputGenerator.t.sol @@ -1,46 +1,49 @@ -// SPDX-License-Identifier: UNLICENSED -// pragma solidity ^0.8.0; - -// import {Test, console2} from "forge-std/Test.sol"; -// import {CoinbaseSmartWallet} from "../src/CoinbaseSmartWallet.sol"; -// import {CoinbaseSmartWalletFactory} from "../src/CoinbaseSmartWalletFactory.sol"; -// import {ERC1271InputGenerator} from "../src/utils/ERC1271InputGenerator.sol"; - -// contract CoinbaseSmartWallet1271InputGeneratorTest is Test { -// CoinbaseSmartWalletFactory factory; -// CoinbaseSmartWallet implementation; -// CoinbaseSmartWallet deployedAccount; -// bytes[] owners; - -// function setUp() public { -// implementation = new CoinbaseSmartWallet(); -// factory = new CoinbaseSmartWalletFactory(address(implementation)); -// } - -// function testGetReplaySafeHashForDeployedAccount() public { -// owners.push(abi.encode(address(1))); -// deployedAccount = CoinbaseSmartWallet(payable(factory.createAccount(owners, 0))); - -// bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5; -// bytes32 replaySafeHash = deployedAccount.replaySafeHash(hash); -// ERC1271InputGenerator generator = new ERC1271InputGenerator(deployedAccount, hash, address(0), ""); -// assertEq(bytes32(address(generator).code), replaySafeHash); -// } - -// function testGetReplaySafeHashForUndeployedAccount() public { -// owners.push(abi.encode(address(1))); -// CoinbaseSmartWallet undeployedAccount = CoinbaseSmartWallet(payable(factory.getAddress(owners, 0))); -// bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5; -// ERC1271InputGenerator generator = new ERC1271InputGenerator( -// undeployedAccount, -// hash, -// address(factory), -// abi.encodeWithSignature("createAccount(bytes[],uint256)", owners, 0) -// ); - -// // This is now deployed. -// bytes32 replaySafeHash = undeployedAccount.replaySafeHash(hash); - -// assertEq(bytes32(address(generator).code), replaySafeHash); -// } -// } +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {Test, console2} from "forge-std/Test.sol"; +import {CoinbaseSmartWallet} from "../src/CoinbaseSmartWallet.sol"; +import {CoinbaseSmartWalletFactory} from "../src/CoinbaseSmartWalletFactory.sol"; +import {ERC1271InputGenerator} from "../src/utils/ERC1271InputGenerator.sol"; + +contract CoinbaseSmartWallet1271InputGeneratorTest is Test { + CoinbaseSmartWalletFactory factory; + CoinbaseSmartWallet implementation; + CoinbaseSmartWallet deployedAccount; + bytes[] owners; + + function setUp() public { + implementation = new CoinbaseSmartWallet(); + factory = new CoinbaseSmartWalletFactory(); + } + + function testGetReplaySafeHashForDeployedAccount() public { + owners.push(abi.encode(address(1))); + deployedAccount = CoinbaseSmartWallet(payable(factory.createAccount(owners, 0, address(implementation)))); + + bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5; + bytes32 replaySafeHash = deployedAccount.replaySafeHash(hash); + ERC1271InputGenerator generator = new ERC1271InputGenerator(deployedAccount, hash, address(0), ""); + assertEq(bytes32(address(generator).code), replaySafeHash); + } + + function testGetReplaySafeHashForUndeployedAccount() public { + owners.push(abi.encode(address(1))); + CoinbaseSmartWallet undeployedAccount = + CoinbaseSmartWallet(payable(factory.getAddress(owners, 0, address(implementation)))); + bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5; + ERC1271InputGenerator generator = new ERC1271InputGenerator( + undeployedAccount, + hash, + address(factory), + abi.encodeWithSelector( + CoinbaseSmartWalletFactory.createAccount.selector, owners, 0, address(implementation) + ) + ); + + // This is now deployed. + bytes32 replaySafeHash = undeployedAccount.replaySafeHash(hash); + + assertEq(bytes32(address(generator).code), replaySafeHash); + } +}