Storing call_data in quotes and order_quotes tables #3124
Conversation
|
Reminder: Please update the DB Readme. Caused by: |
database/sql/V075__add_call_data_to_quotes_and_order_quotes.sql
Outdated
Show resolved
Hide resolved
database/sql/V075__add_call_data_to_quotes_and_order_quotes.sql
Outdated
Show resolved
Hide resolved
|
@mstrug is this ready for review? |
Yes |
database/sql_revert/V075__revert_add_metadata_to_quotes_and_order_quotes.sql
Outdated
Show resolved
Hide resolved
| -- This migration script is reversible. | ||
|
|
||
| -- Step 1: Add two new columns to the quotes table | ||
| ALTER TABLE quotes |
There was a problem hiding this comment.
for historic quotes verified columns value could be wrong
That's true although I'd prefer a cleaner DB model over perfect historic data that will never be read. Also if anybody really cares about the accuracy of that data we know when the migration got applied and can detect historic values with the {} metadata value.
To support deserialization of |
| metadata: quote.data.metadata.try_into().map_err( | ||
| |e: serde_json::Error| { | ||
| AddOrderError::MetadataSerializationFailed(e.into()) | ||
| }, | ||
| )?, |
There was a problem hiding this comment.
nit:
| metadata: quote.data.metadata.try_into().map_err( | |
| |e: serde_json::Error| { | |
| AddOrderError::MetadataSerializationFailed(e.into()) | |
| }, | |
| )?, | |
| metadata: quote.data.metadata.try_into().map_err(AddOrderError::MetadataSerializationFailed)?, |
There was a problem hiding this comment.
To apply that change, I also had to updated definition of AddOrderError::MetadataSerializationFailed to take serde_json::Error as its unnamed field.
| #[serde(untagged)] | ||
| enum QuoteMetadataDeserializationHelper { | ||
| Data(QuoteMetadata), | ||
| Empty {}, |
There was a problem hiding this comment.
If we were to add this to QuoteMetadata wouldn't work directly without the need of another enum? 🤔
There was a problem hiding this comment.
As for QuoteMetadata I use #[serde(tag = "version")] to achieve this flattening and converting QuoteMetadata variants to version json tag, which implies requiring that version tag exists in the input json, which is not possible for empty json object.
Generally, to get automatic conversion from empty json object {} to Empty {} variant, we need to use #[serde(untagged)] which cannot be used on QuoteMetadata where we use #[serde(tag = "version")].
Alternative approach that I have also considered is a custom implementation of the serde deserialize trait, but this becomes more complicated and requires more code (additional implementation of visitor trait) comparing to helper type approach.
| @@ -44,6 +51,12 @@ pub enum TradeKind { | |||
| Regular(Trade), | |||
| } | |||
|
|
|||
| impl Default for TradeKind { | |||
There was a problem hiding this comment.
nit: cannot this be directly derived?
There was a problem hiding this comment.
Unfortunately it cannot be derived directly, as Rust currently is not supporting #[default] attribute on non-unit enum variants.
Description
Added two new non-null columns:
metadata(json) andverified(boolean) into tablesquotesandorder_quotes.Existing rows in database will be updated with
{}andfalsevalues.metadatacolumn is dedicated to store any additional data for the quote in json format. Currently it is used to store interactions received for/quoteresponse from the solvers, but it is prepared to store other data in future through versionedQuoteMetadatastruct as required.Database module only checks json validity, upper layers (orderbook, autopilot) uses
QuoteMetadatastruct for storing specific data in the database.Metadata can be used for verification/auditing of quotes/orders.
Added database migration scripts (update and revert) and updated database readme file.
How to test
Existing tests, also added dedicated e2e tests and unit tests for this new functionality .