Encode `/quote` response JIT orders and pre-interactions #3038

squadgazzz · 2024-10-03T19:22:44Z

Description

Follow up to #3033, which uses the newly added /quote response fields with pre-interaction and JIT orders, encodes them into the settlement in order to verify the provided quote with this data.

How to test

New e2e tests:

Ensures that an order can be fulfilled using the CoW AMM liquidity only.
Tests the quote encoding logic.

squadgazzz · 2024-10-10T17:07:02Z

crates/e2e/tests/e2e/cow_amm.rs

+ // todo: figure out why this is even required.
+ dai.mint(onchain.contracts().gp_settlement.address(), to_wei(30))
+ .await;


The PR shouldn't be merged until this is resolved.

Can't reproduce the revert when the settlement contract is not funded enough using a manual rpc call with either anvil or forked tenderly node. So weird. Tried different options with/without state overrides, different block numbers. All the same: the simulation passes, but not in the test...

17292a6 (#3038) fixes this 🤷‍♂️

squadgazzz · 2024-10-11T07:40:53Z

crates/e2e/tests/e2e/cow_amm.rs

+ // todo: this should be removed by providing a surplus capturing owners list(https://github.com/cowprotocol/services/pull/3048)
+ solvers_dto::solution::Trade::Fulfillment(solvers_dto::solution::Fulfillment {
+ order: [0u8; 56],
+ executed_amount: to_wei(230) - fee_user,
+ fee: None,
+ }),


This dummy trade is required to pass the following check in order to accept the solution:

services/crates/driver/src/domain/quote.rs

Lines 122 to 131 in e1c93e9

Quote::new(

eth,

self,

// TODO(#1468): choose the best solution in the future, but for now just pick the

// first solution

solutions

.into_iter()

.find(|solution| !solution.is_empty(auction.surplus_capturing_jit_order_owners()))

.ok_or(QuotingFailed::NoSolutions)?,

)

All the regular trades are still skipped as it was before when building a quote response:

services/crates/driver/src/domain/quote.rs

Lines 39 to 86 in e1c93e9

impl Quote {

fn new(eth: &Ethereum, order: &Order, solution: competition::Solution) -> Result<Self, Error> {

let sell_price = solution

.clearing_price(order.tokens.sell)

.ok_or(QuotingFailed::ClearingSellMissing)?

.to_big_rational();

let buy_price = solution

.clearing_price(order.tokens.buy)

.ok_or(QuotingFailed::ClearingBuyMissing)?

.to_big_rational();

let order_amount = order.amount.0.to_big_rational();

let amount = match order.side {

order::Side::Sell => order_amount

.mul(sell_price)

.checked_div(&buy_price)

.context("div by zero: buy price")?,

order::Side::Buy => order_amount

.mul(&buy_price)

.checked_div(&sell_price)

.context("div by zero: sell price")?,

};

Ok(Self {

amount: eth::U256::from_big_rational(&amount)?,

pre_interactions: solution.pre_interactions().to_vec(),

interactions: solution

.interactions()

.iter()

.map(|i| encode::interaction(i, eth.contracts().settlement()))

.collect::<Result<Vec<_>, _>>()?

.into_iter()

.flatten()

.collect(),

solver: solution.solver().address(),

gas: solution.gas(),

tx_origin: *solution.solver().quote_tx_origin(),

jit_orders: solution

.trades()

.iter()

.filter_map(|trade| match trade {

solution::Trade::Jit(jit) => Some(jit.clone()),

_ => None,

})

.collect(),

})

}

}

Is this actually a dummy trade? Isn't this how a real solver would actually indicate that it wants to settle the fake order generated for the quote?

I don't really see this trade used in the quote response at all.

MartinquaXD · 2024-10-11T08:59:43Z

crates/e2e/tests/e2e/cow_amm.rs

+ onchain.contracts().weth.deposit()
+ );
+
+ // Fund the settlement contract with WETH so it can pay out the user order.


This should not be needed if the liquidity actually comes from the cow amm order.

Then, I assume there is still something that needs to be corrected since the simulation fails without this funding for both quote and settlement e2e tests.

MartinquaXD · 2024-10-11T09:05:05Z

crates/e2e/tests/e2e/cow_amm.rs

+ colocation::start_driver(
+ onchain.contracts(),
+ vec![
+ colocation::start_baseline_solver(


We are not using the regular baseline solver for anything anymore, right? Then this can be removed and the comment above be amended.

It is used for native price fetching.

services/crates/e2e/src/setup/services.rs

Lines 107 to 109 in af9d499

fn api_autopilot_arguments() -> impl Iterator<Item = String> {

[

"--native-price-estimators=test_quoter|http://localhost:11088/test_solver".to_string(),

MartinquaXD · 2024-10-11T09:08:57Z

crates/e2e/tests/e2e/cow_amm.rs

+ &onchain.contracts().domain_separator,
+ SecretKeyRef::from(&SecretKey::from_slice(bob.private_key()).unwrap()),
+ );
+ let user_order_id = services.create_order(&user_order).await.unwrap();


To check that the quote actually gets verified it would be good to first get a quote from the API and assert that it has verified: true. Then you can call create_order() and pass the generated quote id.
Should effectively not change anything but it would make it clear in the test that checking that the quote was verified is important.

I forgot to mention that in the description. I wanted to merge these two e2e tests into a single one, but that also requires changes in the mocked solver. The PR is already huge, maybe it makes sense to accomplish this as a follow-up.

crates/e2e/tests/e2e/cow_amm.rs

MartinquaXD · 2024-10-11T09:17:06Z

crates/e2e/tests/e2e/cow_amm.rs

+/// Tests quotes verification that contains CoW AMM JIT orders.
+async fn cow_amm_quoting(web3: Web3) {


I see there is a separate test for quoting specifically now. Can this not be merged with the other new test?

#3038 (comment)

MartinquaXD · 2024-10-11T09:30:16Z

crates/shared/src/price_estimation/trade_verifier.rs

+ dto::Side::Buy => OrderKind::Buy,
+ dto::Side::Sell => OrderKind::Sell,
+ },
+ partially_fillable: false,


This is a slightly problematic detail I didn't consider before. The JIT order API for the /settle call doesn't allow for partially fillable orders because that assumes only truly new JIT orders being proposed there and for those fill-or-kill should be sufficient.
But for the /quote endpoint the JIT orders returned could also be user orders (masked as JIT orders to not introduce even more enums). So if a solver would like to return a quote based on a partially fillable user order in the book this would not work here.
I think the API needs to allow sending the partially_fillable flag with the JIT orders.

That means, the solvers API needs to be also changed since now it doesn't contain this field in the JitOrder json, right?

services/crates/driver/src/infra/solver/dto/solution.rs

Lines 278 to 298 in 39b3f5b

#[serde_as]

#[derive(Debug, Deserialize)]

#[serde(rename_all = "camelCase", deny_unknown_fields)]

struct JitOrder {

sell_token: eth::H160,

buy_token: eth::H160,

receiver: eth::H160,

#[serde_as(as = "serialize::U256")]

sell_amount: eth::U256,

#[serde_as(as = "serialize::U256")]

buy_amount: eth::U256,

valid_to: u32,

#[serde_as(as = "serialize::Hex")]

app_data: [u8; order::APP_DATA_LEN],

kind: Kind,

sell_token_balance: SellTokenBalance,

buy_token_balance: BuyTokenBalance,

signing_scheme: SigningScheme,

#[serde_as(as = "serialize::Hex")]

signature: Vec<u8>,

}

It was intended like this in the /solve API but I guess it makes sense to keep these 2 APIs the same.

crates/shared/src/price_estimation/trade_verifier.rs

MartinquaXD · 2024-10-11T09:40:07Z

crates/shared/src/price_estimation/trade_verifier.rs

+ clearing_prices.push(jit_order.buy_amount);
+ clearing_prices.push(jit_order.executed_amount);


@harisang in this PR we are testing that quotes can be settled (and verified) against JIT orders. We only introduced the concept of JIT orders in the /quote response because solvers could also settled against user orders by indexing the orderbook and pretending the user order is a JIT order.
In this particular part this would cause JIT orders to be settled at the limit price in the settlement. So if a solver returns a regular user order as a JIT order which would not reflect what would happen in a real settlement entirely correctly. I assumed this should be fine but I actually never asked the solver team about it.
Do you foresee any significant issues from this detail? Asking because we'd otherwise have to be able to know which JIT orders are actually surplus capturing in the quote verification which would be a huge PITA.

In the competition encoding, executed amount is not used here:

services/crates/driver/src/domain/competition/solution/encoding.rs

Lines 116 to 125 in 559ebf3

super::Trade::Jit(trade) => {

(

Price {

// Jit orders are matched at limit price, so the sell token is worth

// buy.amount and vice versa

sell_token: trade.order().sell.token.into(),

sell_price: trade.order().buy.amount.into(),

buy_token: trade.order().buy.token.into(),

buy_price: trade.order().sell.amount.into(),

},

Maybe that is why the e2e test requires settlement contract to be funded with DAI:

services/crates/e2e/tests/e2e/cow_amm.rs

Lines 1207 to 1209 in 8fd8cf6

// todo: figure out why this is even required.

dai.mint(onchain.contracts().gp_settlement.address(), to_wei(30))

.await;

But when using plain sell/buy amounts, the simulation gets reverted with a vague error, so I couldn't understand what exactly is wrong with that case.

MartinquaXD · 2024-10-11T09:41:41Z

crates/shared/src/trade_finding/external.rs

- #[serde_as(as = "BytesHex")]
- pub app_data: Vec<u8>,


What's the reason for this change? The original version is more correct, no?

The reason is that the AppDataHash needs to be constructed later here:

services/crates/shared/src/price_estimation/trade_verifier.rs

Line 427 in 0617f4f

app_data: AppDataHash::from_str(&jit_order.app_data)?,

Which means that the Vec needs to be encoded back to String and only then to the AppDataHash. The change avoids back and forth decoding/encoding of the original String from the response.

MartinquaXD · 2024-10-11T09:44:21Z

crates/e2e/tests/e2e/cow_amm.rs

+ // todo: this should be removed by providing a surplus capturing owners list(https://github.com/cowprotocol/services/pull/3048)
+ solvers_dto::solution::Trade::Fulfillment(solvers_dto::solution::Fulfillment {
+ order: [0u8; 56],
+ executed_amount: to_wei(230) - fee_user,
+ fee: None,
+ }),


Is this actually a dummy trade? Isn't this how a real solver would actually indicate that it wants to settle the fake order generated for the quote?

crates/driver/src/domain/competition/solution/encoding.rs

crates/driver/src/infra/api/routes/quote/dto/quote.rs

m-lord-renkse · 2024-10-14T09:25:43Z

crates/e2e/tests/e2e/cow_amm.rs

+
+ // Compensate a delay between the `CurrentBlockStream` and the actual onchain
+ // data.
+ tokio::time::sleep(tokio::time::Duration::from_secs(2)).await;


I don't fully understand why these sleeps are needed 🤔

Autopilot receives onchain data using CurrentBlockStream, which has a delay between the actual onchain operations making the test fail since autopilot doesn't get informed about all the operations above.

I see, is there a way we can check this by DB query polling? mostly asking because it could be faster/more precise

squadgazzz added 2 commits October 3, 2024 20:11

Encode jit orders

a2e6719

Use DTO directly

3b245a3

squadgazzz force-pushed the 3027/encode-jit-orders branch 2 times, most recently from 89b9a9d to 8ab3ebb Compare October 4, 2024 11:52

e2e test

1a3a3b8

squadgazzz force-pushed the 3027/encode-jit-orders branch from 8ab3ebb to 1a3a3b8 Compare October 4, 2024 11:53

squadgazzz added 3 commits October 4, 2024 17:15

Fix

6c7b454

Fix test

5809401

Attempt to the fix

559ebf3

squadgazzz force-pushed the 3027/encode-jit-orders branch from 56f5358 to 559ebf3 Compare October 8, 2024 16:57

squadgazzz added 3 commits October 8, 2024 18:30

Same signatures

04c9b5c

Encoding fix

0617f4f

PR reference

6882a55

squadgazzz mentioned this pull request Oct 10, 2024

Driver: Include non-surplus-capturing-jit-orders in the driver /solve #3048

Open

squadgazzz marked this pull request as ready for review October 10, 2024 16:52

squadgazzz requested a review from a team as a code owner October 10, 2024 16:52

Comments

8fd8cf6

squadgazzz commented Oct 10, 2024

View reviewed changes

squadgazzz commented Oct 11, 2024

View reviewed changes

MartinquaXD reviewed Oct 11, 2024

View reviewed changes

squadgazzz added 2 commits October 11, 2024 12:05

Do not panic

0975fc5

Fix AMM balance check

240bba3

m-lord-renkse reviewed Oct 14, 2024

View reviewed changes

crates/driver/src/domain/competition/solution/encoding.rs Show resolved Hide resolved

m-lord-renkse reviewed Oct 14, 2024

View reviewed changes

crates/driver/src/infra/api/routes/quote/dto/quote.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed Oct 14, 2024

View reviewed changes

squadgazzz added 2 commits October 14, 2024 11:48

Minor

0fc1416

Weird fix

17292a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode `/quote` response JIT orders and pre-interactions #3038

Encode `/quote` response JIT orders and pre-interactions #3038

squadgazzz commented Oct 3, 2024 •

edited

Loading

squadgazzz Oct 10, 2024

squadgazzz Oct 10, 2024

squadgazzz Oct 14, 2024

squadgazzz Oct 11, 2024

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024

squadgazzz Oct 11, 2024

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024 •

edited

Loading

MartinquaXD Oct 14, 2024

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024 •

edited

Loading

MartinquaXD Oct 11, 2024

squadgazzz Oct 11, 2024 •

edited

Loading

MartinquaXD Oct 11, 2024

m-lord-renkse Oct 14, 2024

squadgazzz Oct 14, 2024

m-lord-renkse Oct 14, 2024

	Quote::new(
	eth,
	self,
	// TODO(#1468): choose the best solution in the future, but for now just pick the
	// first solution
	solutions
	.into_iter()
	.find(\|solution\| !solution.is_empty(auction.surplus_capturing_jit_order_owners()))
	.ok_or(QuotingFailed::NoSolutions)?,
	)

	impl Quote {
	fn new(eth: &Ethereum, order: &Order, solution: competition::Solution) -> Result<Self, Error> {
	let sell_price = solution
	.clearing_price(order.tokens.sell)
	.ok_or(QuotingFailed::ClearingSellMissing)?
	.to_big_rational();
	let buy_price = solution
	.clearing_price(order.tokens.buy)
	.ok_or(QuotingFailed::ClearingBuyMissing)?
	.to_big_rational();
	let order_amount = order.amount.0.to_big_rational();

	let amount = match order.side {
	order::Side::Sell => order_amount
	.mul(sell_price)
	.checked_div(&buy_price)
	.context("div by zero: buy price")?,
	order::Side::Buy => order_amount
	.mul(&buy_price)
	.checked_div(&sell_price)
	.context("div by zero: sell price")?,
	};

	Ok(Self {
	amount: eth::U256::from_big_rational(&amount)?,
	pre_interactions: solution.pre_interactions().to_vec(),
	interactions: solution
	.interactions()
	.iter()
	.map(\|i\| encode::interaction(i, eth.contracts().settlement()))
	.collect::<Result<Vec<_>, _>>()?
	.into_iter()
	.flatten()
	.collect(),
	solver: solution.solver().address(),
	gas: solution.gas(),
	tx_origin: *solution.solver().quote_tx_origin(),
	jit_orders: solution
	.trades()
	.iter()
	.filter_map(\|trade\| match trade {
	solution::Trade::Jit(jit) => Some(jit.clone()),
	_ => None,
	})
	.collect(),
	})
	}
	}

	fn api_autopilot_arguments() -> impl Iterator<Item = String> {
	[
	"--native-price-estimators=test_quoter\|http://localhost:11088/test_solver".to_string(),

		/// Tests quotes verification that contains CoW AMM JIT orders.
		async fn cow_amm_quoting(web3: Web3) {

	#[serde_as]
	#[derive(Debug, Deserialize)]
	#[serde(rename_all = "camelCase", deny_unknown_fields)]
	struct JitOrder {
	sell_token: eth::H160,
	buy_token: eth::H160,
	receiver: eth::H160,
	#[serde_as(as = "serialize::U256")]
	sell_amount: eth::U256,
	#[serde_as(as = "serialize::U256")]
	buy_amount: eth::U256,
	valid_to: u32,
	#[serde_as(as = "serialize::Hex")]
	app_data: [u8; order::APP_DATA_LEN],
	kind: Kind,
	sell_token_balance: SellTokenBalance,
	buy_token_balance: BuyTokenBalance,
	signing_scheme: SigningScheme,
	#[serde_as(as = "serialize::Hex")]
	signature: Vec<u8>,
	}

		clearing_prices.push(jit_order.buy_amount);
		clearing_prices.push(jit_order.executed_amount);

	super::Trade::Jit(trade) => {
	(
	Price {
	// Jit orders are matched at limit price, so the sell token is worth
	// buy.amount and vice versa
	sell_token: trade.order().sell.token.into(),
	sell_price: trade.order().buy.amount.into(),
	buy_token: trade.order().buy.token.into(),
	buy_price: trade.order().sell.amount.into(),
	},

	// todo: figure out why this is even required.
	dai.mint(onchain.contracts().gp_settlement.address(), to_wei(30))
	.await;

Encode /quote response JIT orders and pre-interactions #3038

Are you sure you want to change the base?

Encode /quote response JIT orders and pre-interactions #3038

Conversation

squadgazzz commented Oct 3, 2024 • edited Loading

Description

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Encode `/quote` response JIT orders and pre-interactions #3038

Encode `/quote` response JIT orders and pre-interactions #3038

squadgazzz commented Oct 3, 2024 •

edited

Loading

squadgazzz Oct 11, 2024 •

edited

Loading

squadgazzz Oct 11, 2024 •

edited

Loading

squadgazzz Oct 11, 2024 •

edited

Loading