[Hebao 2] Design discussion

[dong77]

Currently, the wallet creation is still costly, the reason is that after we deploy wallet proxies, we have to re-config each wallet by adding multiple Modules and do multiple method bindings (newly deployed proxies don't share implementation contract data).

The idea is to manage modules as Versions. Then each wallet will only have 2 important data:

• The address of a specific Version contract
• The owner address

The address of a universal VersionManager can be a constant value in BaseWallet to save gas.

When a wallet proxy is deployed, we don't need to initialize any state variable for a blank wallet; when we decided to use a blank wallet, we can then set the version and the owner address. Note that Controller, Module, Store are not visible to Wallet. These shall be implementation details to specific Versions.

Currently, the onlyFromModule modifier checks the BaseWallet's module-related data. In the new implementation, Wallet shall delegate the msg.sender validation to its current Version contract. Bound methods are also maintained by Version contracts.

Upgrade to new versions simply means to change the wallet's version address.

Transactions and static bound method invocation will be a bit slower, as we have to access states in Versions instead of the state variables inside a wallet.

[dong77]

interface Wallet
{
    function owner() external view returns (address);

    function version() external view returns (address);

    /// @dev Set a new owner.
    function setOwner(address newOwner) external;

    /// @dev Adds a new module. The `init` method of the module
    ///      will be called with `address(this)` as the parameter.
    ///      This method must throw if the module has already been added.
    /// @param newVersion The new version's address.
    function setVersion(address newVersion) external;

    /// @dev Performs generic transactions. Any module that has been added to this
    ///      wallet can use this method to transact on any third-party contract with
    ///      msg.sender as this wallet itself.
    ///
    ///      Note: 1) this method must ONLY allow invocations from a module that has
    ///      been added to this wallet. The wallet owner shall NOT be permitted
    ///      to call this method directly. 2) Reentrancy inside this function should
    ///      NOT cause any problems.
    ///
    /// @param mode The transaction mode, 1 for CALL, 2 for DELEGATECALL.
    /// @param to The desitination address.
    /// @param value The amount of Ether to transfer.
    /// @param data The data to send over using `to.call{value: value}(data)`
    /// @return returnData The transaction's return value.
    function transact(
        uint8    mode,
        address  to,
        uint     value,
        bytes    calldata data
        )
        external
        returns (bytes memory returnData);
}

interface Version
{
    function label() external view returns (string memory);

    /// @dev Activates this version for the given wallet (msg.sender).
    function activate() external;

    /// @dev Checks if a module has been added to this version.
    /// @param _module The module to check.
    /// @return True if the module exists; False otherwise.
    function hasModule(address _module) external view returns (bool);

    /// @dev Returns the module the given method has been bound to.
    /// @param _method The method's 4-byte selector.
    /// @return _module The address of the bound module. If no binding exists,
    ///                 returns address(0) instead.
    function boundMethodModule(bytes4 _method) external view returns (address _module);
}

interface Module
{
     /// @dev Activates the module for the given wallet (msg.sender). 
     ///            This shall be called by the Version.
    function activate(address wallet) external;

    function bindableMethods() external pure returns(bytes4[] memory);
}

[dong77]

It's pretty hard to maintain backward compatibility, especially after our previous release, Wallet proxy is not upgradable.

[Brechtpd]

Seems like this would be similar to just hard coding modules into the wallet implementation contract, but just making the proxy we deploy for the wallet upgradeable. So instead of setVersion we would just set the new wallet implementation contract which would have all supported module addresses available locally (hasModule/boundMethodModule). Benefit is also that we can update the wallet however we want.

So we basically don't care that much about people being able to /remove modules? Though I guess we could have different wallet implementations that enable different modules so not much of difference really. But this makes it so modules aren't a user feature any more, just a design pattern to create fixed wallets, which seems to be quite different to what I thought the initial goal was. If gas costs would be of no concern (e.g. the smart wallet is on L2) what do we actually want with modules?

[Brechtpd]

Some gas measurements:

create a wallet without ENS:                      252,606  
create a wallet with ENS:                         488,947  
create a wallet with ENS from Blank (no meta tx): 298,262
create a wallet with ENS from Blank (meta tx):    364,958
create a wallet Blank:                            208,813
create a wallet Blank (no modules init):           92,672

By far the biggest cost for creating a wallet is registering the ENS name, using 250,000 gas to do so, around 50% of the total cost of creating a wallet.

Initializing the modules costs around 115,000 gas (4 * SSTORE at 20,000 gas + some call overhead etc...), around 20% of the total cost of creating a wallet. Or a bit more than the cost of doing a single transfer from the smart wallet. Smart wallets are expensive on L1 either way, no way around that.

Whatever we change can have some impact, but by always using ENS we only work on the remaining 50% of the cost for the actual wallet creation. For upgrading wallets of course this isn't that much of a problem. Though it's unlikely (or even good) that we will update the wallet/modules frequently, so I don't think this should be an important criteria in the long run.

Switching to an upgradeable proxy is unlikely to lower the costs of the wallet creation itself by much because it will be a bit more expensive to deploy such a proxy. It will also increase some costs but lower some others, so not sure if using the wallet will be cheaper or expensive, probably around the same. This will help reduce the cost of upgrading the wallet.

[dong77]

Great feedback. These numbers are especially insightful. I didn't realize ENS is so expensive. Maybe we should look into related code instead of changing module design at all.

I totally agree with what you said above about upgradable implementation. I'm not determined to make any changes yet regarding this issue. The current design is quite nice compared to Argent's new Feature design, I prefer it's being as simple as possible.

[dong77]

Brecht, I talked with Kongliang this afternoon. He proposed this idea:

All transactions are on the wallet address itself, not it's modules. This means the proxy delegateCall the real wallet implementation, which looks up for the module behind it based on the method selector, then do another delegateCall to that module. The module will invoke an internal method inside the Wallet implementation and write data in the wallet contract directly using a general-purpose data store. The goal is to avoid calling modules that call back to the module... All based on delegate call and all methods are bound to the wallet based on selectors (no methods shall have the same name).

To avoid binding modules/methods for each wallet, the actual bindings are managed by a version. The wallet will consult its current Version for the methord-selector based module lookup. Migrating to another version simply means changing the Version address.

Do you think this will work?

[Brechtpd]

This will work, because this is essentially what we do with the exchange contract for example using standard solidity libraries. The exchange contract is also behind a proxy, and then we call into libraries which can be either internal or public. In the case they are public delegateCall is used to do the call to execute the library code. And all data is always loaded/stored is stored directly in the exchange contract itself.

So I think the easiest implementation for this is probably just using libraries and using an upgradeable wallet contract so we can just write standard solidity code and perhaps exploit some extra efficiencies we can make some module functions internal.

Not sure if this save a whole lot of gas though, maybe 10%-20% if I had to guess but not irremediable sure..