Add tracking of known transactions to wallet #137

rvl · 2019-04-01T08:25:12Z

Relates to issue #90.

Overview

Added a field to the Wallet type to track transaction metadata
Changed applyBlock to update this field.
Added properties for the above

Comments

This is WIP - it doesn't work yet - tests get stuck in a loop!
I don't know yet how to deal with time-dependent meta -- e.g. transaction time and "confirmations" (transaction depth from tip).

KtorZ · 2019-04-01T09:46:23Z

src/Cardano/Wallet/Primitive/Model.hs

+        deepseq (rnf pending) $
+        deepseq (rnf txMeta) $
+        deepseq (rnf sl) $
+        deepseq (rnf s) ()


reads better indeed.

KtorZ · 2019-04-01T09:48:22Z

src/Cardano/Wallet/Primitive/Types.hs

@@ -298,24 +302,32 @@ data TxMeta = TxMeta
    , depth :: !(Quantity "block" Natural)
    , status :: !TxStatus
    , direction :: !Direction
-    , timestamp :: !Timestamp
+    -- , timestamp :: !Timestamp -- fixme: can't calculate this yet


Timestamp should be computed rather than stored I believe. It's redundant with the slotId anyway. In the previous implementation, we arbitrarily chosed the beginning of a slot as the absolute date/time timestamp for a transaction (so that, we could also recover them from the chain when restoring a wallet).

This is also true for the depth. These values are relative to the current tip.

KtorZ · 2019-04-01T09:49:51Z

src/Cardano/Wallet/Primitive/Types.hs

+        deepseq (rnf n) $
+        deepseq (rnf st) $
+        deepseq (rnf dir) $
+        deepseq (rnf sl) ()


Can't we simply use the default instance for this one using Generic ? What's the rational of manually defining it here 🤔 ?

Yes - I didn't have time to add a NFData instance for Quantity, so worked around it here. But now the Quantity has gone from this data type.

KtorZ · 2019-04-01T14:07:48Z

src/Cardano/Wallet/Primitive/Model.hs

@@ -206,18 +223,33 @@ utxoFromTx tx@(Tx _ outs) =
 prefilterBlock
    :: Block
    -> Wallet s
-    -> (UTxO, Set TxIn, s)
-prefilterBlock b (Wallet !utxo _ _ !s) =
+    -> (UTxO, Set TxIn, Set Tx, s)


I've extended this one to also return our transactions (and avoid having to re-traverse the block in order to construct the metadata).

KtorZ · 2019-04-01T14:11:14Z

src/Cardano/Wallet/Primitive/Types.hs

    , status :: !TxStatus
    , direction :: !Direction
-    , timestamp :: !Timestamp
    , slotId :: !SlotId


So, I've removed the depth here since it depends on the current chain tip. The timestamp is also a bit tricky because we need to know the epoch length for each previous epoch in order to compute this correctly (timestamp = corresponding UTC value for the beginning of the slot). I've also removed it for now and I suggest to leave the calculation of this one to later.

I will still have to have a type to represent these fields for the user to implement return json return type for new transaction but I am assuming this return type will be created in Wallet.Api.Types where Primitive modules contain minimal data which might be persisted later (and probably will be) :

{ "id": "1423856bc91c49e928f6f30f4e8d665d53eb4ab6028bd0ac971809d514c92db1", "amount": { "quantity": 42000000, "unit": "lovelace" }, "inserted_at": { "time": "2019-04-01T19:26:18.474Z", "block": { "slot_number": 1337, "epoch_number": 14 } }, "depth": { "quantity": 0, "unit": 1337 }, "direction": "outgoing", "inputs": [ { "address": "2cWKMJemoBam7gg1y5K2aFDhAm5L8fVc96NfxgcGhdLMFTsToNAU9t5HVdBBQKy4iDswL", "amount": { "quantity": 42000000, "unit": "lovelace" } } ], "outputs": [ { "address": "2cWKMJemoBam7gg1y5K2aFDhAm5L8fVc96NfxgcGhdLMFTsToNAU9t5HVdBBQKy4iDswL", "amount": { "quantity": 42000000, "unit": "lovelace" } } ], "status": "pending" }

Yes indeed, that's where the difference between our API domain and business domain lies.

KtorZ · 2019-04-01T14:12:49Z

src/Cardano/Wallet/Primitive/Types.hs

-    } deriving (Show, Eq, Generic)
+    } deriving (Show, Eq, Ord, Generic)
+
+instance NFData TxMeta


@rvl Not sure why the instance was defined by-hand for TxMeta 🤔 ? I've changed that to rely on the default instance (which uses the generic representation). If there was a particular reason not to, I'd suggest to attach a comment explaining why ?

It was just a workaround

KtorZ · 2019-04-01T14:15:50Z

test/unit/Cardano/Wallet/Primitive/ModelSpec.hs

@@ -127,7 +130,8 @@ prop_applyBlockBasic s =
        in
            (ShowFmt utxo === ShowFmt utxo') .&&.
            (availableBalance wallet === balance utxo') .&&.
-            (totalBalance wallet === balance utxo')
+            (totalBalance wallet === balance utxo') .&&.
+            (getTxMetas wallet === mempty)


Note that the test still fails because of this last assertion, which is actually wrong (hence the failure). Actually, we will collect tx metas as part of the block application (because some of the addresses in the blocks are ours). Not sure if we can (and should) do any assertions in this particular prop on tx metas. This prop is about comparing the UTxO and balance of the basic model with our model (to make sure that our additions don't break the base logic).

For tx metas, they are probably some other properties we would like to check (like, if the UTxO isn't empty, the tx meta shouldn't be empty too..) or maybe a few more useful one. I haven't gave it many thoughts.

KtorZ · 2019-04-01T14:16:14Z

test/unit/Cardano/Wallet/Primitive/ModelSpec.hs

-    { ourAddresses :: Set Address
-    , discoveredAddresses :: Set Address
+    { _ourAddresses :: Set (ShowFmt Address)
+    , _discoveredAddresses :: Set (ShowFmt Address)


Makes the failure a bit nicer in QC.

KtorZ · 2019-04-01T14:17:47Z

test/unit/Cardano/Wallet/Primitive/ModelSpec.hs

-        [ WalletState (Set.fromList ours') mempty
-        | ours' <- shrinkList pure (Set.toList ours)
-        ]
+    shrink = genericShrink


So this was the reason for the tests hanging :| ... The shrinker was actually shrinking to itself, which caused QC to loop ... and loop ... and loop... I've changed for a generic shrinker which seems to do the job here as long as we make the output slightly nicer through the ShowFmt

Hm - I don't understand why this is so?

If wallet state is defined as its now as a pair of Set's - than my naive understanding is that shrinking for Set should work similary as shrinking for list [] .

but quick check at genericShrink $ fromList ([1,2,3] :: [Int]) tells me No instance for (Generic (Set Int)) which I didn't expect :/ . So I wonder why this is not defined - and if its not defined how ghc can derive Generic for WalletState ?

I thought Set is represented as a tree internally so I am confused why ghci can't find Generic reprensetantion of it. I see it can derive it somehow though. I wanted to have a look at internal representation of genericShrink for WalletState to understand why its stuck - seems unclear to me at first glance

It is / was stuck because the previous shrinker would in some cases yield the same value as the one being shrunk (which will cause QuickCheck to enter a shrinking-loop).

yes that makes sense - I didn't look at the old version.

I am still puzzled why ghci tells that Generic isnt defined for Set Int :|

There's no instance of Generic for Set. It can be derived, but it has to be explicit I suppose.

KtorZ · 2019-04-02T06:40:26Z

test/unit/Cardano/Wallet/Primitive/ModelSpec.hs

+              (Set.map status txMeta === Set.singleton InLedger) .&&.
+              (Set.findMin (Set.map txMetaSlotId txMeta) >=? SlotId 14 0) .&&.
+              (Set.findMax (Set.map txMetaSlotId txMeta) <=? SlotId 14 19)
+            ))


Hmm.. I'd rather have that in another property. It's good to keep tests focused on testing one and one thing. Here, it's really about comparing the UTxO.

Also, I don't get the first and two last assertions; Set.map direction txMeta === Set.singleton Incoming happens to be true on our testing data but isn't a property of tx metas in general 😕

KtorZ · 2019-04-02T06:42:39Z

src/Cardano/Wallet/Primitive/Types.hs

-    | Incoming
+    = Outgoing -- ^ Funds exit the wallet.
+    | Incoming -- ^ Funds enter the wallet.
+    | Internal -- ^ Payment from the wallet to itself.


Yeah, I was wondering yesterday if we couldn't give this one a special semantic :/ I was going for a simpler / less granular semantic where Outgoing just means not Incoming and Incoming means that there's no inputs from the wallet itself. So a transaction from the wallet to the wallet would effectively be labelled as Outgoing. I think it's nice to be slightly more granular here with the Internal.

it seemed that the shrinker shrunk wallet state to itself in some cases, which cause QC to enter in an infinite loop. Using a generic shrinker instead seems to do the job nicely.

…imitive.ModelSpec

…tion ordering in a block

We may actually end up managing checkpoints slightly differently than with k=2160 checkpoints in memory. Until we have finalized the design for checkpoints and rollbacks, we are better off without this as it just gets in the way

KtorZ

Reviewed, wrapped up and rebased.

rvl added this to the Transaction creation, submission & Coin Selection milestone Apr 1, 2019

rvl self-assigned this Apr 1, 2019

paweljakubas mentioned this pull request Apr 1, 2019

Keep track of known transactions in the wallet primitive logic #90

Closed

3 tasks

KtorZ self-requested a review April 1, 2019 09:45

KtorZ reviewed Apr 1, 2019

View reviewed changes

KtorZ reviewed Apr 2, 2019

View reviewed changes

KtorZ and others added 7 commits April 2, 2019 20:22

Fix 'Arbitrary WalletState' shrinker

e28ed92

it seemed that the shrinker shrunk wallet state to itself in some cases, which cause QC to enter in an infinite loop. Using a generic shrinker instead seems to do the job nicely.

Add tracking of known transactions to wallet

b3f2a61

Add Buildable instance for TxMeta and get nicer failure from QC in Pr…

00619ef

…imitive.ModelSpec

Fix chain producer from the HTTP-bridge

4d1c017

review prefiltering & fix calculation of metadata direction

7e5ebf8

Change 'Set Tx' to '[Tx]' in block representation to preserve transac…

5c7d690

…tion ordering in a block

remove checkpoints & NonEmpty wrapper around wallet state

77c5807

We may actually end up managing checkpoints slightly differently than with k=2160 checkpoints in memory. Until we have finalized the design for checkpoints and rollbacks, we are better off without this as it just gets in the way

KtorZ mentioned this pull request Apr 2, 2019

Fix the HTTP-bridge chain-producer #138

Closed

1 task

KtorZ force-pushed the rvl/90/wallet-apply-transactions branch from 8d996b9 to 77c5807 Compare April 2, 2019 18:24

KtorZ approved these changes Apr 2, 2019

View reviewed changes

KtorZ mentioned this pull request Apr 2, 2019

Compute metadata for transaction #101

Closed

KtorZ merged commit c8a276e into master Apr 2, 2019

KtorZ deleted the rvl/90/wallet-apply-transactions branch April 2, 2019 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracking of known transactions to wallet #137

Add tracking of known transactions to wallet #137

rvl commented Apr 1, 2019

KtorZ Apr 1, 2019

KtorZ Apr 1, 2019

KtorZ Apr 1, 2019

rvl Apr 2, 2019

KtorZ Apr 1, 2019

KtorZ Apr 1, 2019

akegalj Apr 1, 2019

KtorZ Apr 2, 2019

KtorZ Apr 1, 2019

rvl Apr 2, 2019

KtorZ Apr 1, 2019

KtorZ Apr 1, 2019

KtorZ Apr 1, 2019

akegalj Apr 1, 2019 •

edited

Loading

KtorZ Apr 2, 2019 •

edited

Loading

akegalj Apr 2, 2019

KtorZ Apr 2, 2019

KtorZ Apr 2, 2019

KtorZ Apr 2, 2019

KtorZ left a comment

Add tracking of known transactions to wallet #137

Add tracking of known transactions to wallet #137

Conversation

rvl commented Apr 1, 2019

Overview

Comments

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

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

akegalj Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ Apr 2, 2019 • edited Loading

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

KtorZ left a comment

Choose a reason for hiding this comment

akegalj Apr 1, 2019 •

edited

Loading

KtorZ Apr 2, 2019 •

edited

Loading