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.

Sign up for GitHub

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

Jump to bottom

feat(wallet): inject coingecko id for custom tokens #19489

Merged
merged 7 commits into from
Aug 4, 2023
Merged

feat(wallet): inject coingecko id for custom tokens #19489

merged 7 commits into from
Aug 4, 2023

Conversation

onyb
Copy link
Member

@onyb onyb commented Jul 31, 2023

Resolves brave/brave-browser#31975

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • 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 wiki
    • npm run lint, npm run presubmit wiki, 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:

Auto-fill Coingecko ID when adding custom token to Visible Assets

visible.assets.mov

Infer Coingecko ID when adding custom token through wallet_watchAsset

Prerequisites

  1. Switch to Polygon network
  2. Balancer Aave Boosted MATIC from https://www.coingecko.com/en/coins/balancer-aave-boosted-wmatic
Before (1.56.x) After

@onyb onyb requested a review from a team as a code owner July 31, 2023 07:03
@onyb onyb self-assigned this Jul 31, 2023
@onyb onyb requested review from a team as code owners July 31, 2023 07:03
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core labels Jul 31, 2023
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

supermassive
supermassive reviewed Jul 31, 2023
View reviewed changes
components/brave_wallet_ui/common/slices/api.slice.ts Outdated
Comment on lines 832 to 803
providesTags: (res, err, { chainId, contractAddress }) =>
err
? ['CoingeckoId', 'UNKNOWN_ERROR']
: [{ type: 'CoingeckoId', id: `${chainId}-${contractAddress}` }]
}),
Copy link
Collaborator

@supermassive supermassive Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need providesTags sections for this endpoint?
afaik it only makes sense if there are multiple endpoints dealing with same 'entity' but we have only one

Copy link
Contributor

@josheleonard josheleonard Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We will want a tag for this so we can invalidate a coingecko id for a token

https://redux-toolkit.js.org/rtk-query/usage/cache-behavior#re-fetching-after-mutations-by-invalidating-cache-tags

StephenHeaps
StephenHeaps approved these changes Jul 31, 2023
View reviewed changes
Copy link
Collaborator

@StephenHeaps StephenHeaps left a comment

Choose a reason for hiding this comment

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

iOS ++; opened https://github.com/brave/brave-ios/issues/7808 to handle Coingecko ID auto-fill on iOS.

components/brave_wallet/common/brave_wallet.mojom Outdated Show resolved Hide resolved
@onyb onyb force-pushed the f/wallet/coingecko-injection branch from 887c94d to 0456fc4 Compare August 3, 2023 20:13
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

supermassive
supermassive approved these changes Aug 4, 2023
View reviewed changes
Copy link
Collaborator

@supermassive supermassive left a comment

Choose a reason for hiding this comment

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

wallet core lgtm

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

josheleonard
josheleonard approved these changes Aug 4, 2023
View reviewed changes
Copy link
Contributor

@josheleonard josheleonard left a comment

Choose a reason for hiding this comment

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

font-end approved with suggestions

components/brave_wallet_ui/components/shared/add-custom-token-form/add-custom-token-form.tsx Outdated Show resolved Hide resolved
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@onyb onyb enabled auto-merge (squash) August 4, 2023 21:07
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@onyb onyb merged commit fa66bcb into master Aug 4, 2023
6 checks passed
@onyb onyb deleted the f/wallet/coingecko-injection branch August 4, 2023 21:50
@github-actions github-actions bot added this to the 1.58.x - Nightly milestone Aug 4, 2023
@srirambv srirambv mentioned this pull request Aug 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supermassive supermassive supermassive approved these changes

@StephenHeaps StephenHeaps StephenHeaps approved these changes

@josheleonard josheleonard josheleonard approved these changes

Assignees

@onyb onyb

Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet
Projects
None yet
Milestone
1.58.x - Release
Development

Successfully merging this pull request may close these issues.

Auto-fill Coingecko IDs while adding custom tokens
5 participants
@onyb @brave-builds @supermassive @StephenHeaps @josheleonard