Display readable Safe contract errors

[iamacook]

What it solves

Resolves #2804

How this PR fixes it

Errors returned from the contract are verified against the CONTRACT_ERRORS map and, if found, displayed in a snackbar notification.

A map of errors codes-messages was added and a decoding function created. If an if found, it returns [CODE]: [MESSAGE] otherwise the message. The notification object in createTransaction/processTransaction was moved to facilitate adding the returned error.

How to test it

Trigger a contract-level error and await a notification which should contain the formatted message, i.e.

1. Enable module
2. Try to enable the same module again

Screenshots

[github-actions]

CLA Assistant Lite All Contributors have signed the CLA.

[github-actions]

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	3	N/A

• Result: ✅ success
• Annotations: 0 total

---

^{Report generated by eslint-plus-action}

[coveralls]

Pull Request Test Coverage Report for Build 1467862977

• 6 of 30 (20.0%) changed or added relevant lines in 5 files are covered.
• 4 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.02%) to 31.695%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/InfiniteScroll/index.tsx	0	4	0.0%
src/logic/safe/store/actions/createTransaction.ts	0	6	0.0%
src/logic/contracts/safeContractErrors.ts	5	12	41.67%
src/logic/safe/store/actions/processTransaction.ts	0	7	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/components/InfiniteScroll/index.tsx	1	22.73%
src/logic/safe/store/actions/processTransaction.ts	1	1.15%
src/logic/safe/store/actions/createTransaction.ts	2	3.0%

Totals
Change from base Build 1462240983:	0.02%
Covered Lines:	2991
Relevant Lines:	8395

---

💛 - Coveralls

[github-actions]

• Safe Rinkeby
• Safe Mainnet
• Safe Polygon
• Safe BSC
• Safe Arbitrum
• Unified app on dev: https://safe-team.dev.gnosisdev.com/app/

[github-actions]

E2E Tests Passed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1467894233

[katspaugh]

Looks great! 👍

[francovenica]

Tried to trigger several errors by using the Contract Interaction and triggering the following tx:
Enable the same module twice
Disable a module that is not there anymore
Approve a SafeTxHash that doesn't exist
Trying to add an owner that is already there
Trying to set a policies that is not possible (3 required owners when there is only 1 owner for example)

All these only trigger only the error "Transaction Failed"

At some point I was able to trigger the GS013 error, but I wasn't even going for it, so idk what trigger it.
This one happened with I tried to send more ETH than the amount I had on the safe, also left the value SafeTxGas in 0. But I tried this scenario several times and never triggered again.

Conclusion:
I know the new errors are here because I came to one by accident, but they are really hard to trigger.

[iamacook]

Tried to trigger several errors by using the Contract Interaction and triggering the following tx: Enable the same module twice Disable a module that is not there anymore Approve a SafeTxHash that doesn't exist Trying to add an owner that is already there Trying to set a policies that is not possible (3 required owners when there is only 1 owner for example)

All these only trigger only the error "Transaction Failed"

At some point I was able to trigger the GS013 error, but I wasn't even going for it, so idk what trigger it. This one happened with I tried to send more ETH than the amount I had on the safe, also left the value SafeTxGas in 0. But I tried this scenario several times and never triggered again.

Conclusion: I know the new errors are here because I came to one by accident, but they are really hard to trigger.

Thanks for the assessment @francovenica. I'll look into this a bit deeper.

[katspaugh]

As I understand, module/owner interactions weren't affected in this PR. It's only about Send Funds.

[iamacook]

As I understand, module/owner interactions weren't affected in this PR. It's only about Send Funds.

Correct. Contract error messages are only retrieved in createTransaction and processTransaction.

[francovenica]

@iamacook
So we checked in the console and the GS errors are shown there, but they appear when the gas estimation takes place, before you submit the tx. When you actually execute the transaction the "Transaction Failed" error shows up in the snackbar

Snapshots and how to reproduce them:
New Transaction > Contract interaction
Enter as recipient the MC 1.3.0 0xd9Db270c1B5E3Bd161E8c8503c55cEABeE709552
Select ChangeThreshold method and set the value to something higher than the amount of owners that safe has
Change the recipient from the MC 1.3.0 to the safe itself
Open the dev console
Now submit to reach the review step and wait for the gas calculation

New Transaction > Contract interaction
Enter as recipient the MC 1.3.0 0xd9Db270c1B5E3Bd161E8c8503c55cEABeE709552
Select EnableModule and enter this one 0xCFbFaC74C26F8647cBDb8c5caf80BB5b32E43134
Open the dev console
Now submit to reach the review step and wait for the gas calculation
Now you should see the error in the console

New Transaction > Contract interaction
Enter as recipient the MC 1.3.0 0xd9Db270c1B5E3Bd161E8c8503c55cEABeE709552
Select EnableModule and enter this one 0xCFbFaC74C26F8647cBDb8c5caf80BB5b32E43134
Change the recipient to the current safe address you are in.
Execute the tx, it should be successful.
Start the process again (basically trying to enable the same module again)
When you reach the review step you should see the error in the console

[iamacook]

Thanks for looking into this again @francovenica. If you look at the error object, it seems as though err.arguments (green) contains the error code, whereas err.message (red) is seemingly a 'standard' message.

The current code checks whether err.message contains a GS### code and displays a snackbar with either the parsed contract error or err.message

  code ? `${code}: ${CONTRACT_ERRORS[code]}` : err.message

I'll look into what err.arguments is because this doesn't seem to be a standard error object and get back to you.

[iamacook]

I've looked into your findings some more @francovenica. We don't currently show snackbars for 'Gas estimation failed' notifications. I don't think anything needs to be changed here. What do you think? @katspaugh

I am still looking into the 'Transaction failed' errors. It seems like we have been using a depricated ABI decoder.

[iamacook]

After discussing this through with @katspaugh, it seems like the current implementation is sufficient. I've cleaned up the code a bit, as well as added a few small fixes for other bugs I found on the way:

1. Over-tracking GA. Now pathnames are logged only when they change, not repeatedly when the location object from react-router changes.
2. A memory leak on the InfiniteScroll wrappers.

This is now open for testing again @francovenica, but nothing will have visually changed when compared to your original findings.

---

For future reference: it seems as though 'Transaction failed' is occurring from within the getNotification() middleware, which is called repeatedly after a transaction has finished. I took a deep dive into the allEvents() Safe contract method, which seemed to return only an empty array. Contrarily, safeInstance.getPastEvents() did return the following (although before a transaction is indexed by us, so I believe):

[katspaugh]

Maybe we should return null in this case?
And display the original tx err.

[katspaugh]

Good catch! Please add the search too.

[francovenica]

Tried again with the same scenarios but I had different results

First I tried to execute the "ChangeTrheshold" method from the MC contract to a threshold much higher than the amount of owners.
I didn't have the errors in the console like the first time when I created the tx. Instead I had the error GS013 "Lack of gas" when the estimation was triggered just before executing the tx.
So basically I'm having the error of lack of gas instead of the "Invalid threshold which was the GS201"

I tried another tx where I was trying to enable a module, but the receiver was an invalid target.
In this case again I had the error of lack of gas again in the snackbar. It seems that error in the snack bar shows when you leave the gaslimit in 0 intentionally in the Advanced options and try to execute them (I had to add gas to execute the tx, but I did it directly in the MM pop up)

[github-actions]

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	3	N/A

• Result: ✅ success
• Annotations: 0 total

---

^{Report generated by eslint-plus-action}

[francovenica]

I tried again and I have the same results as my previous comment
#2911 (comment)

I was ok having the errors at least popping up in the console, like I showed in this comment:
#2911 (comment)
But it seems that we did nothing to change how that was working so I'm confused

Also, the Acceptance criteria of this ticket I believe, was to show in the snackbar all the GS error messages, but after all the discussions we had with Aaron and Ivan it seems that's not possible.
So in order to finish this ticket we should properly define the AC; what the user is expected to see happening when of those types of GS errors happen and not keep trying to work on this ticket anymore.
I'd involve Johannes in the discussion of this ticket so we can have an AC for it.

[katspaugh]

@francovenica a simple question, does this PR make the situation better or worse? Does it introduce new bugs?

[iamacook]

@francovenica, I am unable to find the root cause of why that specific log is no longer showing. As discussed, the contract errors that occur during the creation/processing of transactions are now displayed as we agreed in our group call. Other error messages may be present in the notifications middleware, as explained in my above comment.

I feel like this this PR is now getting out of hand. In order to show all errors, this will require a great deal more of research/trial and error. I think it should be split into two further tickets:

1. Retrieving error messages from the notification middleware.
2. Logging the error message that is seemingly not there anymore.

Please can you also give your opinions @katspaugh @johannesmoormann

[katspaugh]

I think the AC's specified in the description (see the Scope section) are met. We explicitly say that not all GS errors would be caught.

The disappearance of the log bothers me, but I think the improvements outweigh this. Merging now.

Edit: @francovenica I do get your point and appreciate your diligence. As the reporter, I accept this ticket while being aware of the things you mentioned.

[johannesmoormann]

agree with @katspaugh and his decision. if we want to spend more time on this issue, it needs to be rediscussed, as the implementation already took some considerable time compared to the effort that was originally estimated.