Skip to content
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 for getTransactionReceipt returning empty 'from' address. #242

Merged
merged 3 commits into from
Jun 12, 2023

Conversation

telackey
Copy link
Contributor

@telackey telackey commented Jun 9, 2023

Fixes a bug where the wrong signer is used for getTransactionReceipt, leading to a null 'from' value, and unifies the code used to obtain a Signer for a tx.

@telackey telackey requested a review from i-norden June 9, 2023 23:52
@telackey telackey self-assigned this Jun 9, 2023
@telackey telackey marked this pull request as ready for review June 10, 2023 03:59
@roysc
Copy link
Collaborator

roysc commented Jun 10, 2023

So this was only triggered on EIP-2718 (post Berlin?) transactions? This reminds me, the API unit tests don't cover them, I wanted to make an issue for that

@i-norden
Copy link
Collaborator

@roysc yeah also the integration tests comparing geth results to ipld-eth-server results compare responses from getTransactionReceipt after the JSON returned by theses endpoints has been unpacked into a *types.Receipt, which does not have a "from" field. We should be comparing the full JSON responses.

Copy link
Collaborator

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Will create an issue for improved test coverage.

@i-norden i-norden mentioned this pull request Jun 12, 2023
@telackey telackey merged commit 77f861d into v5 Jun 12, 2023
5 checks passed
@telackey telackey deleted the telackey/receiptfix branch June 12, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants