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

Relax trade verification checks for pre-interactions #3081

Merged
merged 6 commits into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/contracts/artifacts/Solver.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion crates/contracts/artifacts/Trader.json

Large diffs are not rendered by default.

28 changes: 24 additions & 4 deletions crates/contracts/solidity/Solver.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ contract Solver {
/// @param nativeToken - ERC20 version of the chain's token
/// @param receiver - address receiving the bought tokens
/// @param settlementCall - the calldata of the `settle()` call
/// @param mockPreconditions - controls whether things like ETH wrapping
/// or setting allowance should be done on behalf of the
/// user to support quote verification even if the user didn't
/// wrap their ETH or set the necessary allowances yet.
///
/// @return gasUsed - gas used for the `settle()` call
/// @return queriedBalances - list of balances stored during the simulation
Expand All @@ -46,19 +50,35 @@ contract Solver {
address buyToken,
address nativeToken,
address payable receiver,
bytes calldata settlementCall
bytes calldata settlementCall,
bool mockPreconditions
) external returns (
uint256 gasUsed,
uint256[] memory queriedBalances
) {
require(msg.sender == address(this), "only simulation logic is allowed to call 'swap' function");

// Prepare the trade in the context of the trader so we are allowed
// to set approvals and things like that.
Trader(trader).prepareSwap(settlementContract, sellToken, sellAmount, nativeToken, receiver);
if (mockPreconditions) {
Trader(trader)
.prepareSwap(
settlementContract,
sellToken,
sellAmount,
nativeToken
);
}

// Warm the storage for sending ETH to smart contract addresses.
// We allow this call to revert becaues it was either unnecessary in the first place
// or failing to send `ETH` to the `receiver` will cause a revert in the settlement
// contract.
receiver.call{value: 0}("");
Comment on lines +73 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it moved here just as a part of refactoring, or is it essential for the changes to work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having to warm the storage for sending ETH to a smart contract wallet is a very annoying implementation detail the user shouldn't have to worry about. This part should always happen and should not lead to reverts for any reasonable smart contract receiver.
And since the prepareSwap() step is now optional depending on the existence of pre-interactions I moved it out of there. Also the context of this doesn't matter (i.e. whether the trader or the solver sends it).


this.storeBalance(sellToken, address(settlementContract), false);
this.storeBalance(buyToken, address(settlementContract), false);
uint256 gasStart = gasleft();
// TODO can we assume the overhead of this function call to be negligible due to inlining?
address(settlementContract).doCall(settlementCall);
gasUsed = gasStart - gasleft() - _simulationOverhead;
this.storeBalance(sellToken, address(settlementContract), false);
Expand All @@ -69,7 +89,7 @@ contract Solver {
/// @dev Helper function that reads the `owner`s balance for a given `token` and
/// stores it. These stored balances will be returned as part of the simulation
/// `Summary`.
/// @param token - which token's we read the balance from
/// @param token - which token we read the balance from
/// @param owner - whos balance we are reading
/// @param countGas - controls whether this gas cost should be discounted from the settlement gas.
function storeBalance(address token, address owner, bool countGas) external {
Expand Down
14 changes: 3 additions & 11 deletions crates/contracts/solidity/Trader.sol
Original file line number Diff line number Diff line change
Expand Up @@ -61,21 +61,18 @@ contract Trader {
// settlement contract anyway.
receive() external payable {}

/// @dev Prepares everything needed by the trader for successfully executing the swap.
/// This includes giving the required approval, wrapping the required ETH (if needed)
/// and warming the needed storage for sending native ETH to smart contracts.
/// @dev Executes needed actions on behalf of the trader to make the trade possible.
/// (e.g. wrapping ETH and setting approvals)
/// @param settlementContract - pass in settlement contract because it does not have
/// a stable address in tests.
/// @param sellToken - token being sold by the trade
/// @param sellAmount - expected amount to be sold according to the quote
/// @param nativeToken - ERC20 version of the chain's native token
/// @param receiver - address that will receive the bought tokens
function prepareSwap(
ISettlement settlementContract,
address sellToken,
uint256 sellAmount,
address nativeToken,
address payable receiver
address nativeToken
) external {
require(!alreadyCalled(), "prepareSwap can only be called once");

Expand Down Expand Up @@ -105,11 +102,6 @@ contract Trader {

uint256 availableSellToken = IERC20(sellToken).balanceOf(address(this));
require(availableSellToken >= sellAmount, "trader does not have enough sell_token");
// Warm the storage for sending ETH to smart contract addresses.
// We allow this call to revert becaues it was either unnecessary in the first place
// or failing to send `ETH` to the `receiver` will cause a revert in the settlement
// contract.
receiver.call{value: 0}("");
}

/// @dev Validate all signature requests. This makes "signing" CoW protocol
Expand Down
121 changes: 120 additions & 1 deletion crates/e2e/tests/e2e/hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ use {
ethcontract::{Bytes, H160, U256},
model::{
order::{OrderCreation, OrderCreationAppData, OrderKind},
quote::{OrderQuoteRequest, OrderQuoteSide, SellAmount},
signature::{hashed_eip712_message, EcdsaSigningScheme, Signature},
},
number::nonzero::U256 as NonZeroU256,
reqwest::StatusCode,
secp256k1::SecretKey,
serde_json::json,
Expand Down Expand Up @@ -42,6 +44,12 @@ async fn local_node_gas_limit() {
run_test(gas_limit).await;
}

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

async fn gas_limit(web3: Web3) {
let mut onchain = OnchainComponents::deploy(web3).await;

Expand Down Expand Up @@ -304,8 +312,14 @@ async fn signature(web3: Web3) {
// Place Orders
let mut order = OrderCreation {
from: Some(safe.address()),
// Quotes for trades where the pre-interactions deploy a contract
// at the `from` address currently can't be verified.
// To not throw an error because we can't get a verifiable quote
// we make the order partially fillable and sell slightly more than
// `from` currently has.
sell_amount: to_wei(6),
partially_fillable: true,
sell_token: token.address(),
sell_amount: to_wei(5),
buy_token: onchain.contracts().weth.address(),
buy_amount: to_wei(3),
valid_to: model::time::now_in_epoch_seconds() + 300,
Expand Down Expand Up @@ -469,3 +483,108 @@ async fn partial_fills(web3: Web3) {
2.into()
);
}

/// Checks that quotes can be verified which need the pre-hooks
/// to run before the requested trade could be executed.
async fn quote_verification(web3: Web3) {
let mut onchain = OnchainComponents::deploy(web3.clone()).await;

let chain_id = web3.eth().chain_id().await.unwrap();

let [trader] = onchain.make_accounts(to_wei(1)).await;
let [solver] = onchain.make_solvers(to_wei(1)).await;

let safe_infra = safe::Infrastructure::new(&web3).await;

// Prepare the Safe creation transaction, but don't execute it! This will
// be executed as a pre-hook.
let safe_creation_builder = safe_infra.factory.create_proxy(
safe_infra.singleton.address(),
ethcontract::Bytes(
safe_infra
.singleton
.setup(
vec![trader.address()], // owners
1.into(), // threshold
H160::default(), // delegate call
Bytes::default(), // delegate call bytes
safe_infra.fallback.address(),
H160::default(), // relayer payment token
0.into(), // relayer payment amount
H160::default(), // relayer address
)
.tx
.data
.unwrap()
.0,
),
);
let safe_address = safe_creation_builder.clone().view().call().await.unwrap();
safe_creation_builder.clone().send().await.unwrap();

let safe = Safe::deployed(
chain_id,
GnosisSafe::at(&web3, safe_address),
trader.clone(),
);

let [token] = onchain
.deploy_tokens_with_weth_uni_v2_pools(to_wei(100_000), to_wei(100_000))
.await;
token.mint(safe.address(), to_wei(5)).await;
tx!(
trader.account(),
token.approve(onchain.contracts().allowance, to_wei(5))
);

// Sign transaction transfering 5 token from the safe to the trader
// to fund the trade in a pre-hook.
let transfer_builder = safe.sign_transaction(
token.address(),
token
.transfer(trader.address(), to_wei(5))
.tx
.data
.unwrap()
.0,
0.into(),
);
let transfer = Hook {
target: transfer_builder.tx.to.unwrap(),
call_data: transfer_builder.tx.data.unwrap().0,
gas_limit: 100_000,
};

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

let quote = services
.submit_quote(&OrderQuoteRequest {
from: trader.address(),
sell_token: token.address(),
buy_token: onchain.contracts().weth.address(),
side: OrderQuoteSide::Sell {
sell_amount: SellAmount::BeforeFee {
value: NonZeroU256::try_from(to_wei(5)).unwrap(),
},
},
app_data: OrderCreationAppData::Full {
full: json!({
"metadata": {
"hooks": {
"pre": [transfer],
},
},
})
.to_string(),
},
..Default::default()
})
.await
.unwrap();

// quote can be verified although the trader only get the necessary
// sell tokens with a pre-hook
assert!(quote.verified);
}
5 changes: 5 additions & 0 deletions crates/shared/src/price_estimation/trade_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,11 @@ impl TradeVerifier {
self.native_token,
verification.receiver,
Bytes(settlement.data.unwrap().0),
// only if the user did not provide pre-interactions is it safe
// to set up the trade's pre-conditions on behalf of the user.
// if the user provided pre-interactions it's reasonable to assume
// that they will set up all the necessary details for the trade.
verification.pre_interactions.is_empty(),
)
.tx;

Expand Down
Loading