Clean up storage #2818

tcharding · 2020-06-10T00:48:56Z

Refactor the storage module, this one is kinda big. Patches are discreet and explained in their commit logs.

Patch 1 - Fix up use statements
Patch 2 - Shorten the qualified path usage
Patch 3 - Refactor load of the spawn::Swap type. This refactor uses the style of generics as done for Save<CreatedSwap<A, B>>. Adds a couple of macros also, specifically one is added because the trait bounds for diesel trait BelongingtoDsl was too complicated to work out.
Patch 4 - Creates a sub-module http_api under storage and puts the load imlps for AliceSwap and BobSwap in it. Refactors heavily.
Patch 5 - trivial code re-order.

Patches 4 and 5 are the big ones, the diffs are not that easy to read - may be better to check out the branch locally and have a look at whats going on.

If there is anything I can do to make review easier for future reviews of these big refactor PRs please say.

Thanks

thomaseizinger · 2020-06-10T01:04:44Z

cnd/src/storage.rs

        &self,
        id: LocalSwapId,
-    ) -> anyhow::Result<spawn::Swap<herc20::Params, halight::Params>> {
+    ) -> anyhow::Result<(Swap, Herc20, Halight, Option<SecretHash>)> {


Could you make Herc20 and Halight generic here with a trait-bound to make the belonging_to method available?

Even though it is just a private method, having a struct with field names would make it clearer that .1 is alpha and .2 is beta.

If we establish this and succeed in making this generic, this would be a nice place to do #2781.

As a follow-up, we can define traits for:

derive_or_unwrap_secret_hash

build_herc20_params

build_halight_params

build_hbit_params

and make one generic impl that loads Params, similar to what we did for Save !

Can all be a follow-up so we don't make this one too big.

Thanks for the review, will process this information and work in what I can make work :)

Even though it is just a private method, having a struct with field names would make it clearer that .1 is alpha and .2 is beta.

These are not sides (alpha/beta), this function returns the trading pair - it is agnostic to the side.

cnd/src/storage/http_api.rs

luckysori

This is very good!

luckysori · 2020-06-10T23:34:39Z

cnd/src/storage.rs

-        wrapper_types::custom_sql_types::Text,
-        ForSwap, NoHalightRedeemIdentity, NoHalightRefundIdentity, NoHerc20RedeemIdentity,
-        NoHerc20RefundIdentity, NoSecretHash, Save, Sqlite,
+        self, ForSwap, Halight, Hbit, Herc20, NoHalightRedeemIdentity, NoHalightRefundIdentity,


🤔 This patch doesn't introduce the problem, but I find the names Halight, Hbit and Herc20 too general. Even if you're in the context of the storage module. I would find it clearer if we used them with a more qualified name, e.g table::Halight or even db::table::Halight (sorry I like long names [sometimes]).

db::<protocol-name> would be better, but still a bit lacking imo.

I have to disagree with you here mate. Apart from the fact that I always favour short names; the storage module is different in that it fully 'owns' the database, so bringing types from the db into scope is the natural thing to do IMO. The fact that the new tables are in the tables module is not important, when I'm writing/reading code in the storage module it is just like we are in the db layer so all the types are implicitly db types. Does that make sense? We don't have to agree :) (on this point, that's why I originally brought SecretHash into scope from the db and used comit::SecretHash when needed, you did it the other way around and I left it as such when I rebased :)

Yeah, I'm okay with this even if I don't love it 😉

the storage module is different in that it fully 'owns' the database

Would be nice to express that in our module hierarchy.

Oh, I like that.

db and seed being private scope modules under storage. Will wait for the remove of rfc003 before doing this, adding to my TODO list.

luckysori · 2020-06-10T23:40:19Z

cnd/src/storage.rs

-    spawn,
-    swap_protocols::state::Get,
-    LocalSwapId, Protocol, Role, Side, SwapContext,
+    spawn, Get, LocalSwapId, Protocol, Role, RootSeed, Side, SwapContext,


🙃 Very minor nit, but importing Get by itself is not great. Maybe it doesn't matter because it's just in the imports section, but I would rather see something like state::Get.

ok, I'm good with that. Will do.

cnd/src/storage/http_api.rs

thomaseizinger

Very nice work man!

Review was super easy patch-by-patch, really enjoyed that :)

Thanks!

thomaseizinger · 2020-06-12T01:10:00Z

cnd/src/storage.rs

-                let swap: Swap = swaps::table
-                    .filter(swaps::local_swap_id.eq(key))
-                    .first(conn)?;
+pub trait LoadTables<A, B> {


Nit: Rename to LoadProtocolTables?

Yep can do.

On second thoughts, this does not lead just the protocol tables, it also includes the Swaps table and the SecretHash table. Admittedly this is a very generic name and will likely clash next time we need to load a set of different tables. Leaving as is for now.

thomaseizinger · 2020-06-12T01:12:18Z

cnd/src/storage.rs

+                alpha.assert_side(Side::Alpha)?;
+                beta.assert_side(Side::Beta)?;


This is the only place where we use assert_side right?

Why not just:

Suggested change

alpha.assert_side(Side::Alpha)?;

beta.assert_side(Side::Beta)?;

if alpha.side.0 != Side::Alpha || beta.side.0 != Side::Beta {

anyhow::bail!("...")

}

I had that first but it didn't build. Unless I'm mistaken, we don't know the types here so we need to use a trait.

This should work. You are inside a macro so the code will just expand. Isn't the field called side for all tables?

tcharding · 2020-06-12T01:37:30Z

bors r+

bors · 2020-06-12T01:58:02Z

Merge conflict.

The import statements in the storage module are too noisy. The storage module fully 'owns' the database, it makes sense to just bring all the types from the db into scope. In an effort to move the whole code base in this direction we can also export `Get` up to the root level module and import it without the qualified path.

Now we have the tables types pub used in the db module we can shorten the qualified path used to differentiate the db `SecretHash` from the comit crate `SecretHash`.

Refactor the impl blocks for AliceSwap/BobSwap types by: - Move all the code for loading http_api module types into a sub-module of storage - Add generic conversion traits for creating the `Finalized[AsFoo]` types - Add From impls for asset conversion - Use recently added load_tables method to get the data we need instead of inline access to the database

`Get` is a bit unclear, lets qualify the import with it's module. Use qualified path `state::Get` when bring `Get` into scope.

mergify · 2020-06-12T03:23:32Z

bors r+

bors · 2020-06-12T03:57:35Z

Build succeeded:

tcharding requested review from thomaseizinger and luckysori June 10, 2020 00:49

github-actions bot assigned tcharding Jun 10, 2020

thomaseizinger reviewed Jun 10, 2020

View reviewed changes

luckysori approved these changes Jun 10, 2020

View reviewed changes

tcharding force-pushed the 2768-storage branch from fa89866 to 12bb057 Compare June 11, 2020 23:01

tcharding requested review from thomaseizinger, da-kami, D4nte and luckysori June 11, 2020 23:07

tcharding marked this pull request as ready for review June 11, 2020 23:07

thomaseizinger approved these changes Jun 12, 2020

View reviewed changes

tcharding force-pushed the 2768-storage branch from 7510f2d to 665db00 Compare June 12, 2020 01:33

tcharding added 6 commits June 12, 2020 12:46

Use shorter qualified path

7a8d0d7

Now we have the tables types pub used in the db module we can shorten the qualified path used to differentiate the db `SecretHash` from the comit crate `SecretHash`.

Refactor Load<spawn::Swap>

1ee6a38

Use uniform order for redeem/refund

614e7c3

Use qualified path for Get

29eae91

`Get` is a bit unclear, lets qualify the import with it's module. Use qualified path `state::Get` when bring `Get` into scope.

tcharding force-pushed the 2768-storage branch from 665db00 to 29eae91 Compare June 12, 2020 02:48

bors bot merged commit 46cad0b into dev Jun 12, 2020

mergify bot deleted the 2768-storage branch June 12, 2020 03:57

luckysori mentioned this pull request Jun 17, 2020

Assert side when loading HTLC parameters #2781

Closed

thomaseizinger mentioned this pull request Jun 26, 2020

Add assertions to ensure we retrieve the expected data when loading from the database #2722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up storage #2818

Clean up storage #2818

tcharding commented Jun 10, 2020 •

edited

thomaseizinger Jun 10, 2020

thomaseizinger Jun 10, 2020

thomaseizinger Jun 10, 2020

thomaseizinger Jun 10, 2020

tcharding Jun 10, 2020

tcharding Jun 10, 2020

luckysori left a comment

luckysori Jun 10, 2020

luckysori Jun 10, 2020

tcharding Jun 11, 2020

luckysori Jun 12, 2020

thomaseizinger Jun 12, 2020 •

edited

tcharding Jun 12, 2020

luckysori Jun 10, 2020

tcharding Jun 11, 2020

thomaseizinger left a comment •

edited

thomaseizinger Jun 12, 2020

tcharding Jun 12, 2020

tcharding Jun 12, 2020

thomaseizinger Jun 12, 2020

tcharding Jun 12, 2020

thomaseizinger Jun 12, 2020

tcharding commented Jun 12, 2020

bors bot commented Jun 12, 2020

mergify bot commented Jun 12, 2020

bors bot commented Jun 12, 2020

		alpha.assert_side(Side::Alpha)?;
		beta.assert_side(Side::Beta)?;

Clean up storage #2818

Clean up storage #2818

Conversation

tcharding commented Jun 10, 2020 • edited

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

luckysori 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 Jun 12, 2020 • edited

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 • edited

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

tcharding commented Jun 12, 2020

bors bot commented Jun 12, 2020

mergify bot commented Jun 12, 2020

bors bot commented Jun 12, 2020

tcharding commented Jun 10, 2020 •

edited

thomaseizinger Jun 12, 2020 •

edited

thomaseizinger left a comment •

edited