fix: linting warnings and initializer logic (#46)

## Summary
This PR addresses the linting warnings and corrects the logic in
`WalletStorageInitializable`.

## Detail
### Changeset
* Resolve all warnings (thanks to Ashutosh!)
* Correct the logic in the unused code within
`WalletStorageInitializable`
* Eliminate redundant calls in `SingleOwnerMSCA` constructor
* Add a sanity check `!= address(0)` in `SingleOwnerMSCA`

### Checklist
- [x] Did you add new tests and confirm all tests pass? (`yarn test`)
- [ ] Did you update relevant docs? (docs are found in the `docs`
folder)
- [x] Do your commits follow the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard?
- [x] Does your PR title also follow the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard?
- [ ] If you have a breaking change, is it [correctly reflected in your
commit
message](https://www.conventionalcommits.org/en/v1.0.0/#examples)? (e.g.
`feat!: breaking change`)
- [x] Did you run lint (`yarn lint`) and fix any issues?
- [x] Did you run formatter (`yarn format:check`) and fix any issues
(`yarn format:write`)?

## Testing
* No new tests have been added for style changes.
* New tests have been included for the fixes:
* For the `walletStorageInitializer` fix, I have forked the OZ
JavaScript tests and rewritten them using Foundry.
* The remaining two changes are already covered by existing tests (we
don't allow installing `address(0)` and
`testInitializeSingleOwnerMSCAWithRuntimeValidation`).

## Documentation
n/a

.solhint-src.json

@@ -1,6 +1,7 @@
 {
   "extends": "solhint:recommended",
   "rules": {
-    "compiler-version": ["error", "0.8.24"]
+    "compiler-version": ["error", "0.8.24"],
+    "func-visibility": ["error", { "ignoreConstructors": true }]
   }
 }

.solhint-test.json

@@ -4,6 +4,7 @@
     "compiler-version": ["error", "0.8.24"],
     "no-unused-vars": "off",
     "no-console": "off",
-    "func-name-mixedcase": "off"
+    "func-name-mixedcase": "off",
+    "func-visibility": ["error", { "ignoreConstructors": true }]
   }
 }

foundry.toml

@@ -3,7 +3,7 @@ fs_permissions = [{ access = "read-write", path = "./gas"},
     { access = "read", path = "./test/fixtures"}]
 src = 'src'
 out = 'out'
-libs = ['lib']
+libs = ['lib', 'node_modules']
 solc_version = "0.8.24"
 test = 'test'
 via_ir = true

package.json

@@ -5,7 +5,7 @@
   "scripts": {
     "clean": "rm -rf cache artifacts typechain-types",
     "build": "forge build",
-    "lint": "solhint -w 103 -c .solhint-src.json './src/**/*.sol' && solhint -w 241 -c .solhint-test.json './test/**/*.sol' && solhint -w 0 -c .solhint-script.json './script/**/*.sol'",
+    "lint": "solhint -w 0 -c .solhint-src.json './src/**/*.sol' && solhint -w 0 -c .solhint-test.json './test/**/*.sol' && solhint -w 0 -c .solhint-script.json './script/**/*.sol'",
     "test": "forge test -vv",
     "gasreport": "forge test --gas-report > gas/reports/gas-report.txt",
     "coverage": "forge coverage --ir-minimum",

script/002_DeployUpgradableMSCAFactory.s.sol

@@ -42,7 +42,7 @@ contract DeployUpgradableMSCAFactoryScript is Script {
             factory = UpgradableMSCAFactory(EXPECTED_FACTORY_ADDRESS);
             console.log("Found existing factory at expected address: %s", address(factory));
         }
-        console.log("Account implementation address: %s", address(factory.accountImplementation()));
+        console.log("Account implementation address: %s", address(factory.ACCOUNT_IMPLEMENTATION()));
         vm.stopBroadcast();
     }
 }

script/003_DeploySingleOwnerMSCAFactory.s.sol

@@ -38,7 +38,7 @@ contract DeploySingleOwnerMSCAFactoryScript is Script {
             factory = SingleOwnerMSCAFactory(EXPECTED_FACTORY_ADDRESS);
             console.log("Found existing single owner MSCA factory at expected address: %s", address(factory));
         }
-        console.log("Account implementation address: %s", address(factory.accountImplementation()));
+        console.log("Account implementation address: %s", address(factory.ACCOUNT_IMPLEMENTATION()));
         vm.stopBroadcast();
     }
 }

script/005_DeployECDSAAccountFactory.s.sol

@@ -40,7 +40,7 @@ contract DeployECDSAAccountFactoryScript is Script {
             factory = ECDSAAccountFactory(EXPECTED_FACTORY_ADDRESS);
         }
         console.log("Factory address: %s", address(factory));
-        console.log("Account implementation address: %s", address(factory.accountImplementation()));
+        console.log("Account implementation address: %s", address(factory.ACCOUNT_IMPLEMENTATION()));
         vm.stopBroadcast();
     }
 }

src/account/CoreAccount.sol

@@ -19,6 +19,7 @@
 pragma solidity 0.8.24;
 
 import {DefaultCallbackHandler} from "../callback/DefaultCallbackHandler.sol";
+import {InvalidLength, UnauthorizedCaller} from "../common/Errors.sol";
 import {ExecutionUtils} from "../utils/ExecutionUtils.sol";
 import {BaseAccount} from "@account-abstraction/contracts/core/BaseAccount.sol";
 import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol";
@@ -44,7 +45,7 @@ abstract contract CoreAccount is
 {
     // bytes4(keccak256("isValidSignature(bytes32,bytes)")
     bytes4 internal constant EIP1271_MAGIC_VALUE = 0x1626ba7e;
-    IEntryPoint public immutable entryPointAddress;
+    IEntryPoint public immutable ENTRY_POINT_ADDRESS;
 
     /**
      * @dev This empty reserved space is put in place to allow future versions to add new
@@ -57,18 +58,21 @@ abstract contract CoreAccount is
 
     function _checkOwner() internal view override {
         // directly from EOA owner, or through the account itself (which gets redirected through execute())
-        require(msg.sender == owner() || msg.sender == address(this), "Caller is not the owner");
+        if (!(msg.sender == owner() || msg.sender == address(this))) {
+            revert UnauthorizedCaller();
+        }
     }
 
     /// @custom:oz-upgrades-unsafe-allow constructor
     // for immutable values in implementations
     constructor(IEntryPoint _newEntryPoint) {
-        entryPointAddress = _newEntryPoint;
+        ENTRY_POINT_ADDRESS = _newEntryPoint;
         // lock the implementation contract so it can only be called from proxies
         _disableInitializers();
     }
 
     // for mutable values in proxies
+    // solhint-disable-next-line func-name-mixedcase
     function __CoreAccount_init(address _newOwner) internal onlyInitializing {
         __Ownable_init();
         transferOwnership(_newOwner);
@@ -77,7 +81,7 @@ abstract contract CoreAccount is
 
     /// @inheritdoc BaseAccount
     function entryPoint() public view virtual override returns (IEntryPoint) {
-        return entryPointAddress;
+        return ENTRY_POINT_ADDRESS;
     }
 
     /// @dev Please override this method
@@ -105,8 +109,9 @@ abstract contract CoreAccount is
         whenNotPaused
     {
         _requireFromEntryPointOrOwner();
-        require(dest.length == func.length, "wrong array lengths");
-        require(dest.length == value.length, "wrong array lengths");
+        if (dest.length != func.length || dest.length != value.length) {
+            revert InvalidLength();
+        }
         for (uint256 i = 0; i < dest.length; i++) {
             ExecutionUtils.callAndRevert(dest[i], value[i], func[i]);
         }
@@ -116,7 +121,9 @@ abstract contract CoreAccount is
      * @dev Require the function call went through EntryPoint or owner.
      */
     function _requireFromEntryPointOrOwner() internal view {
-        require(msg.sender == address(entryPoint()) || msg.sender == owner(), "account: not EntryPoint or Owner");
+        if (!(msg.sender == address(entryPoint()) || msg.sender == owner())) {
+            revert UnauthorizedCaller();
+        }
     }
 
     /**

src/account/v1/ECDSAAccount.sol

@@ -47,6 +47,7 @@ contract ECDSAAccount is CoreAccount, UUPSUpgradeable, BaseERC712CompliantAccoun
     /// @inheritdoc UUPSUpgradeable
     // The {_authorizeUpgrade} function must be overridden to include access restriction to the upgrade mechanism.
     // Authorize the owner to upgrade the contract.
+    // solhint-disable-next-line no-empty-blocks
     function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}
 
     /// @custom:oz-upgrades-unsafe-allow constructor

src/account/v1/factory/ECDSAAccountFactory.sol

@@ -18,9 +18,9 @@
  */
 pragma solidity 0.8.24;
 
-import "../ECDSAAccount.sol";
-import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
-import "@openzeppelin/contracts/utils/Create2.sol";
+import {ECDSAAccount, IEntryPoint} from "../ECDSAAccount.sol";
+import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {Create2} from "@openzeppelin/contracts/utils/Create2.sol";
 
 /**
  * @dev Account factory that creates the upgradeable account signed and verified by ECDSA.
@@ -29,10 +29,10 @@ contract ECDSAAccountFactory {
     event AccountCreated(address indexed proxy, address indexed owner);
 
     // logic implementation
-    ECDSAAccount public immutable accountImplementation;
+    ECDSAAccount public immutable ACCOUNT_IMPLEMENTATION;
 
     constructor(IEntryPoint _entryPoint) {
-        accountImplementation = new ECDSAAccount(_entryPoint);
+        ACCOUNT_IMPLEMENTATION = new ECDSAAccount(_entryPoint);
     }
 
     /**
@@ -76,7 +76,7 @@ contract ECDSAAccountFactory {
         account = ECDSAAccount(
             payable(
                 new ERC1967Proxy{salt: _salt}(
-                    address(accountImplementation), abi.encodeCall(ECDSAAccount.initialize, (_owner))
+                    address(ACCOUNT_IMPLEMENTATION), abi.encodeCall(ECDSAAccount.initialize, (_owner))
                 )
             )
         );
@@ -92,7 +92,7 @@ contract ECDSAAccountFactory {
         bytes32 code = keccak256(
             abi.encodePacked(
                 type(ERC1967Proxy).creationCode,
-                abi.encode(address(accountImplementation), abi.encodeCall(ECDSAAccount.initialize, (_owner)))
+                abi.encode(address(ACCOUNT_IMPLEMENTATION), abi.encodeCall(ECDSAAccount.initialize, (_owner)))
             )
         );
         // call computeAddress(salt, bytecodeHash, address(this))

src/callback/DefaultCallbackHandler.sol

@@ -64,5 +64,7 @@ contract DefaultCallbackHandler is IERC721Receiver, IERC1155Receiver, IERC777Rec
         uint256 amount,
         bytes calldata userData,
         bytes calldata operatorData
-    ) external pure override {}
+    ) external pure override 
+    // solhint-disable-next-line no-empty-blocks
+    {}
 }

src/common/Errors.sol

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2024 Circle Internet Group, Inc. All rights reserved.
+
+ * SPDX-License-Identifier: GPL-3.0-or-later
+
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+pragma solidity 0.8.24;
+
+/**
+ * @notice Throws when the caller is unexpected.
+ */
+error UnauthorizedCaller();
+
+error InvalidLength();
+
+error Unsupported();

src/libs/CastLib.sol

@@ -28,6 +28,7 @@ library CastLib {
     function toAddressArray(SetValue[] memory values) internal pure returns (address[] memory addresses) {
         bytes32[] memory valuesBytes;
 
+        // solhint-disable-next-line no-inline-assembly
         assembly ("memory-safe") {
             valuesBytes := values
         }
@@ -37,6 +38,7 @@ library CastLib {
             valuesBytes[i] >>= 96;
         }
 
+        // solhint-disable-next-line no-inline-assembly
         assembly ("memory-safe") {
             addresses := valuesBytes
         }

src/libs/MessageHashUtils.sol

@@ -38,6 +38,7 @@ library MessageHashUtils {
      */
     function toTypedDataHash(bytes32 domainSeparator, bytes32 structHash) internal pure returns (bytes32 digest) {
         /// @solidity memory-safe-assembly
+        // solhint-disable-next-line no-inline-assembly
         assembly {
             let ptr := mload(0x40)
             mstore(ptr, hex"1901")

src/msca/6900/shared/common/Errors.sol

@@ -18,11 +18,6 @@
  */
 pragma solidity 0.8.24;
 
-/**
- * @notice Throws when the caller is unexpected.
- */
-error UnauthorizedCaller();
-
 /**
  * @notice Throws when the selector is not found.
  */
@@ -49,10 +44,6 @@ error InvalidInitializationInput();
 
 error Create2FailedDeployment();
 
-error InvalidLength();
-
-error Unsupported();
-
 error NotImplemented(bytes4 selector, uint8 functionId);
 
 error InvalidItem();

src/msca/6900/shared/libs/AddressDLLLib.sol

@@ -117,6 +117,8 @@ library AddressDLLLib {
             results[count] = current;
             current = dll.next[current];
         }
+
+        // solhint-disable-next-line no-inline-assembly
         assembly ("memory-safe") {
             mstore(results, count)
         }

src/msca/6900/shared/libs/Bytes4DLLLib.sol

@@ -110,6 +110,7 @@ library Bytes4DLLLib {
             results[count] = current;
             current = dll.next[current];
         }
+        // solhint-disable-next-line no-inline-assembly
         assembly ("memory-safe") {
             mstore(results, count)
         }