-
Notifications
You must be signed in to change notification settings - Fork 217
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
Store TxHistory outside of the wallet state #150
Conversation
8d162b4
to
430ffcf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
, readCheckpoint = \key -> | ||
Map.lookup key <$> readMVar wallets | ||
fmap fst . Map.lookup key <$> readMVar db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so basically we are storing the pair (wallet state, txs) and keep it separated. And we have separate db calls for the first one and transactions part. And txs history is not polluting and affecting wallet state -> straightforward property to test
@@ -103,7 +111,7 @@ data Wallet s where | |||
Wallet :: (IsOurs s, NFData s, Show s) | |||
=> UTxO -- Unspent tx outputs belonging to this wallet | |||
-> Set Tx -- Pending transactions | |||
-> Set (Tx, TxMeta) -- Transaction history | |||
-> Set (Hash "Tx", TxMeta) -- Transaction history |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, we probably do not need to store whole transaction content, its identifier is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, I think we don't even need to store the TxMeta
. Not sure why I kept them here 🤔 ...
\db -> (checkCoverage $ dbMergeTxHistoryProp db) | ||
it "can't read Tx history if there's no checkpoint" $ | ||
\db -> (property $ dbPutTxHistoryBeforeCheckpointProp db) | ||
it "putTxHistory leaves the wallet state untouched" $ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh here is the property I thought immediately when got idea behind decoupling of wallet state and txs 💯 but there is symmetry to be tested, ie., also on the contrary - changing the state leaves TxHistory unaffected, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing the state leaves TxHistory unaffected, right?
Indeed. I've got a bit lazy here 😅 I'll add.
sumOfSizes k = sum $ Map.size . snd <$> restrictTo k keyValPairs | ||
prop = monadicIO $ liftIO $ do | ||
forM_ keyValPairs $ \(key, val) -> do | ||
putCheckpoint db key (initWallet $ DummyState 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so we are here putting tx histories using between (10,50) pairs, let's say X and having X wallet state checkpoints, each having DummyState 0
. Then, for each key form X we read tx history. And expect that at each key the corresponding value is the same from res and when looking at keyValPairs (here Map.unions gather values if the key occurs more than once)
restrictTo k = filter ((== k) . fst) | ||
-- Make sure that we have some conflicting insertion to actually test the | ||
-- semantic of the DB Layer. | ||
cond = map sumOfSizes ids /= map sizeOfUnion ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so in 90% of cases we have the situation that the same key occurred more than once in keyValPairs and hence the values coalesced. If yes, can't we just put :
(L.length . L.nub) ids < L.length ids
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm.... 🤔 It's true that we only get conflict if we have the same wallet ids and some similar tx ids. So it should be enough indeed to just look at the ids and not at the balance to know whether we're going to have conflict. The balance being different is just a consequence of the id being in conflict. Good point. I'll simplify that 👍
This is because transactions may end up being rather big and, are of little use for the wallet state. Instead, keeping track of the tx ids only is sufficient. Storing transactions on a separate level in the DB also allows for querying them independently with, later, more advanced features like sorting and filtering
430ffcf
to
4c17333
Compare
Issue Number
#90
Overview
Tx
outside of the wallet state (which does now only keep track of the known TxId and TxMeta)putTxHistory
andreadTxHistory
Comments
This is because transactions may end up being rather big and, are of little use for the wallet
state. Instead, keeping track of the tx ids only is sufficient. Storing transactions on a
separate level in the DB also allows for querying them independently with, later, more advanced
features like sorting and filtering
NOTE: More tests on the wallet model are introduced by @rvl in #148 so I really focused on testing the DB layer here.