Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tracking of known transactions to wallet #137

Merged
merged 7 commits into from
Apr 2, 2019

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Apr 1, 2019

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).

deepseq (rnf pending) $
deepseq (rnf txMeta) $
deepseq (rnf sl) $
deepseq (rnf s) ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reads better indeed.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

deepseq (rnf n) $
deepseq (rnf st) $
deepseq (rnf dir) $
deepseq (rnf sl) ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

, status :: !TxStatus
, direction :: !Direction
, timestamp :: !Timestamp
, slotId :: !SlotId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} deriving (Show, Eq, Generic)
} deriving (Show, Eq, Ord, Generic)

instance NFData TxMeta
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just a workaround

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{ ourAddresses :: Set Address
, discoveredAddresses :: Set Address
{ _ourAddresses :: Set (ShowFmt Address)
, _discoveredAddresses :: Set (ShowFmt Address)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes the failure a bit nicer in QC.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@akegalj akegalj Apr 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@KtorZ KtorZ Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :|

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(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)
))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😕

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

KtorZ and others added 7 commits April 2, 2019 20:22
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.
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 KtorZ mentioned this pull request Apr 2, 2019
1 task
@KtorZ KtorZ force-pushed the rvl/90/wallet-apply-transactions branch from 8d996b9 to 77c5807 Compare April 2, 2019 18:24
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed, wrapped up and rebased.

@KtorZ KtorZ merged commit c8a276e into master Apr 2, 2019
@KtorZ KtorZ deleted the rvl/90/wallet-apply-transactions branch April 2, 2019 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants