MetaMask / MetaMask/metamask-extension
Clean up currency conversion code
- 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
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