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(suite): use estimated gas limit for EVM token transfers #11897

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

tomasklim
Copy link
Member

Description

No idea why we had hardcoded value 200_000

I am also suprised that there was no token which required more than 200_000

@tomasklim tomasklim requested a review from matejkriz as a code owner April 4, 2024 15:16
@tomasklim tomasklim requested review from AdamSchinzel and removed request for matejkriz April 4, 2024 15:17
Copy link
Contributor

@AdamSchinzel AdamSchinzel left a comment

Choose a reason for hiding this comment

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

LGTM

@tomasklim tomasklim merged commit 26df2f2 into develop Apr 4, 2024
22 of 23 checks passed
@tomasklim tomasklim deleted the fix/erc20-gas-limit branch April 4, 2024 15:36
@tomasklim
Copy link
Member Author

@trezor/qa please try to send some tokens on MATIC and ETH network 🙏

@tomasklim tomasklim mentioned this pull request Apr 4, 2024
@tomasklim tomasklim added this to the Suite Trends H1/2024 milestone Apr 4, 2024
@bosomt
Copy link
Contributor

bosomt commented Apr 5, 2024

QA NOK

ETH

  • 13.1938494 USDP sent from Ethereum 1
  • Gas limit 66863

POLY

tested two tokens with default Gas limit:21000, both failed:

  • Transaction signing error: intrinsic gas too low: needed 21644, allowed 21000
  • Transaction signing error: intrinsic gas too low: needed 21584, allowed 21000

Info:

  • Suite version: desktop 24.4.0 (5231fe0)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/24.4.0 Chrome/118.0.5993.159 Electron/27.3.8 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T2B1 2.7.0 regular (revision 45e8a842a31e62a6d43d7f6ccac62a45e1198ef0)
  • Transport: BridgeTransport 2.0.33

@tomasklim
Copy link
Member Author

@bosomt does sending data on polygon work? 🤯

@bosomt
Copy link
Contributor

bosomt commented Apr 5, 2024

@tomasklim
Copy link
Member Author

tomasklim commented Apr 5, 2024

@bosomt can you test again the tokens? I dont have that issue 😀 maybe record your steps

@tomasklim
Copy link
Member Author

@bosomt

@bosomt
Copy link
Contributor

bosomt commented Apr 9, 2024

Tried to send two random tokens that we got from airdrop.
Gas is always Gas limit:21000 and it always fails

https://youtu.be/sKniXEEp3bU

Info:

  • Suite version: web 24.4.0 (a880750)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:124.0) Gecko/20100101 Firefox/124.0
  • OS: MacIntel
  • Screen: 1663x1080
  • Device: Trezor T2B1 2.7.0 regular (revision 45e8a842a31e62a6d43d7f6ccac62a45e1198ef0)
  • Transport: BridgeTransport 2.0.33

@bosomt
Copy link
Contributor

bosomt commented Apr 9, 2024

@tomasklim and sorry for late reply

@tomasklim
Copy link
Member Author

Okay, you are right, it works for some tokens and for some it does not. Especially it does not for some unrecognised. It needs an investigation. The problem is definitely not in Suite, maybe the contract of tokens is somehow modified for these scams.

Applies to both ETH and MATIC

Screenshot 2024-04-09 at 9 24 52
Screenshot 2024-04-09 at 9 24 44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants