Binance version 2 trading widget #5160

ryanml · 2020-04-05T23:41:12Z

Fixes: brave/brave-browser#9118

Submitter Checklist:

Test Plan:

Defined in issue

Screenshots:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

components/binance/browser/buildflags/buildflags.gni

emerick

Basically LGTM, had a few comments nothing really major.

browser/binance/binance_service_factory.cc

browser/extensions/api/binance_api.cc

browser/extensions/api/binance_api.h

browser/extensions/api/binance_apitest.cc

components/binance/browser/binance_json_parser.cc

components/binance/browser/binance_protocol_handler.cc

browser/brave_profile_prefs.cc

common/network_constants.cc

common/network_constants.h

browser/extensions/api/binance_api.h

browser/extensions/api/binance_api.cc

test/data/extensions/api_test/binance/background.js

emerick

LGTM, a couple of minor nits nothing big.

emerick · 2020-04-08T20:46:36Z

components/binance/browser/binance_protocol_handler.cc

+  std::string query;
+
+  if (url.has_query()) {
+    query = base::StrCat({


Just FYI, net::AppendQueryParameter is good for this kind of thing.

ya AppendQueryParameter it was used a lot of times in this PR but this file was copied from the rewards uphold protocol handler.

components/binance/browser/binance_service.cc

components/brave_new_tab_ui/components/default/binance/index.tsx

ryanml · 2020-04-10T19:59:03Z

PR previously green, latest failure unrelated cc: @bbondy

Binance version 2 trading widget

srirambv · 2020-04-16T07:23:00Z

Verification passed on

Brave	1.9.21 Chromium: 81.0.4044.92 (Official Build) nightly (64-bit)
Revision	32921c79b6f01a0fb2deef0e1d45b42f96581051-refs/branch-heads/4044@{#883}
OS	Windows 10 OS Version 1909 (Build 18363.778)

Verified Buy functionality works as before
Verified Connect state is only shown for .com and switching to .us hides the Connect
Verified clicking Connect opens up Binance login page. Upon successful authentication redirects to permission page
Verified after successfully authenticating widget is shown and assets are in hidden view
Verified with the widget in the connected state, clicking on buy and selecting .us loads correct landing page for buy asset page

Deposit

Verified assets can be filtered via the search input
Verified Deposit Address is shown correctly for an asset when available
Verified copy address works and is copied into the clipboard
Verified QR code is shown correctly for an asset when available
Verified assets show message when no address is available. Copy and QR code buttons are not shown

Conversion

Verified changing the asset updates the available balance displayed for that asset.
Verified when the balance is not available correct error message is shown when trying to preview conversion
Verified when incorrect value is entered and tried to convert shows correct error message
Verified quote expires after 30s and goes back to convert view
Verified Conversion preview shows the value correctly
Verified upon conversion success message is shown
Verified upon conversation and asset shown on dashboard on Binance.com widget also updates the value automatically
Verified data can be refreshed manually as well after conversion
Verified correct error is shown when conversion value is not correct

Summary View

Verified default view on the widget is summary view
Verified default state is hidden summary view after widget is connected
Verified clicking on the eye shows/hides the summary view

Menu settings

Verified clicking on hide widget from menu hides the widget
Verified clicking on Learn More loads https://brave.com/binance/ page in a new tab
Verified refresh data from menu fetches new data from Binance
Verified disconnect shows warn message about losing widget state
Verified clicking disconnects bring back widget to pre-connect state

Logged the following follow up issues

Disconnect and reconnect widget doesn't always update widget state Disconnect and reconnect widget doesn't always update widget state brave-browser#9258
Reload newtab page reset widget to default summary view Reload newtab page reset widget to default summary view brave-browser#9257
Binance widget should be accessible via tab index [Desktop] Binance widget should be accessible via tab index brave-browser#9256
Incorrect state of widgets on different tabs Incorrect state of widgets on different tabs brave-browser#9255
Preview conversion doesn't always start from 30sec Preview conversion doesn't always start from 30sec brave-browser#9254
Widget looses connect state when network is disconnected Widget looses connect state when network is disconnected brave-browser#9253
Menu items icons not visible on dark mode Menu items icons not visible on dark mode brave-browser#9261
Fiat value text is not properly visibleFiat value text is not properly visible brave-browser#9263

Verification passed on

Brave	1.9.24 Chromium: 81.0.4044.92 (Official Build) nightly (64-bit)
Revision	32921c79b6f01a0fb2deef0e1d45b42f96581051-refs/branch-heads/4044@{#883}
OS	Linux

Verified Buy functionality works as before
Verified Connect state is only shown for .com and switching to .us hides the Connect
Verified clicking Connect opens up Binance login page. Upon successful authentication redirects to permission page
Verified after successfully authenticating widget is shown and assets are in hidden view
Verified with the widget in the connected state, clicking on buy and selecting .us loads correct landing page for buy asset page

Deposit

Verified assets can be filtered via the search input
Verified Deposit Address is shown correctly for an asset when available
Verified copy address works and is copied into the clipboard
Verified QR code is shown correctly for an asset when available
Verified assets show message when no address is available. Copy and QR code buttons are not shown

Conversion

Verified changing the asset updates the available balance displayed for that asset.
Verified when the balance is not available correct error message is shown when trying to preview conversion
Verified when an incorrect value is entered and tried to convert shows correct error message
Verified quote expires after 30s and goes back to convert view
Verified Conversion preview shows the value correctly
Verified upon conversion success message is shown
Verified upon conversation and asset shown on dashboard on Binance.com widget also updates the value automatically
Verified data can be refreshed manually as well after conversion

Summary View

Verified default view on the widget is summary view
Verified default state is hidden summary view after widget is connected
Verified clicking on the eye shows/hides the summary view

Menu settings

Verified clicking on hide widget from menu hides the widget
Verified clicking on Learn More loads https://brave.com/binance/ page in a new tab
Verified refresh data from menu fetches new data from Binance
Verified disconnect shows warn message about losing widget state
Verified clicking disconnects bring back widget to pre-connect state

Verification passed on

Brave	1.9.21 Chromium: 81.0.4044.92 (Official Build) nightly (64-bit)
Revision	32921c79b6f01a0fb2deef0e1d45b42f96581051-refs/branch-heads/4044@{#883}
OS	macOS Version 10.15.4 (Build 19E287)

Verified Buy functionality works as before
Verified Connect state is only shown for .com and switching to .us hides the Connect
Verified clicking Connect opens up Binance login page. Upon successful authentication redirects to permission page
Verified after successfully authenticating widget is shown and assets are in hidden view
Verified with the widget in the connected state, clicking on buy and selecting .us loads correct landing page for buy asset page

Deposit

Verified assets can be filtered via the search input
Verified Deposit Address is shown correctly for an asset when available
Verified copy address works and is copied into the clipboard
Verified QR code is shown correctly for an asset when available
Verified assets show message when no address is available. Copy and QR code buttons are not shown

Conversion

Verified changing the asset updates the available balance displayed for that asset.
Verified when the balance is not available correct error message is shown when trying to preview conversion
Verified when an incorrect value is entered and tried to convert shows correct error message
Verified quote expires after 30s and goes back to convert view
Verified Conversion preview shows the value correctly
Verified upon conversion success message is shown
Verified upon conversation and asset shown on dashboard on Binance.com widget also updates the value automatically
Verified data can be refreshed manually as well after conversion

Summary View

Verified default view on the widget is summary view
Verified default state is hidden summary view after widget is connected
Verified clicking on the eye shows/hides the summary view

Menu settings

Verified clicking on hide widget from menu hides the widget
Verified clicking on Learn More loads https://brave.com/binance/ page in a new tab
Verified refresh data from menu fetches new data from Binance
Verified disconnect shows warn message about losing widget state
Verified clicking disconnects bring back widget to pre-connect state

diracdeltas · 2020-04-17T22:47:08Z

components/brave_new_tab_ui/components/default/binance/index.tsx

+
+  connectBinance = () => {
+    const { binanceClientUrl } = this.props
+    window.open(binanceClientUrl, '_self')


set noopener. https://developer.mozilla.org/en-US/docs/Web/API/Window/open

diracdeltas · 2020-04-17T22:49:50Z

components/brave_new_tab_ui/containers/newTab/index.tsx

@@ -302,6 +351,73 @@ class NewTabPage extends React.Component<Props, State> {
    window.open('https://brave.com/binance/', '_blank')


set noopener(technically this isn't part of this PR but i was never tagged for review on the original one)

diracdeltas · 2020-04-17T22:56:04Z

components/brave_new_tab_ui/reducers/new_tab_reducer.ts

+
+    case types.ON_BINANCE_CLIENT_URL:
+      state = { ...state }
+      state.binanceState.binanceClientUrl = payload.clientUrl


maybe add some validation that this URL is HTTPS and connects to an expected hostname?

Binance version 2 trading widget

ryanml assigned bbondy and ryanml Apr 5, 2020

ryanml requested review from bbondy, cezaraugusto and emerick April 6, 2020 06:58

ryanml added this to the 1.9.x - Nightly milestone Apr 6, 2020

ryanml added feature/eth-wallet feature/newtab labels Apr 6, 2020

ryanml marked this pull request as ready for review April 6, 2020 07:01

ryanml force-pushed the binance-version-2 branch from 0420ce3 to 9e48959 Compare April 6, 2020 07:02

ryanml commented Apr 6, 2020

View reviewed changes

components/binance/browser/buildflags/buildflags.gni Outdated Show resolved Hide resolved

ryanml force-pushed the binance-version-2 branch 3 times, most recently from b994eda to 89d30a1 Compare April 6, 2020 21:40

emerick reviewed Apr 6, 2020

View reviewed changes

ryanml force-pushed the binance-version-2 branch 3 times, most recently from 909bd22 to 0f28948 Compare April 7, 2020 17:21

bbondy requested changes Apr 7, 2020

View reviewed changes

ryanml force-pushed the binance-version-2 branch from c9dd709 to 191d1a0 Compare April 7, 2020 21:04

ryanml requested a review from emerick April 8, 2020 20:28

emerick approved these changes Apr 8, 2020

View reviewed changes

ryanml and others added 8 commits April 9, 2020 00:41

Initial NTP widget with buy support

dc115fc

Adding front-end resources for full widget

4c354c2

Adding backend API, controller, service

9aa0fd3

Adding PKCE code challenge store

2ed0595

Adding account balance retrieval, json parser tests

953a7ea

Adding conversion quote endpoint

887f80b

Adding V1 API code, TickerPrice and Volume endpoints

4270169

Collapse BinanceController into BinanceService

8ddd979

ryanml and others added 5 commits April 9, 2020 00:41

Moving connect button from header, disconnect action to widget menu

fa40f54

Implement review feedback

596cb27

Make sure no API access in extensions

3972d72

Add Binance service tests for all APIs

06048c5

Moving data update actions to newTab, refresh action in menu

cb730fc

ryanml force-pushed the binance-version-2 branch from c5ffb36 to ec475e9 Compare April 9, 2020 07:41

ryanml commented Apr 9, 2020

View reviewed changes

components/brave_new_tab_ui/components/default/binance/index.tsx Show resolved Hide resolved

Fixing bug where new NTP pages show un-authed view

1d7fea7

ryanml force-pushed the binance-version-2 branch from ec475e9 to 1d7fea7 Compare April 9, 2020 19:33

ryanml requested a review from bbondy April 9, 2020 21:45

ryanml and others added 2 commits April 9, 2020 18:33

Reordering nav menu, fixing widget menu persistence

8f64a15

Add and use API to revoke token

74c48b1

bbondy approved these changes Apr 10, 2020

View reviewed changes

ryanml merged commit b520029 into master Apr 10, 2020

ryanml deleted the binance-version-2 branch April 10, 2020 20:02

ryanml added a commit that referenced this pull request Apr 13, 2020

Merge pull request #5160 from brave/binance-version-2

9da3e06

Binance version 2 trading widget

ryanml mentioned this pull request Apr 13, 2020

Binance version 2 trading widget (Uplift to 1.8.x) #5236

Merged

4 tasks

srirambv mentioned this pull request Apr 16, 2020

Binance Version 2 Trading Widget brave/brave-browser#9118

Closed

diracdeltas reviewed Apr 17, 2020

View reviewed changes

ryanml mentioned this pull request Apr 18, 2020

noopener should be in use for opening Binance auth and referral URLs brave/brave-browser#9317

Closed

bsclifton pushed a commit that referenced this pull request Apr 21, 2020

Merge pull request #5160 from brave/binance-version-2

fd897a9

Binance version 2 trading widget

This was referenced Apr 23, 2020

Missing license for crypto/coins.ttf brave/brave-browser#9407

Closed

Enforce licenses in third_party code added to brave-core brave/brave-browser#9408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binance version 2 trading widget #5160

Binance version 2 trading widget #5160

ryanml commented Apr 5, 2020 •

edited by bbondy

Loading

emerick left a comment

emerick left a comment

emerick Apr 8, 2020

bbondy Apr 9, 2020

ryanml commented Apr 10, 2020

srirambv commented Apr 16, 2020 •

edited

Loading

diracdeltas Apr 17, 2020

diracdeltas Apr 17, 2020

diracdeltas Apr 17, 2020

		@@ -302,6 +351,73 @@ class NewTabPage extends React.Component<Props, State> {
		window.open('https://brave.com/binance/', '_blank')

Binance version 2 trading widget #5160

Binance version 2 trading widget #5160

Conversation

ryanml commented Apr 5, 2020 • edited by bbondy Loading

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

After-merge Checklist:

emerick left a comment

Choose a reason for hiding this comment

emerick left a comment

Choose a reason for hiding this comment

emerick Apr 8, 2020

Choose a reason for hiding this comment

bbondy Apr 9, 2020

Choose a reason for hiding this comment

ryanml commented Apr 10, 2020

srirambv commented Apr 16, 2020 • edited Loading

Deposit

Conversion

Summary View

Menu settings

Deposit

Conversion

Summary View

Menu settings

Deposit

Conversion

Summary View

Menu settings

diracdeltas Apr 17, 2020

Choose a reason for hiding this comment

diracdeltas Apr 17, 2020

Choose a reason for hiding this comment

diracdeltas Apr 17, 2020

Choose a reason for hiding this comment

ryanml commented Apr 5, 2020 •

edited by bbondy

Loading

srirambv commented Apr 16, 2020 •

edited

Loading