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

[BUG] Fix the TokenRateController API #7903

Closed
sethkfman opened this issue Nov 22, 2023 · 2 comments
Closed

[BUG] Fix the TokenRateController API #7903

sethkfman opened this issue Nov 22, 2023 · 2 comments
Labels
Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-mobile-platform type-bug Something isn't working

Comments

@sethkfman
Copy link
Contributor

sethkfman commented Nov 22, 2023 •
edited by tommasini
Loading

What is this about?

This issue is to replace the CoinGecko API on the TokenRates controller. The API should be replaced with the public PriceAPI written by the codefi team which is documented here.

Scenario

  • GIVEN a user has an account and assets in their wallet
  • WHEN they navigate to the wallet view
  • AND they review their assets
  • AND they have a network connection
  • THEN there is a fiat balance displayed

Design

No response

Technical Details

  1: 'ethereum',
  10: 'optimistic-ethereum',
  25: 'cronos',
  56: 'binance-smart-chain',
  57: 'syscoin',
  66: 'okex-chain',
  70: 'hoo-smart-chain',
  82: 'meter',
  88: 'tomochain',
  100: 'xdai',
  106: 'velas',
  122: 'fuse',
  128: 'huobi-token',
  137: 'polygon-pos',
  250: 'fantom',
  288: 'boba',
  321: 'kucoin-community-chain',
  324: 'zksync',
  361: 'theta',
  1088: 'metis-andromeda',
  1284: 'moonbeam',
  1285: 'moonriver',
  8453: 'base',
  8545: 'shiden network',
  10000: 'smartbch',
  42161: 'arbitrum-one',
  42220: 'celo',
  42262: 'oasis',
  43114: 'avalanche',
  59144: 'linea',
  333999: 'polis-chain',
  1313161554: 'aurora',
  1666600000: 'harmony-shard-0',
  • Create a core PR that applies these changes to the TokenRatesController
  • Test the API calls and confirm data objects have the same format as what CoinGecko returns

Data flow

image

Threat Modeling Framework

This is a public API and does not require an API token.

Acceptance Criteria

Token rates are displaying with the new API integration when there is a network connection.

References

Slack - https://consensys.slack.com/archives/C065W3877E3/p1700574338429709
@digiwand digiwand mentioned this issue Nov 22, 2023
Closed
@vpintorico vpintorico added Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing type-bug Something isn't working labels Nov 23, 2023
Gudahtt added a commit that referenced this issue Nov 23, 2023
@Gudahtt
chore(patch): Use core branch for assets-controllers patch
e6df9f9 
The `@metamask/assets-controllers` patch for the TokenRatesController
has been updated to use a branch on the core monorepo. Using a core
branch for these patch changes lets us more easily lint and type check
the changes (and even test them if we feel the need), and avoids the
need to work with compiled JavaScript (which can be tricky, especially
when dealing with async code).

The patch is functionally identical to before; the only differences are
type fixes, and a comment explaining the patch and how it is
maintained.

So far only the `TokenRatesController` portion of the patch uses this
branch, but we can extend this strategy later to the other files in
this patch, if we find that useful.

Relates to #7903
@Gudahtt Gudahtt mentioned this issue Nov 23, 2023
Merged
13 tasks
Gudahtt added a commit that referenced this issue Nov 24, 2023
@Gudahtt
chore(patch): Use core branch for assets-controllers patch
7c8405d 
The `@metamask/assets-controllers` patch for the TokenRatesController
has been updated to use a branch on the core monorepo. Using a core
branch for these patch changes lets us more easily lint and type check
the changes (and even test them if we feel the need), and avoids the
need to work with compiled JavaScript (which can be tricky, especially
when dealing with async code).

The patch is functionally identical to before; the only differences are
type fixes, and a comment explaining the patch and how it is
maintained.

So far only the `TokenRatesController` portion of the patch uses this
branch, but we can extend this strategy later to the other files in
this patch, if we find that useful.

Relates to #7903
Gudahtt added a commit that referenced this issue Nov 24, 2023
@Gudahtt
chore(patch): Use core branch for assets-controllers patch
210f858 
The `@metamask/assets-controllers` patch for the TokenRatesController
has been updated to use a branch on the core monorepo. Using a core
branch for these patch changes lets us more easily lint and type check
the changes (and even test them if we feel the need), and avoids the
need to work with compiled JavaScript (which can be tricky, especially
when dealing with async code).

The patch is functionally identical to before; the only differences are
type fixes, and a comment explaining the patch and how it is
maintained.

So far only the `TokenRatesController` portion of the patch uses this
branch, but we can extend this strategy later to the other files in
this patch, if we find that useful.

Relates to #7903
Gudahtt added a commit that referenced this issue Nov 28, 2023
@Gudahtt
chore(patch): Use core branch for assets-controllers patch (#7922)
b58f4f6 
## **Description**

The `@metamask/assets-controllers` patch for the TokenRatesController
has been updated to use a branch on the core monorepo. Using a core
branch for these patch changes lets us more easily lint and type check
the changes (and even test them if we feel the need), and avoids the
need to work with compiled JavaScript (which can be tricky, especially
when dealing with async code).

The patch is functionally identical to before; the only differences are
type fixes, and a comment explaining the patch and how it is maintained.

So far only the `TokenRatesController` portion of the patch uses this
branch, but we can extend this strategy later to the other files in this
patch, if we find that useful.

## **Related issues**

Relates to #7903

## **Manual testing steps**

N/A, no functional changes

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@sethkfman sethkfman changed the title Fix the TokenRateController API [BUG] Fix the TokenRateController API Nov 30, 2023
@sethkfman
Copy link
Contributor Author

Related to issue #7887

@metamaskbot metamaskbot added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed in-progress labels Dec 5, 2023
@sethkfman sethkfman mentioned this issue Dec 7, 2023
Closed
13 tasks
@metamaskbot metamaskbot added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 18, 2023
@metamaskbot metamaskbot added blocked and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 5, 2024
@tommasini
Copy link
Contributor

Moving this to done since it was done on this #8143

@github-actions github-actions bot removed the blocked label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-mobile-platform type-bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@vpintorico @gauthierpetetin @sethkfman @metamaskbot @tommasini