Skip to content

Commit

Permalink
Reuse contract instance for better performance (#2628)
Browse files Browse the repository at this point in the history 
# Description
First task of #2627

Creating contract instances with `ethcontract-rs` is very costly
(especially in tight loops).
A very significant amount of time is spent creating instances to query
allowances.


# Changes
Instead of creating 1 contract instance per token to fetch balances for
we now create a single erc20 instance and reuse it for all `allowance()`
calls. This is unfortunately pretty ugly since `ethcontract-rs` doesn't
really expose the generated calldata very nicely and doesn't really
compose with `web3`.

## How to test
Correctness should be covered by e2e tests
Performance was verified using the benchmark mentioned in
#2627

It's a bit hard to see but fetching allowances went from ~20% to ~7.6%
of the entire runtime.
The overall runtime of the axum request handler went from ~74% -> 63% of
the runtime.
Overall I think the performance gains are significant enough to warrant
the slightly ugly but very local performance hack until we replace
`ethcontract-rs` with `alloy` entirely.

Before
<img width="1728" alt="Screenshot 2024-04-17 at 21 32 46"
src="https://github.com/cowprotocol/services/assets/19190235/9d8d9cdf-ed72-4bff-a40b-c3cbf5418dbf">


Optimized
<img width="1728" alt="Screenshot 2024-04-17 at 21 28 35"
src="https://github.com/cowprotocol/services/assets/19190235/005aa2cd-7679-4d49-8220-c86cd25988a1">
  • Loading branch information
@MartinquaXD
MartinquaXD authored Apr 18, 2024
1 parent ae05328 commit 03c69f9
Showing 1 changed file with 52 additions and 74 deletions.
126 changes: 52 additions & 74 deletions crates/solver/src/interactions/allowances.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
use {
crate::interactions::Erc20ApproveInteraction,
anyhow::{anyhow, bail, ensure, Context as _, Result},
anyhow::{anyhow, ensure, Context as _, Result},
contracts::{dummy_contract, ERC20},
ethcontract::{batch::CallBatch, errors::ExecutionError, H160, U256},
ethcontract::{H160, U256},
ethrpc::Web3,
maplit::hashmap,
shared::{
Expand All @@ -16,11 +16,11 @@ use {
std::{
collections::{HashMap, HashSet},
slice,
sync::Arc,
},
web3::error::TransportError,
web3::types::CallRequest,
};

const MAX_BATCH_SIZE: usize = 100;
#[cfg_attr(test, mockall::automock)]
#[async_trait::async_trait]
pub trait AllowanceManaging: Send + Sync {
Expand Down Expand Up @@ -136,11 +136,17 @@ impl Interaction for Approval {
pub struct AllowanceManager {
web3: Web3,
owner: H160,
erc20: Arc<ERC20>,
}

impl AllowanceManager {
pub fn new(web3: Web3, owner: H160) -> Self {
Self { web3, owner }
// Only instantiate a single contract instance to generate calldata.
// The actual call will be done with the web3 instance using the respective
// `token` as the `to` address. (performance optimization)
let erc20 = Arc::new(ERC20::at(&web3, owner));

Self { web3, owner, erc20 }
}
}

Expand All @@ -149,6 +155,7 @@ impl AllowanceManaging for AllowanceManager {
async fn get_allowances(&self, tokens: HashSet<H160>, spender: H160) -> Result<Allowances> {
Ok(fetch_allowances(
self.web3.clone(),
self.erc20.clone(),
self.owner,
hashmap! { spender => tokens },
)
Expand All @@ -166,7 +173,13 @@ impl AllowanceManaging for AllowanceManager {
.insert(request.token);
}

let allowances = fetch_allowances(self.web3.clone(), self.owner, spender_tokens).await?;
let allowances = fetch_allowances(
self.web3.clone(),
self.erc20.clone(),
self.owner,
spender_tokens,
)
.await?;
let mut result = Vec::new();
for request in requests {
let allowance = allowances
Expand All @@ -181,6 +194,7 @@ impl AllowanceManaging for AllowanceManager {

async fn fetch_allowances<T>(
web3: ethcontract::Web3<T>,
erc20: Arc<ERC20>,
owner: H160,
spender_tokens: HashMap<H160, HashSet<H160>>,
) -> Result<HashMap<H160, Allowances>>
Expand All @@ -189,25 +203,26 @@ where
T::Batch: Send,
T::Out: Send,
{
let mut batch = CallBatch::new(web3.transport());
let results: Vec<_> = spender_tokens
let futures = spender_tokens
.into_iter()
.flat_map(|(spender, tokens)| tokens.into_iter().map(move |token| (spender, token)))
.map(|(spender, token)| {
let allowance = ERC20::at(&web3, token)
.allowance(owner, spender)
.batch_call(&mut batch);
(spender, token, allowance)
})
.collect();

batch.execute_all(MAX_BATCH_SIZE).await;
let web3 = web3.clone();
let erc20 = erc20.clone();

async move {
let calldata = erc20.allowance(owner, spender).m.tx.data.unwrap();
let req = CallRequest::builder().to(token).data(calldata).build();
let allowance = web3.eth().call(req, None).await;
(spender, token, allowance)
}
});
let results: Vec<_> = futures::future::join_all(futures).await;

let mut allowances = HashMap::new();
for (spender, token, allowance) in results {
let allowance = match allowance.await {
Ok(value) => value,
Err(err) if is_batch_error(&err.inner) => bail!(err),
let allowance = match allowance {
Ok(value) => U256::from(value.0.as_slice()),
Err(err) => {
tracing::warn!("error retrieving allowance for token {:?}: {}", token, err);
continue;
Expand All @@ -224,20 +239,6 @@ where
Ok(allowances)
}

fn is_batch_error(err: &ExecutionError) -> bool {
match &err {
ExecutionError::Web3(web3::Error::Transport(TransportError::Message(message))) => {
// Currently, there is no sure-fire way to determine if a Web3 error
// is caused because of a failing batch request, or some a call
// specific error, so we test that the method starts with "Batch"
// string as a best guess.
// <https://github.com/gnosis/ethcontract-rs/issues/550>
message.starts_with("Batch")
}
_ => false,
}
}

#[cfg(test)]
mod tests {
use {
Expand Down Expand Up @@ -397,31 +398,28 @@ mod tests {
let web3 = mock::web3();
web3.transport()
.mock()
.expect_execute_batch()
.returning(move |calls| {
Ok(calls
.into_iter()
.map(|(method, params)| {
assert_eq!(method, "eth_call");

let call =
serde_json::from_value::<CallRequest>(params[0].clone()).unwrap();
assert_eq!(call.data.unwrap(), allowance_call_data(owner, spender));

let to = call.to.unwrap();
if to == addr!("1111111111111111111111111111111111111111") {
Ok(allowance_return_data(1337.into()))
} else if to == addr!("2222222222222222222222222222222222222222") {
Err(web3::Error::Decoder("test error".to_string()))
} else {
panic!("call to unexpected token {to:?}")
}
})
.collect())
.expect_execute()
.returning(move |method, params| {
assert_eq!(method, "eth_call");

let call = serde_json::from_value::<CallRequest>(params[0].clone()).unwrap();
assert_eq!(call.data.unwrap(), allowance_call_data(owner, spender));
let to = call.to.unwrap();

if to == addr!("1111111111111111111111111111111111111111") {
Ok(allowance_return_data(1337.into()))
} else if to == addr!("2222222222222222222222222222222222222222") {
Err(web3::Error::Decoder("test error".to_string()))
} else {
panic!("call to unexpected token {to:?}")
}
});

let erc20 = Arc::new(ERC20::at(&web3, Default::default()));

let allowances = fetch_allowances(
web3,
erc20,
owner,
hashmap! {
spender => hashset![H160([0x11; 20]), H160([0x22; 20])],
Expand All @@ -440,24 +438,4 @@ mod tests {
},
);
}

#[tokio::test]
async fn fetch_fails_on_batch_errors() {
let web3 = mock::web3();
web3.transport()
.mock()
.expect_execute_batch()
.returning(|_| Err(web3::Error::Decoder("test error".to_string())));

let allowances = fetch_allowances(
web3,
H160([1; 20]),
hashmap! {
H160([2; 20]) => hashset![H160([0x11; 20]), H160([0x22; 20])],
},
)
.await;

assert!(allowances.is_err());
}
}

0 comments on commit 03c69f9

Please sign in to comment.