fix: don't mark tx as REVERTED when in block, check tx receipt status instead. #82
Conversation
| @@ -83,11 +83,21 @@ export async function getTransaction( | |||
|
|
|||
| const receipt = await eth.getTransactionReceipt(hash) | |||
|
|
|||
| if (receipt == null) { | |||
There was a problem hiding this comment.
I'm not sure about this, sometimes the tx can be dropped or reorg leaving it outside the main-chain. If I'm not wrong, the receipt will be null too leaving it forever loading.
Maybe if receipt == null we can check again what we do at line 53.
What was the case that motivates the PR?
There was a problem hiding this comment.
If it would get dropped, then wouldn't it be checked later on and moved from "pending" to "dropped" status?
Motivation for this PR: I was sending transactions in my app and listening to later actions FETCH_TRANSACTION_SUCCESS and FETCH_TRANSACTION_FAILURE, to update/refresh my app status accordingly. It became a problem though when I was
wrongly having FETCH_TRANSACTION_FAILURE as if they were reverted, when they actually weren't.
There was a problem hiding this comment.
Cool! do you have examples?
Dropped & replaced yes, will happen what you said. But it was reorg and not included in the chain the tx will be a ghost one like it never happened.
There was a problem hiding this comment.
I was just creating a transaction, and listening to updates by using selectors to grab pending/confirmed transactions from the redux state.
Not sure how to handle the reorg case. In this scenario, what would be the expected result? Following the original code logic + your logic, I think you imply that it should be marked as Reverted? However maybe that is a different issue, in order to be sure if the tx was actually placed in the main-chain (ie. "confirmed ~100%"?) , there should be some check of the # of confirmations, and act on the tx result after asserting that enough confirmations have been achieved.
The current change on this PR, just keeps the tx status as "Pending" (instead of wrongly "Reverted"), until more information is available to say otherwise. Should this change also include logic about the block hash and # confirmations? This SO answer proposes a strategy for achieving this
There was a problem hiding this comment.
Should this change also include logic about the block hash and # confirmations? This SO answer proposes a strategy for achieving this.
This is exactly what I was trying to explain.
There was a problem hiding this comment.
@tmilar is this still in your backlog?
There was a problem hiding this comment.
Hi @nachomazzara , I'm sorry but I'm not actively working on this project anymore.
Though if it still matters to you let me know, I can make an effort to complete the PR when I have some free time.
There was a problem hiding this comment.
It would be great if you can complete it :)
Fixes #81.