-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ethapi): make eth_getTransactionReceipt
null for unexecuted/unknown transactions
#1006
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test you can augment with these scenarios?
} else { | ||
Ok(None) | ||
error(ExitCode::USR_ILLEGAL_ARGUMENT, "incompatible transaction") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can this happen? When the transaction we requested was a system transaction? Eg top-down finality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do those not have a receipt we can return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds reasonable for an app to find this tx in an explorer or via the block API, then request its receipt. Not returning it breaks assumptions, but returning an error will cause breakage, I'm sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other kind of message is IPC mesage, which I believe the receipts are not handled like signed txns, we might not get a proper txn receipt. Or we can just return None
regardless of whatever error and just log these error using metrics
fendermint/eth/api/src/apis/eth.rs
Outdated
) | ||
.await | ||
.context("failed to convert to receipt")?; | ||
let Some(res) = data.tx_by_hash(tx_hash).await? else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this var tx?
eth_getTransactionReceipt
returns null for unexecuted transactionseth_getTransactionReceipt
null for unexecuted/unknown transactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cryptoAtwill We don't seem to have structured integration tests for the Eth API, so covering this in a test case is a non-starter for now. Did you successfully test this manually?
Pending test results from @cryptoAtwill. |
I have tested this using:
The result I got: {"jsonrpc":"2.0","result":null,"id":1}. I think it will only be triggered under certain race condition, this PR should not have caused extra errors. |
This one addresses #1003.
From the issue above, it seems the txn hash is found but the block information is not found. This could be the latest block is not yet confirmed. This PR checks if block header is found when txn hash is present, it will return None. Else if the block header is found but block data is not found, it will throw an error.