Fix: show non-owner execution warning only on tx creation

[katspaugh]

What it solves

Non-owner error message should never appear when executing an existing tx.
It can only be shown if a non-owner is trying to create a tx (currently only possible via a Sage App).

How to test

Case 1:

• Make a tx that would inevitably fail
• Try to execute it as a non-owner
• It should say: "tx will fail"

Case 2:

• As a non-owner, try to create a tx from Drain Safe
• It should say: "not an owner"

[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	0	N/A

• Result: ✅ success
• Annotations: 0 total

---

^{Report generated by eslint-plus-action}

[coveralls]

Pull Request Test Coverage Report for Build 1818188925

• 3 of 7 (42.86%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.04%) to 33.675%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/TransactionFailText/index.tsx	3	7	42.86%

Totals
Change from base Build 1818042425:	0.04%
Covered Lines:	3233
Relevant Lines:	8574

---

💛 - Coveralls

[github-actions]

Deployment links

🟠 Rinkeby	⚪ Mainnet	🟣 Polygon	🟡 BSC	⚫ Arbitrum	🟢 Gnosis Chain

[iamacook]

This is needed. It was put back on the request of Safe Apps.

[github-actions]

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

Failed tests:

• ❌ Read-only transaction creation and review Read-only transaction creation and review
• ❌ Safe Balances Safe Balances

[katspaugh]

You mean we should keep the warning but make it say "you're not connected to the safe"?

AFAIU, any tx can be executed by a non-owner. But that selector also checks if you're connected at all.

[iamacook]

Perhaps we should also make it Safe App conditional.

[katspaugh]

I'm not sure I understand why it should be different for Safe Apps.

[iamacook]

@JagoFigueroa, can we remove this? I believe it was you that requested it be put back?

[JagoFigueroa]

Hey guys, if my memory doesn’t fail me I reported this because we went from showing this message to not showing any kind of message at all when a non owner was trying to create + execute (or just create) any tx via safe apps.

Going back to that scenario of not providing any feedback why the tx can’t be submitted would not be super critical for us or anything but I don’t see why we should lose this.

Cheers!

[katspaugh]

Going back to that scenario of not providing any feedback why the tx can’t be submitted would not be super critical for us or anything but I don’t see why we should lose this.

Please clarify what exactly we're losing.
I'm removing "You are currently not an owner of this Safe and won't be able to submit this transaction." – this message is simply not relevant anymore as anyone can execute a transaction.

Instead, This transaction will most likely fail. ${errorDesc} will be displayed. Which actually does contain the reason of the failure.

[JagoFigueroa]

This is what we show if a non owner goes to the apps section and tries to perform any operation for a safe via safe apps:

This is a message that makes sense to have as it provides proper feedback and because I am trying to create a tx (or maybe create + execute depending on the threshold) as a non owner and I shouldn't be able to.

For executions that will be done after the tx has already been created? should be all bueno for us but the problem would be to lose this message when tx creation is involved.

[katspaugh]

Oh, gotcha, makes thanks. Thanks Jago!

[katspaugh]

I'll adjust the logic so that this message is shown only on tx creation.

[iamacook]

👍

[francovenica]

I'm trying to test this but I'm getting an error that I'm not sure if it is related to this PR or not.

Here I'm trying to create a tx in the tx builder with a non-owner.
try this contract 0x49d4450977E2c95362C13D3a31a09311E0Ea26A6 (is a dummy contract with test methods, only for rinkeby)
Set the "Test address" method and paste the address of the contract
Add it and try to create the tx.

Trying to do a regular "send funds" also throws the error that the tx will fail, which this one in particular should not (all the values in the adv options are fine and the safe has funds so the tx has no reason to fail)

I only see this error in this PR. I tried both tx types in dev and there is no error there.

[katspaugh]

@francovenica I've just updated it from latest dev, should be good now.

[liliya-soroka]

This is what we show if a non owner goes to the apps section and tries to perform any operation for a safe via safe apps:

This is a message that makes sense to have as it provides proper feedback and because I am trying to create a tx (or maybe create + execute depending on the threshold) as a non owner and I shouldn't be able to.

For executions that will be done after the tx has already been created? should be all bueno for us but the problem would be to lose this message when tx creation is involved.

@katspaugh , no message on tx creation using safe-app as a non-owner

[katspaugh]

On dev, it's also not shown. Apparently a bug in the modal redesign.
I'll fix it here.

Update: I've just checked on prod, this error message isn't displayed there either. It just blocks the submit button.

[katspaugh]

Fixed ✅

[francovenica]

Looks good to me

when creating a tx
Tx created in the tx builder by non-owner show the error message of "non-owner" and disable the submit button
Tx created in the tx builder by non-owners which are expected to fail also show the "non-owner" message and cannot be submitted
Tx created in the tx builder by owners which are expected to fail show the "tx likely to fail" error message, this can still be submitted
Tx created in the tx builder by owners which are correct show no error message

when executing
Tx ready to be executed and expected to fail show the error of "tx likely to fail" for both owners and non-owners
Tx ready to be executed and expected to succeed show no error message at all.