coinbase / smart-wallet

MIT License
258 stars 47 forks source link

Get CoinbaseSmartContractFactory and ERC1271 to 100% coverage #37

Closed stevieraykatz closed 3 months ago

stevieraykatz commented 3 months ago

As part of an ongoing effort to achieve 100% coverage for all contracts in this repo...

CoinbaseSmartContractFactory

  1. Added a missing test for initCodeHash()
  2. Added two new tests to catch missing branch in createAccount for cases when the account has already been intitialized and another case wherein the account cannot be initialized because of malformed owner bytes.

ERC1271

Currently, foundry coverage doesn't like the fact that we return the return-declared salt and extensions. Adding an explicit check that the return values match the uninitialized args doesn't tick the coverage box. However, in uncommitted changes (shown below), explicitly defining the values of salt and extensions gets this test to pass with "full coverage". Since explicitly declaring these variables (again) adds ~50 gas, we shouldn't make the change just for the sake of the coverage report.

Bug reported to foundry: https://github.com/foundry-rs/foundry/issues/7476

    function eip712Domain()
        external
        view
        virtual
        returns (
            bytes1 fields,
            string memory name,
            string memory version,
            uint256 chainId,
            address verifyingContract,
            bytes32 salt,
            uint256[] memory extensions
        )
    {
        fields = hex"0f"; // `0b1111`.
        (name, version) = _domainNameAndVersion();
        chainId = block.chainid;
        verifyingContract = address(this);
        // salt = salt; // `bytes32(0)`.
        // extensions = extensions; // `new uint256[](0)`.
        salt = bytes32(0);
        extensions = new uint256[](0); 
    }

Coverage report as of this PR:

| File                                             | % Lines          | % Statements     | % Branches      | % Funcs         |
|--------------------------------------------------|------------------|------------------|-----------------|-----------------|
| script/DeployERC4337Factory.s.sol                | 0.00% (0/8)      | 0.00% (0/12)     | 100.00% (0/0)   | 0.00% (0/2)     |
| src/CoinbaseSmartWallet.sol                      | 100.00% (46/46)  | 100.00% (65/65)  | 100.00% (22/22) | 100.00% (13/13) |
| src/CoinbaseSmartWalletFactory.sol               | 100.00% (10/10)  | 100.00% (10/10)  | 100.00% (4/4)   | 100.00% (4/4)   |
| src/ERC1271.sol                                  | 85.71% (12/14)   | 89.47% (17/19)   | 100.00% (2/2)   | 100.00% (6/6)   |
| src/MultiOwnable.sol                             | 100.00% (27/27)  | 100.00% (38/38)  | 80.00% (8/10)   | 100.00% (13/13) |
| src/utils/ERC1271InputGenerator.sol              | 0.00% (0/9)      | 0.00% (0/14)     | 0.00% (0/6)     | 0.00% (0/1)     |
| test/CoinbaseSmartWallet/SmartWalletTestBase.sol | 0.00% (0/16)     | 0.00% (0/18)     | 0.00% (0/1)     | 0.00% (0/6)     |
| test/CoinbaseSmartWallet/UpgradeToAndCall.t.sol  | 100.00% (1/1)    | 100.00% (1/1)    | 100.00% (0/0)   | 50.00% (1/2)    |
| test/MultiOwnable/MultiOwnableTestBase.t.sol     | 0.00% (0/3)      | 0.00% (0/3)      | 100.00% (0/0)   | 0.00% (0/1)     |
| test/mocks/MockCoinbaseSmartWallet.sol           | 50.00% (1/2)     | 66.67% (2/3)     | 100.00% (0/0)   | 50.00% (1/2)    |
| test/mocks/MockEntryPoint.sol                    | 40.00% (2/5)     | 33.33% (2/6)     | 0.00% (0/2)     | 33.33% (1/3)    |
| test/mocks/MockMultiOwnable.sol                  | 100.00% (1/1)    | 100.00% (1/1)    | 100.00% (0/0)   | 100.00% (1/1)   |
| test/mocks/MockTarget.sol                        | 80.00% (4/5)     | 100.00% (4/4)    | 0.00% (0/1)     | 66.67% (2/3)    |
| Total                                            | 70.75% (104/147) | 72.16% (140/194) | 75.00% (36/48)  | 73.68% (42/57)  |