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: Handle safe creation errors #1591

Merged
merged 1 commit into from
Jan 24, 2023
Merged

fix: Handle safe creation errors #1591

merged 1 commit into from
Jan 24, 2023

Conversation

usame-algan
Copy link
Member

@usame-algan usame-algan commented Jan 24, 2023

What it solves

Resolves #1558

Note: I couldn't reproduce ethers returning an error but the transaction still being executed/being in the mempool.

How this PR fixes it

Uses the generic ERROR instead of TIMEOUT as a fall-through when creating a safe. This makes more sense since ethers returns a specific error code for timeouts but not for other possible errors like the one reported.

How to test it

  1. Create a new safe
  2. Select 21000 gas limit in your wallet
  3. Submit transaction
  4. Observe a generic error in the UI instead of a timeout error
  5. Create a new safe
  6. Select extremely low gas and maxPrioFee
  7. Reload the page
  8. Wait for 6.5 minutes
  9. Observe a timeout error

@github-actions
Copy link

github-actions bot commented Jan 24, 2023

Branch preview

✅ Deploy successful!

https://fix_safe_creation_error--webcore.review-web-core.5afe.dev

@github-actions
Copy link

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
Copy link
Member

Looks good to me.

With the gas set to 21000, the wallet shows a generic error:

image

Setting a low gas/maxPrioFee and refreshing also resulted in a timeout after 6.5 minutes: (I made sure that the experimental gas UI is turned on in MetaMask.)

image

Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

The fix makes sure we display only a timeout error if that's what we get from ethers. A generic error otherwise is more correct 👍

@francovenica
Copy link
Contributor

It works fine for me as well, but the timeout I had to test it in another network, since in arbitrum what ever number you set in the gas price and fee the tx will fail right away or succeed, there is no in between

@katspaugh katspaugh merged commit 7a65685 into dev Jan 24, 2023
@katspaugh katspaugh deleted the fix-safe-creation-error branch January 24, 2023 14:57
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 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.

Creating a safe on Arbitrum instantly times out
5 participants