MetaMask / MetaMask/metamask-extension

Clean up currency conversion code

Open
#6,290 0 comments 0 reactions 0 assignees View on GitHub
area-UI type-refactor
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

Below is copied from https://github.com/MetaMask/metamask-extension/pull/4895#issuecomment-408452748 Once we clear the blockers from the epic this is in, we should:

- [x] Audit the codebase for all use of conversion type utilities, or conversions without utilities
- [x] Design a minimum viable api
- [ ] write a suite of tests for existing methods that depend on the "conversion-util", do not mock it away
- [ ] research existing methods in other libraries (e.g. `web3.fromWei(web3.toWei('1', 'gwei'), 'ether')`)
- [x] Between the existing base "conversion-util" and relevant methods in other libraries, rewrite the base "conversion-util" to be very readable and user friendly
- [x] In the spirit of the below comments, reduce code duplication with resepect to conversions throughout the codebase. Start by making a list of tasks, and then do them
- [ ] Write documentation for our base conversion utility and the convenience methods built on top of those

We are going to forever have issues with needing to render numbers in different ways in different places.

It would be nice if we could make these transformations - of data from some value of cryptocurrency to some nicely formatted string for rendering - simpler, DRYer and less error prone.

For instance, after this PR we have:

```
const fiatTransactionAmount = this.getFiatTransactionAmount()
const roundedFiatTransactionAmount = roundExponential(fiatTransactionAmount)
return formatCurrency(roundedFiatTransactionAmount, currentCurrency)
```
The three functions called here require 4 inputs: `tokenAmount, currentCurrency, conversionRate, contractExchangeRate`. `this.getFiatTransactionAmount()` passes these for inputs to a util which in turn calls the conversion util, which in turn calls a series of operations on a Bignumber. `roundExponential` the does some operations with Bignumber, conditional on the passed values size, and `currencyFormatter` with some other library. In the same component we have to do the same three function calls plus a function call to do some addition, which uses a util which uses the conversion util...

I guess it would be nice if we could get the desired result by passing the four params to one utility method (or maybe a component). And that utility be something that can be used everywhere we need to render currencies.

I suppose that was what I was aiming for with the conversion-util. However, apart from the issue of the code being obscure and inaccessible, it also seems that we still need a separation of concerns between operations on values for the sake of getting a correct value and operations on values for the sake of getting a string to be rendered in a desired format.

So probably some combination of an improvement of the conversion-util and then creating a new generalized utility, or maybe component, for the sake of correct rendering.

I think when we aim to make this improvement, we should aim to create an API that would be desirable and easily used by other developers. We should even think beyond the Metamask project... this is an issues that all sorts of wallet and dapps will face. So, before we begin designing the solution, we should probably research other open source projects that have to solve this issue in their UIs and see what there solution is.

Contributor guide

Open the contributing guide

Research direction

Start by reading the existing conversion-util and the utility methods that depend on it, then research how other open-source wallet and dapp UIs handle cryptocurrency conversion and rendering. Define the separation between value conversion and display formatting, along with an API that supports the listed use cases. Done means the existing methods are covered by tests, duplication is reduced, and documentation describes the base utility and convenience methods.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.