Return error on order replacement when it's not safe by mrnaveira · Pull Request #3323 · cowprotocol/services

mrnaveira · 2025-03-12T10:22:19Z

Description

We want to allow users to "edit" orders, implemented by canceling a previous order when creating a new one. We have a replacedOrder property for this.

One concern is that this could lead to double spending when the old order is part of the current auction and the winning solver intends to settle it.

The goal of this PR is to mitigate this issue as much as possible by checking whether any solver has bid on the original order in the last few auctions. The POST /orders method now returns a 400 Bad Request in that case.

A follow-up PR can extend this logic to the cancellation endpoint. In this case, instead of rejecting the cancellation outright, we may prefer to return a warning message instead.

Changes

Added a new database operation load_latest_competitions to fetch all the competitions from the last N auctions.
Updated order replacement logic in the orderbook to verify that the order is not included in any recent competitions (using the new order_is_actively_bid_on function).
Introduced a more granular order replacement error specification via the new OrderReplacementError enum.
Updated openapi.yml specification with a new error variant in OrderPostError.
New orderbook argument active_order_competition_threshold.

How to test

Created a new e2e scenario, try_replace_active_order_test, under the replace_order e2e module, which triggers the new error.

However, the ideal scenario would be to test this when the old order is included in any solution by using a mock solver that always returns a bad solution containing the old order. However, achieving this would require the ability to mock the driver in e2e tests, which we currently lack. A follow-up PR can address this.

Related Issues

Fixes #3315

github-actions · 2025-03-12T10:22:35Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

mrnaveira · 2025-03-12T10:24:41Z

I have read the CLA Document and I hereby sign the CLA

MartinquaXD · 2025-03-12T10:45:55Z

crates/orderbook/src/orderbook.rs

+        // The number of past solver competitions we want to look back at
+        const COMPETITIONS_COUNT: u32 = 2;


Would be nice to make this a configurable option.

I created a new active_order_competition_threshold argument for the orderbook

crates/orderbook/src/orderbook.rs

crates/e2e/tests/e2e/replace_order.rs

squadgazzz

I am not sure that checking the 2 solver competitions is enough here. Since we are able to settle auctions in parallel for different winning solvers and also have a settlement queue, we should probably also check all the competitions with the deadline above the current block. The deadline is stored in the competition_auctions table. That would return all the ongoing competitions and then they can be joined with the solver_competitions table. That way we won't miss anything unless I am overthinking.

upd: that way, the threshold config can be dropped.

squadgazzz · 2025-03-12T12:34:17Z

crates/e2e/tests/e2e/replace_order.rs

+    run_test(try_replace_active_order_test).await;
+}
+
+async fn try_replace_active_order_test(web3: Web3) {


nit: It seems like it tests not an active but already executed order replacement.

You are right. The test is not ideal.

As I mentioned in the PR testing section, achieving proper testing requires the ability to mock the driver in e2e tests, which we currently lack. A follow-up PR can address this.

A follow-up PR can address this.

I already implemented the mock. The PR is not ready yet. So to avoid working on the same thing, please wait for it.

You are right. The test is not ideal.

The test name should reflect that.

@MartinquaXD any thoughts on the competition_auctions proposal?

The concern with the settle queue and parallel auctions makes sense to me but I think given how relatively rarely this feature will probably be used we shouldn't over engineer it. The submission deadline is currently roughly as long as 3 auctions so if we want to be safe without over-engineering we can just set the lookback to 5 and call it a day. If the frontend reports that people often run into this error we can revise the strategy.

Accidentally pressed the wrong button

MartinquaXD

LGTM. I believe just one last thing missing. The orderbook/openapi.yml should reference the new error type in the OrderPostErrors.

Approved assuming this gets addressed.

crates/orderbook/src/orderbook.rs

crates/orderbook/src/arguments.rs

github-actions · 2025-03-12T16:48:26Z

Reminder: Please consider backward compatibility when modifying the API specification.
If breaking changes are unavoidable, ensure:

You explicitly pointed out breaking changes.
You communicate the changes to affected teams (at least Frontend team and SAFE team).
You provide proper versioning and migration mechanisms.

Caused by:

crates/orderbook/openapi.yml

mrnaveira added 5 commits March 11, 2025 14:24

check active order in replacement orders

780ccb4

add conversion from replacement errors to api errors

7ed7efa

new try_replace_active_order_test

b5a70a4

use bad_request status for active order replacement error

b64a09d

fmt fix

b26a304

mrnaveira requested a review from a team as a code owner March 12, 2025 10:22

Merge branch 'main' into order-replacement-check-unsafe

fbe67b5

github-actions bot added a commit that referenced this pull request Mar 12, 2025

@mrnaveira has signed the CLA from Pull Request #3323

f3d9a27

MartinquaXD reviewed Mar 12, 2025

View reviewed changes

mrnaveira added 4 commits March 12, 2025 12:40

fetch the orders from the solutions instead of the auctions

46cf199

improve e2d test scenario

6c07163

new active_order_competition_threshold argument

02b9042

fix fmt

72525a8

squadgazzz previously approved these changes Mar 12, 2025

View reviewed changes

mrnaveira marked this pull request as draft March 12, 2025 13:00

mrnaveira marked this pull request as ready for review March 12, 2025 13:00

clarify that the test is not ideal

dab175d

MartinquaXD approved these changes Mar 12, 2025

View reviewed changes

crates/orderbook/src/orderbook.rs Outdated Show resolved Hide resolved

squadgazzz approved these changes Mar 12, 2025

View reviewed changes

crates/orderbook/src/arguments.rs Outdated Show resolved Hide resolved

mrnaveira added 3 commits March 12, 2025 17:09

bump the default lookback value to 5

645168b

improve OldOrderActivelyBidOn error message

f8b9973

add new error variant to openapi

dd68f62

improve new post order variant message

3c75f37

mrnaveira merged commit efb55ae into main Mar 13, 2025
11 checks passed

mrnaveira deleted the order-replacement-check-unsafe branch March 13, 2025 08:27

github-actions bot locked and limited conversation to collaborators Mar 13, 2025

		// The number of past solver competitions we want to look back at
		const COMPETITIONS_COUNT: u32 = 2;

Conversation

mrnaveira commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Related Issues

Uh oh!

github-actions bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrnaveira commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

squadgazzz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinquaXD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrnaveira commented Mar 12, 2025 •

edited

Loading

github-actions bot commented Mar 12, 2025 •

edited

Loading

squadgazzz left a comment •

edited

Loading