-
Notifications
You must be signed in to change notification settings - Fork 868
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
(Desktop) FTX for NTP #8629
(Desktop) FTX for NTP #8629
Conversation
170e870
to
6a72d26
Compare
b58aa94
to
a4a310d
Compare
cf90ec1
to
56aed23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chromium_src
changes LGTM
06a4dff
to
84801d2
Compare
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this json potentially contain sensitive data or PII?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of them not, some of them yes. I'll remove the JSON output for all of them. Thanks, I didn't notice these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base::Optional<base::Value>& records_v = value_with_error.value; | ||
|
||
if (!records_v) { | ||
LOG(ERROR) << "Invalid response, could not parse JSON, JSON is: " << json; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a88598a
to
799c791
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits and after fixing percent and infinite throbbing issue shared in DM.
f84617c
to
6f4c503
Compare
browser/ftx/sources.gni
Outdated
brave_browser_ftx_deps = [] | ||
|
||
if (ftx_enabled) { | ||
brave_browser_ftx_sources += [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a circular dependency here? If not it should just be added as a normal dep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser/ftx/ftx_protocol_handler.h
Outdated
|
||
#include <string> | ||
|
||
#include "chrome/browser/external_protocol/external_protocol_handler.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser/ui/BUILD.gn
Outdated
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be inside enable_ftx to match https://github.com/brave/brave-core/pull/8629/files#diff-7deaca579ce61106651458308d2792dc2e5bf9f6958adeb897cac92c45263f24R54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildflag_header("buildflags") { | ||
header = "buildflags.h" | ||
flags = [ | ||
"FTX_ENABLED=$ftx_enabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the convention is enable_feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
assert(ftx_enabled) | ||
|
||
source_set("common") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
components/ftx/browser/ftx_service.h
Outdated
#include "base/macros.h" | ||
#include "base/memory/scoped_refptr.h" | ||
#include "base/memory/weak_ptr.h" | ||
#include "base/scoped_observer.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typedef std::vector<TokenPriceData> FTXFuturesData; | ||
typedef std::map<std::string, double> FTXAccountBalances; | ||
|
||
class FTXService : public KeyedService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have some comments explaining what this is since FTX is not exactly intuitive? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kRetriesCountOnNetworkChange, | ||
network::SimpleURLLoader::RetryMode::RETRY_ON_NETWORK_CHANGE); | ||
|
||
auto iter = url_loaders_.insert(url_loaders_.begin(), std::move(url_loader)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url_loaders_ should be cleared in KeyedService::Shutdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "base/strings/string_number_conversions.h" | ||
#include "base/values.h" | ||
|
||
bool FTXJSONParser::GetFuturesDataFromJSON(const std::string& json, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't necessarily need to happen in this PR, but we should be using JSONValueConverter for this kind of thing
const std::map<std::string, std::string>& headers) { | ||
FTXFuturesData data; | ||
if (status >= 200 && status <= 299) { | ||
FTXJSONParser::GetFuturesDataFromJSON(body, &data, futures_filter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return value is ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty data will be returned. If we handle false
return value then we can show error UI. Follow-up here brave/brave-browser#15852
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a VLOG and also doesn't provide any useful information
result.Append(std::move(point)); | ||
} | ||
|
||
Respond(OneArgument(std::move(result))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just going to respond with an empty ListValue if there was an error. Seems like we should respond with an error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty result handed on the frontend, but no error is shown even though it should. Follow-up here brave/brave-browser#15852
browser/ftx/ftx_service_factory.cc
Outdated
|
||
KeyedService* FTXServiceFactory::BuildServiceInstanceFor( | ||
content::BrowserContext* context) const { | ||
return new FTXService(Profile::FromBrowserContext(context)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTXService takes BrowserContext so no reason for Profile::FromBrowserContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
} // namespace | ||
|
||
FTXService::FTXService(content::BrowserContext* context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow-up brave/brave-browser#15851
browser/extensions/api/ftx_api.cc
Outdated
Profile::FromBrowserContext(context)); | ||
} | ||
|
||
bool IsFTXAPIAvailable(content::BrowserContext* context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually there's really not much reason for this because GetFTXService
will return null in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser/ftx/ftx_protocol_handler.cc
Outdated
std::string auth_token = parts["code"]; | ||
Profile* profile = | ||
Profile::FromBrowserContext(web_contents->GetBrowserContext()); | ||
FTXServiceFactory::GetInstance()->GetForProfile(profile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is going to crash if it's called from Tor or Guest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEPS are ok and approved based on making critical changes discussed in DM
SonarCloud is complaining about duplicated crypto assets. We have a plan to use a shared set, contained in this PR, as a follow-up. |
macOS test failure is unrelated
linux test failure is unrelated
post-init failure is due to audit-deps which is fixed on master now. Everything else passed. Merging... |
ftx_protocol_handler.cc
)OSCrypt
.ftx_service.cc
,ftx_json_parser.cc
)Resolves brave/brave-browser#15790
Review instructions
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:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on