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

client: add missing tx fields to getBlockByHash #1881

Merged
merged 5 commits into from
May 16, 2022
Merged

Conversation

acolytec3
Copy link
Contributor

Fixes #1880 by adding additional fields to transaction bodies as specified by the JSON-RPC spec

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #1881 (445ad15) into master (8e1c3cf) will increase coverage by 0.00%.
The diff coverage is 70.00%.

Impacted file tree graph

Flag Coverage Δ
block 85.57% <ø> (ø)
blockchain 83.82% <ø> (ø)
client 76.42% <70.00%> (+<0.01%) ⬆️
common 94.19% <ø> (ø)
devp2p 82.34% <ø> (ø)
ethash 90.76% <ø> (ø)
trie 80.02% <ø> (ø)
tx 88.20% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member

Tests are failing probably because the JSON fields now are extended. Could you also add a simple test that verifies that these parameters are available when this method is called?

@jochem-brouwer

This comment was marked as off-topic.

@acolytec3
Copy link
Contributor Author

Tests are failing probably because the JSON fields now are extended. Could you also add a simple test that verifies that these parameters are available when this method is called?

Done and done :-)

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

LGTM!

@holgerd77
Copy link
Member

I compared the files touched here with the ones on the optimistic PR #1878, and this should be mergeable without causing too much hazzle, will do.

@holgerd77 holgerd77 merged commit 4217ed6 into master May 16, 2022
@holgerd77 holgerd77 deleted the missing-tx-fields branch May 16, 2022 08:57
g11tech pushed a commit that referenced this pull request May 24, 2022
* client: add missing tx fields to getBlockByHash

* Add test for "includeTransactions"

* Fix test

* numbers to hex

* DRY

Co-authored-by: Ryan Ghods <ryan@ryanio.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transactions in eth_getBlockByHash lack several fields (hash, etc)
4 participants