Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

Query order presignature status #1025

Merged
merged 5 commits into from
Aug 31, 2021
Merged

Query order presignature status #1025

merged 5 commits into from
Aug 31, 2021

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Aug 26, 2021

This PR adds a SignaturePending status to order for pre-sign orders in the OrderMetadata for signalling that a pre-sign order is waiting for a pre-signature in the GPv2 settlement contract.

Please note that my SQL skillz are 👎. So please scrutinize the query modification that was made.

Test Plan

Added new unit tests for the added order status computation. Added an ignored test for the Postgres query for presignature status that can be manually run:

$ cargo test -p orderbook -- --ignored postgres_presignature_status --test-threads 1
    Finished test [unoptimized + debuginfo] target(s) in 0.14s
     Running unittests (target/debug/deps/orderbook-68be6fafd7ab0abc)

running 1 test
test database::orders::tests::postgres_presignature_status ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 59 filtered out; finished in 0.06s

Query Rational

We use a subquery in order to get the latest presignature.

nlordell=# explain SELECT
  (o.signing_scheme = 'presign' AND COALESCE((
    SELECT (NOT p.signed) as unsigned
    FROM presignature_events p
    WHERE o.uid = p.order_uid
    ORDER BY p.block_number DESC, p.log_index DESC
    LIMIT 1
  ), true)) as presignature_pending
FROM orders o
GROUP BY o.uid;
                                        QUERY PLAN                                        
------------------------------------------------------------------------------------------
 HashAggregate  (cost=12.88..260.70 rows=230 width=58)
   Group Key: o.uid
   ->  Seq Scan on orders o  (cost=0.00..12.30 rows=230 width=61)
   SubPlan 1
     ->  Limit  (cost=1.06..1.06 rows=1 width=17)
           ->  Sort  (cost=1.06..1.06 rows=1 width=17)
                 Sort Key: p.block_number DESC, p.log_index DESC
                 ->  Seq Scan on presignature_events p  (cost=0.00..1.05 rows=1 width=17)
                       Filter: (o.uid = order_uid)
(9 rows)

This seems to be more performant than a JOIN LEFT LATERAL.

@nlordell nlordell requested a review from a team August 26, 2021 15:13
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2021

Codecov Report

Merging #1025 (df63bd1) into main (3709b51) will decrease coverage by 0.12%.
The diff coverage is 63.86%.

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
- Coverage   64.47%   64.35%   -0.13%     
==========================================
  Files         123      123              
  Lines       23350    23471     +121     
==========================================
+ Hits        15056    15105      +49     
- Misses       8294     8366      +72     

@vkgnosis
Copy link
Contributor

Could we rebase this set of branches on main so that it includes the change to SQL query to make reviewing easier?

Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SQL query looks good to me.

Maybe an extra test could be that an expired order that is pending a presignature returns "expired" (and not "pending").

@nlordell
Copy link
Contributor Author

Could we rebase this set of branches on main so that it includes the change to SQL query to make reviewing easier?

@vkgnosis should be rebased now.

@nlordell nlordell requested a review from vkgnosis August 31, 2021 07:25
@nlordell nlordell requested a review from vkgnosis August 31, 2021 09:35
@@ -447,7 +447,7 @@ components:
OrderStatus:
description: The current order status
type: string
enum: [open, fulfilled, cancelled, expired]
enum: [signaturePending, open, fulfilled, cancelled, expired]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anxolin @W3stside @alfetopito

Just pinging you guys so you are aware of the new order status (for an order that was accepted by the orderbook and is waiting for the on-chain presignature).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting us know.
I've created one task for the explorer gnosis/gp-ui#628

I believe Anxo is taking care of cowswap changes.

Base automatically changed from presignature-order-status to main August 31, 2021 10:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants