Removes transition btc->bat code #13039

NejcZdovc · 2018-02-06T15:47:32Z

Resolves #12712

Do not squash commits

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

old wallet

create wallet on version 0.18 so that you have BTC wallet
copy that wallet into this branch
run browser
check payments page (you should see new wallet created for you)
check profile folder to see if there is file ledger-state-btc.json

new wallet

create wallet on 0.20
add some BAT into it
copy that wallet into this branch
run browser
make sure that no money was lost and that old wallet is still there

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2018-02-06T16:08:45Z

Codecov Report

Merging #13039 into master will decrease coverage by 0.32%.
The diff coverage is 26.66%.

@@            Coverage Diff             @@
##           master   #13039      +/-   ##
==========================================
- Coverage   56.26%   55.94%   -0.33%     
==========================================
  Files         282      281       -1     
  Lines       28027    27805     -222     
  Branches     4585     4552      -33     
==========================================
- Hits        15770    15556     -214     
+ Misses      12257    12249       -8

Flag	Coverage Δ
#unittest	`55.94% <26.66%> (-0.33%)`	⬇️

Impacted Files	Coverage Δ
app/browser/reducers/ledgerReducer.js	`44.16% <ø> (-2.31%)`	⬇️
js/actions/appActions.js	`18.8% <ø> (-1.31%)`	⬇️
app/renderer/components/styles/animations.js	`100% <ø> (ø)`	⬆️
app/renderer/components/preferences/paymentsTab.js	`71.22% <ø> (-0.61%)`	⬇️
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
app/sessionStore.js	`88.06% <ø> (ø)`	⬆️
...r/components/preferences/payment/enabledContent.js	`75.35% <ø> (+0.01%)`	⬆️
app/browser/api/ledgerNotifications.js	`70.95% <ø> (-4.38%)`	⬇️
app/locale.js	`35.77% <ø> (ø)`	⬆️
app/browser/api/ledger.js	`58.79% <26.66%> (-1.51%)`	⬇️
... and 8 more

bsclifton · 2018-02-20T04:52:40Z

@NejcZdovc can we get a rebase on this one?

NejcZdovc · 2018-02-20T04:57:22Z

@bsclifton rebased

bsclifton · 2018-02-20T05:47:26Z

app/browser/api/ledger.js

@@ -2390,147 +2388,6 @@ const deleteSynopsis = () => {
  synopsis.publishers = {}
 }

-// fix for incorrectly persisted state (see #11585)
-const yoDawg = (stateState) => {


RIP the yoDawg bug fix 💯
cc: @evq

bsclifton · 2018-02-20T06:08:40Z

app/browser/api/ledger.js

+    parsedData.properties.wallet &&
+    parsedData.properties.wallet.keychains
+
+  if (isBTC) {


I think we should add verbose logging here- something like:
Bitcoin wallet detected; backing up to "ledger-state-btc.json" and creating new BAT wallet

bsclifton · 2018-02-20T06:15:25Z

I looked over the code changes closely and they look great 👍
Test plan 1 worked great 👍
Test plan 2 is failing for me... I copied my production profile (both session-store-1 and ledger-state.json) and the following error is logged to console:

An uncaught exception occurred in the main process Uncaught Exception:
TypeError: First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object.
    at fromObject (buffer.js:274:9)
    at Function.Buffer.from (buffer.js:106:10)
    at Object.Client.getWalletPassphrase (/Users/clifton/Documents/browser-laptop/node_modules/bat-client/index.js:508:46)
    at getStateInfo (/Users/clifton/Documents/browser-laptop/app/browser/api/ledger.js:1524:43)
    at Object.onInitRead (/Users/clifton/Documents/browser-laptop/app/browser/api/ledger.js:2079:11)
    at ledgerReducer (/Users/clifton/Documents/browser-laptop/app/browser/reducers/ledgerReducer.js:302:27)
    at handleAppAction (/Users/clifton/Documents/browser-laptop/js/stores/appStore.js:230:16)
    at callbacks.forEach (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:107:7)
    at Array.forEach (<anonymous>)
    at AppDispatcher.dispatchToOwnRegisteredCallbacks (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:106:20)
    at AppDispatcher.dispatchInternal (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:132:10)
    at runCallback (timers.js:672:20)
    at tryOnImmediate (timers.js:645:5)
    at processImmediate [as _immediateCallback] (timers.js:617:5)

NejcZdovc · 2018-02-20T12:40:41Z

@bsclifton second test plan error is not related to this PR. Will be fixed with this one #13201

Resolves brave#12712 Auditors: Test Plan:

bsclifton

Rebased after merging #13201 and verified it's working as expected 😄 👍

bsclifton · 2018-02-20T16:16:53Z

@evq @LaurenWags @srirambv would you all care to give this one a review too? 😄

LaurenWags · 2018-02-20T21:11:19Z

Tests Ran:

Old Profile and (production) wallet from 0.18.36 (last released version with BTC wallets) Payments were ON and my ledger table had data - PASSED
a. took a copy of profile from 0.18.36 where Payments were ON and ledger table had data (wallet had 0 BTC), renamed the profile copy to be brave-development
b. built and ran PR
c. Verified did not see transition overlay (bouncing balls), verified ledger data was retained, verified wallet was created (addresses and QR codes visible in 'Add Funds'), verified presence of ledger-state-btc.json file in brave-development profile folder.
Old Profile and (production) wallet from 0.18.36 (last released version with BTC wallets) Payments were OFF and my ledger table had data - PASSED
a. took a copy of profile from 0.18.36 where Payments were OFF and ledger table had data (wallet had 0 BTC), renamed the profile copy to be brave-development
b. built and ran PR
c. Enabled Payments from Preferences
d. Verified did not see transition overlay (bouncing balls), verified ledger data was retained, verified wallet was created (addresses and QR codes visible in 'Add Funds'), verified presence of ledger-state-btc.json file in brave-development profile folder.
Profile and (staging) wallet from 0.20.42. Payments were ON and my ledger table had data - PASSED
a. took a copy of a profile from 0.20.42 where Payments were ON and ledger table had data (staging wallet had 10 BAT), renamed the profile copy to be brave-development
b. built and ran PR
c. Verified did not see transition overlay (bouncing balls), verified ledger data was retained, verified wallet was retained (addresses and QR codes visible in 'Add Funds', no BAT lost), verified there is not a ledger-state-btc.json file in brave-development profile folder.
Profile and (staging) wallet from 0.20.42. Payments were OFF and my ledger table had data - PASSED
a. took a copy of a profile from 0.20.42 where Payments were OFF and ledger table had data (staging wallet had 10 BAT), renamed the profile copy to be brave-development
b. built and ran PR
c. Enabled Payments from Preferences
d. Verified did not see transition overlay (bouncing balls), verified ledger data was retained, verified wallet was retained (addresses and QR codes visible in 'Add Funds', no BAT lost), verified there is not a ledger-state-btc.json file in brave-development profile folder.

LaurenWags

Tests ran are detailed in #13039 (comment) and all were as expected/passed.

bsclifton · 2018-02-21T06:42:21Z

master aedf064

@NejcZdovc will need your help cherry-picking to 0.22.x and 0.21.x 😄

Removes transition btc->bat code

NejcZdovc · 2018-02-21T07:04:03Z

0.22 b68673f
0.21 ff3e0ec

Removes transition btc->bat code

NejcZdovc · 2018-02-24T12:26:10Z

0.21 twitch 9f85d9f

NejcZdovc added feature/rewards PR/work-in-progress ⚒ labels Feb 6, 2018

NejcZdovc added this to the 0.20.x Hotfix 3 (Ledger improvments) milestone Feb 6, 2018

NejcZdovc self-assigned this Feb 6, 2018

alexwykoff modified the milestones: 0.20.x Hotfix 3 (Ledger improvments), 0.21.x (Beta Channel) Feb 6, 2018

NejcZdovc force-pushed the feature/#12712-btc branch from 1335cfb to 4998255 Compare February 7, 2018 08:25

NejcZdovc removed the PR/work-in-progress ⚒ label Feb 7, 2018

NejcZdovc requested review from bsclifton, evq, srirambv and LaurenWags and removed request for bsclifton and evq February 7, 2018 09:15

NejcZdovc force-pushed the feature/#12712-btc branch 3 times, most recently from 06ec55e to 1e04fd3 Compare February 19, 2018 05:30

NejcZdovc force-pushed the feature/#12712-btc branch from 1e04fd3 to b7f1e31 Compare February 20, 2018 04:57

bsclifton reviewed Feb 20, 2018

View reviewed changes

NejcZdovc added 2 commits February 20, 2018 09:12

Removes transition btc->bat code

ccee122

Resolves brave#12712 Auditors: Test Plan:

Adds new wallet creation for old btc wallets

c8d0d7f

bsclifton force-pushed the feature/#12712-btc branch from b7f1e31 to c8d0d7f Compare February 20, 2018 16:12

bsclifton approved these changes Feb 20, 2018

View reviewed changes

LaurenWags approved these changes Feb 20, 2018

View reviewed changes

bsclifton merged commit aedf064 into brave:master Feb 21, 2018

NejcZdovc pushed a commit that referenced this pull request Feb 21, 2018

Merge pull request #13039 from NejcZdovc/feature/#12712-btc

b68673f

Removes transition btc->bat code

NejcZdovc pushed a commit that referenced this pull request Feb 21, 2018

Merge pull request #13039 from NejcZdovc/feature/#12712-btc

ff3e0ec

Removes transition btc->bat code

mrose17 mentioned this pull request Feb 23, 2018

phase two transitioning from the BTC proof-of-concept to BAT #12712

Closed

NejcZdovc modified the milestones: 0.21.x (Beta Channel), 0.21.x (Twitch) Feb 24, 2018

NejcZdovc pushed a commit that referenced this pull request Feb 24, 2018

Merge pull request #13039 from NejcZdovc/feature/#12712-btc

9f85d9f

Removes transition btc->bat code

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes transition btc->bat code #13039

Removes transition btc->bat code #13039

NejcZdovc commented Feb 6, 2018 •

edited by bsclifton

Loading

codecov-io commented Feb 6, 2018 •

edited

Loading

bsclifton commented Feb 20, 2018

NejcZdovc commented Feb 20, 2018

bsclifton Feb 20, 2018

bsclifton Feb 20, 2018

bsclifton commented Feb 20, 2018

NejcZdovc commented Feb 20, 2018

bsclifton left a comment

bsclifton commented Feb 20, 2018

LaurenWags commented Feb 20, 2018 •

edited

Loading

LaurenWags left a comment

bsclifton commented Feb 21, 2018

NejcZdovc commented Feb 21, 2018

NejcZdovc commented Feb 24, 2018

Removes transition btc->bat code #13039

Removes transition btc->bat code #13039

Conversation

NejcZdovc commented Feb 6, 2018 • edited by bsclifton Loading

Do not squash commits

Submitter Checklist:

Test Plan:

Reviewer Checklist:

codecov-io commented Feb 6, 2018 • edited Loading

Codecov Report

bsclifton commented Feb 20, 2018

NejcZdovc commented Feb 20, 2018

bsclifton Feb 20, 2018

Choose a reason for hiding this comment

bsclifton Feb 20, 2018

Choose a reason for hiding this comment

bsclifton commented Feb 20, 2018

NejcZdovc commented Feb 20, 2018

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Feb 20, 2018

LaurenWags commented Feb 20, 2018 • edited Loading

LaurenWags left a comment

Choose a reason for hiding this comment

bsclifton commented Feb 21, 2018

NejcZdovc commented Feb 21, 2018

NejcZdovc commented Feb 24, 2018

NejcZdovc commented Feb 6, 2018 •

edited by bsclifton

Loading

codecov-io commented Feb 6, 2018 •

edited

Loading

LaurenWags commented Feb 20, 2018 •

edited

Loading