Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Implement Multicall functionality for batched calls #43

Merged
merged 15 commits into from
Jul 3, 2020

Conversation

roynalnaruto
Copy link
Collaborator

@roynalnaruto roynalnaruto commented Jul 1, 2020

Motivation

This PR targets #36

The motivation is to be able to read multiple values from the blockchain state (calls may be made to different smart contracts). This reduces the number of round trips and hence saves time.

Solution

The solution has been inspired by the one prescribed in the #36 issue. We aggregate the calldata in a Multicall struct instance and then call Maker's Multicall contract, which is deployed on multiple Ethereum networks.

Using the Multicall API would look something, like implemented in the test:

// initiate the Multicall instance
let multicall = Multicall::new(client, Some(multicall_contract.address())).await?;
// OR
// In this case, ethers-rs will fetch the Multicall contract address from the client's network ID
// let multicall = Multicall::new(client, None).await?;
//
// OR
// let multicall = Multicall::new(client.clone(), None)
//     .await?
//     .add_call(first_call)
//     .add_call(second_call);

// add calls to multicall
let multicall = multicall.add_call(call_1);
let multicall = multicall.add_call(call_2);

// In solidity
// (u256 blockNumber, []bytes memory returnData)
let return_data: (String, (String, Address), Address, Address) =
    multicall.call().await?;

Pending tasks

  • Document the Multicall module, as it is also exported
  • I was not really sure what Err type I should return. I have simply used the ContractError::ConstructorError type. This should of course be changed, but not sure if I should add another error type to the ContractError enums? Or have a separate enum for Multicall?
  • Make a small fix to the Abigen logic
    At present, for a smart contract function with a single argument, the generated binding is:
pub fn aggregate(
    &self,
    calls: Vec<(Address, Vec<u8>)>,
) -> ContractCall<P, S, (U256, Vec<Vec<u8>>)> {
    self.0
        .method_hash([37, 45, 186, 66], (calls, )) // <--- note this
        .expect("method not found (this should never happen)")
}

This does make sense for multi-argument function, where a tuple is required. But in the case of a single argument, I think we should prefer something like this:

pub fn aggregate(
    &self,
    calls: Vec<(Address, Vec<u8>)>,
) -> ContractCall<P, S, (U256, Vec<Vec<u8>>)> {
    self.0
        .method_hash([37, 45, 186, 66], calls) // <--- and this
        .expect("method not found (this should never happen)")
}

Otherwise, the arguments supplied to the function call cannot be encoded and the ethabi::Function::encode_input throws an InvalidData error.

  • Handle edge-cases on the MAX number of calls allowed to be aggregated
    Ethers-rs supports at the most 16 elements in a tuple for tokenization/detokenization. We must take note of that and either:
  • Increase this limit in the ethers-core/src/tokens.rs
  • Limit the number of calls in the Multicall.calls vector (this has to be done either way)

So that we don't risk a Detokenization error.

  • Re-evaluate ethers-rs's tokenization/detokenization logic
    This sub-task is mainly concerned with the traits Tokenizable, TokenizableItem and the way Detokenize is implemented for types that do implement Tokenizable. Please note the pre-processing and post-processing done to the tokens. This is done because:
  • ethabi tokenizes a tuple return value as a vector of those individual tokens
    So, for a function returning (string, address), ethabi would tokenize it as:
let tokens: Vec<Token> = vec![Token::String(str_val), Token::Address(addr_val)]

But ethers-rs denotes them as a Token::Tuple, hence:

let tokens: Vec<Token> = vec![Token::Tuple(vec![Tuple::String(str_val), Tuple::Address(addr_val)])]

We can adapt to their tokenization approach (it already was like that before, when nested tuples wouldn't be possible to detokenize). But we will have to then either give up on the nested tokenization and also on tokenization of a vector of tuples. If not, we would have to customize ethabi to suit our needs, or continue with this pre and post-processing approach I have taken (To be fair, I think this wouldn't be THAT bad).

  • Make another small fix to the Abigen logic
    At the moment, a contract function that is not view returns a H256 for the transaction hash, as we expect a user to send a transaction to that function. But even a call could be made to such a function, and it could return some data. In order to address this, as suggested by @gakonst himself, we would need a struct similar to the NameOrAddress, and calling it HashOrData<T>. This should be used as the D: Detokenize type for such a function, as it could return a H256 on ContractCall.send() or T on ContractCall.call().

@roynalnaruto roynalnaruto requested a review from gakonst July 1, 2020 12:22
@roynalnaruto roynalnaruto changed the title Implement Multicall functionality for batched calls [WIP] Implement Multicall functionality for batched calls Jul 1, 2020
@roynalnaruto roynalnaruto linked an issue Jul 1, 2020 that may be closed by this pull request
Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Functionally looks good, we're missing docs + a test where there are state mutating functions (in addition to the current one which nicely tests the calls) - you already have your checklist above, but just re-iterating

I also think that since add_call already returns Self, we can remove the add_calls function and just encourage people to go with the builder pattern.

I think that instead of returning a new error in the MultiCall, it's fine to panic if an address is not set and if the address is not found in the address book (just add it with a # Panics doc).

ethers-contract/src/multicall.rs Outdated Show resolved Hide resolved
ethers-contract/src/multicall.rs Outdated Show resolved Hide resolved
ethers-contract/src/multicall.rs Outdated Show resolved Hide resolved
ethers-contract/src/multicall.rs Outdated Show resolved Hide resolved
ethers-contract/src/multicall.rs Outdated Show resolved Hide resolved
ethers-contract/tests/contract.rs Outdated Show resolved Hide resolved
ethers-contract/src/multicall.rs Outdated Show resolved Hide resolved
@roynalnaruto roynalnaruto requested a review from gakonst July 2, 2020 14:43
@roynalnaruto roynalnaruto changed the title [WIP] Implement Multicall functionality for batched calls Implement Multicall functionality for batched calls Jul 2, 2020
@roynalnaruto
Copy link
Collaborator Author

Added another utility method eth_balance_of to append a call for fetching an address' ETH balance. Included in the docs and tests.

@gakonst gakonst force-pushed the master branch 2 times, most recently from 1dd2e86 to b3debd0 Compare July 3, 2020 14:44
Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

LGTM!

@gakonst gakonst merged commit a9bb98b into gakonst:master Jul 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batched Transactions
2 participants