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.

Sign up for GitHub

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

Jump to bottom

enforce minimum amount for Evm token transfers #293

Merged
merged 8 commits into from
Aug 15, 2023
Merged

enforce minimum amount for Evm token transfers #293

merged 8 commits into from
Aug 15, 2023

Conversation

sebastianscatularo
Copy link
Contributor

For token transfers from Evm the maximum decimal supported at this moment is 8 decimals, which lends to a minimum amount of 0.00000001 to be possible to transfer, below that the Token Bridge contract rounds to zero. To avoid a lack of funds now the UI prevents users to input amounts below that amount.

see: https://github.com/wormhole-foundation/wormhole/blob/96682bdbeb7c87bfa110eade0554b3d8cbf788d2/ethereum/contracts/bridge/Bridge.sol#L259

@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 9, 2023 14:04 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 9, 2023 14:04 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 9, 2023 14:04 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 9, 2023 14:04 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo self-assigned this Aug 9, 2023
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 9, 2023 14:14 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 9, 2023 14:16 — with GitHub Actions Inactive
raop155
raop155 approved these changes Aug 9, 2023
View reviewed changes
src/hooks/useMinimumAmountGuard.ts Outdated Show resolved Hide resolved
@sebastianscatularo sebastianscatularo force-pushed the bugs/enforce-minimum-amount-for-evm-tokens branch from 46937e6 to 1ca6879 Compare August 14, 2023 20:05
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 14, 2023 20:06 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 14, 2023 20:06 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 14, 2023 20:06 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 14, 2023 20:06 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 14, 2023 20:18 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 14, 2023 20:19 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo added the bug Something isn't working label Aug 14, 2023
@sebastianscatularo
Copy link
Contributor Author

@SamantaCasal you could test it here https://testnet.portal-bridge-ui.pages.dev/3fe5de76dc/

gabzim
gabzim reviewed Aug 14, 2023
View reviewed changes
src/components/Transfer/Source.tsx Outdated
@@ -260,6 +267,8 @@ function Source() {
value={amount}
onChange={handleAmountChange}
disabled={isTransferDisabled || shouldLockFields}
error={isBelowMinimum}
helperText={isBelowMinimum ? "Amount is below minimum" : ""}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the error message could be more helpful? “Amount sent is too small. The amount must be equal or greater than X.” Or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed an updated version with this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@SamantaCasal
Copy link

QA Ok!
Evidence:
Screenshot 2023-08-14 at 11 39 41 PM
Screenshot 2023-08-14 at 11 39 49 PM

Keep in mind, only 6 decimal places are shown in the dropdown to select tokens, then the balance is displayed 0.000000
FYI: @sebastianscatularo
Screenshot 2023-08-15 at 12 23 53 AM

@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 15, 2023 00:35 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 15, 2023 00:35 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 15, 2023 00:35 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 15, 2023 00:35 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 15, 2023 00:46 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 15, 2023 00:47 — with GitHub Actions Inactive
@SamantaCasal
Copy link

SamantaCasal commented Aug 15, 2023 •
edited
Loading

Ok!
Tested from: https://preview.portal-bridge-ui.pages.dev/3fe5de76dc/#/transfer
✅ Ok, Eth:
Screenshot 2023-08-15 at 3 54 27 PM
Screenshot 2023-08-15 at 3 54 34 PM

❗ Fail wEth in Avalanche
The wEth token has 18 decimal places https://testnet.snowtrace.io/token/0xc07c754ef7473d315D973F7D9F7858C2eCe0a0a6
But in the token it shows me the limit of 8 decimal places, however, I can continue the transaction by entering a smaller number.
Evidence:
Screenshot 2023-08-15 at 4 01 47 PM

@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 15, 2023 14:05 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 15, 2023 14:05 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 15, 2023 14:05 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 15, 2023 14:05 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Testnet August 15, 2023 14:14 — with GitHub Actions Inactive
@sebastianscatularo sebastianscatularo temporarily deployed to Cloudflare-Preview August 15, 2023 14:18 — with GitHub Actions Inactive
@SamantaCasal
Copy link

ok!
Screenshot 2023-08-15 at 4 36 25 PM

@sebastianscatularo sebastianscatularo force-pushed the bugs/enforce-minimum-amount-for-evm-tokens branch from f3ff95f to f12c1cf Compare August 15, 2023 14:47
@sebastianscatularo sebastianscatularo merged commit 1a8a016 into main Aug 15, 2023
sebastianscatularo added a commit that referenced this pull request Aug 18, 2023
@sebastianscatularo
enforce minimum amount for Evm token transfers (#293)
049538f 
* Enforce minimum amount for evm token transfers

* code review observations

* fix decimals to the max supported for evm chains if the decimals are higher

* [skip ci] - run prettier

* code review observations

* [skip ci] - cosmetic changes

* disable next button if the amount is below minimum

* [skip ci] - bump version
@sebastianscatularo sebastianscatularo deleted the bugs/enforce-minimum-amount-for-evm-tokens branch September 8, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabzim gabzim gabzim left review comments

@valentinoConti valentinoConti valentinoConti approved these changes

@yuli-ferna yuli-ferna yuli-ferna approved these changes

@raop155 raop155 raop155 approved these changes

@M-Picco M-Picco Awaiting requested review from M-Picco

Assignees

@sebastianscatularo sebastianscatularo

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sebastianscatularo @SamantaCasal @gabzim @raop155 @yuli-ferna @valentinoConti