unify 'watchWallet', 'listen', 'processWallet', 'tick' etc .. into one 'restoreWallet' #172
Conversation
| , withLock | ||
| :: forall e a. () | ||
| => ExceptT e m a | ||
| -> ExceptT e m a |
There was a problem hiding this comment.
Tests for this incoming soon.
There was a problem hiding this comment.
LGTM at first glance. Will also think about this chunk of code if I have chances to review more tests for this. See that in tests we have some discrepances between expectations and actual results ... would be really easier if those magic numbers like 21599 would have some names
| rbNextBlocks | ||
| :: Monad m | ||
| => HttpBridge m -- ^ http-bridge API | ||
| -> SlotId -- ^ Starting point | ||
| -> ExceptT ErrNetworkUnreachable m [Block] | ||
| rbNextBlocks network start = maybeTip (getNetworkTip network) >>= \case | ||
| Just (tipHash, tipHdr) -> do | ||
| epochBlocks <- nextStableEpoch (epochNumber start) | ||
| epochBlocks <- | ||
| if slotNumber start >= 21599 |
There was a problem hiding this comment.
every epoch has 21600 slots counting from 0 to 21599, right? Maybe it would be good to inject this value for the sake of testing boundaries without relying on mainnet
| n0 = flat ep0 sl0 | ||
| n1 = flat ep1 sl1 | ||
| tolerance = 5 | ||
| in if distance n0 n1 < tolerance then |
There was a problem hiding this comment.
later we can use soon to be distance function from Primitives.Types
| -- tip of the wallet state. | ||
| let nonEmpty = not . null . transactions | ||
| let (h,q) = first (filter nonEmpty) $ splitAt (length blocks - 1) blocks | ||
| let (txs, cp') = applyBlocks (h ++ q) cp |
There was a problem hiding this comment.
isn't (h,q) first splitting blocks into two parts, then by first applying filter to both parts, and then when used as (h++q) the same as filter nonEmpty blocks ???
There was a problem hiding this comment.
ahh, no. Control.Arrow 's first will filter for the first group only, the second leaving intact... so q has one block and can be with or without transactions...but when it comes to applyBlocks if it has transactions belonging to us it has chances to be processed, but if not having transactions then no chances for sure...so why not use instead of (h++q) still filter nonEmpty blocks?
There was a problem hiding this comment.
As the note above indicates, we want to process the last block of the batch, always even if empty or irrelevant to us to update our current tip accordingly!
…e 'restoreWallet'
KtorZ
force-pushed
the
KtorZ/review-wallet-restore
branch
from
e4404c6 to
54934eb
Compare
|
By the way, restoring from
|
Issue Number
#96
Overview
I have reviewed the wallet layer to introduce one resilient
restoreWalletfunction instead of the test placeholderswatchWalletandprocessWalletfunctions.restoreWalletalso computes (and update) the progress.I have slightly changed the http bridge implementation and API semantic so that if will get all blocks STRICTLY after the provided slots (was inclusive before). This seemingly means that we will never get to fetch the very first slot (
SlotId 0 0) through thenextBlocksfunction which is most probably fine for every chain we have; or, simply needs to be handled by the wallet layer code itself.I have added some dummy logging until we introduce an actual logger.
Comments
I have only tested this manually by running the benchmark (but I am planning to add more tests) to see whether the wallet was correctly restored. Note that, the "restoreWallet" never ends, it's actually a full-blown worker which will keep the node in sync and will only stop if the node is deleted.