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

Add Multicall module #2608

Merged
merged 18 commits into from Apr 7, 2021
Merged

Add Multicall module #2608

merged 18 commits into from Apr 7, 2021

Conversation

martriay
Copy link
Contributor

@martriay martriay commented Mar 22, 2021

Fixes #2558

  • Tests
  • Documentation
  • Changelog entry

@martriay martriay marked this pull request as draft March 22, 2021 23:31
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me.
You could shorten it even more by doing :
results[i] = Address.functionDelegateCall(address(this), data[i]);

Depending on how solidity does optimization, that might even save a memory copy.
Next we'll need tests and documentation

@Amxx
Copy link
Collaborator

Amxx commented Mar 24, 2021

For the record: https://github.com/Uniswap/uniswap-v3-periphery/blob/main/contracts/base/Multicall.sol
It would be great to measure gas usage against this

@martriay martriay changed the title Batch call Add BatchCall module Mar 24, 2021
@martriay
Copy link
Contributor Author

Doing some quick tests:

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity 0.8.0;
pragma abicoder v2;

import "./ERC20Mock.sol";

/// @title Multicall
/// @notice Enables calling multiple methods in a single call to the contract
abstract contract Multicall {
    function multicall(bytes[] calldata data) external payable returns (bytes[] memory results) {
        results = new bytes[](data.length);
        for (uint256 i = 0; i < data.length; i++) {
            (bool success, bytes memory result) = address(this).delegatecall(data[i]);

            if (!success) {
                // Next 5 lines from https://ethereum.stackexchange.com/a/83577
                if (result.length < 68) revert();
                assembly {
                    result := add(result, 0x04)
                }
                revert(abi.decode(result, (string)));
            }

            results[i] = result;
        }
    }
}

contract UniswapMultiCallTokenMock is ERC20Mock, Multicall {
    constructor (uint256 initialBalance) ERC20Mock("BatchCallToken", "BCT", msg.sender, initialBalance) {}
}

v.s.

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "../utils/BatchCall.sol";
import "./ERC20Mock.sol";

contract BatchCallTokenMock is ERC20Mock, BatchCall {
    constructor (uint256 initialBalance) ERC20Mock("BatchCallToken", "BCT", msg.sender, initialBalance) {}
}

results in

·------------------------------------|----------------------------|-------------|-----------------------------·
|        Solc version: 0.8.0         ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 10000000 gas  │
·····································|····························|·············|······························
|  Methods                                                                                                    │
·······················|·············|··············|·············|·············|···············|··············
|  Contract            ·  Method     ·  Min         ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
·······················|·············|··············|·············|·············|···············|··············
|  BatchCallTokenMock  ·  batchCall  ·           -  ·          -  ·      88620  ·            1  ·          -  │
·······················|·············|··············|·············|·············|···············|··············
|  Deployments                       ·                                          ·  % of limit   ·             │
·····································|··············|·············|·············|···············|··············
|  BatchCallTokenMock                ·           -  ·          -  ·    1897353  ·         19 %  ·          -  │
·------------------------------------|--------------|-------------|-------------|---------------|-------------·
·-------------------------------------------|----------------------------|-------------|-----------------------------·
|            Solc version: 0.8.0            ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 10000000 gas  │
············································|····························|·············|······························
|  Methods                                                                                                           │
······························|·············|··············|·············|·············|···············|··············
|  Contract                   ·  Method     ·  Min         ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
······························|·············|··············|·············|·············|···············|··············
|  UniswapMultiCallTokenMock  ·  multicall  ·           -  ·          -  ·      85551  ·            1  ·          -  │
······························|·············|··············|·············|·············|···············|··············
|  Deployments                              ·                                          ·  % of limit   ·             │
············································|··············|·············|·············|···············|··············
|  UniswapMultiCallTokenMock                ·           -  ·          -  ·    1918374  ·       19.2 %  ·          -  │
·-------------------------------------------|--------------|-------------|-------------|---------------|-------------·

@martriay martriay marked this pull request as ready for review March 25, 2021 03:15
@Amxx
Copy link
Collaborator

Amxx commented Mar 25, 2021

·------------------------------------|----------------------------|-------------|-----------------------------·
|        Solc version: 0.8.0         ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 10000000 gas  │
·····································|····························|·············|······························
|  Methods                                                                                                    │
·······················|·············|··············|·············|·············|···············|··············
|  Contract            ·  Method     ·  Min         ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
·······················|·············|··············|·············|·············|···············|··············
|  BatchCallTokenMock  ·  batchCall  ·           -  ·          -  ·      88620  ·            1  ·          -  │
·······················|·············|··············|·············|·············|···············|··············
|  Deployments                       ·                                          ·  % of limit   ·             │
·····································|··············|·············|·············|···············|··············
|  BatchCallTokenMock                ·           -  ·          -  ·    1897353  ·         19 %  ·          -  │
·------------------------------------|--------------|-------------|-------------|---------------|-------------·
·-------------------------------------------|----------------------------|-------------|-----------------------------·
|            Solc version: 0.8.0            ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 10000000 gas  │
············································|····························|·············|······························
|  Methods                                                                                                           │
······························|·············|··············|·············|·············|···············|··············
|  Contract                   ·  Method     ·  Min         ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
······························|·············|··············|·············|·············|···············|··············
|  UniswapMultiCallTokenMock  ·  multicall  ·           -  ·          -  ·      85551  ·            1  ·          -  │
······························|·············|··············|·············|·············|···············|··············
|  Deployments                              ·                                          ·  % of limit   ·             │
············································|··············|·············|·············|···············|··············
|  UniswapMultiCallTokenMock                ·           -  ·          -  ·    1918374  ·       19.2 %  ·          -  │
·-------------------------------------------|--------------|-------------|-------------|---------------|-------------·

This shows that, without optimisation, uniswap multicall is more expensive to deploy by about 20k gas, but less expensive to use..
We would need to run that with optimisation though ...

doing COMPILE_MODE=production will enable that

@martriay
Copy link
Contributor Author

Should I switch to their implementation?

·-------------------------------------------|---------------------------|-------------|-----------------------------·
|            Solc version: 0.8.0            ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 10000000 gas  │
············································|···························|·············|······························
|  Methods                                                                                                          │
······························|·············|·············|·············|·············|···············|··············
|  Contract                   ·  Method     ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
······························|·············|·············|·············|·············|···············|··············
|  BatchCallTokenMock         ·  batchCall  ·          -  ·          -  ·      85618  ·            1  ·          -  │
······························|·············|·············|·············|·············|···············|··············
|  UniswapBatchCallTokenMock  ·  multicall  ·          -  ·          -  ·      82695  ·            1  ·          -  │
······························|·············|·············|·············|·············|···············|··············
|  Deployments                              ·                                         ·  % of limit   ·             │
············································|·············|·············|·············|···············|··············
|  BatchCallTokenMock                       ·          -  ·          -  ·    1139352  ·       11.4 %  ·          -  │
············································|·············|·············|·············|···············|··············
|  UniswapBatchCallTokenMock                ·          -  ·          -  ·    1120295  ·       11.2 %  ·          -  │
·-------------------------------------------|-------------|-------------|-------------|---------------|-------------·

@martriay martriay changed the title Add BatchCall module Add Multicall module Mar 26, 2021
@frangio
Copy link
Contributor

frangio commented Mar 27, 2021

The additional cost is probably that we are calling extcodesize in functionDelegateCall. I think we should keep this check, and it will become a lot cheaper with the Berlin hardfork anyway.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my suggestions for testing this looks good.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/utilities.adoc Show resolved Hide resolved
test/utils/BatchCall.test.js Outdated Show resolved Hide resolved
Amxx
Amxx previously approved these changes Mar 30, 2021
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this looks good to me, I'd personally go with uniswap implementation if that can save some gas, or do a manual try-catch block ... but that is not a strong opinion.

@martriay
Copy link
Contributor Author

Linking @Amxx's payable exploit analysis here as rationale for removing the payable modifier.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all!

@frangio frangio enabled auto-merge (squash) April 7, 2021 17:21
@frangio frangio merged commit 7f6a166 into master Apr 7, 2021
@frangio frangio deleted the batch-call branch April 7, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multicall module
3 participants