Storing quotes longer, in case an on-chain order placement is intended #414

josojo · 2022-08-05T07:58:08Z

This PR contains the logic for the new quoting system. I implemented it as Nick suggested in this closed PR: https://github.com/cowprotocol/services/pull/396  

Let me summarise the logic for placing orders again:  

For EIP712/EthSign orders: No logic change: quotes are stored with the standard validity 
For PreSign orders: Currently, we are generating quotes with the standard validity time. And then we require an order placement in a short time via the API, however, the tx can be mined later. We wanna keep this workflow for the time being. It could be removed at some later point.
Additionally, we prepare the PreSign for onchain order placements: the quotes will have a longer validity and order placement via on-chain event can be introduced later.
For EIP1271 orders : Same as Presign orders. For the ETH-flow we want to enable order placements via on-chain transactions. Hence, we give the option to quote with EIP1271ForOnchainOrder, and then the quote will be stored longer.

We introduce this kind of on-chain order placement without an API-call call, as it allows new features, e.g. onchain calculating the valid_to of an order. See the thread here: https://cowservices.slack.com/archives/C036BL4AANP/p1657018904390399

PS: This PR substitutes the PR: #355. I think the split into two PRs makes no longer sense as the logic is so tightly connected. Though it makes this PR a little bit bigger.

Test Plan

None

josojo · 2022-08-05T07:59:52Z

crates/model/src/quote.rs

@@ -16,6 +15,20 @@ pub enum PriceQuality {
    Optimal,
 }

+#[derive(Eq, PartialEq, Clone, Copy, Debug, Default, Deserialize, Serialize, Hash)]
+#[serde(rename_all = "lowercase")]
+pub enum QuoteSigningScheme {


An alternative implementation could look like this:

#[derive( Debug, Default, Deserialize, Serialize)] #[serde(rename_all = "camelCase")] pub enum QuoteSigningScheme { #[default] Eip712, EthSign, Eip1271AsOnchainOrder, /// EIP orders can be off-chain and on-chain orders Eip1271 { #[serde(default)] intended_onchain_order: bool }, PreSign { #[serde(default)] intended_onchain_order: bool }, }

But I never managed to make it backwards compatible with the current API. Hence I decided for the current form. See my playground: https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=db2ec1559dc581fa97c01ad7e2a70ce3

One thing I'm not super a fan of is introducing new artificial signingScheme values that don't actually exist.

Here is a way that should be backwards compatible (basically it allows an additional onchainOrder: bool field in the JSON that gets "matched" up with the signingScheme field): https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=c0cbff908ab7446fc457453299f8e01c

Its almost the same as yours, I just added #[serde(tag = "signingScheme")] to the QuoteSigngingScheme type.

Wow... the "tag" command is smart!

But unfortunately, it is not fully backwards compatible. Serde default and serde flatten do not work together, but your solution requires both:
serde-rs/serde#1626

Here is your playground with a failing test: https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=68ed5b31ba40052762430b6d8e742002

I would love to take your solution, but I could not find a solution for that failing unit test

Serde default and serde flatten do not work together, but your solution requires both:

So close! Dang, I didn't know.

In this case, I would recommend deserializing to an intermediate structure and then

match (data.signing_scheme, data.onchain_order) { (scheme, true) if scheme.is_ecdsa_scheme() => bail!("ECDSA-signed orders cannot be on-chain"), // ... }

Something like: https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=0d309b803690a14fbb9cfe9e8a37b1fe

What's since is that serde generates the code to convert from the intermediary struct to the QuoteSigningScheme struct without "leaking" the abstraction.

vkgnosis · 2022-08-05T10:26:18Z

-- please only review once I have fixed my tests --

Could you turn this into a draft PR until it should be reviewed? There is a button for that when you create the PR but also should be somewhere for already existing ones.

codecov-commenter · 2022-08-05T10:49:11Z

Codecov Report

Merging #414 (77440f4) into main (7423971) will increase coverage by 0.05%.
The diff coverage is 72.47%.

❗ Current head 77440f4 differs from pull request most recent head d095931. Consider uploading reports for the commit d095931 to get more accurate results

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
+ Coverage   63.91%   63.96%   +0.05%     
==========================================
  Files         228      228              
  Lines       43917    44252     +335     
==========================================
+ Hits        28068    28308     +240     
- Misses      15849    15944      +95

josojo · 2022-08-05T11:01:01Z

-- please only review once I have fixed my tests --
Could you turn this into a draft PR until it should be reviewed? There is a button for that when you create the PR but also should be somewhere for already existing ones.

I am very sorry about the unnecessary noise. I will setup git hooks next so that it will not happen again.

vkgnosis · 2022-08-05T11:15:29Z

It is completely fine to have PRs that are not meant for review yet. I just ask that you mark those as Draft so that's clear. I often make PRs to see whether CI succeeds.

crates/orderbook/src/order_validation.rs

crates/database/src/quotes.rs

nlordell · 2022-08-05T15:25:13Z

crates/orderbook/src/order_quoting.rs

@@ -713,7 +745,8 @@ mod tests {
                    sell_token_price: 0.2,
                },
                kind: OrderKind::Sell,
-                expiration: now + chrono::Duration::seconds(QUOTE_VALIDITY_SECONDS),
+                expiration: now + Duration::seconds(STANDARD_QUOTE_VALIDITY_SECONDS),


Unrelated to this PR (so not asking it to be done here) - but we should probably give the same "allow for configuration" for the STANDARD_QUOTE_VALIDITY_SECONDS parameter.

nlordell

Changes look great in general.

Just have one comment about how we represent the new signing schemes in JSON.

fedgiac

The changes work for me.
I don't like that signing schemes are used to store information that has nothing to do with signing schemes, but my understanding is that this is a workaround because we wouldn't be able to decode the current JSON representation of quotes after these changes if we add a flag onchainOrder (Serde doesn't support that in our setup).
Would it be so bad to break backward compatibility for quotes?

crates/orderbook/src/order_quoting.rs

fedgiac · 2022-08-08T15:35:29Z

database/sql/V027__quotes_adding_onchain_signing_scheme.sql

+-- Adding new column on table quotes to differentiate the two on-chain signing schemes
+-- This is required as different signing schemes have different expirations
+CREATE TYPE OnchainSigningScheme AS ENUM ('eip1271', 'presign');


(Nits, feel free to ignore.)
I imagine this will be reused to add different behavior to different quote types in the future if needed (e.g., ETH flow orders have a longer expiration for whatever reason).
Then I'd rename OnchainSigningScheme to QuoteType just to point out that the signing scheme has nothing to do with this entry (except that it so happens that the two types we have now correspond to the two signature types).
We could also have a different order type for normal quotes instead of making the field NULLable (maybe eoa?).

I think this is a fair suggestion. Maybe "OnchainQuoteType"?

I kinda like that Federico suggests making it even more general, not just for onchain orders.
I would call it QuoteType and introduce a new enum value eoa. eoa would then also be the default value.

This would also remove the "weird" IS NOT DISTINCT FROM syntax.

I wouldn't call it eoa since, techincally, you can place EIP-1271 and Presign orders as well.

Maybe standard?

Also, super nit-picky, but can we use Kind instead of Type, as it is the "Rust-y" naming convention for these kinds of enums?

nlordell

Changes look good to me! Only comment is the nit for naming of the onchain_signing_scheme: OnchainSigningScheme to kind: QuoteKind to make it more future proof.

database/sql/V027__quotes_adding_quote_kind.sql

crates/orderbook/src/order_quoting.rs

nlordell

Looks great! Thanks for incorporating all the changes.

Storing quotes longer, in case an on-chain order placement is intended

a8bd380

josojo requested a review from a team as a code owner August 5, 2022 07:58

josojo requested a review from fedgiac August 5, 2022 07:58

josojo commented Aug 5, 2022

View reviewed changes

formatting

a853a5c

fixing tests

96aed46

cargo fmt

5a06118

josojo mentioned this pull request Aug 5, 2022

Changing the quote-life-time depending on the order signature #355

Closed

vkgnosis reviewed Aug 5, 2022

View reviewed changes

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

vkgnosis reviewed Aug 5, 2022

View reviewed changes

crates/database/src/quotes.rs Outdated Show resolved Hide resolved

nlordell reviewed Aug 5, 2022

View reviewed changes

nlordell reviewed Aug 6, 2022

View reviewed changes

valentins suggestion

36f0a4f

fedgiac approved these changes Aug 8, 2022

View reviewed changes

Nick's nice suggestion for backwards compability

8fc2a86

nlordell approved these changes Aug 9, 2022

View reviewed changes

vkgnosis approved these changes Aug 9, 2022

View reviewed changes

josojo added 3 commits August 9, 2022 18:22

Federico's suggestion

673f75e

Federico's suggestion

3a7e5ff

Merge branch 'main' into differnt_quote_lifetimes3

77440f4

nlordell reviewed Aug 9, 2022

View reviewed changes

database/sql/V027__quotes_adding_quote_kind.sql Show resolved Hide resolved

nlordell reviewed Aug 9, 2022

View reviewed changes

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

nlordell approved these changes Aug 9, 2022

View reviewed changes

Merge branch 'main' into differnt_quote_lifetimes3

d095931

josojo enabled auto-merge (squash) August 10, 2022 08:59

josojo merged commit 5fc256c into main Aug 10, 2022

josojo deleted the differnt_quote_lifetimes3 branch August 10, 2022 09:01

github-actions bot locked and limited conversation to collaborators Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storing quotes longer, in case an on-chain order placement is intended #414

Storing quotes longer, in case an on-chain order placement is intended #414

josojo commented Aug 5, 2022 •

edited

Loading

josojo Aug 5, 2022 •

edited

Loading

nlordell Aug 6, 2022 •

edited

Loading

josojo Aug 7, 2022

nlordell Aug 8, 2022 •

edited

Loading

vkgnosis commented Aug 5, 2022

codecov-commenter commented Aug 5, 2022 •

edited

Loading

josojo commented Aug 5, 2022

vkgnosis commented Aug 5, 2022

nlordell Aug 5, 2022

nlordell left a comment

fedgiac left a comment

fedgiac Aug 8, 2022

nlordell Aug 9, 2022

josojo Aug 9, 2022

nlordell Aug 9, 2022 •

edited

Loading

nlordell left a comment

nlordell left a comment

Storing quotes longer, in case an on-chain order placement is intended #414

Storing quotes longer, in case an on-chain order placement is intended #414

Conversation

josojo commented Aug 5, 2022 • edited Loading

Test Plan

josojo Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

nlordell Aug 6, 2022 • edited Loading

Choose a reason for hiding this comment

josojo Aug 7, 2022

Choose a reason for hiding this comment

nlordell Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

vkgnosis commented Aug 5, 2022

codecov-commenter commented Aug 5, 2022 • edited Loading

Codecov Report

josojo commented Aug 5, 2022

vkgnosis commented Aug 5, 2022

nlordell Aug 5, 2022

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

fedgiac left a comment

Choose a reason for hiding this comment

fedgiac Aug 8, 2022

Choose a reason for hiding this comment

nlordell Aug 9, 2022

Choose a reason for hiding this comment

josojo Aug 9, 2022

Choose a reason for hiding this comment

nlordell Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

josojo commented Aug 5, 2022 •

edited

Loading

josojo Aug 5, 2022 •

edited

Loading

nlordell Aug 6, 2022 •

edited

Loading

nlordell Aug 8, 2022 •

edited

Loading

codecov-commenter commented Aug 5, 2022 •

edited

Loading

nlordell Aug 9, 2022 •

edited

Loading