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

fix: remove redundant PENDING_FAILED status #3360

Merged
merged 18 commits into from
Jan 26, 2022
Merged

fix: remove redundant PENDING_FAILED status #3360

merged 18 commits into from
Jan 26, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Jan 24, 2022

What it solves

Resolves #3352

How this PR fixes it

The PENDING_FAILED status was seemingly added as a placeholder and was causing the issue that it was set onError, irregardless of whether the transaction was a signature or execution. It has now been removed and the 'local statuses' converted to a 'pending transactions' array that stores and array of safeTxHashes instead.

The coniditons of performing an off-chain signature now first take into account whether off-chain signing is possible: !isFinalization && canSignOffChain.

The choice to rename isExecution to isFinalization is included here as it is more declarative.

How to test it

The PENDING status should be shown when a signature is requested/the transaction is executed. The button text should also represent whether the transaction needs to be confirmed or executed.

The PENDING statuses should be broadcast across windows/tabs. If you open two tabs/windows of the queue list and execute the transaction in one, it should show the updated status in the other.

This logic affects a lot of cases across all transactions. This has been tested locally across all of the following, but it is best to check them again:

On a 1/? Safe:

  • Rejecting the creation of a transaction.
  • Creating a transaction and executing it immediately.
  • Creating a transaction and executing it, rejecting the request.
  • Queueing a transaction and approving it by an owner on a 1/1 Safe.
    • Confirm and execute it by an owner.
    • Confirm and execute it by a third owner.
    • Confirm and execute it by a non-owner.

On a 2/? Safe:

  • Create a transaction on a 2/1 Safe.
    • Confirm and execute it by an owner.
    • Confirm and execute it by a third owner.
    • Confirm and execute it by a non-owner.

Creating and executing rejection transactions on the above should also be tested.

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

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.

@github-actions
Copy link

github-actions bot commented Jan 24, 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 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@coveralls
Copy link

coveralls commented Jan 24, 2022

@github-actions
Copy link

Deployment links

🟠 Rinkeby Mainnet 🟣 Polygon 🟡 BSC Arbitrum 🟢 Gnosis Chain

@github-actions
Copy link

github-actions bot commented Jan 24, 2022

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

Failed tests:

  • ❌ Address book Address book
  • ❌ Safe Apps List Safe Apps List
  • ❌ Read-only transaction creation and review Read-only transaction creation and review
  • ❌ Safe Balances Safe Balances

@JagoFigueroa
Copy link

JagoFigueroa commented Jan 24, 2022

Hola caballeros! Unfortunately I've been able to reproduce the issue on a safe with 2 owners by doing what Franco shows in the gif:

  1. Drain account app, perform a drain of a safe with 2 owners (you can drain them to yourself for testing).
  2. Submit and sign tx with the first one.
  3. With the second owner, confirm + execute but reject the tx on metamask.
    Tx is shown as:

Screenshot 2022-01-24 at 14 10 54

Edit: Tx is shown as send tx because I only have this asset in the safe :)

Cheers!

@iamacook
Copy link
Member Author

@JagoFigueroa, this issue should now be address. I am putting this into review now but would appreciate your insight on it again once it has passed.

@JagoFigueroa
Copy link

Good and bad news.

The tx from the screenshot that I posted above that was prompted to be executed was changed correctly to prompting the user to confirm it instead :)

but repeating the process of rejecting this or any other tx on metmask is making txs get stuck with pending status:
Screenshot 2022-01-24 at 17 47 41

@JagoFigueroa
Copy link

Working like a charm now after the fix, thank you @iamacook

I have still a few scenarios left to check tomorrow but it is looking good ;)

@JagoFigueroa
Copy link

Buenos días! Testing now on a safe with a 1 / 3 threshold.

  1. I create a tx but don't execute it.
  2. I go to Queue screen and see that I am prompted to execute the tx (all bueno as it has the required signatures)
    Screenshot 2022-01-25 at 08 53 25
  3. I execute the tx but I reject it on Metamask
  4. Transaction changes as if it needs confirmation
    Screenshot 2022-01-25 at 08 53 50

@iamacook iamacook changed the title fix: only sign off-chain if possible fix: remove redundant PENDING_FAILED status Jan 25, 2022
@iamacook iamacook marked this pull request as draft January 25, 2022 12:15
@iamacook iamacook marked this pull request as ready for review January 25, 2022 12:36
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.

Key comment: use a set or a hash map for pending txns. Easier to lookup and delete items.

@iamacook
Copy link
Member Author

I am aware of the merge conflict. I'll hold off on merging dev here until after review to avoid having lots of file changes polluting the PR.

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.

Looketh Goode!

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.

✅ well done!

@francovenica
Copy link
Contributor

Jago gave it the thumbs up and I also check it for a bit too check that nothing obvious pops up.

Looks good to me as well.

@iamacook iamacook merged commit 2a81c9b into dev Jan 26, 2022
@iamacook iamacook deleted the confirm-button branch January 26, 2022 17:00
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 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.

Confirm button changes to execute if tx is rejected in MM
6 participants