Db refactor #295

DeliciousHair · 2021-10-13T05:00:43Z

DB code refactoring for #88

The primary goal was to remove all of the calls to serde_json::to_string() for the data-handling, thus enabling us to do (more or less):

Order {
    row.column,
    ...
}

as well as clean up the SQL for easier reading. This has mostly been accomplished, with further refinements easily accomplished once the upstream issues in sqlx are addressed. See #314 for issues we are tracking.

thomaseizinger

Thanks!

At the moment, there are multiple changes mangled up here. I can already see that this will prove tricky to untangle into multiple, distinct commits.

I would suggest the following:

Make a new branch off master where you just change the SQL queries to use the with syntax and move them into separate files without any of the improvements around sqlx::Type etc.

Given that you already have the queries, that should be reasonably easy. On top of that, we can then introduce sqlx::Type etc and get rid of all the serde_json stuff!

thomaseizinger · 2021-10-13T05:05:28Z

daemon/migrations/20210903050345_create_cfd_and_order_tables.sql

+    leverage                        integer     not null,
+    liquidation_price               text        not null,
+    creation_timestamp_seconds      integer     not null,
+    creation_timestamp_nanoseconds  integer     not null,


I think I'd rather drop the nanoseconds. What do you think @da-kami ?

@thomaseizinger @da-kami I've left the ns in for the moment as they are now handled without issue. Can drop them, but it looks like that may lead to a bit more work upstream as the timestamps are generated by SystemTime::new() so if the ns are dropped, things may not line up.

Not a big deal either way, this just seemed the path of least resistance for the moment.

thomaseizinger · 2021-10-13T05:06:58Z

daemon/src/db.rs

@@ -296,8 +306,8 @@ pub async fn load_cfd_by_order_id(
    let max_quantity = serde_json::from_str(row.max_quantity.as_str()).unwrap();


Parsing max_quantity via serde here is not going to work is it? We are only to_stringing it on the insert.

FYI, all things that involve serde_json::from_str() will go away with one exception:

// TODO: // still have the use of serde_json::from_str() here, which will be dealt with // via https://github.com/comit-network/hermes/issues/290 Ok(Cfd { order, quantity_usd: Usd(Decimal::from_str(&row.quantity_usd).unwrap()), state: serde_json::from_str(row.state.as_str()).unwrap(), })

daemon/src/db.rs

thomaseizinger · 2021-10-13T05:07:40Z

daemon/src/model.rs

@@ -14,7 +14,8 @@ use uuid::Uuid;

 pub mod cfd;

-#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq)]
+#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq, sqlx::Type)]
+#[sqlx(type_name = "String")]


IIRC, this only does something for postgres so we can remove it.

This is unaddressed !

thomaseizinger · 2021-10-13T05:08:32Z

daemon/src/db_sql/query_load_cfd_by_order_id.sql

+    state.state
+
+from order
+    inner join state on state.order_id = order.order_id


Missing newlines on these.

thomaseizinger · 2021-10-13T05:10:11Z

daemon/src/db_sql/query_load_all_cfds.sql

@@ -0,0 +1,69 @@
+with order as (


I'd prefer if the changes were made in two steps:

Moving queries to external files

Changing up the query to use with

thomaseizinger · 2021-10-13T06:56:58Z

Actually, all things considered it is not that important to split things up into multiple commits so feel free to squash everything together!

Overall, these changes are a major improvement, thank you!

thomaseizinger

Thank you! Massive improvement over what we currently have!

Found what I think is a regression.
A few unaddressed comments and commits need to be squashed together.

daemon/src/db_sql/query_load_all_cfds.sql

thomaseizinger · 2021-10-13T23:22:28Z

daemon/src/model.rs

@@ -14,7 +14,8 @@ use uuid::Uuid;

 pub mod cfd;

-#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq)]
+#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq, sqlx::Type)]
+#[sqlx(type_name = "String")]


This is unaddressed !

daemon/src/db.rs

daemon/src/model/cfd.rs

thomaseizinger

Thanks!

2 more tiny changes and this is good to go in. Just amend the now only commit for that (git commit --amend). Also, can you reword the commit message to something more descriptive please?

thomaseizinger · 2021-10-14T04:15:06Z

Cargo.lock

@@ -4,9 +4,9 @@ version = 3

 [[package]]
 name = "ahash"
-version = "0.7.4"
+version = "0.7.5"


These update must have been accidental. Can you undo that please?

git checkout master -- Cargo.lock should do the trick.

thomaseizinger · 2021-10-14T04:16:49Z

daemon/src/db.rs

    })
 }

 pub async fn insert_cfd(cfd: Cfd, conn: &mut PoolConnection<Sqlite>) -> anyhow::Result<()> {
-    let mut tx = conn.begin().await?;


Cool! No more transaction for this!

thomaseizinger · 2021-10-14T04:19:05Z

daemon/src/model.rs

@@ -13,7 +13,7 @@ use uuid::Uuid;

 pub mod cfd;

-#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq)]
+#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq, sqlx::Type)]


I think this is not actually doing anything because we are not specifying #[sqlx(transparent)].

Can you remove it to avoid confusion about what this is doing?

Oops! Yes, removed now.

The primary goal was to remove all of the calls to `serde_json::to_string()` for the data-handling, thus enabling us to do (more or less): ```rust Order { row.column, ... } ``` as well as clean up the SQL for easier reading. This has mostly been accomplished, with further refinements easily accomplished once the upstream issues in `sqlx` are addressed. See #314 for issues we are tracking.

thomaseizinger

Thanks!

Feel free to bors r+ once CI is green.

thomaseizinger · 2021-10-14T05:43:31Z

bors r+

bors · 2021-10-14T05:55:48Z

Build succeeded:

334: Revert "Db refactor" r=thomaseizinger a=thomaseizinger Reverts #295 Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

thomaseizinger suggested changes Oct 13, 2021

View reviewed changes

thomaseizinger reviewed Oct 13, 2021

View reviewed changes

daemon/src/model/cfd.rs Outdated Show resolved Hide resolved

DeliciousHair force-pushed the db_refactor branch from ff04b2f to e43536a Compare October 14, 2021 04:12

DeliciousHair mentioned this pull request Oct 14, 2021

Additional DB refactoring #314

Closed

4 tasks

thomaseizinger suggested changes Oct 14, 2021

View reviewed changes

thomaseizinger marked this pull request as ready for review October 14, 2021 04:26

thomaseizinger requested a review from da-kami October 14, 2021 04:26

DeliciousHair force-pushed the db_refactor branch from 8687307 to 5cf88a2 Compare October 14, 2021 05:36

thomaseizinger approved these changes Oct 14, 2021

View reviewed changes

bors bot merged commit 4900538 into master Oct 14, 2021

bors bot deleted the db_refactor branch October 14, 2021 05:55

da-kami mentioned this pull request Oct 14, 2021

Refactor the database loading/saving for readability #88

Closed

thomaseizinger mentioned this pull request Oct 15, 2021

Revert "Db refactor" #334

Merged

bors bot added a commit that referenced this pull request Oct 15, 2021

Merge #334

228d5dc

334: Revert "Db refactor" r=thomaseizinger a=thomaseizinger Reverts #295 Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Db refactor #295

Db refactor #295

DeliciousHair commented Oct 13, 2021 •

edited

thomaseizinger left a comment

thomaseizinger Oct 13, 2021

DeliciousHair Oct 13, 2021

thomaseizinger Oct 13, 2021

DeliciousHair Oct 13, 2021

thomaseizinger Oct 13, 2021

thomaseizinger Oct 13, 2021

thomaseizinger Oct 13, 2021

thomaseizinger Oct 13, 2021

thomaseizinger commented Oct 13, 2021

thomaseizinger left a comment

thomaseizinger Oct 13, 2021

thomaseizinger left a comment

thomaseizinger Oct 14, 2021

thomaseizinger Oct 14, 2021

thomaseizinger Oct 14, 2021

DeliciousHair Oct 14, 2021

thomaseizinger left a comment

thomaseizinger commented Oct 14, 2021

bors bot commented Oct 14, 2021

		@@ -296,8 +306,8 @@ pub async fn load_cfd_by_order_id(
		let max_quantity = serde_json::from_str(row.max_quantity.as_str()).unwrap();

Db refactor #295

Db refactor #295

Conversation

DeliciousHair commented Oct 13, 2021 • edited

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Oct 13, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Oct 14, 2021

bors bot commented Oct 14, 2021

DeliciousHair commented Oct 13, 2021 •

edited