-
Notifications
You must be signed in to change notification settings - Fork 973
Fix persistence edge-case issue with ledger client #11495
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11495 +/- ##
==========================================
- Coverage 52.5% 52.45% -0.05%
==========================================
Files 268 268
Lines 25241 25248 +7
Branches 4028 4030 +2
==========================================
- Hits 13252 13245 -7
- Misses 11989 12003 +14
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might still be an edge case where the callback from onBitcoinToBatTransitioned is handled first and the browser exits before onLedgerCallback is handled. So the ledger-state.json would be BTC, ledger-newstate.json would be BAT but the browser would not attempt to finish the transition process (since it would believe it was already over).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of merging this as is and opening up a new issue for the above if it is indeed possible.
The timing required to hit the edge case I mentioned above would have to be much tighter than the edge case this fixes.
@@ -74,6 +74,7 @@ const v2RulesetPath = 'ledger-rulesV2.leveldb' | |||
let v2PublishersDB | |||
const v2PublishersPath = 'ledger-publishersV2.leveldb' | |||
const statePath = 'ledger-state.json' | |||
const newClientPath = 'ledger-newstate.json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to const newClientPath = pathName('ledger-newstate.json')
, so that we call pathName
only once. Same can be done with statePath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at doing this, but since it uses electron.app.getPath()
(which may or may not be initialized at the time module is loaded), I think changing that may be better to do in a different PR. Thanks for the suggestion though 😄
Ran into an issue while testing... Here's what I did:
|
sorry but what's the status of this? error mentioned by @bsclifton seems related to a different issue, is this blocked on something? |
@cezaraugusto thanks for checking up on this 😄 I checked with @evq and we're good to merge this! I captured the above issue with brave-intl/bat-client#18 so that we can investigate further |
Fix persistence edge-case issue with ledger client
Fix persistence edge-case issue with ledger client
Fix persistence edge-case issue with ledger client
Fixes #11494
Auditors: @evq
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests