Extend DB Layer & Wallet to support wallet metadata #158

KtorZ · 2019-04-05T15:32:44Z

Issue Number

Overview

I have reviewed a bit the interface of the DB layer to be more consistent across various "resources"
I have extended the db layer to handle wallet metadata, and fixed the MVar implementation accordingly
I have updated tests and wrote a few more for the metadata operations

Comments

This is still very drafty, the MVar tests are a lot of the same thing. I'd like to spend some time refactoring them in order to abstract away the core properties we are testing on each resource, and make them more readable.

This slightly increases the readability and makes future (incoming) extensions easier

paweljakubas

LGTM - really fond of the construct :

newtype ErrSome (operation :: Symbol)

paweljakubas · 2019-04-08T18:04:32Z

src/Cardano/Wallet/DB.hs

@@ -62,13 +94,18 @@ data DBLayer m s = DBLayer
        :: PrimaryKey WalletId
        -> m (Map (Hash "Tx") (Tx, TxMeta))
        -- ^ Fetch the current transaction history of a known wallet. Returns an
-        -- empty map if the wallet isn't found.
+        -- empty map if the wallet istn't found.


this was not necessary amendment

Erf. Seems like a merge resolution gone wrong.

paweljakubas · 2019-04-08T18:05:36Z

src/Cardano/Wallet/DB.hs

-    = ErrNoSuchWallet WalletId
-    deriving (Show, Eq)
+-- | Can't perform given operation because there's no wallet
+newtype ErrNoSuchWallet (operation :: Symbol)


neat way to encode additional info to which operation the error is related 💯

paweljakubas · 2019-04-08T18:07:32Z

src/Cardano/Wallet/DB/MVar.hs

+data Database s = Database
+    { wallet :: Wallet s
+    , metadata :: WalletMetadata
+    , txHistory :: Map (Hash "Tx") (Tx, TxMeta)


to these days cannot understand why we need TxMeta (not related to this PR)

We need them because our users need them ;)
Those pieces of information (direction, timestamp, total amount etc..) are very valuable to API consumers.

paweljakubas · 2019-04-08T18:11:47Z

src/Cardano/Wallet/DB/MVar.hs

+                                   Wallet Metadata
+        -----------------------------------------------------------------------}
+
+        , putWalletMeta = \key@(PrimaryKey wid) meta -> ExceptT $ do


wonder if fine-tuning type with "Symbol" would not do the trick here to embrace one impl of putCheckpoint and readCheckpoint for both checkpoint and metadata - the implementations are very close ....

paweljakubas · 2019-04-08T18:36:27Z

test/unit/Cardano/Wallet/DB/MVarSpec.hs

-    putCheckpoint db key cp1
-    txs' <- readTxHistory db key
-    txs' `shouldBe` txs
+dbPutCheckpointIsolationProp db key (cp,meta,txs) cp' = monadicIO $ liftIO $ do


there is a number of tests that are permutations of put{TxHistory, WalletMeta, Checkpoint}. Basically, we have :

CreateWallet

Two puts

Two reads where one does not overlap with one of 2.
I wonder if we can replace all those tests with one, where we create wallet, than shuffle list of pairs (operation and arg like txs, cp meta). Moreover, we can make it more general. For instance, generate :

([Wallet DummyState], [WalletMetadata], [Map (Hash "Tx") (Tx, TxMeta)])

and even make sure the lengths of those three lists are different. then construct operations with generated values using those. Then shuffle. In that case we will test a number of what should be independent operations, some of them updating and at the end we expect last values of each list to be returned upon corresponding reads ...

Of course this can be a separate PR - but I think it would be valuable to have those tests, mainly because we will end up with one sequential test and one parallel test for a lot of permutations and multiple updates

Indeed! That's what I mentioned to you on Slack. I am not yet fully satisfied with the testing situation here, although it tests things, we can feel the duplication and weight of the tests here.
Although it's tricky to come up with a unified approach for operation agnostic properties here, i think it's valuable in the long run.

KtorZ · 2019-04-09T06:36:38Z

~~Merging as-is for now. Will tackle tests refactoring as debts (adding this to the blackboard).~~

rvl

Looks good

rvl · 2019-04-09T07:51:25Z

src/Cardano/Wallet.hs

-        putCheckpoint db (PrimaryKey wid) cp'
-        unsafeRunExceptT $ putTxHistory db (PrimaryKey wid) txs -- Safe after ^
+        -- FIXME
+        -- Note that, the two calls below are _safe_ under the assumption that


Maybe also add a note that there is no transaction surrounding readCheckpoint and putCheckpoint because there will be only one thread applying blocks.

… factoring out a few things

KtorZ · 2019-04-09T10:49:14Z

@paweljakubas I did take a shot at refactoring the tests to make them easier to read and be less-redundant. I actually caught an implementation bug doing so :)
It's kinda nice now, and it'll nicely extend with new DB operations.

KtorZ changed the title ~~Ktor z/145/wallet metadata~~ Extend DB Layer & Wallet to support wallet metadata Apr 5, 2019

KtorZ self-assigned this Apr 5, 2019

use a record type instead of a tuple for our mocked db

52dddfa

This slightly increases the readability and makes future (incoming) extensions easier

KtorZ marked this pull request as ready for review April 8, 2019 15:34

Anviking mentioned this pull request Apr 8, 2019

Finalize manipulation of wallet metadata #145

Closed

3 tasks

KtorZ force-pushed the KtorZ/145/wallet-metadata branch from d163142 to 5b3d203 Compare April 8, 2019 15:35

KtorZ mentioned this pull request Apr 8, 2019

Scaffold WalletLayer for tx creation #164

Merged

3 tasks

paweljakubas approved these changes Apr 8, 2019

View reviewed changes

rvl reviewed Apr 9, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/145/wallet-metadata branch from 5b3d203 to 19e9502 Compare April 9, 2019 10:39

KtorZ added 9 commits April 9, 2019 12:43

add correct support for wallet metadata in the wallet layer

042806b

write a few more property to tests WalletMeta: room for abstracting &…

f805b06

… factoring out a few things

define a generic property 'read after put'

b7db50e

add generic property "can't put before wallet exists" for db resources

fee0feb

add generic 'isolation' property

0f8e1e9

re-organize generic properties around properties (and not resources)

5af4423

add generic property to test sequential insertions to DB

ec6374b

add generic property for parallel DB insertion

762f785

add extra properties to test db initialization functions

c3b1a93

KtorZ force-pushed the KtorZ/145/wallet-metadata branch from 19e9502 to c3b1a93 Compare April 9, 2019 10:48

KtorZ merged commit eb50d4d into master Apr 9, 2019

KtorZ deleted the KtorZ/145/wallet-metadata branch April 9, 2019 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend DB Layer & Wallet to support wallet metadata #158

Extend DB Layer & Wallet to support wallet metadata #158

KtorZ commented Apr 5, 2019 •

edited

Loading

paweljakubas left a comment

paweljakubas Apr 8, 2019 •

edited

Loading

KtorZ Apr 9, 2019

paweljakubas Apr 8, 2019

paweljakubas Apr 8, 2019 •

edited

Loading

KtorZ Apr 9, 2019

paweljakubas Apr 8, 2019

paweljakubas Apr 8, 2019

KtorZ Apr 9, 2019

KtorZ commented Apr 9, 2019 •

edited

Loading

rvl left a comment

rvl Apr 9, 2019

KtorZ commented Apr 9, 2019

Extend DB Layer & Wallet to support wallet metadata #158

Extend DB Layer & Wallet to support wallet metadata #158

Conversation

KtorZ commented Apr 5, 2019 • edited Loading

Issue Number

Overview

Comments

paweljakubas left a comment

Choose a reason for hiding this comment

paweljakubas Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ Apr 9, 2019

Choose a reason for hiding this comment

paweljakubas Apr 8, 2019

Choose a reason for hiding this comment

paweljakubas Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ Apr 9, 2019

Choose a reason for hiding this comment

paweljakubas Apr 8, 2019

Choose a reason for hiding this comment

paweljakubas Apr 8, 2019

Choose a reason for hiding this comment

KtorZ Apr 9, 2019

Choose a reason for hiding this comment

KtorZ commented Apr 9, 2019 • edited Loading

rvl left a comment

Choose a reason for hiding this comment

rvl Apr 9, 2019

Choose a reason for hiding this comment

KtorZ commented Apr 9, 2019

KtorZ commented Apr 5, 2019 •

edited

Loading

paweljakubas Apr 8, 2019 •

edited

Loading

paweljakubas Apr 8, 2019 •

edited

Loading

KtorZ commented Apr 9, 2019 •

edited

Loading