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

Conversation

@iamacook
Copy link
Contributor

@iamacook iamacook commented Feb 17, 2022

What it solves

Resolves #3531

How this PR fixes it

Pending transactions that exist in the store on mount are watched for their success on the chain. If they succeed or alternatively timeout, their pending status is removed. This depends on if the transaction was (un-)successfully mined within 50 blocks since the session loaded.

How to test it

  1. Create a transaction and execute it with a low gas amount.
  2. Cancel the transaction with a high gas amount.
  3. Refresh the page and observe the pending status persisting.
  4. Within a period of 50 blocks it will either switch to successful if it was mined, or cleared with a notification.

Screenshots

image

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@iamacook iamacook marked this pull request as draft February 17, 2022 17:41
@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 2 0
Ignored 0 N/A
  • Result: ✅ success

  • Annotations: 2 total


[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods


[warning] react-hooks/exhaustive-deps

verifies the list of dependencies for Hooks like useEffect and similar


Report generated by eslint-plus-action

@github-actions
Copy link

Deployment links

🟠 Rinkeby Mainnet 🟣 Polygon 🟡 BSC Arbitrum 🟢 Gnosis Chain

@github-actions
Copy link

github-actions bot commented Feb 17, 2022

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

Failed tests:

  • ❌ Add an existing safe Add an existing safe
  • ❌ Read-only transaction creation and review Read-only transaction creation and review
  • ❌ Safe Apps List Safe Apps List

Base automatically changed from pending-txs to dev February 21, 2022 12:34
@github-actions
Copy link

github-actions bot commented Mar 1, 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 Mar 1, 2022

@iamacook iamacook marked this pull request as ready for review March 1, 2022 16:15
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.

I would refactor the watcher in the following way:

  • Make a function that iterates over pending txns, launches a monitor for them
  • The monitor itself should be a function that takes a single tx
  • Write unit tests for both functions

@iamacook iamacook marked this pull request as draft March 1, 2022 16:39
@iamacook iamacook temporarily deployed to Manual March 2, 2022 08:11 Inactive
@iamacook iamacook temporarily deployed to Manual March 2, 2022 09:43 Inactive
@iamacook iamacook temporarily deployed to Manual March 2, 2022 09:54 Inactive
@iamacook iamacook temporarily deployed to Manual March 2, 2022 10:57 Inactive
@iamacook iamacook temporarily deployed to Manual March 2, 2022 13:24 Inactive
@iamacook iamacook temporarily deployed to Manual March 2, 2022 15:47 Inactive
@iamacook iamacook temporarily deployed to Manual March 2, 2022 16:25 Inactive
@iamacook iamacook marked this pull request as ready for review March 2, 2022 16:27
@iamacook iamacook requested a review from katspaugh March 2, 2022 16:27
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 great!

Copy link
Contributor

@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.

Looks good! And easy to follow 👍

)
.encodeABI()
: txHash && safeInstance.methods.approveHash(txHash).encodeABI()
: this.txHash && safeInstance.methods.approveHash(this.txHash).encodeABI()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? I see that before we were destructuring txHash from this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

txHash wasn't updating when destructured

Copy link
Contributor

@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.

Great feature! 👍

@francovenica
Copy link
Contributor

Looks goods

So I tried a couple of times to create a tx, setting the gas price in low and then cancelling it right away, refresh the page and see that the tx is shown in pending until it "realizes" after a few minutes that the tx was not mined.

@iamacook iamacook merged commit 0ff3119 into dev Mar 9, 2022
@iamacook iamacook deleted the pending-watcher branch March 9, 2022 16:36
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 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.

Pending statuses persist for cancelled transactions loaded from sessionStorage

7 participants