-
Notifications
You must be signed in to change notification settings - Fork 473
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
Conversation
Branch preview✅ Deploy successful! https://underpriced_ledger--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
src/components/tx/modals/BatchExecuteModal/ReviewBatchExecute.tsx
Outdated
Show resolved
Hide resolved
src/components/new-safe/create/steps/StatusStep/useSafeCreation.ts
Outdated
Show resolved
Hide resolved
After discussion, all wallets now receive gas estimation for Safe creation/batch execute transactions (as we are already doing for all other transactions). |
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.
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.
src/components/new-safe/create/steps/StatusStep/__tests__/useSafeCreation.test.ts
Outdated
Show resolved
Hide resolved
It works for ETH just fine. For Poly I still have issues. |
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) Are you using the latest firmware? |
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. |
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
Note: this should not affect other wallets and, as such, they need also be tested accordingly.
Checklist