Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brave Payments import failing can cause entire import to fail #2421

Closed
bsclifton opened this issue Dec 9, 2018 · 1 comment · Fixed by brave/brave-core#1064
Closed

Brave Payments import failing can cause entire import to fail #2421

bsclifton opened this issue Dec 9, 2018 · 1 comment · Fixed by brave/brave-core#1064

Comments

@bsclifton
Copy link
Member

bsclifton commented Dec 9, 2018

Test plan

  1. Have a Muon install which has a wallet with a non-zero balance
  2. Launch Brave Core from the CLI with fresh profile
  3. Use importer to import Brave (old). This should succeed (shows finish message)
  4. Run import again, this time only picking Brave Payments
  5. Because your wallet has a non-zero balance, the restore should fail
  6. Error is logged to terminal
  7. Import shows finish message, despite the second import failing

Description

When the upgrade happens from Muon to Brave Core, the importer is ran. If the importer encounters problems while importing Brave Payments, it cancels the import. This happens in the following scenarios:

  • unable to get an instance of rewards service
  • if wallet creation (creating the wallet to be imported into) fails
  • restore fails
  • backup of existing wallet fails (user already has a wallet, but with 0 balance)
  • getting wallet properties (ex: balance) returns an error

Instead of cancelling the import (which may roll back the bookmarks import or other imported items), we should continue with the import. The user can then restore their wallet manually by either:

  • trying to import Brave Payments only using Settings => Import (in Brave Core)
  • Launching Muon, exporting keywords, restoring keywords in Brave Core
@bsclifton bsclifton self-assigned this Dec 9, 2018
bsclifton added a commit to brave/brave-core that referenced this issue Dec 10, 2018
…ents, allow import to finish.

Errors are still logged, but import calls `FinishLedgerImport` instead of `Cancel` (which may abort progress made)

Fixes brave/brave-browser#2421
@bsclifton bsclifton added this to the 0.58.x - Release milestone Dec 13, 2018
@LaurenWags
Copy link
Member

LaurenWags commented Dec 18, 2018

Verified passed with

Brave 0.58.14 Chromium: 71.0.3578.98 (Official Build) (64-bit)
Revision 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS Mac OS X
  • Verified Test Plan in description
    This error was logged in terminal [27863:775:1218/124309.314326:ERROR:brave_profile_writer.cc(221)] Brave Rewards wallet existed before import and has a balance of 65.3453; skipping Brave Payments import.
    And the completion message still displayed
    screen shot 2018-12-18 at 12 43 57 pm

Verification passed on

Brave 0.58.14 Chromium: 71.0.3578.98 (Official Build) (64-bit)
Revision 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS Windows 7

Used STR from OP, got log message
[12884:16068:1219/011131.137:ERROR:brave_profile_writer.cc(221)] Brave Rewards wallet existed before import and has a balance of 24; skipping Brave Payments import.

Also used a different wallets in b-c (27 BAT) and b-l (24BAT) and got this log message:
[12884:16068:1219/011315.591:ERROR:brave_profile_writer.cc(221)] Brave Rewards wallet existed before import and has a balance of 27; skipping Brave Payments import.

Verification passed on

Brave 0.58.15 Chromium: 71.0.3578.98 (Official Build) (64-bit)
Revision 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS Linux

Used STR from OP, got log message
[28280:28280:1220/015034.784410:ERROR:brave_profile_writer.cc(221)] Brave Rewards wallet existed before import and has a balance of 22; skipping Brave Payments import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment