Skip to content
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

Merged
merged 22 commits into from
Aug 19, 2022
Merged

Conversation

nvonpentz
Copy link
Member

@nvonpentz nvonpentz commented Mar 7, 2022

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:

  • 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
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • 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:

Test Plan:

Market.Data.mov

@nvonpentz
Copy link
Member Author

Waiting on @muliswilliam to validate this branch works with his frontend code + the new ratios endpoint before requesting review.

@muliswilliam muliswilliam mentioned this pull request Mar 16, 2022
25 tasks
@nvonpentz nvonpentz added this to the 1.38.x - Nightly milestone Mar 16, 2022
@nvonpentz nvonpentz marked this pull request as ready for review March 16, 2022 17:09
@nvonpentz nvonpentz requested a review from a team as a code owner March 16, 2022 17:09
muliswilliam
muliswilliam previously approved these changes Mar 16, 2022
@diracdeltas
Copy link
Member

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

@muliswilliam muliswilliam mentioned this pull request Mar 17, 2022
25 tasks
@nvonpentz nvonpentz force-pushed the markets-ratios-proxy branch 2 times, most recently from b3a7fb9 to c792450 Compare March 23, 2022 15:38
@nvonpentz nvonpentz force-pushed the markets-ratios-proxy branch 2 times, most recently from 2ff4285 to c1cc1a9 Compare March 31, 2022 17:03
@nvonpentz nvonpentz requested a review from a team as a code owner March 31, 2022 18:09
@muliswilliam muliswilliam changed the title Add Ratios API for coingecko coin markets data Add Ratios API for coingecko coin markets data and markets tab Mar 31, 2022
@muliswilliam muliswilliam changed the title Add Ratios API for coingecko coin markets data and markets tab Add Ratios API for coingecko coin markets data Mar 31, 2022
package.json Outdated Show resolved Hide resolved
components/brave_wallet_ui/utils/format-prices.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@bbondy
Copy link
Member

bbondy commented Apr 1, 2022

Pls avoid dependencies at all costs, only add if absolutely necessary (and if adding, they need security review).

@diracdeltas
Copy link
Member

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

yrliou
yrliou previously approved these changes Apr 5, 2022
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backend change LGTM.

components/brave_wallet/browser/brave_wallet_constants.cc Outdated Show resolved Hide resolved
@muliswilliam
Copy link
Contributor

@diracdeltas @bbondy I have removed any dependencies I had introduced to this feature. Does it still need to be in chrome-untrusted frame?

@diracdeltas
Copy link
Member

I have removed any dependencies I had introduced to this feature. Does it still need to be in chrome-untrusted frame?

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

@muliswilliam
Copy link
Contributor

muliswilliam commented Apr 5, 2022

I have removed any dependencies I had introduced to this feature. Does it still need to be in chrome-untrusted frame?

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 @solana/web3.js dependency is already on master. Most likely added to add solana support. Aren't most interfaces on the wallet displaying external data? For example asset price history is fetched from coingecko too. As far as I know, it is not sandboxed.

Copy link
Member

@goodov goodov left a 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.

@muliswilliam muliswilliam merged commit 0f0777a into master Aug 19, 2022
@muliswilliam muliswilliam deleted the markets-ratios-proxy branch August 19, 2022 05:57
@github-actions github-actions bot added this to the 1.44.x - Nightly milestone Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project