-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
fix: currency data type #4741
base: main
Are you sure you want to change the base?
fix: currency data type #4741
Conversation
Co-authored-by: Charly Chevalier <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like string
is a good initial choice here to preserve precision and accuracy which gets lost when converting the value to floating point number
?
Is there some reason why continuing to return string
s is problematic here? If so, could we consider a precise decimal type (e.g. BigNumber
)?
Nice properties of decimal strings is that they are (relatively) non-ambiguous, serialize cleanly to/from JSON, and don't rely on any specific implementations.
I see that CurrencyRateState
is still handling and exposing rates as number
s - should we consider doing the inverse and changing that instead?
I agree with your statement @legobeat. This was the reason I did it as |
I don't mind returning a string, I think this is a good technical choice. The only problem here is that the API returns a number and it is never stringified. We should stringify the return value if we want to keep the string type @gantunesr @legobeat |
Explanation
There's an inconsistency regarding the currency rate data type, this PR changes it from
string
tonumber
References
Changelog
@metamask/assets-controller
RatesController
data type update.number
ofconversionRate
andusdConversionRate
fromRate
Checklist