[Wallet] Fix web3 syncing sagas #809
Conversation
| @@ -17,7 +17,7 @@ export function* waitForRehydrate() { | |||
|
|
|||
| export function* waitWeb3LastBlock() { | |||
| yield waitForGethConnectivity() | |||
| yield checkWeb3Sync() | |||
| yield waitForWeb3Sync() | |||
There was a problem hiding this comment.
@cmcewen IIRC you recommended calling this check but wait is what it's really doing. Mind if we rename?
There was a problem hiding this comment.
Volpe and I had a discussion about this - the reason I don't like using wait is that it actually makes a call to get the latest block to the geth node. Since I think we treat the waitFor generators as free in terms of cost, I would be wary of someone just adding this elsewhere and we end up doing a bunch of duplicate polling. I think waitFor should imply solely waiting on state changes.
| _CHECK_SYNC_PROGRESS_TIMEOUT, | ||
| _checkWeb3SyncProgressClaim, | ||
| checkWeb3Sync, | ||
| checkWeb3SyncProgress, |
There was a problem hiding this comment.
@martinvol I do like the convention of _ before functions only exported to test, but until this VSCode bug gets fixed, it just causes a headache :(
prettier/prettier-vscode#716
We'd need to either disable VS code import organizing entirely (would not recommend) or fix this file manually before lint will pass.
Suggest we table this convention for now.
There was a problem hiding this comment.
Cool, no problem. Disabling VS code import organizing might not be a bad a idea since the pre-commit hook also sorts imports.
There was a problem hiding this comment.
Yeah but the way it de-dupes imports for you and removes unused ones is really nice
| } else { | ||
| Logger.error(TAG, `checking web3 sync progress: ${error}`) | ||
| Logger.error(TAG, 'Unexpected sync error', error) |
There was a problem hiding this comment.
@martinvol it's better to provide the error as the 3rd param to Logger.error, which has special logic to extract and format the error message nicely
| } | ||
| return false |
There was a problem hiding this comment.
@martinvol previously, you were not throwing or returning false in this case, which allowed the calling function to finish as though syncing were done.
| @@ -36,68 +34,67 @@ const MNEMONIC_BIT_LENGTH = (ETH_PRIVATE_KEY_LENGTH * 8) / 2 | |||
|
|
|||
| const TAG = 'web3/saga' | |||
| // The timeout for web3 to complete syncing and the latestBlock to be > 0 | |||
| const CHECK_SYNC_PROGRESS_TIMEOUT = 60000 | |||
| export const SYNC_TIMEOUT = 60000 | |||
| const BLOCK_CHAIN_CORRUPTION_ERROR = "Error: CONNECTION ERROR: Couldn't connect to node on IPC." | |||
There was a problem hiding this comment.
Can you remove this to a json as nice to have?
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
=========================================
- Coverage 66.94% 66.9% -0.05%
=========================================
Files 239 239
Lines 7121 7103 -18
Branches 408 474 +66
=========================================
- Hits 4767 4752 -15
+ Misses 2277 2273 -4
- Partials 77 78 +1
Continue to review full report at Codecov.
|
* master: Export geth metrics from StatefulSet tx-nodes and validators (#827) Adjust governance end-to-end gold total supply block number (#853) [Blockchain API] Add currency conversion endpoint to blockchain api (#855) [Wallet] Fix Payment Requests and DRY up confirmation card code (#831) Use HTTPS instead of SSH for ethereumjs-vm (#850) Kill end-to-end celo-blockchain instances with SIGINT (#849) [CLI]Minor code improvement in CLI, remove ts-ignore-next-line (#796) [wallet] Always allow users to view backup key (#825) [contractkit] Complete Exchange Wrapper (#801) [Wallet] Fix web3 syncing sagas (#809) Aaronmgdr/social links connect (#792) Deploy integration (#806) [wallet] upgrade react-redux (#812)
Description
The current web3 sync saga would sometimes allow sagas waiting for web3 ready to proceed even before web3 is really ready, causing tx errors. This fixes that issue and cleans up a bit, particularly around the related redux state.
Tested
Synced to Alfajores and sent a tx. Confirmed logs look right.
Related issues
Backwards compatibility
Yes