(Desktop) FTX for NTP #8629

petemill · 2021-04-23T16:39:11Z

Performs oauth pattern
- Receives auth token (at ftx_protocol_handler.cc)
- Requests authentication token and stores encrypted in regular settings using OSCrypt.
Implements FTX API calls and JSON parsing (ftx_service.cc, ftx_json_parser.cc)
Adds an extension API to call service methods from the New Tab Page WebUI (courtesy of @ryanml) (see note below re: mojom).
Adds UI for FTX on the New Tab Page (courtesy of @AlanBreck).

Review instructions

For building, you must have the correct API key and secret environment variables in your config, see internal wiki.
Please contact me for test FTX credentials.

Note on WebUI API

Like all other NTP crypto widgets, this creates an Extension API even though it is not called from an Extension and only called from the NTP WebUI. These should all be converted to mojom interfaces. Created follow-up here: brave/brave-browser#15770

Test Plan:

All intended UI states are on storybook via npm run storybook:

Not yet initialized

Not connected

Not connected (View markets)

Connected: summary

Connected: markets

Connected: markets, asset detail (loading)

Connected: markets, asset detail

Connected: markets, asset detail (fetch error)

Convert

Convert: no balance

Convert: quote loading

Convert: quote ready

Convert: quote submitting

Convert: complete

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

mkarolin

chromium_src changes LGTM

diracdeltas · 2021-05-12T23:32:14Z

components/ftx/browser/ftx_json_parser.cc

+          json, base::JSONParserOptions::JSON_PARSE_RFC);
+  base::Optional<base::Value>& records_v = value_with_error.value;
+  if (!records_v) {
+    LOG(ERROR) << "Invalid response, could not parse JSON, JSON is: " << json;


could this json potentially contain sensitive data or PII?

Some of them not, some of them yes. I'll remove the JSON output for all of them. Thanks, I didn't notice these.

These are all removed now via https://github.com/brave/brave-core/compare/84801d2cc4081191007ee6c6c4a1f81f7a208ceb..a88598a8bf6966ef5c2e6cbe823d1c7a3b632db2

diracdeltas · 2021-05-12T23:32:49Z

components/ftx/browser/ftx_json_parser.cc

+  base::Optional<base::Value>& records_v = value_with_error.value;
+
+  if (!records_v) {
+    LOG(ERROR) << "Invalid response, could not parse JSON, JSON is: " << json;


same as above

fixed via
https://github.com/brave/brave-core/compare/84801d2cc4081191007ee6c6c4a1f81f7a208ceb..a88598a8bf6966ef5c2e6cbe823d1c7a3b632db2

diracdeltas · 2021-05-12T23:33:09Z

components/ftx/browser/ftx_json_parser.cc

+          json, base::JSONParserOptions::JSON_PARSE_RFC);
+  base::Optional<base::Value>& records_v = value_with_error.value;
+  if (!records_v) {
+    LOG(ERROR) << "Invalid response, could not parse JSON, JSON is: " << json;


same as above

fixed via https://github.com/brave/brave-core/compare/84801d2cc4081191007ee6c6c4a1f81f7a208ceb..a88598a8bf6966ef5c2e6cbe823d1c7a3b632db2

simonhong

LGTM with nits and after fixing percent and infinite throbbing issue shared in DM.

browser/brave_profile_prefs.cc

components/ftx/browser/ftx_service.h

browser/ftx/ftx_protocol_handler.cc

browser/ftx/ftx_service_factory.h

browser/ftx/ftx_protocol_handler.cc

browser/extensions/api/ftx_api.h

browser/extensions/api/ftx_api.cc

browser/ui/BUILD.gn

components/ftx/common/pref_names.h

bridiver · 2021-05-14T04:54:15Z

browser/ftx/sources.gni

+brave_browser_ftx_deps = []
+
+if (ftx_enabled) {
+  brave_browser_ftx_sources += [


is there a circular dependency here? If not it should just be added as a normal dep

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T04:57:19Z

browser/ftx/ftx_protocol_handler.h

+
+#include <string>
+
+#include "chrome/browser/external_protocol/external_protocol_handler.h"


This header is not used here and you are missing headers for GURL, WebContents Optional and Origin. See https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:00:27Z

browser/ui/BUILD.gn

@@ -235,6 +235,8 @@ source_set("ui") {
    "//brave/components/brave_wayback_machine:buildflags",
    "//brave/components/cosmetic_filters/resources/data:generated_resources",
    "//brave/components/crypto_dot_com/browser/buildflags:buildflags",
+    "//brave/components/ftx/browser/buildflags:buildflags",
+    "//brave/components/ftx/common",


this needs to be inside enable_ftx to match https://github.com/brave/brave-core/pull/8629/files#diff-7deaca579ce61106651458308d2792dc2e5bf9f6958adeb897cac92c45263f24R54

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:00:58Z

components/ftx/browser/buildflags/BUILD.gn

+buildflag_header("buildflags") {
+  header = "buildflags.h"
+  flags = [
+    "FTX_ENABLED=$ftx_enabled",


the convention is enable_feature

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:04:29Z

components/ftx/common/BUILD.gn

+
+assert(ftx_enabled)
+
+source_set("common") {


since these are only used in browser this component doesn't need browser and common
"A component used only by the browser process contains code directly in its directory." - https://www.chromium.org/developers/design-documents/cookbook

bridiver · 2021-05-14T05:06:09Z

components/ftx/browser/ftx_service.h

+#include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"


this header is not used in this file and others (like base/callback.h) are missing. See https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:12:42Z

components/ftx/browser/ftx_service.h

+typedef std::vector<TokenPriceData> FTXFuturesData;
+typedef std::map<std::string, double> FTXAccountBalances;
+
+class FTXService : public KeyedService {


can we have some comments explaining what this is since FTX is not exactly intuitive? :)

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:18:09Z

components/ftx/browser/ftx_service.cc

+      kRetriesCountOnNetworkChange,
+      network::SimpleURLLoader::RetryMode::RETRY_ON_NETWORK_CHANGE);
+
+  auto iter = url_loaders_.insert(url_loaders_.begin(), std::move(url_loader));


url_loaders_ should be cleared in KeyedService::Shutdown

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:20:57Z

components/ftx/browser/ftx_json_parser.cc

+#include "base/strings/string_number_conversions.h"
+#include "base/values.h"
+
+bool FTXJSONParser::GetFuturesDataFromJSON(const std::string& json,


this doesn't necessarily need to happen in this PR, but we should be using JSONValueConverter for this kind of thing

bridiver · 2021-05-14T05:21:37Z

components/ftx/browser/ftx_service.cc

+    const std::map<std::string, std::string>& headers) {
+  FTXFuturesData data;
+  if (status >= 200 && status <= 299) {
+    FTXJSONParser::GetFuturesDataFromJSON(body, &data, futures_filter);


return value is ignored

Empty data will be returned. If we handle false return value then we can show error UI. Follow-up here brave/brave-browser#15852

bridiver · 2021-05-14T05:22:01Z

components/ftx/browser/ftx_json_parser.cc

+          json, base::JSONParserOptions::JSON_PARSE_RFC);
+  base::Optional<base::Value>& records_v = value_with_error.value;
+  if (!records_v) {
+    LOG(ERROR) << "FTX GetFuturesDataFromJSON: did not understand json";


this should be a VLOG and also doesn't provide any useful information

bridiver · 2021-05-14T05:25:18Z

browser/extensions/api/ftx_api.cc

+    result.Append(std::move(point));
+  }
+
+  Respond(OneArgument(std::move(result)));


this is just going to respond with an empty ListValue if there was an error. Seems like we should respond with an error here

empty result handed on the frontend, but no error is shown even though it should. Follow-up here brave/brave-browser#15852

bridiver · 2021-05-14T05:29:32Z

browser/ftx/ftx_service_factory.cc

+
+KeyedService* FTXServiceFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  return new FTXService(Profile::FromBrowserContext(context));


FTXService takes BrowserContext so no reason for Profile::FromBrowserContext

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:31:07Z

components/ftx/browser/ftx_service.cc

+
+}  // namespace
+
+FTXService::FTXService(content::BrowserContext* context)


passing in BrowserContext should be avoided because it makes this impossible to use for ios and is generally considered an anti-pattern. It is better to pass in the url_loader_factory and user prefs to avoid any content dependencies

follow-up brave/brave-browser#15851

bridiver · 2021-05-14T05:32:52Z

browser/extensions/api/ftx_api.cc

+      Profile::FromBrowserContext(context));
+}
+
+bool IsFTXAPIAvailable(content::BrowserContext* context) {


I think IsFTXAvailable should be defined somewhere else so the implementation stays in sync with https://github.com/brave/brave-core/pull/8629/files#diff-05d82cf839b090feae4f8623d86beebe9c71b528eca857c3a235c19d65a7fbfeR21

actually there's really not much reason for this because GetFTXService will return null in that case

removed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver · 2021-05-14T05:39:26Z

browser/ftx/ftx_protocol_handler.cc

+    std::string auth_token = parts["code"];
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents->GetBrowserContext());
+    FTXServiceFactory::GetInstance()->GetForProfile(profile)


this is going to crash if it's called from Tor or Guest

fixed via https://github.com/brave/brave-core/compare/6f4c503619e82a440ef7db9782288e922c37d5e4..f4d887c0baf39543331107f2e22f9b0ad4986c02

bridiver

DEPS are ok and approved based on making critical changes discussed in DM

petemill · 2021-05-14T06:39:37Z

SonarCloud is complaining about duplicated crypto assets. We have a plan to use a shared set, contained in this PR, as a follow-up.

petemill · 2021-05-14T17:36:44Z

macOS test failure is unrelated

[2021-05-14T08:37:30.822Z] 1 test crashed:

[2021-05-14T08:37:30.822Z]     AdBlockServiceTest.CnameCloakedRequestsGetBlocked (../../brave/browser/brave_shields/ad_block_service_browsertest.cc:586)

linux test failure is unrelated


[2021-05-14T07:21:35.059Z]     BraveAdsBrowserTest/BraveAdsUpgradeBrowserTest.UpgradePath/PreferencesForVersion062WithRewardsEnabled_ForSupportedLocale_ForNewlySupportedLocale_RewardsShouldBeEnabled_AdsShouldBeDisabled (../../brave/components/brave_ads/browser/ads_service_browsertest.cc:1044)

[2021-05-14T07:21:35.059Z]     BraveAdsBrowserTest/BraveAdsUpgradeBrowserTest.UpgradePath/PreferencesForVersion062WithRewardsEnabled_ForUnsupportedLocale_ForUnsupportedLocale_RewardsShouldBeEnabled_AdsShouldBeDisabled (../../brave/components/brave_ads/browser/ads_service_browsertest.cc:1044)

[2021-05-14T07:21:35.059Z]     BraveAdsBrowserTest/BraveAdsUpgradeBrowserTest.UpgradePath/PreferencesForVersion070WithRewardsAndAdsEnabled_ForSupportedLocale_ForUnsupportedLocale_RewardsShouldBeEnabled_AdsShouldBeEnabled (../../brave/components/brave_ads/browser/ads_service_browsertest.cc:1044)

[2021-05-14T07:21:35.059Z]     BraveAdsBrowserTest/BraveAdsUpgradeBrowserTest.UpgradePath/PreferencesForVersion072WithRewardsAndAdsEnabled_ForSupportedLocale_ForUnsupportedLocale_RewardsShouldBeEnabled_AdsShouldBeEnabled (../../brave/components/brave_ads/browser/ads_service_browsertest.cc:1044)

[2021-05-14T07:21:35.059Z] null

post-init failure is due to audit-deps which is fixed on master now.

Everything else passed. Merging...

petemill force-pushed the ntp-ftx branch 6 times, most recently from 170e870 to 6a72d26 Compare May 11, 2021 04:06

petemill changed the title ~~FTX for NTP~~ (Desktop) FTX for NTP May 11, 2021

petemill force-pushed the ntp-ftx branch 4 times, most recently from b58aa94 to a4a310d Compare May 11, 2021 07:11

petemill self-assigned this May 11, 2021

petemill force-pushed the ntp-ftx branch 2 times, most recently from cf90ec1 to 56aed23 Compare May 12, 2021 06:03

petemill requested review from simonhong and bsclifton May 12, 2021 06:04

petemill force-pushed the ntp-ftx branch from 56aed23 to 136aaa6 Compare May 12, 2021 06:30

petemill marked this pull request as ready for review May 12, 2021 17:14

petemill requested a review from a team as a code owner May 12, 2021 17:14

mkarolin approved these changes May 12, 2021

View reviewed changes

petemill force-pushed the ntp-ftx branch 2 times, most recently from 06a4dff to 84801d2 Compare May 12, 2021 18:48

diracdeltas reviewed May 12, 2021

View reviewed changes

petemill force-pushed the ntp-ftx branch 2 times, most recently from a88598a to 799c791 Compare May 13, 2021 02:52

simonhong approved these changes May 13, 2021

View reviewed changes

ryanml and others added 2 commits May 13, 2021 19:15

Adding FTX service with API calls and JSON parser

8c974d1

New Tab Page WebUI: FTX Widget UI

35bc0d8

petemill added 2 commits May 13, 2021 19:15

Storybook: allow extra strings to be provided by each story

dd21ca6

WebUI: l18n allow simple string injection of a react component

21ea8df

petemill force-pushed the ntp-ftx branch 3 times, most recently from f84617c to 6f4c503 Compare May 14, 2021 04:51

petemill requested a review from bridiver as a code owner May 14, 2021 04:51

bridiver reviewed May 14, 2021

View reviewed changes

bridiver approved these changes May 14, 2021

View reviewed changes

NTP FTX: wire up UI actions to FTX API

f4d887c

petemill force-pushed the ntp-ftx branch from 6f4c503 to f4d887c Compare May 14, 2021 06:29

petemill merged commit 68bb77d into master May 14, 2021

petemill deleted the ntp-ftx branch May 14, 2021 17:37

petemill added this to the 1.26.x - Nightly milestone May 14, 2021


		#include <string>

		#include "chrome/browser/external_protocol/external_protocol_handler.h"


		} // namespace

		FTXService::FTXService(content::BrowserContext* context)


		assert(ftx_enabled)

		source_set("common") {

(Desktop) FTX for NTP #8629

(Desktop) FTX for NTP #8629

Conversation

petemill commented Apr 23, 2021 • edited

Review instructions

Note on WebUI API

Test Plan:

Not yet initialized

Not connected

Not connected (View markets)

Connected: summary

Connected: markets

Connected: markets, asset detail (loading)

Connected: markets, asset detail

Connected: markets, asset detail (fetch error)

Convert

Convert: no balance

Convert: quote loading

Convert: quote ready

Convert: quote submitting

Convert: complete

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

mkarolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill May 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill May 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong left a 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

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

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

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

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

bridiver left a comment

Choose a reason for hiding this comment

petemill commented May 14, 2021

petemill commented May 14, 2021 • edited

petemill commented Apr 23, 2021 •

edited

petemill May 13, 2021 •

edited

petemill May 13, 2021 •

edited

petemill commented May 14, 2021 •

edited