Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Fix) Error when using up to the max amount of decimals for token transfer#1576

Merged
fernandomg merged 5 commits intodevelopmentfrom
fix/1575-error-on-send-max
Nov 9, 2020
Merged

(Fix) Error when using up to the max amount of decimals for token transfer#1576
fernandomg merged 5 commits intodevelopmentfrom
fix/1575-error-on-send-max

Conversation

@fernandomg
Copy link
Contributor

@fernandomg fernandomg commented Nov 6, 2020

This PR closes #1575, by fixing toTokenUnit function decimals round.

@fernandomg fernandomg self-assigned this Nov 6, 2020
@github-actions
Copy link

github-actions bot commented Nov 6, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 6, 2020 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/rinkeby/app

dasanra
dasanra suggested changes Nov 6, 2020
View reviewed changes
src/logic/tokens/utils/humanReadableValue.ts Outdated
return amountBN.toFixed(0, BigNumber.ROUND_DOWN)
}

return amountBN.toFixed()
Copy link
Collaborator

@dasanra dasanra Nov 6, 2020

Choose a reason for hiding this comment

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

Couldn't we just remove the IF and ensure that every number is rounded to have no decimals using ROUND_DOWN approximation?

Copy link
Contributor Author

@fernandomg fernandomg Nov 6, 2020

Choose a reason for hiding this comment

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

indeed! so simple now. Thanks.

@fernandomg fernandomg requested a review from dasanra November 6, 2020 11:45
@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/rinkeby/app

@dasanra
Copy link
Collaborator

dasanra commented Nov 6, 2020

@fernandomg nice. Thank you for adding the tests also

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 6, 2020

Travis automatic deployment:
https://pr1576--safereact.review.gnosisdev.com/volta/app

@francovenica
Copy link
Contributor

francovenica commented Nov 9, 2020

Tested in this safe: https://pr1576--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x0BdeD4FaDffdb12240E27954264071442a26fd45/transactions

I stored 0.000123123123123123 ETH in that safe
Tried to send 0.0001231231231231225 in the dev env to see the issue reported
Tried to send that same amount in this PR and it worked just fine

Looks good to me

matextrem
matextrem approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@matextrem matextrem left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@fernandomg fernandomg merged commit 325adda into development Nov 9, 2020
@fernandomg fernandomg deleted the fix/1575-error-on-send-max branch November 9, 2020 20:04
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@liliya-soroka liliya-soroka Awaiting requested review from liliya-soroka

4 more reviewers

@francovenica francovenica francovenica approved these changes

@matextrem matextrem matextrem approved these changes

@Agupane Agupane Agupane approved these changes

@dasanra dasanra dasanra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fernandomg fernandomg

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error when sending max ETH

5 participants

@fernandomg @dasanra @francovenica @matextrem @Agupane