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: Safe creation/batch execution gas estimation #2232

Merged
merged 3 commits into from
Jul 10, 2023
Merged

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Jul 5, 2023

What it solves

Resolves #2221

How this PR fixes it

We were not estimating the gas for Safe creation/batch execution transactions. This adds the relevant gas parameters to each transaction accordingly.

How to test it

  1. Connect a Ledger to Polygon
  2. Observe the Safe creation flow estimating the network fee in the UI correctly, as well as successfully creating a Safe.
  3. Observe batch execution working as expected.

Note: this should not affect other wallets and, as such, they need also be tested accordingly.

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Branch preview

✅ Deploy successful!

https://underpriced_ledger--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

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 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@iamacook iamacook marked this pull request as draft July 5, 2023 09:55
@iamacook iamacook changed the title fix: underpriced transactions on Ledger devices fix: Safe creation/batch execution gas estimation Jul 5, 2023
@iamacook
Copy link
Member Author

iamacook commented Jul 5, 2023

After discussion, all wallets now receive gas estimation for Safe creation/batch execute transactions (as we are already doing for all other transactions).

@iamacook iamacook marked this pull request as ready for review July 5, 2023 14:11
@iamacook iamacook requested a review from schmanu July 5, 2023 14:11
Copy link
Member

@schmanu schmanu left a comment

Choose a reason for hiding this comment

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

Looking good for now. Just a minor remark in one of the unit tests.

As we discussed earlier we should at some point in the future refactor the useGasPrice hook to return the correct tx option overrides depending on if EIP-1559 is enabled or not, such that we do not need to handle that case anywhere outside that hook.

@francovenica
Copy link
Contributor

It works for ETH just fine.

For Poly I still have issues.
I noticed now that the fee is no longer a default "0.001", now it actually shows an number, but I still get an error when I try to start the creation. This error happens as soon as I click "submit", I didn't even get to use the device ( so maybe the issue is not about ledger itself, Let me know if I should create another ticket)

image

@iamacook
Copy link
Member Author

For Poly I still have issues. I noticed now that the fee is no longer a default "0.001", now it actually shows an number, but I still get an error when I try to start the creation. This error happens as soon as I click "submit", I didn't even get to use the device ( so maybe the issue is not about ledger itself, Let me know if I should create another ticket)

All seems to work with my Nano X: (the indexing took a long time so I cut the recording short but the Safe was created)

ledger

Are you using the latest firmware?

@francovenica
Copy link
Contributor

So I keep having the issue, but after using the Retry button a couple of times it went through, so I believe this has nothing to do with ledger and safe creation.
I'll check again during the next RC regression

@iamacook iamacook merged commit 9aa0bb7 into dev Jul 10, 2023
7 checks passed
@iamacook iamacook deleted the underpriced-ledger branch July 10, 2023 16:25
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't create safes with ledger
4 participants