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

Fix confirm token transaction amount display #7081

Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Aug 29, 2019

The token amount displayed when confirming a token transaction was wrongly being converted to a string in the container. As a result, the conversion into the user's preferred currency would fail.

A default value of '0' was added for the token amount as well, to prevent undefined from being rendered as the value. Really the value should never be undefined, but it was rather difficult to handle that case without a deeper investigation into how it might occur. The 0 default is consistent with existing rendering logic.

@Gudahtt Gudahtt requested review from danjm and whymarrh as code owners August 29, 2019 16:51
The token amount displayed when confirming a token transaction was
wrongly being converted to a string in the container. As a result, the
conversion into the user's preferred currency would fail.

A default value of '0' was added for the token amount as well, to
prevent `undefined` from being rendered as the value. Really the value
should never be undefined, but it was rather difficult to handle that
case without a deeper investigation into how it might occur. The 0
default is consistent with existing rendering logic.
@Gudahtt Gudahtt force-pushed the fix-confirm-token-transaction-amount-display branch from 3232f6c to 93cdcbb Compare August 30, 2019 23:20
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to fix the immediate issue.

An improvement that should be made at some point is to move the calculation of the amount in the user's preferred currency into the container instead of doing it within a component method. This applies to multiple calculations done in the component however, so I think it's okay to skip such changes for this PR... in fact this is really an unrelated code structure/organization point. I suppose I bring it up because an alternative way to fix this would be to ensure that methods that operate on this value can accept numbers in string or number type (as BN.js does, for instance), but this is another separate discussion.

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 31, 2019

Agreed - the current situation leaves much to be desired.

I would also like to move things around to ensure that it's impossible for the token amount to be undefined. The effort that would require seems daunting though. Maybe that could be done as part of an effort to move the state management for confirming transactions into a "ducks" style module.

@Gudahtt Gudahtt merged commit 87cf0ce into MetaMask:develop Aug 31, 2019
@Gudahtt Gudahtt deleted the fix-confirm-token-transaction-amount-display branch August 31, 2019 20:44
@Gudahtt Gudahtt added this to the UI Sprint 19 [Aug 19] milestone Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants