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/common: remove yParity from tx json interface #3501

Closed
wants to merge 2 commits into from

Conversation

jochem-brouwer
Copy link
Member

This removes the alias yParity from the tx library, I introduced it for 7702 (thinking that we also used this on other txs - but we don't). This removes the alias, since there was (before this PR) either the value v in non-7702 txs, or yParity on 7702 txs.

Note: the tests fixed in #3498 still pass (just checked).

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.38%. Comparing base (ef20930) to head (5dd80f3).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 72.85% <ø> (?)
blockchain 84.84% <ø> (?)
common 94.10% <ø> (ø)
devp2p 0.00% <ø> (ø)
evm 68.91% <ø> (?)
genesis 0.00% <ø> (?)
statemanager 64.52% <ø> (ø)
trie 53.04% <ø> (-0.04%) ⬇️
tx 78.40% <ø> (?)
util 69.68% <ø> (ø)
vm 58.63% <ø> (?)

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

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

ScottyPoi
ScottyPoi previously approved these changes Jul 12, 2024
Copy link
Contributor

@ScottyPoi ScottyPoi left a comment

Choose a reason for hiding this comment

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

Tests fixed in #3495 now failing

@ScottyPoi ScottyPoi dismissed their stale review July 12, 2024 20:42

Actually tests are failing

@jochem-brouwer jochem-brouwer marked this pull request as draft July 12, 2024 20:46
@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Jul 12, 2024

Have marked to draft, did not read the spec too well, it seems for txs in RPC we should indeed have yParity. For 4844, 1559, 2930, (7702?) this field is mandatory, but these txs can optionally have a v field. Note: for Legacy txs the v field is mandatory and yParity does not seem to be an option. Source: https://ethereum.github.io/execution-apis/api-documentation/ (check eth_getTransactionByHash)

@jochem-brouwer
Copy link
Member Author

I will close this PR, since this actually degrades matching the spec instead of increasing it, thanks for catching this @ScottyPoi 😄 👍

@holgerd77 holgerd77 deleted the cleanup-tv-yparity-v-alias branch July 15, 2024 08:07
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