Replace hardcoded constant of 10 with a value set in constant config.
Details
Details
- Reviewers
emack Fabien - Group Reviewers
Restricted Project - Commits
- rABCf502b7b831b7: [Cashtab] Replace magic number with constant
Confirm we are just replacing a magic number with a constant
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 26331 Build 52232: Build Diff cashtab-tests Build 52231: arc lint + arc unit
Event Timeline
Comment Actions
A few extra refreshes seem to be happening which then confuses cashtab into momentarily thinking this is an empty wallet
| cashtab/src/config/websocket.js | ||
|---|---|---|
| 11 ↗ | (On Diff #44104) | It's impossible to guess what this does without reading the code, let alone guessing it's a refresh interval and what the unit is. This is a very bad name |
| cashtab/src/config/websocket.js | ||
|---|---|---|
| 11 ↗ | (On Diff #44107) | I don't get it. Why is this an interval if the goal is to trigger an immediate sync ? And how is this value relevant as a config option ? |
| cashtab/src/config/websocket.js | ||
|---|---|---|
| 11 ↗ | (On Diff #44107) | This diff is step one in a broader overhaul of how Cashtab handles wallet refreshes. The current system was originally designed for bch-api, then persisted for some time because chronik websocket connections were unreliable. Now that we are on chronik and have reliable websocket connections -- there is much room for improvement. The way Cashtab works right now -- there is always an ongoing interval to update the wallet every 10s. When Cashtab detects an incoming tx from the websocket, it changes this interval from 10s to 10ms (almost immediate...maybe zero would work? I don't remember, from when I implemented this there was probably some reason to go with a low nonzero number). So, the wallet immediately updates its utxo set on getting a tx. And in that function, flips the interval from 10ms back to 10s. The long term vision here is to do this almost all with websockets. The whole "have a refresh interval running all the time in the background" approach should be deprecated in favor with some kind of routine that triggers in the event of a lost websocket connection. Anyway this change is only replacing a magic number. Then will patch so wallet updates immediately on load, fixing a current bad ux issue in the extension (other diff in this stack). Then refactors for optimizations. |
| cashtab/src/config/websocket.js | ||
|---|---|---|
| 11 ↗ | (On Diff #44107) | Ok I better understand what this value is for, and now it's clear that it doesn't belong in a config file. This belongs in the place it's used with a proper comment explaining what it does and why it's not zero if needed. This value is not intended to be configurable. |
| cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 52 ↗ | (On Diff #44123) | Please comment what this is, what the unit is and why it can't be zero (if it really can't be zero) |
| cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 124 ↗ | (On Diff #44139) | you have debug logs here |