[input-output-hk/cardano-sl] CBR-238 Use persistent state in data layer (#3012)

adinapoli-iohk requested changes on this pull request.

Apart from a minor thing I would like to clarify with @uroboros , this LGTM.

I am a bit concerned about the amount of TODOs we have in this code, not for the TODOs themselves, but because we might actually forget implementing that part of the code 😉

@edsko , do you feel this is a valid concerns or virtually all the stuff left here unimplemented have a corresponding CBR-227 issue? :)

>
-available :: PassiveWallet -> WalletId -> IO Utxo
-available pw wid = do
– State utxo pending _ getPOSIXTime

@uroboros Are we sure this is correct? AFAIK `getPOSIXTime` returns a `NominalDiffTime` (which is treated as seconds by conversion functions) and `Timestamp` requires `Microsecond`s to be passed, so that’s 10^-6 precision, but here you are multiplying by 10^3, so shouldn’t this be:

“`haskell
getCurrentTimestamp = Timestamp . round . (* 1000000) getPOSIXTime
“`

>
-balance :: Utxo -> Balance
-balance = sumCoins . map txOutValue . utxoOutputs
+– | Notify all the wallets in the PassiveWallet of a new block
+applyBlock :: PassiveWallet
+ -> ResolvedBlock
+ -> IO ()
+applyBlock pw@PassiveWallet{..} b
+ = do
+ blocksByAccount
-totalBalance :: PassiveWallet -> WalletId -> IO Balance
-totalBalance pw wid = do
– availableBalance’ change pw wid
– return $ availableBalance’ + changeBalance’
+– | Apply multiple blocks, one at a time, to all wallets in the PassiveWallet
+–
+– TODO: this will be the responsibility of @matt-noonan’s worker thread

If possible I would do the same as proposed above, i.e. replacing the TODO with also the relevant CBR ticket which would tackle this.

>
-totalBalance :: PassiveWallet -> WalletId -> IO Balance
-totalBalance pw wid = do
– availableBalance’ change pw wid
– return $ availableBalance’ + changeBalance’
+– | Apply multiple blocks, one at a time, to all wallets in the PassiveWallet
+–
+– TODO: this will be the responsibility of @matt-noonan’s worker thread
+applyBlocks :: PassiveWallet
+ -> OldestFirst [] ResolvedBlock
+ -> IO ()
+applyBlocks pw = mapM_ (applyBlock pw)

Minor thing, this could have been eta-reduced 😉

> +
+ {- potential future kinds of wallet IDs:
+ — | HD wallet with sequentially generated addresses
+ | WalletIdHdSeq …
+
+ — | External wallet (all crypto done off-site, like hardware wallets)
+ | WalletIdExt …
+ -}
+
+ deriving (Eq, Ord)
+
+– | Map of Wallet Master keys indexed by WalletId
+–
+– TODO: We may need to rethink having this in-memory
+– ESK should _not_ end up in the wallet’s acid-state log
+type WalletESKs = Map WalletId EncryptedSecretKey

Is this TODO still a concern? IIRC @edsko introduced the concept of `InDB` exactly to prevent this kind of situation from happening. Can we accidentally store an `EncryptedSecretKey` inside `acid-state`, as it stand?

This post was last modified on June 14, 2018, 4:16 pm