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

Get CoinbaseSmartContractFactory and ERC1271 to 100% coverage #37

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

stevieraykatz
Copy link
Contributor

@stevieraykatz stevieraykatz commented Mar 19, 2024

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: foundry-rs/foundry#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)  |

@stevieraykatz stevieraykatz changed the title Get CoinbaseSmartContractFactory to 100% coverage Get CoinbaseSmartContractFactory and ERC1271 to 100% coverage Mar 19, 2024
@stevieraykatz stevieraykatz marked this pull request as ready for review March 19, 2024 21:10
@stevieraykatz stevieraykatz merged commit 009c269 into main Mar 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants