Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

fix: reject Safe creation if reverted #3692

Merged
merged 12 commits into from
Mar 22, 2022
Merged

fix: reject Safe creation if reverted #3692

merged 12 commits into from
Mar 22, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Mar 17, 2022

What it solves

Rejected Safe creation transactions.

How this PR fixes it

The transaction receipt returned by the transaction monitor is checked for receipt.status === false to determine if it is reverted. If so, it is rejected.

The deploymentTx itself will throw if it reverts and we therefore not need check it's status.

How to test it

Attempt to create a Safe with a transaction that will revert. If not successful:

The original transaction would throw an error along the lines of "EVM reverted the transaction".
The sped-up transaction would throw an error in the console: "Sped-up tx reverted".

Screenshots

image

@iamacook iamacook self-assigned this Mar 17, 2022
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Mar 17, 2022

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

@github-actions
Copy link

Deployment links

🟠 Rinkeby Mainnet 🟣 Polygon 🟡 BSC Arbitrum 🟢 Gnosis Chain

@iamacook iamacook marked this pull request as draft March 17, 2022 12:55
@iamacook iamacook marked this pull request as ready for review March 17, 2022 13:21
@iamacook
Copy link
Member Author

Forgive the over-commits. Seems like GitHub's outage lead to some strange behaviour.

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Code looks good!
Could you post a screenshot of the creation page when a tx reverts? Thanks!

@iamacook
Copy link
Member Author

Could you post a screenshot of the creation page when a tx reverts? Thanks!

Please see the description.

Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

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

👍

@francovenica
Copy link
Contributor

I'm getting this error by setting a low value gas limit
For this one I set the default 523k gas limit to 123k in the MM popup
image

Base automatically changed from clear-failed-pending to dev March 22, 2022 06:35
@iamacook
Copy link
Member Author

I'm getting this error by setting a low value gas limit For this one I set the default 523k gas limit to 123k in the MM popup image

This is as intended. The error object is also logged for the user.

@francovenica
Copy link
Contributor

Ok, looks good to me.

@iamacook iamacook merged commit 1b1ecde into dev Mar 22, 2022
@iamacook iamacook deleted the check-creation-reversion branch March 22, 2022 13:18
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2022
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.

4 participants