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

Clean up currency conversion code #6290

Open
4 of 7 tasks
danjm opened this issue Mar 12, 2019 · 0 comments
Open
4 of 7 tasks

Clean up currency conversion code #6290

danjm opened this issue Mar 12, 2019 · 0 comments
Labels
area-UI Relating to the user interface. type-refactor

Comments

@danjm
Copy link
Contributor

danjm commented Mar 12, 2019

Below is copied from #4895 (comment) Once we clear the blockers from the epic this is in, we should:

  • Audit the codebase for all use of conversion type utilities, or conversions without utilities
  • 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'))
  • Between the existing base "conversion-util" and relevant methods in other libraries, rewrite the base "conversion-util" to be very readable and user friendly
  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-UI Relating to the user interface. type-refactor
Projects
None yet
Development

No branches or pull requests

1 participant