Market orders deprecation date remove #2536

sunce86 · 2024-03-19T01:20:41Z

Description

Removes market_orders_deprecation_date as required in #2462. (no need to remove rank_by_surplus_date just yet)

Once removed, tests started failing, so we need to convert all tests for market orders to work with limit orders or to remove them if they don't make sense anymore.

How to test

Existing tests.

sunce86 · 2024-03-25T16:53:05Z

The PR is ready to be reviewed now.

MartinquaXD

Looks good overall just surprised by 2 test changes.

MartinquaXD · 2024-03-25T20:09:33Z

crates/e2e/tests/e2e/onchain_settlement.rs

-    );
-
-    let services = Services::new(onchain.contracts()).await;
-    let solver_endpoint = colocation::start_naive_solver().await;


Why does this test have to be removed. I think it's one of few (maybe the only one?) that tests that the naive solver is actually able to settle batches.

Naive solver does not support limit orders. When switched to using baseline (which does not do batch) I ended up with a classic limit order e2e test which is already covered with other tests.

Ahh, right. Forgot that naive doesn't support computing the fee. 👌

How hard would it be to teach the naive solver to compute fees? On a high level it should be less hard than what the baseline solver does since it only ever uses one kind of pool? I think it would be good to have some basic CoW test coverage.

So I think we agreed on slack to ditch Naive solver (this PR is happy for it).

Can we capture a task to bring this test back if we resurrect naive solver (I do think for e2e tests specifically it would be good to have a solver that's capable of matching basic CoWs)

crates/e2e/tests/e2e/smart_contract_orders.rs

m-lord-renkse · 2024-03-26T09:52:33Z

LGMT

fleupold

Looks good, but I think we are removing a few e2e tests that are still adding value.

fleupold · 2024-03-26T14:50:21Z

crates/e2e/tests/e2e/eth_integration.rs

@@ -125,6 +123,6 @@ async fn eth_integration(web3: Web3) {
    );
    assert_eq!(
        trader_b_eth_balance_after - trader_b_eth_balance_before,
-        49_776_284_258_574_379_601_u128.into()


These tests strike me as useful as a chocolate teapot, basically whenever they fail we just rerun the assertion and update the value (so it's not really testing anything useful).

I think the proper assert here would be
trader_b_eth_balance_after >= 49 eth

fixed as suggested by myself 😄

fleupold · 2024-03-26T14:51:04Z

crates/e2e/tests/e2e/ethflow.rs

@@ -148,122 +131,6 @@ async fn eth_flow_tx_zero_fee(web3: Web3) {
    .await;
 }

-async fn eth_flow_tx(web3: Web3) {


Why is this test removed, it still seems legit?

Yeah, I spent some time figuring out why some tests were removed. Would appreciate more context on this.

Not actually removed, it's just code diff shows it in a weird way.

local_node_eth_flow_insufficient_fee and local_node_eth_flow_zero_fee are removed.

and local_node_eth_flow_tx is changed to be actually local_node_eth_flow_zero_fee :)

fleupold · 2024-03-26T14:51:50Z

crates/e2e/tests/e2e/limit_orders.rs

@@ -297,132 +291,6 @@ async fn two_limit_orders_test(web3: Web3) {
    assert!(balance_after_b.checked_sub(balance_before_b).unwrap() >= to_wei(2));
 }

-async fn mixed_limit_and_market_orders_test(web3: Web3) {


Ditto, this test case still seems useful

If we remove market order as we do not support it anymore, we are left with 1 limit order (which is actually in-market) and we already have this test covered with local_node_single_limit_order

fleupold · 2024-03-26T14:53:43Z

crates/e2e/tests/e2e/onchain_settlement.rs

-    );
-
-    let services = Services::new(onchain.contracts()).await;
-    let solver_endpoint = colocation::start_naive_solver().await;


How hard would it be to teach the naive solver to compute fees? On a high level it should be less hard than what the baseline solver does since it only ever uses one kind of pool? I think it would be good to have some basic CoW test coverage.

crates/shared/src/order_validation.rs

fleupold · 2024-03-28T10:55:27Z

crates/e2e/tests/e2e/onchain_settlement.rs

-    );
-
-    let services = Services::new(onchain.contracts()).await;
-    let solver_endpoint = colocation::start_naive_solver().await;


Can we capture a task to bring this test back if we resurrect naive solver (I do think for e2e tests specifically it would be good to have a solver that's capable of matching basic CoWs)

sunce86 · 2024-03-28T21:39:52Z

crates/database/src/onchain_broadcasted_orders.rs

@@ -13,7 +13,6 @@ pub enum OnchainOrderPlacementError {
    DisabledOrderClass,
    ValidToTooFarInFuture,
    InvalidOrderData,
-    InsufficientFee,


Hm, wonder If I am allowed to delete the variant (for backward compatibility with older orders that might have this error)

You are right, this would probably lead to issues if the order is read from disk

I reverted the bare minimum of InsufficientFee in database so that, when we fetch old orders that have this error, nothing breaks.

sunce86 · 2024-03-29T12:21:28Z

Captured the remaining task: #2578

sunce86 added 14 commits March 19, 2024 01:59

initial commit

6e7a660

eth flow tests

8708c60

deprecate market order tests

1c37cd1

Merge branch 'main' into market_orders_deprecation_date-remove

15e1ffe

fix fee in test

23e885e

more test fixes

05c3697

removed test

038f5fd

remove mode

5e28857

fix order_cancellation

b782f9a

fix smart contract test

80ddc22

fix solver competition test

f17e65f

fix smart contract test

45f42f5

Merge branch 'main' into market_orders_deprecation_date-remove

7c66186

fix vault_balances

5bd29bf

sunce86 changed the title ~~DRAFT: Market orders deprecation date remove~~ Market orders deprecation date remove Mar 25, 2024

sunce86 marked this pull request as ready for review March 25, 2024 16:52

sunce86 requested a review from a team as a code owner March 25, 2024 16:52

MartinquaXD reviewed Mar 25, 2024

View reviewed changes

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

View reviewed changes

MartinquaXD approved these changes Mar 26, 2024

View reviewed changes

fleupold reviewed Mar 26, 2024

View reviewed changes

sunce86 added 2 commits March 27, 2024 17:45

fix test assert

84fb664

update openapi

9349f04

sunce86 requested review from fleupold and squadgazzz March 27, 2024 16:48

fleupold approved these changes Mar 28, 2024

View reviewed changes

sunce86 commented Mar 28, 2024

View reviewed changes

Merge branch 'main' into market_orders_deprecation_date-remove

0004af9

Revert insufficient fee error in database

4622d85

sunce86 mentioned this pull request Mar 29, 2024

feat: Revert onchain_settlement e2e test #2578

Closed

sunce86 merged commit ad8cd1b into main Mar 29, 2024
9 checks passed

sunce86 deleted the market_orders_deprecation_date-remove branch March 29, 2024 12:21

github-actions bot locked and limited conversation to collaborators Mar 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Market orders deprecation date remove #2536

Market orders deprecation date remove #2536

sunce86 commented Mar 19, 2024 •

edited

Loading

sunce86 commented Mar 25, 2024

MartinquaXD left a comment

MartinquaXD Mar 25, 2024

sunce86 Mar 26, 2024

MartinquaXD Mar 26, 2024

fleupold Mar 26, 2024

sunce86 Mar 27, 2024

fleupold Mar 28, 2024

m-lord-renkse commented Mar 26, 2024

fleupold left a comment

fleupold Mar 26, 2024

sunce86 Mar 26, 2024

sunce86 Mar 27, 2024

fleupold Mar 26, 2024

squadgazzz Mar 26, 2024

sunce86 Mar 27, 2024

sunce86 Mar 27, 2024

sunce86 Mar 27, 2024

fleupold Mar 26, 2024

sunce86 Mar 27, 2024

fleupold Mar 26, 2024

fleupold Mar 28, 2024

sunce86 Mar 28, 2024

fleupold Mar 29, 2024

sunce86 Mar 29, 2024

sunce86 commented Mar 29, 2024

Market orders deprecation date remove #2536

Market orders deprecation date remove #2536

Conversation

sunce86 commented Mar 19, 2024 • edited Loading

Description

How to test

sunce86 commented Mar 25, 2024

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

m-lord-renkse commented Mar 26, 2024

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

sunce86 commented Mar 29, 2024

sunce86 commented Mar 19, 2024 •

edited

Loading