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

fix: transaction remain pending #3525

Merged
merged 5 commits into from
Feb 21, 2022
Merged

fix: transaction remain pending #3525

merged 5 commits into from
Feb 21, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Feb 16, 2022

What it solves

Resolves #3440

How this PR fixes it

Transactions now return the transactionHash from the receipt. After 50 blocks have been mined without successfuly execution, it throws triggering the onError where the pending status is removed.

onComplete (for on-chain) transactions now occurs upon receiving the transaction hash as we save the txHash for the pending transaction. (Immediately executing transactions have no txId and need to be proposed in order to save { [txId]: txHash } in the pending store).

How to test it

1 Create a transaction (with low gas) and then cancel it via a (high gas) cancellation in MM.
2 After 15-30 mins, observe a notification about unsuccessful pend and a console error about the transaction not being mined within 50 blocks.

Screenshots

image

image

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Feb 16, 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

@coveralls
Copy link

coveralls commented Feb 16, 2022

Pull Request Test Coverage Report for Build 1875604863

  • 4 of 16 (25.0%) changed or added relevant lines in 4 files are covered.
  • 55 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 33.613%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/logic/safe/store/reducer/pendingTransactions.ts 1 2 50.0%
src/logic/safe/store/actions/createTransaction.ts 0 11 0.0%
Files with Coverage Reduction New Missed Lines %
src/routes/safe/components/Apps/components/AppFrame.tsx 55 0%
Totals Coverage Status
Change from base Build 1859711200: 0.0%
Covered Lines: 3238
Relevant Lines: 8624

💛 - Coveralls

@github-actions
Copy link

Deployment links

🟠 Rinkeby Mainnet 🟣 Polygon 🟡 BSC Arbitrum 🟢 Gnosis Chain

Comment on lines 204 to 206
.on('error', (err) => {
throw err
})
Copy link
Member

Choose a reason for hiding this comment

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

Then you probably don't need this at all.
Just replacing .once('transactionHash') with .then(({ transactionHash }) => transactionHash) will already make it wait for the final error (or success).

Copy link
Member Author

Choose a reason for hiding this comment

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

We're still investigating this as it takes 50 blocks before an error is emitted, so it's really time-consuming to debug. We're getting there though.

Copy link
Member

Choose a reason for hiding this comment

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

Should be faster on an L2 like Aurora (and free, too).

@github-actions
Copy link

github-actions bot commented Feb 16, 2022

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

@iamacook
Copy link
Member Author

We are going to split this into a second ticket as we have unveiled a deeper issue that is explained here.

@iamacook iamacook marked this pull request as ready for review February 17, 2022 10:54
@iamacook iamacook marked this pull request as draft February 17, 2022 11:15
@iamacook iamacook marked this pull request as ready for review February 17, 2022 14:18
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.

So the fix, as I understood, is that it now correctly waits for the PromiEvent to finish, so that potential post-hash errors aren't ignored like before?

And it's still fast because you call onComplete as soon as the hash is there?

@iamacook
Copy link
Member Author

So the fix, as I understood, is that it now correctly waits for the PromiEvent to finish, so that potential post-hash errors aren't ignored like before?

And it's still fast because you call onComplete as soon as the hash is there?

Exactly

@iamacook iamacook mentioned this pull request Feb 17, 2022
22 tasks
@francovenica
Copy link
Contributor

Noted the comment about the pending tx remaining there forever. I'll check that in that other ticket created for it

Ok, so I tried this twice.
The first time I created a random tx with low gas and as soon as the tx was put in queue in pending status I rejected it in the MM extension. After 13 mins the notification of "Transaction still pending" showed up, but the error didn't mentioned nothing about 50 blocks. After 30 mins I just refreshed the page and reloaded the Queue tab until the Pending tx disappeared
image

The 2nd time I waited for a bit before submitting the cancelation (I wanted the tx to be indexed by the block explorer first, but it never was). I cancelled the tx in the MM extension and waited for over 30 mins and no error notification popped up at all.
image

Both rejections, after more than 1 hour still show as "pending of indexation" in the block explorer although they are shown as executed in the MM extension, idk what is up with that
The cancelations in order:
https://rinkeby.etherscan.io/tx/0x9f8361f4221340cf017239faac25ce83fabaa11773f4b6166976b51275038ccd
https://rinkeby.etherscan.io/tx/0xc2379d047e4094710a54e0995741ffdf80a24c14737ad282a97663c9ca2afba1

I'll try one more time tomorrow to see if I get the 50 blocks error message in the console

@iamacook
Copy link
Member Author

The 50 block error may only show in the developer environment. I'm not sure. The notification is shown when the error would appear in the console - it's triggered by the it.

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.

LGTM

@francovenica
Copy link
Contributor

Passing this one given the comment after my report. Also since the ticket #3531 was created to address the issue with the pending status I'll recheck everything when that ticket is ready

@iamacook iamacook merged commit cbb993c into dev Feb 21, 2022
@iamacook iamacook deleted the pending-txs branch February 21, 2022 12:34
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 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.

Reverted transaction remained pending
5 participants