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

tx: Normalize toJson for different tx types #2707

Merged
merged 2 commits into from
May 17, 2023
Merged

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented May 17, 2023

While sharing a block via json with @acolytec3 it was noticed that jsons didnt have type info in many instances, althrough one could interpret the json type via other fields, but a more verbose type can certainly help in easy identification.

This PR aims to add normalized base transaction json fields in every toJson as well as removes some serialization redundancy of these common fields

Also fixes versionedHashes serialization which didn't have prefixed 0x

@g11tech g11tech added PR state: merge ready package: tx target: master Work to be done towards master branch labels May 17, 2023
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #2707 (1f09b06) into master (ffec3ed) will increase coverage by 1.39%.
The diff coverage is 68.00%.

❗ Current head 1f09b06 differs from pull request most recent head c22a5cc. Consider uploading reports for the commit c22a5cc to get more accurate results

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 90.30% <ø> (-0.41%) ⬇️
blockchain 90.72% <ø> (ø)
client 86.96% <ø> (-0.01%) ⬇️
common 97.05% <ø> (ø)
devp2p 89.11% <ø> (-0.29%) ⬇️
ethash ∅ <ø> (∅)
evm ?
rlp ∅ <ø> (∅)
statemanager 80.92% <ø> (ø)
trie 89.94% <ø> (ø)
tx 96.01% <68.00%> (+0.51%) ⬆️
util 81.34% <ø> (ø)
vm 81.36% <ø> (ø)

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

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

LGTM! Also interesting that we never reported type on the JSON 🤔 Great addition.

@jochem-brouwer jochem-brouwer merged commit 3498ce9 into master May 17, 2023
@holgerd77 holgerd77 deleted the tx-tojson branch May 22, 2023 10:33
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.

3 participants