BAT integration (BAT Mercury) #11231

NejcZdovc · 2017-10-02T05:51:06Z

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.

Resolves #9740 - Second key for Brave Payments is not displayed on the UI after recovering wallet
Resolves #10945 - Implement BAT into payments
Resolves #11193 - Implement new add-funds wizard UI to support BAT
Resolves #11251 - Error in undefined: "getWalletProperties" on master
Resolves #11264 - Add 2 decimal places (rounded for display) to BAT wallet balance
Resolves #11278 - Ledger table data is not cleared properly
Resolves #11285 - Missing text in BAT add funds modal
Resolves #11289 - Copied text overlaps on the info message
Resolves #11292 - Brave wallet backup key 2 overflows
Resolves #11293 - Recovery keys fail both manual/import
Resolves #11308 - Transition the wallet from BTC to BAT

Test Plan:

Reviewer Checklist:

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 · 2017-10-02T06:24:06Z

Codecov Report

Merging #11231 into master will increase coverage by 0.58%.
The diff coverage is 40.56%.

@@            Coverage Diff             @@
##           master   #11231      +/-   ##
==========================================
+ Coverage   51.93%   52.52%   +0.58%     
==========================================
  Files         263      267       +4     
  Lines       24777    24984     +207     
  Branches     3948     3997      +49     
==========================================
+ Hits        12869    13122     +253     
+ Misses      11908    11862      -46

Flag	Coverage Δ
#unittest	`52.52% <40.56%> (+0.58%)`	⬆️

Impacted Files	Coverage Δ
app/locale.js	`35.23% <ø> (ø)`	⬆️
app/renderer/components/styles/global.js	`100% <ø> (ø)`	⬆️
js/constants/appConfig.js	`100% <ø> (ø)`	⬆️
js/constants/config.js	`55.55% <ø> (ø)`	⬆️
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
js/about/aboutActions.js	`9.32% <0%> (ø)`	⬆️
js/about/preferences.js	`45.85% <0%> (ø)`	⬆️
app/renderer/components/common/clipboardButton.js	`35.48% <0%> (ø)`
app/browser/tabs.js	`26.08% <0%> (-0.61%)`	⬇️
...rer/components/preferences/payment/ledgerBackup.js	`37.83% <100%> (+1.94%)`	⬆️
... and 40 more

luixxiul · 2017-10-03T05:43:23Z

app/renderer/components/preferences/payment/addFundsDialog/addFundsDialog.js

+            secondaryColor
+            custom={[
+              styles.currencyIcon,
+              styles.ethIcon


how about styles.icon_currency and styles.icon_currency_eth?

luixxiul · 2017-10-03T05:44:02Z

app/renderer/components/preferences/payment/addFundsDialog/addFundsDialog.js

+            secondaryColor
+            custom={[
+              styles.currencyIcon,
+              styles.btcIcon


how about styles.icon_currency_btc like above?

also styles.icon_curerncy_ltc and styles.icon_currency_bat below.

luixxiul · 2017-10-03T05:49:04Z

app/renderer/components/preferences/payment/addFundsDialog/addFundsDialog.js

+      <div>
+        <header data-l10n-id='addFundsWizardAddressHeader' />
+        <div className={css(styles.addFundsWizardAddress)}>
+          <main className={css(styles.addFundsWizardAddress__address)}>


~~I'd pick styles.wizard and styles.wizard__address to shrink the length (because it should be already clear what kind of wizard it is, from the name addFundsWizardAddress).~~ We could normalize the classnames later.

luixxiul · 2017-10-03T05:49:53Z

app/renderer/components/preferences/payment/addFundsDialog/addFundsDialogFooter.js

+class AddFundsDialogFooter extends ImmutableComponent {
+  render () {
+    return (
+      <section className={css(styles.addFundsFooter)} data-test-id='AddFundsDialogFooter'>


~~Ditto. I would pick styles.footer to shrink the classname.~~

luixxiul · 2017-10-03T05:51:21Z

app/renderer/components/preferences/payment/addFundsDialog/addFundsDialogFooter.js

+      <section className={css(styles.addFundsFooter)} data-test-id='AddFundsDialogFooter'>
+        <div className={css(styles.addFundsFooter__start)}>
+          <img src={upholdLogo}
+            className={css(styles.addFundsFooter__init__uphold_logo)}


As long as there is not another common element, why wouldn't you change this into styles.addFundsFooter__start__uphold_logo?

luixxiul · 2017-10-03T05:51:44Z

app/renderer/components/preferences/payment/addFundsDialog/addFundsDialogFooter.js

+          <img src={upholdLogo}
+            className={css(styles.addFundsFooter__init__uphold_logo)}
+          />
+          <div className={css(styles.addFundsFooter__init__uphold_text)}>


Ditto. We could replace it with styles.addFundsFooter__start__uphold_text

luixxiul · 2017-10-03T05:52:34Z

less/about/preferences.less

  font-weight: 400;
  -webkit-font-smoothing: antialiased;
 }

+strong {
+  font-weight: 600


wouldn't this change cause a visual regression?

I don't think so, only other <strong> tag we have is in URL bar, which is not affected by this

I see. the file will eventually be removed so not a great issue anyway.

luixxiul · 2017-10-03T05:55:37Z

app/renderer/components/common/modalOverlay.js

+        ? <aside className={css(styles.dialog__header__subTitle)}>
+          <div className={cx({
+            [css(styles.dialog__header__subTitle_text)]: true,
+            [customTitleClassesStr]: true


(TODO: we would remove customTitleClassesStr etc. later to avoid cx which sometimes prevents properties from cascading properly)

luixxiul

I left some feedback. @cezaraugusto: would you please check and address them if you would agree? thanks!

luixxiul · 2017-10-03T10:25:23Z

https://travis-ci.org/brave/browser-laptop/jobs/282646416#L1292

    ERROR in ./app/renderer/components/preferences/payment/addFundsDialog/addFundsDialog.js
    Module not found: Error: Can't resolve '../../../../../extensions/brave/img/ledger/fakeQRCode.png' in '/home/travis/build/brave/browser-laptop/app/renderer/components/preferences/payment/addFundsDialog'
     @ ./app/renderer/components/preferences/payment/addFundsDialog/addFundsDialog.js 16:15-83
     @ ./app/renderer/components/preferences/paymentsTab.js
     @ ./js/about/preferences.js
     @ ./js/about/entry.js
     @ multi ./js/about/entry.js

luixxiul · 2017-10-03T10:34:56Z

app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardAddress.js

+    margin: '15px 0'
+  },
+
+  addFundsWizardAddress__main__text_bold: {


luixxiul · 2017-10-03T10:36:38Z

app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardAddress.js

+        <header data-l10n-id='addFundsWizardAddressHeader' />
+        <div className={css(styles.addFundsWizardAddress__main)}>
+          <main className={css(styles.addFundsWizardAddress__inputBox)}>
+            <GroupedFormTextbox type='text'


Is this not be applied here? #7164 (comment)

I don't think so. works normally in this case

thanks for checking :-)

luixxiul · 2017-10-03T10:37:37Z

app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardMain.js

+          />
+          <BrowserButton groupedItem secondaryColor
+            onClick={this.onClickETH}
+            custom={[styles.addFundsWizardMain__currencyIcon, styles.ethIcon]}


styles.ethIcon -> styles.addFundsWizardMain__currencyIcon_eth

luixxiul · 2017-10-03T10:37:50Z

app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardMain.js

+          />
+          <BrowserButton groupedItem secondaryColor
+            onClick={this.onClickBTC}
+            custom={[styles.addFundsWizardMain__currencyIcon, styles.btcIcon]}


styles.btcIcon -> styles.addFundsWizardMain__currencyIcon_btc

luixxiul · 2017-10-03T10:38:18Z

app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardMain.js

+          />
+          <BrowserButton groupedItem secondaryColor
+            onClick={this.onClickLTC}
+            custom={[styles.addFundsWizardMain__currencyIcon, styles.ltcIcon]}


styles.ltcIcon -> styles.addFundsWizardMain__currencyIcon_ltc

luixxiul · 2017-10-03T10:39:03Z

app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardMain.js

+            />
+          <BrowserButton groupedItem secondaryColor
+            onClick={this.onClickBAT}
+            custom={[styles.addFundsWizardMain__currencyIcon, styles.batIcon]}


styles.batIcon -> styles.addFundsWizardMain__currencyIcon_bat

diracdeltas · 2017-10-03T20:27:40Z

package.json

+    "bat-balance": "^0.9.6",
+    "bat-client": "^0.9.0",
+    "bat-publisher": "^0.9.0",
+    "bignumber.js": "^4.0.4",


[minor] there's a new version of bignumber.js out now https://github.com/MikeMcl/bignumber.js/

diracdeltas · 2017-10-03T20:29:58Z

js/constants/config.js

@@ -86,7 +86,6 @@ module.exports = {
  pinterestExtensionPublicKey: 'MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDB95q2hyt49ZDuVnYI91XaZhqQkbXu0X3fzoNxPxhFbfqGKwtts90LJ7lD5DCIfnBg8WGFhp3eW4GxOglAKrnksmJoyAD5PnSAufx8fD3trZvo/ZAqFx1x5Xm3Rm34EgvVXdralgHSYiqcEU/FX3kYnLLhr2TS4lcrsn1KZd/lcQIDAQAB',
  metamaskExtensionId: 'nkbihfbeogaeaoehlefnkodbefgpgknn',
  metamaskPublicKey: 'MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAlcgI4VVL4JUvo6hlSgeCZp9mGltZrzFvc2Asqzb1dDGO9baoYOe+QRoh27/YyVXugxni480Q/R147INhBOyQZVMhZOD5pFMVutia9MHMaZhgRXzrK3BHtNSkKLL1c5mhutQNwiLqLtFkMSGvka91LoMEC8WTI0wi4tACnJ5FyFZQYzvtqy5sXo3VS3gzfOBluLKi7BxYcaUJjNrhOIxl1xL2qgK5lDrDOLKcbaurDiwqofVtAFOL5sM3uJ6D8nOO9tG+T7hoobRFN+nxk43PHgCv4poicOv+NMZQEk3da1m/xfuzXV88NcE/YRbRLwAS82m3gsJZKc6mLqm4wZHzBwIDAQAB',
-  coinbaseOrigin: 'https://buy.coinbase.com',


please remove buy.coinbase.com from the CSP in app/extensions.js

diracdeltas · 2017-10-04T22:25:53Z

Not sure if this has been logged already, but on the welcome screen before payments is enabled:

does the 'Our Partners' text need to be changed? also the FAQ links are pointing to https://brave.com/faq-payments/#brave-payments which is out of date

diracdeltas · 2017-10-04T22:29:07Z

on the second screen:

clicking the settings button doesn't do anything, but it appears enabled instead of greyed-out
clicking the 'create wallet' button also doesn't do anything - console shows ledger client error(1): "Error: HTTP response 404 for GET /v2/wallet/c2f6c9c9-c6da-4071-8ed7-fa9496923469?refresh=true&amount=5&currency=USD"

When i restart the browser and go to Payments, it shows the same error again and then the browser either crashes or the Preferences page no longer works. A non-200 response from the ledger server should not cause browser functionality to break, since this can happen any time.

Resolves #9740 Resolves #10945 Resolves #11251 Resolves #11264 Resolves #11285 Resolves #11289 Resolves #11292 Resolves #11293

Resolves #11193

@mrose17

Fixes balance and transactions with help of @mrose17 use "the latest and greatest" of bat-* and also prepare for new anonymizing proxy

@NejcZdovc

…nit tests originally added with 2ec3ea8 Auditors: @NejcZdovc

- remove contrib panel - set wallet address as readonly - set wallet adress as auto selected - several strings change - changed coins order in wizard - small tweaks on design

Fixes backup key overflow

to allow GET wallet to use altcurrency parameter, as needed; always set both amount/denomination when setting budget

@NejcZdovc

unskip preferences and paymentsTab tests Auditors: @NejcZdovc Test Plan: npm run test -- --grep="PaymentsTab component" npm run test -- --grep="Preferences component unittest" Note that this undercovered two failing tests: npm run test -- grep="PaymentsTab component fundsamount functionality renders full balance correctly" npm run test-- --grep="PaymentsTab component fundsamount functionality renders partial balance correctl"

Fixes clear #11278 Fixes default monthly budget

@NejcZdovc

Fixes #11308 Auditors: @NejcZdovc, @mrose17 Test Plan: `npm run unittest -- --grep="ledger api unit tests"` `npm run unittest -- --grep="ledgerReducer unit tests"`

Adds dynamic min ammount to the wizard Changes min amount in note into users selected amount

@bsclifton

Auditors: @bsclifton Test plan: npm run test -- --grep="add funds welcome screen" npm run test -- --grep="add funds wizard screen"

@mrose17

- transition is called for all wallets - sync is now done before transition - added logic after transition (state updated via onLedgerCallback) Auditors: @mrose17 Set wallet to v1 if v1 properties are found (it should be transitioned on launch) In the roundtrip callback, add `response` to param list when calling during an error changes default debug mode

bsclifton

We verified a $0 wallet and my personal wallet (which had $80). Both transitioned correctly

BAT integration (BAT Mercury)

NejcZdovc added feature/rewards PR/work-in-progress ⚒ labels Oct 2, 2017

NejcZdovc added this to the 0.19.x (Beta Channel) milestone Oct 2, 2017

NejcZdovc self-assigned this Oct 2, 2017

NejcZdovc mentioned this pull request Oct 2, 2017

Adds support for BAT in brave payments #10953

Closed

8 tasks

luixxiul reviewed Oct 3, 2017

View reviewed changes

luixxiul mentioned this pull request Oct 3, 2017

Update "Our Partners" side bar on disabledContent.js to reflect BAT i… #11244

Closed

8 tasks

NejcZdovc force-pushed the bat-integration branch from 8ed4680 to 2a0eedd Compare October 3, 2017 09:32

luixxiul reviewed Oct 3, 2017

View reviewed changes

diracdeltas reviewed Oct 3, 2017

View reviewed changes

NejcZdovc force-pushed the bat-integration branch from fa3ea3e to 2513c64 Compare October 4, 2017 11:10

NejcZdovc mentioned this pull request Oct 4, 2017

Second key for Brave Payments is not displayed on the UI after recovering wallet #9740

Closed

NejcZdovc force-pushed the bat-integration branch from 262acb4 to 8846b38 Compare October 6, 2017 22:27

NejcZdovc and others added 12 commits October 7, 2017 00:30

Adds BAT support

5813411

Resolves #9740 Resolves #10945 Resolves #11251 Resolves #11264 Resolves #11285 Resolves #11289 Resolves #11292 Resolves #11293

Adds addFounds wizard

f852891

Resolves #11193

Implements logic into wizard

a527e1f

Fixes balance and transactions with help of @mrose17 use "the latest and greatest" of bat-* and also prepare for new anonymizing proxy

Fix regression (balance should convert to Number) and add back some u…

8ac1f65

…nit tests originally added with 2ec3ea8 Auditors: @NejcZdovc

Address feedback from design team

b861db0

- remove contrib panel - set wallet address as readonly - set wallet adress as auto selected - several strings change - changed coins order in wizard - small tweaks on design

Fixes l10n and conversion display

49db118

Fixes backup key overflow

use bat-client@0.9.19

5942db2

to allow GET wallet to use altcurrency parameter, as needed; always set both amount/denomination when setting budget

Removes TODO's and fixed wallet recovery

1378693

Fixes clear #11278 Fixes default monthly budget

Ledger client will now transition from BTC => BAT one time (if needed)

99d2d29

Fixes #11308 Auditors: @NejcZdovc, @mrose17 Test Plan: `npm run unittest -- --grep="ledger api unit tests"` `npm run unittest -- --grep="ledgerReducer unit tests"`

Implements new backup logic

9ec88e7

Adds dynamic min ammount to the wizard Changes min amount in note into users selected amount

add tests for addFundsDialog

6ab632a

Auditors: @bsclifton Test plan: npm run test -- --grep="add funds welcome screen" npm run test -- --grep="add funds wizard screen"

NejcZdovc removed the PR/work-in-progress ⚒ label Oct 6, 2017

NejcZdovc force-pushed the bat-integration branch 3 times, most recently from 5ea2529 to 32230b0 Compare October 6, 2017 23:44

bsclifton changed the title ~~Bat integration~~ BAT integration (BAT Mercury) Oct 6, 2017

bsclifton and others added 2 commits October 7, 2017 01:51

Updated to verison 1.0.0 (production)

8781761

bsclifton approved these changes Oct 6, 2017

View reviewed changes

NejcZdovc force-pushed the bat-integration branch from 32230b0 to 8781761 Compare October 6, 2017 23:52

NejcZdovc merged commit ca35372 into master Oct 6, 2017

bsclifton mentioned this pull request Oct 7, 2017

Notification not shown after transition was triggered #11333

Closed

NejcZdovc added a commit that referenced this pull request Oct 7, 2017

Merge pull request #11231 from brave/bat-integration

75a442c

BAT integration (BAT Mercury)

NejcZdovc added a commit that referenced this pull request Oct 7, 2017

Merge pull request #11231 from brave/bat-integration

7fab670

BAT integration (BAT Mercury)

NejcZdovc added a commit that referenced this pull request Oct 7, 2017

Fixes rebase problem from #11231

4822ccb

NejcZdovc added a commit that referenced this pull request Oct 7, 2017

Fixes rebase problem from #11231

babfb84

luixxiul mentioned this pull request Oct 7, 2017

implement new add-funds wizard UI to support BAT #11193

Closed

cezaraugusto deleted the bat-integration branch October 14, 2017 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BAT integration (BAT Mercury) #11231

BAT integration (BAT Mercury) #11231

NejcZdovc commented Oct 2, 2017 •

edited

Loading

codecov-io commented Oct 2, 2017 •

edited

Loading

luixxiul Oct 3, 2017

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017

luixxiul Oct 3, 2017

luixxiul Oct 3, 2017

cezaraugusto Oct 3, 2017

luixxiul Oct 4, 2017

luixxiul Oct 3, 2017

luixxiul left a comment

luixxiul commented Oct 3, 2017

luixxiul Oct 3, 2017

luixxiul Oct 3, 2017

cezaraugusto Oct 3, 2017

luixxiul Oct 4, 2017

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017

luixxiul Oct 3, 2017

diracdeltas Oct 3, 2017

diracdeltas Oct 3, 2017

NejcZdovc Oct 4, 2017

diracdeltas commented Oct 4, 2017 •

edited

Loading

diracdeltas commented Oct 4, 2017 •

edited

Loading

bsclifton left a comment

BAT integration (BAT Mercury) #11231

BAT integration (BAT Mercury) #11231

Conversation

NejcZdovc commented Oct 2, 2017 • edited Loading

codecov-io commented Oct 2, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

luixxiul Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul left a comment

Choose a reason for hiding this comment

luixxiul commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Oct 4, 2017 • edited Loading

diracdeltas commented Oct 4, 2017 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

NejcZdovc commented Oct 2, 2017 •

edited

Loading

codecov-io commented Oct 2, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

luixxiul Oct 3, 2017 •

edited

Loading

diracdeltas commented Oct 4, 2017 •

edited

Loading

diracdeltas commented Oct 4, 2017 •

edited

Loading