Add Account and increase pragma to 0.8.27

[ernestognw]

Description

Adds support for Accounts in OpenZeppelin Community Contracts.

[socket-security]

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

[ernestognw]

Seems like the build is broken because some libraries of the community repository rely on the Calldata library, which is not released yet until 5.3

[ernestognw]

I keep getting the following error when running yarn prepare:

Downloading compiler 0.8.27
 | TypeError: Linearization of inheritance graph impossible
 |   --> contracts/generated/504cdf2b09d152566e5b55ac062f0ff8e65fc29c.sol:14:1:
 |    |
 | 14 | contract MyAccount is Initializable, Account, EIP712, ERC7739, SignerRSA, ERC7821, ERC721Holder, ERC1155Holder {
 |    | ^ (Relevant source part starts here and spans across multiple lines).
 | 
 | 
 | TypeError: Linearization of inheritance graph impossible
 |   --> contracts/generated/0a34ddf6c6eac317acfc9d6a7ad3badf6c2aa72f.sol:16:1:
 |    |
 | 16 | contract MyAccount is Initializable, Account, EIP712, AccountERC7579, ERC7739, SignerECDSA, ERC721Holder, ERC1155Holder {
 |    | ^ (Relevant source part starts here and spans across multiple lines).

However, the output seems fine and is compiling independently. Here's an example:

// SPDX-License-Identifier: MIT
// Compatible with OpenZeppelin Contracts ^5.0.0
pragma solidity ^0.8.27;

import {AbstractSigner} from "@openzeppelin/community-contracts/utils/cryptography/AbstractSigner.sol";
import {Account} from "@openzeppelin/community-contracts/account/Account.sol";
import {AccountERC7579} from "@openzeppelin/community-contracts/account/extensions/AccountERC7579.sol";
import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol";
import {ERC1155Holder} from "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
import {ERC721Holder} from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import {ERC7739} from "@openzeppelin/community-contracts/utils/cryptography/ERC7739.sol";
import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
import {PackedUserOperation} from "@openzeppelin/contracts/interfaces/draft-IERC4337.sol";
import {SignerECDSA} from "@openzeppelin/community-contracts/utils/cryptography/SignerECDSA.sol";

contract MyAccount is Initializable, Account, EIP712, AccountERC7579, ERC7739, SignerECDSA, ERC721Holder, ERC1155Holder {
    constructor() EIP712("MyAccount", "1") {}

    function isValidSignature(bytes32 hash, bytes calldata signature)
        public
        view
        override(AccountERC7579, ERC7739)
        returns (bytes4)
    {
        // ERC-7739 can return the fn selector (success), 0xffffffff (invalid) or 0x77390001 (detection).
        // If the return is 0xffffffff, we fallback to validation using ERC-7579 modules.
        bytes4 erc7739magic = ERC7739.isValidSignature(hash, signature);
        return erc7739magic == bytes4(0xffffffff) ? AccountERC7579.isValidSignature(hash, signature) : erc7739magic;
    }

    function _rawSignatureValidation(bytes32 hash, bytes calldata signature)
        internal
        view
        override(SignerECDSA, AbstractSigner, AccountERC7579)
        returns (bool)
    {
        // Force signer validation first, and fallback to ERC-7579
        // return SignerECDSA._rawSignatureValidation(hash, signature) || AccountERC7579._rawSignatureValidation(hash, signature);
        return super._rawSignatureValidation(hash, signature);
    }

    function initializeECDSA(address signer) public initializer {
        _setSigner(signer);
    }

    // The following functions are overrides required by Solidity.

    function _validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash)
        internal
        override(Account, AccountERC7579)
        returns (uint256)
    {
        return super._validateUserOp(userOp, userOpHash);
    }
}

[CoveMB]

The linearization issue might be due to the inheriting from many contracts, some might share common ancestor? Maybe you could check if that is the case with solc --ast-compact-json

[ernestognw]

The linearization issue might be due to the inheriting from many contracts, some might share common ancestor? Maybe you could check if that is the case with solc --ast-compact-json

I'm familiar with the linearization issue, but the current implementation only produces contracts that can be linearized using Solidity's C3 algorithm for resolution. However, I think it's a dependency issue since previous contracts in the community repo had different ordering, but upgrading @openzeppelin/community-contracts fails since the newest version expects things from OZ Contracts in master (i.e. ENTRYPOINT_V08).

Maybe we'll need to wait until we release 5.4 😢

[CoveMB]

Ha gotcha, maybe you can check that with yarn add @openzeppelin/contracts@github:OpenZeppelin/openzeppelin-contracts to check if it works with (unreleased) code from master

[ernestognw]

Ha gotcha, maybe you can check that with yarn add @openzeppelin/contracts@github:OpenZeppelin/openzeppelin-contracts to check if it works with (unreleased) code from master

Great idea! Though it seemed to pull the latest release tag so also failed. We just released 5.3 so now it should be fine, will keep working on this. Thanks @CoveMB

[ericglau]

LGTM, thanks! I adjusted some styling and types, please review.

upgradeable being forced to false is fine, since we eventually want to support it. access being forced to false doesn't seem very correct, since ideally we'd want to avoid having it in the Account options type in the first place. (I tried removing the unused options but it ended up being too large of a change). But given this is considered experimental, I think this works for now.

[ernestognw]

LGTM, thanks! I adjusted some styling and types, please review.

LGTM too, thanks @ericglau

upgradeable being forced to false is fine, since we eventually want to support it.

Yes, part of future OZ vanilla releases include these contracts

access being forced to false doesn't seem very correct, since ideally we'd want to avoid having it in the Account options type in the first place. (I tried removing the unused options but it ended up being too large of a change). But given this is considered experimental, I think this works for now.

Yeah I also tried and ended up being too much. The account has such a unique authorization model in comparison to other contracts that it might require some rework, but I agree it's fine given it's still in community contracts

[CoveMB]

🔥