-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
|
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.
Hey @alfetopito , changes LGTM!
As a nitpick I can say that sometimes gas estimation might be a bit optimistic and a user will not wee the warning message, however, will not be able to sign the TX due to insuff. funds error, but the issue is reproducible to every network.
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.
Maybe increase the numbers a bit just in case? (if elena managed to get the error in MM)
Also, hardcoded doesn't sound too good. Maybe u can consider what I suggested below
cbb02d8
to
faf6155
Compare
CLA Assistant Lite: I have read the CLA Document and I hereby sign the CLA Leandro seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
Seems that I can't retest it as investment options are no longer available (2 weeks have passed). |
Merging this to cherry-pick into hotfix branch, should be reviewed after! |
Summary
Closes #2389
Gas estimation for gchain now uses proper values
To Test
0xcbeAd39cC0b43d14c2f240c25f3D5234bbDFF10c
and the account to claim with0x2F144ff590C94871f118583E61fdDd06b98120e4
(PK in on the spreadsheet)