-
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
Add Ratios API for coingecko coin markets data #12510
Conversation
Waiting on @muliswilliam to validate this branch works with his frontend code + the new ratios endpoint before requesting review. |
e748235
to
30cb728
Compare
this probably needs a privacy review - note this comment about coingecko: https://github.com/brave/security/issues/663#issuecomment-967770990 also i see this adds some JSON parsing - if it happens in the main process, ideally it would wait until @spylogsster's work on rust JSON validation is done and use that for compliance with rule of 2 https://chromium.googlesource.com/chromium/src/+/master/docs/security/rule-of-2.md |
b3a7fb9
to
c792450
Compare
2ff4285
to
c1cc1a9
Compare
components/brave_wallet_ui/components/asset-wishlist-star/index.tsx
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/components/desktop/views/market/index.tsx
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/components/market-datatable/index.tsx
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/components/market-datatable/index.tsx
Outdated
Show resolved
Hide resolved
Pls avoid dependencies at all costs, only add if absolutely necessary (and if adding, they need security review). |
I mentioned this in the previous PR for the front-end part of this, but I feel this feature should be displayed in a chrome-untrusted frame given the added deps. cc @petemill |
d63e476
to
b6e69b1
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.
backend change LGTM.
@diracdeltas @bbondy I have removed any dependencies I had introduced to this feature. Does it still need to be in chrome-untrusted frame? |
5de27b4
to
2f1f742
Compare
i still see some new deps in https://github.com/brave/brave-core/pull/12510/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519 - are those not used in the webui? regardless i feel this is a good candidate for chrome-untrusted sandboxing because it's displaying external untrusted data |
The |
This reverts commit 9567e47ffa6d9d66ac15f378cc20d8e3aa2140b2.
ef8f6bd
to
5d3f8d4
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.
Resolves brave/brave-browser#21455
Resolves brave/brave-browser#20674
This branch previously only included backend changes, but now includes both frontend and backend changes.
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
Test Plan:
Market.Data.mov