[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 _ <- getWalletState pw wid +-- TODO find a home for this +-- (NOTE: we are abandoning the 'Mockable time' strategy of the Cardano code base) +getCurrentTimestamp :: IO Timestamp +getCurrentTimestamp = Timestamp . round . (* 1000) <$> 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 <- prefilterBlock' pw b + -- TODO proper initialisation @uroboros Is this TODO tracked as part of any CBR item, so that we don't forget about it? I suggest creating a ticket (if not there already) and cross-reference the CBR item here. For example: ``` -- TODO(ryan): Tackle proper initialisation as part of CBR-XXXX ``` >
-totalBalance :: PassiveWallet -> WalletId -> IO Balance
-totalBalance pw wid = do
— availableBalance’ <- availableBalance pw wid - changeBalance' <- balance <$> 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’ <- availableBalance pw wid - changeBalance' <- balance <$> 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?

Добавить комментарий