Protocol fee e2e tests clean-up #2561

squadgazzz · 2024-03-22T16:51:46Z

Description

Removes exhaustive protocol fee e2e tests since tests implemented in the driver crate cover many other cases. Each protocol fee now has a single e2e test.

Fixes #2468

fleupold · 2024-03-26T15:07:01Z

crates/e2e/tests/e2e/protocol_fee.rs

-    .await;
-}
-
-async fn partner_fee_with_not_default_cap_sell_order_test(web3: Web3) {


Is it intended that this fee still has a fee policy for market orders enabled?

Partner fee should replace any configured protocol fee. Why not keep the Market fee here?

Partner fee should replace any configured protocol fee.

Well, not really. It's just an implementation detail right now that it does. I'd hope that we can support multiple fee policies very soon so I'm not sure we want to explicitly test for this yet (but also no strong feeling).

We have this task for it: #2495, hopefully we will implement it soon

fleupold · 2024-03-26T15:08:20Z

crates/e2e/tests/e2e/protocol_fee.rs

+    let protocol_fees = vec![
+        ProtocolFee {
+            policy: fee_policy.clone(),
+            policy_order_class: FeePolicyOrderClass::Market,
+        },
+        // Tests ability of providing multiple fees in the config. In fact, the Market one is used
+        // only since the order is in-market.
+        ProtocolFee {
+            policy: fee_policy,
+            policy_order_class: FeePolicyOrderClass::Limit,
+        },
+    ];


How about making a single test case that contains a limit order with surplus fee and a market order with price improvement fee (and potentially a third order with partner fee) in a batch to make sure all fee models that are used in practice work as expected?

I tried to configure out-of-market order and couldn't make it work yet. Assuming it is more involved change, wanted to tackle it in scope of the #2468 as a separate PR(probably, after the market order fees release and 0x liquidity)

Also, not sure how to keep this assertion with a single batch of multiple orders. Maybe execute them sequentially.

Also, not sure how to keep this assertion with a single batch of multiple orders

How about using different tokens for the different orders?

I tried to configure out-of-market order and couldn't make it work yet.

The easiest way to make an out of market order "work" IMO is to create an AMM with balances A and B, get a quote and place an order this is "out of market" given that quote. Then you send a ton of A into the AMM (making the price change and the order now being executable).

squadgazzz · 2024-03-28T16:24:46Z

crates/e2e/tests/e2e/protocol_fee.rs

-async fn local_node_partner_fee_with_not_default_cap_sell_order_test() {
-    run_test(partner_fee_with_not_default_cap_sell_order_test).await;
-}
+    let mut onchain = OnchainComponents::deploy(web3.clone()).await;


GitHub shows a pretty ugly and misleading diff 🙈

fleupold

Generally I think the test looks good, but let's please get rid of those magic numbers at the bottom and use more meaningful assertions that relate to the actual logic we are testing.

fleupold · 2024-03-28T17:39:20Z

crates/e2e/tests/e2e/protocol_fee.rs

+        ])
+        .await;
+
+    let market_price_improvement_order = OrderCreation {


How do I know this is a market order? I think it would be nice to quote this order and then use limits that are worse than the quote.

Used a slightly decreased value from the quote.

fleupold · 2024-03-28T17:42:05Z

crates/e2e/tests/e2e/protocol_fee.rs

+    tracing::info!("Waiting for liquidity state to update");
+    wait_for_condition(TIMEOUT, || async {
+        // Mint blocks until we evict the cached liquidity and fetch the new state.
+        onchain.mint_block().await;
+        let next = services.submit_quote(&quote_request).await.unwrap();
+        next.quote.buy_amount != quote.quote.buy_amount
+    })
+    .await
+    .unwrap();
+
+    tracing::info!("Waiting for trade.");
+    wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 3 })
+        .await
+        .unwrap();
+
+    wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 0 })
+        .await
+        .unwrap();
+


Why are all of these waits important? Couldn't we just wait for the limit order to trade? This could make this section shorter to read.

I suspect specifically the auction_size == 3 condition is there to make a batch happen. But couldn't this result in a flaky test? For example is it guaranteed that none of the other 2 orders can be settled while we wait for the rebalancing to happen. I believe to make this bullet proof we might have to have a driver endpoint used for quoting and a driver endpoint for solving and only enable the solving endpoint after the auction has the expected size.

Since starting the driver's solving endpoint separately isn't straightforward, replaced with the following assertion, which should probably wait until all the orders are executed and filled: 54507fc

fleupold · 2024-03-28T17:43:03Z

crates/e2e/tests/e2e/protocol_fee.rs

+        .unwrap();
+    assert_approximately(
+        market_price_improvement.metadata.executed_surplus_fee,
+        202972993230628u128.into(),


Are we actually seeing any price improvement here? Nothing about the pool changed, so this order should be executed as expected. I think we should:

Get a quote for the trade and use that as limit price (expected price)

Place the order

Inject more liquidity into the pool

Get another quote (now with expected price improvement).

Assert that executed_surplus_fee > 1/2 of the difference in the two quotes

fleupold · 2024-03-28T17:46:36Z

crates/e2e/tests/e2e/protocol_fee.rs

+        .call()
+        .await
+        .unwrap();
+    assert_approximately(balance_after, 66641691187129028u128.into());


I think instead of using a magic number here (which we likely computed by running the test once and then blindly accepting its outcome as the right value to assert), I think a more robust test would be to assert that balance_after is > 2% of the sell amount.

This would give a more meaningful and robust assertion.

# Conflicts: # crates/e2e/tests/e2e/protocol_fee.rs

fleupold

Assertions look good. I think given the size of this test, the PR would benefit from doing another "clean-up" pass, where we try to minimize code that is not strictly necessary and put ourselves into the shoes of a future maintainer that has to understand what this test does in 6 months from now (adding more context and comments).

I commented with some suggestions in line.

fleupold · 2024-04-02T08:36:15Z

crates/e2e/tests/e2e/protocol_fee.rs

+    let services = Services::new(onchain.contracts()).await;
+    let solver_endpoint =
+        colocation::start_baseline_solver(onchain.contracts().weth.address()).await;
+    colocation::start_driver(
+        onchain.contracts(),
+        vec![SolverEngine {
+            name: "test_solver".into(),
+            account: solver.clone(),
+            endpoint: solver_endpoint,
+        }],
+    );
+    services
+        .start_api(vec![
+            "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver".to_string(),
+        ])
+        .await;


Any reason we cannot use start_protocol_with_args (shorter)?

If we want to batch all the orders, autopilot must be started after all the orders are created.

Is there anything about the batching that is relevant to this test though?

fleupold · 2024-04-02T08:37:08Z

crates/e2e/tests/e2e/protocol_fee.rs

-        vec![ProtocolFeesConfig(vec![protocol_fee])],
+    .await
+    .unwrap();
+    let partner_fee_quote_before = get_quote(


Does partner fee need a quote?

Probably, not. Removed as well as the AMM pool rebalancing for the partner fee order.

crates/e2e/tests/e2e/protocol_fee.rs

fleupold · 2024-04-02T08:38:26Z

crates/e2e/tests/e2e/protocol_fee.rs

+    config.extend(autopilot_config);
+    services.start_autopilot(None, config);
+
+    tracing::info!("Rebalancing limit order AMM pool.");


Can we not rebalance all pools in one go?

What do you mean by that?

Right now you have two blocks of tracing::info!("Rebalancing ..."); (line ~200 and line ~255). I think it would be cleaner to do all AMM rebalancing right after one another.

crates/e2e/tests/e2e/protocol_fee.rs

fleupold · 2024-04-02T08:41:03Z

crates/e2e/tests/e2e/protocol_fee.rs

+    let market_quote_diff = market_quote_after
+        .buy_amount
+        .saturating_sub(market_quote_before.quote.buy_amount);
+    assert!(market_executed_surplus_fee_in_buy_token >= market_quote_diff * 3 / 10);


Why (* 3 / 10). Can you either comment this for future maintainer to know where this value comes from without having to read 300 lines of code or factor this in a variable that is shared between the config and here?

Added comments.

fleupold · 2024-04-02T08:42:07Z

crates/e2e/tests/e2e/protocol_fee.rs

+        market_price_improvement_order.metadata.executed_surplus_fee
+            * market_quote_after.buy_amount
+            / market_quote_after.sell_amount;


nit: maybe factor into a method that can be reused.

fleupold · 2024-04-02T08:46:44Z

crates/e2e/tests/e2e/protocol_fee.rs

+            / partner_fee_quote_after.sell_amount;
+    assert!(
+        partner_fee_executed_surplus_fee_in_buy_token
+            >= partner_fee_quote_after.buy_amount * 2 / 100


If someone changes the fee policy above this test may pass because there is no upper bound check. Can we somehow approximate this?

The difference here comes from the network fee, which is computed by the baseline solver. Not sure it is possible to predict the value based on the given data. The value can be hardcoded. Wdyt?

After reading the test again, I think the fact that we exactly assert on the settlement balances ensures that even if we drastically overcomputed the fee, the test would fail, so I think it's fine.

MartinquaXD

Code can probably be condensed a lot.

MartinquaXD · 2024-04-02T09:23:25Z

crates/e2e/tests/e2e/protocol_fee.rs

-async fn local_node_volume_fee_buy_order() {
-    run_test(volume_fee_buy_order_test).await;
-}
+    limit_order_token.mint(solver.address(), to_wei(1000)).await;


Probably slightly nicer to do sth like:

for token in [limit_order_token, market_order_token, partner_fee_order_token] { // mint token to solver address // approve router contract }

Thx, refactored.

MartinquaXD · 2024-04-02T09:30:26Z

crates/e2e/tests/e2e/protocol_fee.rs

+    let market_price_improvement_order = OrderCreation {
+        sell_token: onchain.contracts().weth.address(),
+        sell_amount,
+        buy_token: market_order_token.address(),


nit: Since we already have the quote it seems more error resistant to re-use the values from the quote whenever possible.
I.e. buy_token: market_quote_before.quote.buy_token or maybe even better sth like:

OrderCreation { buy_amount: market_quote_before.quote.buy_amount * 2 / 3, ..order_from_quote(market_quote_before) }

That way it's absolutely clear that this order was created from a quote and explicitly overwrites a single value to make it work for the test.

Also since we have fixed size arrays we can probably do sth like:

let [market_quote, limit_quote, _parter_fee_quote] = [market_token, limit_token, partner_fee_token].map(|token| /*fetch quote*/);

MartinquaXD · 2024-04-02T09:39:13Z

crates/e2e/tests/e2e/protocol_fee.rs

+        .mint_token_to_weth_uni_v2_pool(&limit_order_token, to_wei(1000))
+        .await;
+
+    tracing::info!("Waiting for liquidity state to update");


Don't we still have the (maybe only theoretical) race condition that we start the autopilot before all liquidity pools are configured such that orders can be settled right away?
If so we don't have any guarantees that these orders actually get settled in the same batch like the test intends. Or am I missing something?

Looks like Felix beat me, and now his commit addresses this.

MartinquaXD · 2024-04-02T09:39:28Z

crates/e2e/tests/e2e/protocol_fee.rs

+    tracing::info!("Waiting for orders metadata update.");
+    let metadata_updated = || async {
+        onchain.mint_block().await;
+        let market_order_updated = services


Can also use an iterator.

MartinquaXD · 2024-04-02T09:40:28Z

crates/e2e/tests/e2e/protocol_fee.rs

+        .saturating_sub(limit_surplus_order.data.buy_amount);
+    assert!(limit_executed_surplus_fee_in_buy_token >= limit_quote_diff * 3 / 10);
+
+    let balance_after = market_order_token


Could also use iterators.

While iterators really help with preparing the data, it will be painful to find out which assertion fails.

Yeah, I only meant to fetch the balances in a loop.

# Conflicts: # crates/e2e/tests/e2e/protocol_fee.rs

fleupold

LGTM (I pushed a minor cleanup on the latest PR)

fleupold

Protocol fee e2e tests clean-up

fc910f3

squadgazzz force-pushed the 2468/protocol-fee-e2e-tests-cleanup branch from 4753ef3 to fc910f3 Compare March 26, 2024 10:26

squadgazzz changed the title ~~WIP: Protocol fee e2e refactoring~~ Protocol fee e2e tests clean-up Mar 26, 2024

squadgazzz marked this pull request as ready for review March 26, 2024 10:33

squadgazzz requested a review from a team as a code owner March 26, 2024 10:33

fleupold reviewed Mar 26, 2024

View reviewed changes

m-lord-renkse approved these changes Mar 27, 2024

View reviewed changes

squadgazzz marked this pull request as draft March 28, 2024 08:54

squadgazzz added 4 commits March 28, 2024 15:23

Combined protocol fees e2e test

ce3090c

Adjusted values

4649e97

Drop redundant tests

fa0df9a

Better debugging with a macro

f9a000e

squadgazzz marked this pull request as ready for review March 28, 2024 15:58

squadgazzz commented Mar 28, 2024

View reviewed changes

fleupold reviewed Mar 28, 2024

View reviewed changes

squadgazzz mentioned this pull request Mar 28, 2024

Fix quote fee handling in market price #2572

Merged

squadgazzz marked this pull request as draft March 29, 2024 10:24

squadgazzz added 9 commits March 29, 2024 10:36

Compare executed surplus fee in buy token with quotes diff

dcea630

Fixed quote values

819fb58

Merge branch 'main' into 2468/protocol-fee-e2e-tests-cleanup

6787db5

# Conflicts: # crates/e2e/tests/e2e/protocol_fee.rs

Enable assertion

3f45632

More assertions

e04daf1

More assertions

e4e6b63

Reduce magic numbers

4d4a230

Fixed assertion

77429f2

Contract balances check

ccdc534

squadgazzz marked this pull request as ready for review March 29, 2024 17:10

squadgazzz added 3 commits March 29, 2024 18:50

More stable assertion

54507fc

Fixed assertion

7fb89b1

Merge branch 'main' into 2468/protocol-fee-e2e-tests-cleanup

1ee7eaa

Refactor the remaining test

722a941

squadgazzz requested a review from m-lord-renkse April 1, 2024 10:56

fleupold reviewed Apr 2, 2024

View reviewed changes

MartinquaXD reviewed Apr 2, 2024

View reviewed changes

squadgazzz and others added 9 commits April 2, 2024 18:44

Merge branch 'main' into 2468/protocol-fee-e2e-tests-cleanup

1baf9cb

# Conflicts: # crates/e2e/tests/e2e/protocol_fee.rs

Fix after merge

de0e326

Remove partner AMM rebalancing

076473b

Logs

0dccc9f

Comments

2170999

surplus_fee_in_buy_token

1ae6789

Minor refactoring

023ab56

Iterators

8c398fc

💄

f74fa26

fleupold approved these changes Apr 3, 2024

View reviewed changes

Simplify startup config

4709659

squadgazzz requested review from MartinquaXD and fleupold April 3, 2024 15:51

squadgazzz mentioned this pull request Apr 3, 2024

Implement 0x liquidity #2218

Merged

fleupold approved these changes Apr 3, 2024

View reviewed changes

MartinquaXD approved these changes Apr 4, 2024

View reviewed changes

m-lord-renkse approved these changes Apr 4, 2024

View reviewed changes

squadgazzz merged commit 5e43504 into main Apr 4, 2024
9 checks passed

squadgazzz deleted the 2468/protocol-fee-e2e-tests-cleanup branch April 4, 2024 08:59

github-actions bot locked and limited conversation to collaborators Apr 4, 2024

Protocol fee e2e tests clean-up #2561

Protocol fee e2e tests clean-up #2561

Conversation

squadgazzz commented Mar 22, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-lord-renkse Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

squadgazzz Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a 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 Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a 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

Choose a reason for hiding this comment

MartinquaXD left a 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

fleupold left a comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

squadgazzz commented Mar 22, 2024 •

edited

Loading

m-lord-renkse Mar 27, 2024 •

edited

Loading

squadgazzz Mar 26, 2024 •

edited

Loading

squadgazzz Mar 26, 2024 •

edited

Loading

squadgazzz Mar 29, 2024 •

edited

Loading