Implement API route for replacing orders #239

nlordell · 2022-05-31T12:33:46Z

Fixes #120

This PR introduces a new PATCH api/v1/orders/${order_uid} route for cancelling an existing order and creating a new replacement order in an atomic operation. This way, if an existing user order goes "out-of-price", the CowSwap UI could replace the old order with a new "in-price" order with a single user signature and request. Also, since the operation is atomic, we guarantee that:

All auctions contain at least one of the orders (cancelling > creating could, in some cases, cause a user to miss an auction if a new auction was cut right after the user order was cancelled but before the new one was created)
No auctions include both orders (creating > cancelling could cause this if an auction was cut right after the new order was created but before the old one was cancelled).

This works by verifying:

That the owner of the replacement transaction matches the owner of the original transaction
The that appData field of the replacement order is equal to the EIP-712 struct hash of a cancellation request for the original order; in Ethers.js this would be:
```
ethers.utils._TypedDataEncoder.hashStruct(
  "OrderCancellation",
  [{ name: "orderUid", type: "bytes" }],
  { orderUid },
);
```
That the replacement order is a signed order; PreSign orders cannot be accepted as replacements as their signature is not verified on order creation (instead, the orderbook waits for an on-chain PreSignature event for the specific order) - meaning that fake PreSign order with no intent on being filled (because the owner would not call setPreSignature(true) for the order) could be used to cancel arbitrary user orders without signatures.

Test Plan

Added new unit tests around replacement PostgreSQL transaction. We were able to reuse almost all of the validation logic for cancelling and creating orders with some light refactoring, so no new tests were needed for those code paths.

Sample script that tests new feature

Make sure to install hdwallet for command-line signing.

#!/bin/bash

create_order() {
  local max_sell_amount="$1"
  local replacement="${2:-0x}"

  local cancellation=$(cat <<JSON
{
  "types": {
    "EIP712Domain": [
      { "name": "name", "type": "string" },
      { "name": "version", "type": "string" },
      { "name": "chainId", "type": "uint256" },
      { "name": "verifyingContract", "type": "address" }
    ],
    "OrderCancellation": [
      { "name": "orderUid", "type": "bytes" }
    ]
  },
  "primaryType": "OrderCancellation",
  "domain": {
    "name": "Gnosis Protocol",
    "version": "v2",
    "chainId": 4,
    "verifyingContract": "0x9008D19f58AAbD9eD0D60971565AA8510560ab41"
  },
  "message": {
    "orderUid": "$replacement"
  }
}
JSON
  )
  local app_data=$(echo "$cancellation" | hdwallet hash typeddata --message-hash -)

  local now=$(date +%s)
  local valid_to=$(( now + 1800 ))

  local order=$(cat <<JSON
{
  "sellToken": "0xc778417e063141139fce010982780140aa0cd5ab",
  "buyToken": "0xa7d1c04faf998f9161fc9f800a99a809b84cfc9d",
  "receiver": "0x0000000000000000000000000000000000000000",
  "sellAmount": "$max_sell_amount",
  "buyAmount": "1000000000000000000000",
  "validTo": $valid_to,
  "appData": "$app_data",
  "feeAmount": "500000000000000",
  "kind": "buy",
  "partiallyFillable": false,
  "sellTokenBalance": "erc20",
  "buyTokenBalance": "erc20"
}
JSON
  )

  local payload=$(cat <<JSON
{
  "types": {
    "EIP712Domain": [
      { "name": "name", "type": "string" },
      { "name": "version", "type": "string" },
      { "name": "chainId", "type": "uint256" },
      { "name": "verifyingContract", "type": "address" }
    ],
    "Order": [
      { "name": "sellToken", "type": "address" },
      { "name": "buyToken", "type": "address" },
      { "name": "receiver", "type": "address" },
      { "name": "sellAmount", "type": "uint256" },
      { "name": "buyAmount", "type": "uint256" },
      { "name": "validTo", "type": "uint32" },
      { "name": "appData", "type": "bytes32" },
      { "name": "feeAmount", "type": "uint256" },
      { "name": "kind", "type": "string" },
      { "name": "partiallyFillable", "type": "bool" },
      { "name": "sellTokenBalance", "type": "string" },
      { "name": "buyTokenBalance", "type": "string" }
    ]
  },
  "primaryType": "Order",
  "domain": {
    "name": "Gnosis Protocol",
    "version": "v2",
    "chainId": 4,
    "verifyingContract": "0x9008D19f58AAbD9eD0D60971565AA8510560ab41"
  },
  "message": $order
}
JSON
  )

  local signature=$(echo $payload | hdwallet sign typeddata -)

  echo $order | jq \
    --arg SIG "$signature" \
    --arg FROM "$(hdwallet address)" \
    '.signature=$SIG | .signingScheme="eip712" | .from=$FROM'
}

order_status() {
  local order_uid="$1"
  curl -s "http://localhost:8080/api/v1/orders/$order_uid" | jq '{ uid, status }'
}

# First place an order that is completely under-priced.
order_uid=$(curl -s 'http://localhost:8080/api/v1/orders' -X POST -H 'Content-Type: application/json' --data "$(create_order 1000)" | jq -r '.')
echo "==> Placed order $order_uid"

echo "==> Order status:"
order_status $order_uid

# Now replace the order with a correctly priced one.
new_order_uid=$(curl -s "http://localhost:8080/api/v1/orders/$order_uid" -X PATCH -H 'Content-Type: application/json' --data "$(create_order 1000000000000000000 $order_uid)" | jq -r '.')
echo "==> Replaced order with $new_order_uid"

echo "==> Old order status:"
order_status $order_uid

echo "==> New order status:"
order_status $new_order_uid

Result:

==> Placed order 0x1c08c03379a2d9a1e6d385552471502faf38e9aafc3cae9ed126f72e6d2a300edbc682d0ec19bd276fd6c2af4e0189b584bb31e162974f8b
==> Order status:
{
  "uid": "0x1c08c03379a2d9a1e6d385552471502faf38e9aafc3cae9ed126f72e6d2a300edbc682d0ec19bd276fd6c2af4e0189b584bb31e162974f8b",
  "status": "open"
}
==> Replaced order with 0xc81b233d74a01f9785e9d9e7d63b577b78a66aaac7864e312a481039bcaeb385dbc682d0ec19bd276fd6c2af4e0189b584bb31e162974f8b
==> Old order status:
{
  "uid": "0x1c08c03379a2d9a1e6d385552471502faf38e9aafc3cae9ed126f72e6d2a300edbc682d0ec19bd276fd6c2af4e0189b584bb31e162974f8b",
  "status": "cancelled"
}
==> New order status:
{
  "uid": "0xc81b233d74a01f9785e9d9e7d63b577b78a66aaac7864e312a481039bcaeb385dbc682d0ec19bd276fd6c2af4e0189b584bb31e162974f8b",
  "status": "open"
}

Release notes

In order to add more metrics around order replacements, I refactored the recently added orderbook metrics (user_order_created and liquidity_order_created) to be a CounterVec and use labels to distinguish between the order "kind" (so user orders or liquidity orders) as well as the operation (created and cancelled). This requires changes to the Grafana dashboards.

fleupold · 2022-05-31T12:46:09Z

If I understand the code correctly (it might be nice to add this to the PR description), as long as I have a signed order from a user I can use it to cancel any other existing order of that user without their explicit consent? I don't think this is necessary a huge blocker (assuming replaying an existing order for that would lead to an error) since users will likely place their orders before someone can intercept them, but I was wondering if you thought about including a "replace_order_uid" field in the app_data json. This way the user would explicitly sign their intent to replace an existing order.

More generally, given that we have seen this could enable a bunch of different use cases, I think it may make sense to invest into a component that can decode, fetch and cache the content behind app_data from IPFS.

nlordell · 2022-05-31T12:57:21Z

as long as I have a signed order from a user I can use it to cancel any other existing order of that user without their explicit consent?

I don't think so, unless I made a mistake somewhere:

        // Verify that the new order is a valid replacement order by checking
        // that the `app_data` encodes an order cancellation and that both the
        // old and new orders have the same signer.
        let cancellation = OrderCancellation {
            order_uid: old_order.metadata.uid,
            ..Default::default()
        };
        if new_order.creation.app_data != cancellation.hash_struct()
            || new_order.metadata.owner != old_order.metadata.owner
        {
            return Err(ReplaceOrderError::InvalidReplacement);
        }

Specifically, the appData needs to be OrderCancellation signing hash for the order that is being replaced.

nlordell · 2022-05-31T13:04:18Z

More generally, given that we have seen this could enable a bunch of different use cases, I think it may make sense to invest into a component that can decode, fetch and cache the content behind app_data from IPFS.

This is definitely the plan - we want to use this for liquidity orders for example.

fedgiac

Do we still allow anyone to create presign orders before any transaction occurred onchain? In case, it could be used by anyone to cancel an order for any user.
Otherwise, assuming that order.metadata.owner is always validated by a signature or transaction, this looks like a good approach to me. (I'd also prefer to see the metadata from appData decoded somewhere else independently, so we can use them to also store other data instead of reusing the same field for multiple purposes.)

crates/orderbook/src/orderbook.rs

MartinquaXD

Looks good overall. I also would like to see proper support for app_data but this "shortcut" seems reasonable enough for now.

MartinquaXD · 2022-05-31T12:49:23Z

crates/orderbook/src/database/orders.rs

+        old_order: &model::order::OrderUid,
+        new_order: &model::order::Order,


Why not take the orders by value instead of taking a reference and copying immediately afterwards?

It was mostly for consistency with the insert_order and cancel_order methods.

This can be changed.

…nd signature

codecov-commenter · 2022-05-31T14:15:41Z

Codecov Report

Merging #239 (12f486b) into main (2fd838f) will decrease coverage by 0.04%.
The diff coverage is 52.17%.

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
- Coverage   64.48%   64.43%   -0.05%     
==========================================
  Files         190      191       +1     
  Lines       39061    39479     +418     
==========================================
+ Hits        25188    25440     +252     
- Misses      13873    14039     +166

nlordell · 2022-05-31T14:56:28Z

I also would like to see proper support for app_data

You could argue that replacement orders should have the exact same app data as the transaction it is replacing. That being said, I agree that we should enhance this "app data" thing.

nlordell · 2022-05-31T15:11:54Z

Do we still allow anyone to create presign orders before any transaction occurred onchain? In case, it could be used by anyone to cancel an order for any user.

How do you think of these things @fedgiac! Definitely an issue. We can scope order replacements to ECDSA-signed orders.

fedgiac

Looks good to me!

crates/orderbook/openapi.yml

crates/orderbook/src/orderbook.rs

Co-authored-by: Federico Giacon <[email protected]>

vkgnosis

Code looks good. Haven't though too deeply on the mechanism.

nlordell requested a review from a team as a code owner May 31, 2022 12:33

Nicholas Rodrigues Lordello added 2 commits May 31, 2022 15:08

Implement API route for replacing orders

74c1a64

Add openapi spec

98df914

fedgiac reviewed May 31, 2022

View reviewed changes

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

MartinquaXD approved these changes May 31, 2022

View reviewed changes

Added unit test to verify we are checking replacement order appdata a…

5ea8b05

…nd signature

nlordell force-pushed the replace-order branch from c543dfd to 5ea8b05 Compare May 31, 2022 14:04

Nicholas Rodrigues Lordello added 2 commits May 31, 2022 17:19

Only accept ECDSA signed replacement orders

d896902

fix flacky test

68eb2d8

nlordell requested a review from fedgiac May 31, 2022 15:24

fedgiac approved these changes May 31, 2022

View reviewed changes

crates/orderbook/openapi.yml Outdated Show resolved Hide resolved

crates/orderbook/openapi.yml Outdated Show resolved Hide resolved

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

Nicholas Rodrigues Lordello and others added 2 commits May 31, 2022 18:50

Update crates/orderbook/openapi.yml

697087f

Co-authored-by: Federico Giacon <[email protected]>

Update crates/orderbook/openapi.yml

be5cfaa

Co-authored-by: Federico Giacon <[email protected]>

vkgnosis approved these changes Jun 1, 2022

View reviewed changes

Merge branch 'main' into replace-order

12f486b

nlordell merged commit 34b2233 into main Jun 1, 2022

nlordell deleted the replace-order branch June 1, 2022 11:20

github-actions bot locked and limited conversation to collaborators Jun 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement API route for replacing orders #239

Implement API route for replacing orders #239

nlordell commented May 31, 2022 •

edited

Loading

fleupold commented May 31, 2022

nlordell commented May 31, 2022 •

edited

Loading

nlordell commented May 31, 2022

fedgiac left a comment •

edited

Loading

MartinquaXD left a comment

MartinquaXD May 31, 2022

nlordell May 31, 2022 •

edited

Loading

codecov-commenter commented May 31, 2022 •

edited

Loading

nlordell commented May 31, 2022

nlordell commented May 31, 2022

fedgiac left a comment

vkgnosis left a comment

		old_order: &model::order::OrderUid,
		new_order: &model::order::Order,

Implement API route for replacing orders #239

Implement API route for replacing orders #239

Conversation

nlordell commented May 31, 2022 • edited Loading

Test Plan

Release notes

fleupold commented May 31, 2022

nlordell commented May 31, 2022 • edited Loading

nlordell commented May 31, 2022

fedgiac left a comment • edited Loading

Choose a reason for hiding this comment

MartinquaXD left a comment

Choose a reason for hiding this comment

MartinquaXD May 31, 2022

Choose a reason for hiding this comment

nlordell May 31, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented May 31, 2022 • edited Loading

Codecov Report

nlordell commented May 31, 2022

nlordell commented May 31, 2022

fedgiac left a comment

Choose a reason for hiding this comment

vkgnosis left a comment

Choose a reason for hiding this comment

nlordell commented May 31, 2022 •

edited

Loading

nlordell commented May 31, 2022 •

edited

Loading

fedgiac left a comment •

edited

Loading

nlordell May 31, 2022 •

edited

Loading

codecov-commenter commented May 31, 2022 •

edited

Loading