Skip to content

Commit

Permalink
Adjust verified token diffs for settlement contract address (#2735)
Browse files Browse the repository at this point in the history
# Description
When verifying a quote we determine its accuracy by how many tokens had
to be paid out of the settlement contract balances. The more tokens were
lost from the buffers during the quote simulation the sloppier the
computed provided quote was.
The one major edge case this has is when the settlement contract itself
is the trader account. Then it currently looks like we pay all of the
buy tokens from the settlement contract and leave all of the sell tokens
in the settlement contract. (effectively the same issue as in
#2681).
Since all these quotes look super inaccurate they all get discarded
leaving no quote to return to the user which currently prevents us from
running the fee withdrawal script.
([log](https://production-6de61f.kb.eu-central-1.aws.cloud.es.io/app/discover#/doc/c0e240e0-d9b3-11ed-b0e6-e361adffce0b/cowlogs-prod-2024.05.14?id=_N_Od48B0pqFYaDRNuDm)
showing the messed up token diff amounts)

# Changes
When we see that the settlement contract is the trader we simply adjust
the sell and buy token diff amounts with the traded buy and sell token
amounts.
Technically this is not 100% correct. If either of the tokens has a fee
on transfer or the solver provided quote would not have sold the entire
token amounts for whatever reason this result will be inaccurate.
But since this is an extreme edge case and the super correct fix would
be way more involved I wanted to float that by you anyway.
  • Loading branch information
MartinquaXD authored and fleupold committed May 16, 2024
1 parent 813fb12 commit d61e2f8
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 19 deletions.
91 changes: 89 additions & 2 deletions crates/e2e/tests/e2e/quote_verification.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
use {
e2e::setup::{run_forked_test_with_block_number, OnchainComponents},
e2e::setup::*,
ethcontract::H160,
ethrpc::Web3,
model::order::{BuyTokenDestination, OrderKind, SellTokenSource},
model::{
order::{BuyTokenDestination, OrderKind, SellTokenSource},
quote::{OrderQuoteRequest, OrderQuoteSide, SellAmount},
},
number::nonzero::U256 as NonZeroU256,
shared::{
price_estimation::{
Expand All @@ -27,6 +30,12 @@ async fn forked_node_bypass_verification_for_rfq_quotes() {
.await;
}

#[tokio::test]
#[ignore]
async fn local_node_verified_quote_for_settlement_contract() {
run_test(verified_quote_for_settlement_contract).await;
}

/// The block number from which we will fetch state for the forked tests.
const FORK_BLOCK_MAINNET: u64 = 19796077;

Expand Down Expand Up @@ -118,3 +127,81 @@ async fn test_bypass_verification_for_rfq_quotes(web3: Web3) {
}
);
}

/// Test that asserts that we can verify quotes where the settlement contract is
/// the trader or receiver.
async fn verified_quote_for_settlement_contract(web3: Web3) {
tracing::info!("Setting up chain state.");
let mut onchain = OnchainComponents::deploy(web3).await;

let [solver] = onchain.make_solvers(to_wei(10)).await;
let [trader] = onchain.make_accounts(to_wei(3)).await;
let [token] = onchain
.deploy_tokens_with_weth_uni_v2_pools(to_wei(1_000), to_wei(1_000))
.await;

// Send 3 ETH to the settlement contract so we can get verified quotes for
// selling WETH.
onchain
.send_wei(onchain.contracts().gp_settlement.address(), to_wei(3))
.await;

tracing::info!("Starting services.");
let services = Services::new(onchain.contracts()).await;
services.start_protocol(solver.clone()).await;

let request = OrderQuoteRequest {
sell_token: onchain.contracts().weth.address(),
buy_token: token.address(),
side: OrderQuoteSide::Sell {
sell_amount: SellAmount::BeforeFee {
value: to_wei(3).try_into().unwrap(),
},
},
..Default::default()
};

// quote where settlement contract is trader and implicit receiver
let response = services
.submit_quote(&OrderQuoteRequest {
from: onchain.contracts().gp_settlement.address(),
receiver: None,
..request.clone()
})
.await
.unwrap();
assert!(response.verified);

// quote where settlement contract is trader and explicit receiver
let response = services
.submit_quote(&OrderQuoteRequest {
from: onchain.contracts().gp_settlement.address(),
receiver: Some(onchain.contracts().gp_settlement.address()),
..request.clone()
})
.await
.unwrap();
assert!(response.verified);

// quote where settlement contract is trader and not the receiver
let response = services
.submit_quote(&OrderQuoteRequest {
from: onchain.contracts().gp_settlement.address(),
receiver: Some(trader.address()),
..request.clone()
})
.await
.unwrap();
assert!(response.verified);

// quote where a random trader sends funds to the settlement contract
let response = services
.submit_quote(&OrderQuoteRequest {
from: trader.address(),
receiver: Some(onchain.contracts().gp_settlement.address()),
..request.clone()
})
.await
.unwrap();
assert!(response.verified);
}
57 changes: 40 additions & 17 deletions crates/shared/src/price_estimation/trade_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,35 @@ impl TradeVerifier {
}
};

let summary = SettleOutput::decode(&output?, query.kind)
let mut summary = SettleOutput::decode(&output?, query.kind)
.context("could not decode simulation output")
.map_err(Error::SimulationFailed)?;

{
// Quote accuracy gets determined by how many tokens had to be paid out of the
// settlement buffers to make the quote happen. When the settlement contract
// itself is the trader or receiver these values need to be adjusted slightly.
let (sell_amount, buy_amount) = match query.kind {
OrderKind::Sell => (query.in_amount.get(), summary.out_amount),
OrderKind::Buy => (summary.out_amount, query.in_amount.get()),
};

// It looks like the contract lost a lot of sell tokens but only because it was
// the trader and had to pay for the trade. Adjust tokens lost downward.
if verification.from == self.settlement.address() {
summary.sell_tokens_lost -= u256_to_big_rational(&sell_amount);
}
// It looks like the contract gained a lot of buy tokens (negative loss) but
// only because it was the receiver and got the payout. Adjust the tokens lost
// upward.
if verification.receiver == self.settlement.address() {
summary.buy_tokens_lost += u256_to_big_rational(&buy_amount);
}
}

tracing::debug!(
lost_buy_amount = %summary.buy_tokens_diff,
lost_sell_amount = %summary.sell_tokens_diff,
lost_buy_amount = %summary.buy_tokens_lost,
lost_sell_amount = %summary.sell_tokens_lost,
gas_diff = ?trade.gas_estimate.unwrap_or_default().abs_diff(summary.gas_used.as_u64()),
time = ?start.elapsed(),
promised_out_amount = ?trade.out_amount,
Expand Down Expand Up @@ -433,10 +456,10 @@ struct SettleOutput {
out_amount: U256,
/// Difference in buy tokens of the settlement contract before and after the
/// trade.
buy_tokens_diff: BigRational,
buy_tokens_lost: BigRational,
/// Difference in sell tokens of the settlement contract before and after
/// the trade.
sell_tokens_diff: BigRational,
sell_tokens_lost: BigRational,
}

impl SettleOutput {
Expand Down Expand Up @@ -469,8 +492,8 @@ impl SettleOutput {
Ok(SettleOutput {
gas_used,
out_amount,
buy_tokens_diff: settlement_buy_balance_before - settlement_buy_balance_after,
sell_tokens_diff: settlement_sell_balance_before - settlement_sell_balance_after,
buy_tokens_lost: settlement_buy_balance_before - settlement_buy_balance_after,
sell_tokens_lost: settlement_sell_balance_before - settlement_sell_balance_after,
})
}
}
Expand All @@ -489,8 +512,8 @@ fn ensure_quote_accuracy(
OrderKind::Sell => (query.in_amount.get(), summary.out_amount),
};

if summary.sell_tokens_diff >= inaccuracy_limit * u256_to_big_rational(&sell_amount)
|| summary.buy_tokens_diff >= inaccuracy_limit * u256_to_big_rational(&buy_amount)
if summary.sell_tokens_lost >= inaccuracy_limit * u256_to_big_rational(&sell_amount)
|| summary.buy_tokens_lost >= inaccuracy_limit * u256_to_big_rational(&buy_amount)
{
return Err(Error::TooInaccurate);
}
Expand Down Expand Up @@ -544,8 +567,8 @@ mod tests {
let sell_more = SettleOutput {
gas_used: 0.into(),
out_amount: 2_000.into(),
buy_tokens_diff: BigRational::from_integer(0.into()),
sell_tokens_diff: BigRational::from_integer(500.into()),
buy_tokens_lost: BigRational::from_integer(0.into()),
sell_tokens_lost: BigRational::from_integer(500.into()),
};

let estimate = ensure_quote_accuracy(&low_threshold, &query, H160::zero(), &sell_more);
Expand All @@ -558,8 +581,8 @@ mod tests {
let pay_out_more = SettleOutput {
gas_used: 0.into(),
out_amount: 2_000.into(),
buy_tokens_diff: BigRational::from_integer(1_000.into()),
sell_tokens_diff: BigRational::from_integer(0.into()),
buy_tokens_lost: BigRational::from_integer(1_000.into()),
sell_tokens_lost: BigRational::from_integer(0.into()),
};

let estimate = ensure_quote_accuracy(&low_threshold, &query, H160::zero(), &pay_out_more);
Expand All @@ -572,8 +595,8 @@ mod tests {
let sell_less = SettleOutput {
gas_used: 0.into(),
out_amount: 2_000.into(),
buy_tokens_diff: BigRational::from_integer(0.into()),
sell_tokens_diff: BigRational::from_integer((-500).into()),
buy_tokens_lost: BigRational::from_integer(0.into()),
sell_tokens_lost: BigRational::from_integer((-500).into()),
};
// Ending up with surplus in the buffers is always fine
let estimate = ensure_quote_accuracy(&low_threshold, &query, H160::zero(), &sell_less);
Expand All @@ -582,8 +605,8 @@ mod tests {
let pay_out_less = SettleOutput {
gas_used: 0.into(),
out_amount: 2_000.into(),
buy_tokens_diff: BigRational::from_integer((-1_000).into()),
sell_tokens_diff: BigRational::from_integer(0.into()),
buy_tokens_lost: BigRational::from_integer((-1_000).into()),
sell_tokens_lost: BigRational::from_integer(0.into()),
};
// Ending up with surplus in the buffers is always fine
let estimate = ensure_quote_accuracy(&low_threshold, &query, H160::zero(), &pay_out_less);
Expand Down

0 comments on commit d61e2f8

Please sign in to comment.