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 rounding issue when sending max tokens #5695

Merged
merged 4 commits into from
Nov 20, 2018

Conversation

alextsg
Copy link
Contributor

@alextsg alextsg commented Nov 7, 2018

Fixes #5682

@alextsg alextsg force-pushed the i5682-rounding branch 2 times, most recently from 9d20640 to 27738ee Compare November 8, 2018 02:01
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

Two small comments but otherwise this looks good to me

const tokenBalanceAmount = await findElement(driver, By.css('.transaction-view-balance__token-balance'))
assert.equal(await tokenBalanceAmount.getText(), '43 TST')
const tokenBalanceAmount = await findElement(driver, By.css('.transaction-view-balance__primary-balance'))
assert.equal(await tokenBalanceAmount.getText(), '43\nTST')
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use \s here in a regular expression, similar to how we have below:

await driver.wait(until.elementTextMatches(balance, /0\s*BAT/))

toDenomination: denomination,
})

displayValue = propsDisplayValue || formatCurrency(convertedValue, toCurrency)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we inline convertedValue could we write this as a single cost displayValue statement without the conditional?

@alextsg alextsg force-pushed the i5682-rounding branch 2 times, most recently from 494f19b to 3536bde Compare November 8, 2018 05:24
@@ -11,6 +11,9 @@ import {
import SendHeader from './send-header/'
import SendContent from './send-content/'
import SendFooter from './send-footer/'
import BigNumber from 'bignumber.js'

window.BigNumber = BigNumber
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to put BigNumber on window? I'm guess this was just for local development/testing purposes?

whymarrh
whymarrh previously approved these changes Nov 9, 2018
@danjm
Copy link
Contributor

danjm commented Nov 11, 2018

I made a PR against this branch to fix a minor issue: alextsg#2

@danjm
Copy link
Contributor

danjm commented Nov 11, 2018

QA'd. All looks good.

As a separate issue, we may want to deal with cases with a lot of significant digits on the confirm screen

screenshot from 2018-11-11 19-45-45

@alextsg
Copy link
Contributor Author

alextsg commented Nov 12, 2018

@danjm Merged your PR and noticed that tests were failing due to number inputs. Why was it necessary to remove the Number() part from return Number(decimalValueString) || 0 in token-input.component.js? getDecimalValue is expected to return a number instead of a string so it's causing some input issues.

@danfinlay
Copy link
Contributor

Fixes #5180.

@alextsg alextsg merged commit 4c87c05 into MetaMask:develop Nov 20, 2018
@alextsg alextsg deleted the i5682-rounding branch November 20, 2018 00:06
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.

4 participants