Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

fix: Fixes BigNumber error due to incorrect number type #296

Merged
merged 8 commits into from
Dec 17, 2018

Conversation

ltfschoen
Copy link
Contributor

Fixes error Uncaught Error: lt() number type has more than 15 significant. It occurred when for example the users balance was 0.999368704999999998 ETH, the tx fee was 0.000063 (i.e. 3Gwei * 21000)

Fixes error `Uncaught Error: lt() number type has more than 15 significant` that occurs when for example the users
balance is 0.999368704999999998, the tx fee is 0.000063 (i.e. 3Gwei * 21000)
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Thanks, I think I've had this error before, but didn't know how to reproduce.
I'd let a pro confirm before merging.

@amaury1093
Copy link
Collaborator

@ltfschoen I merged your PRs chronologically, but this one has some conflicts with 2 previous ones. Could you rebase this PR on top of latest master?

Feature-wise, we can re-test everything from this PR.

@ltfschoen
Copy link
Contributor Author

@amaurymartiny I've fixed the merge conflicts and used BigNumber in a couple of other places. The main change that resolves the error is .toString() though

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Works well, tiny comment


if (!amount || isNaN(amount)) {
if (!amount || amountBn.isNaN()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

!amount is not useful here, since amount.toString() would throw an error before.

Could you add the test if (!amount) {...} before the new BigNumber initialisation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've pushed a commit with this change

@@ -161,11 +161,11 @@ class Send extends Component {
preValidate = values => {
const { balance, token } = this.props;
const amount = +values.amount;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use BigNumbers here, the + is useless

Copy link
Contributor Author

@ltfschoen ltfschoen Dec 15, 2018

Choose a reason for hiding this comment

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

I found that when the user enters a zero value (i.e. 0, 0.0, etc), the following returned different values:

  • !values.amount returned false
  • !+values.amount returned true
  • amountBn.isNaN() returned false

So if I replaced +values.amount with values.amount or just relied upon amountBn.isNaN() it would allow zero values (which would deviate from the current implementation where zero values are not permitted).
So in the commit that I've pushed I've added a .isZero() check as well.

@amaury1093 amaury1093 merged commit 7f515a0 into master Dec 17, 2018
@amaury1093 amaury1093 deleted the luke-250-insufficient-balance branch December 17, 2018 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants