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

feat: add missing Tx receipt info #1931

Merged
merged 12 commits into from
Mar 25, 2023

Conversation

chirag-bgh
Copy link
Contributor

@chirag-bgh chirag-bgh commented Mar 23, 2023

Closes #1922

TODO:

  • Gas used
  • state_root

Remaining log fields :

  • log_index
  • transaction_log_index
  • removed

@mattsse mattsse added the A-rpc Related to the RPC implementation label Mar 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023

Codecov Report

Merging #1931 (86a8710) into main (aaa99f6) will increase coverage by 0.14%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1931      +/-   ##
==========================================
+ Coverage   72.93%   73.07%   +0.14%     
==========================================
  Files         417      425       +8     
  Lines       51376    52131     +755     
==========================================
+ Hits        37471    38096     +625     
- Misses      13905    14035     +130     
Flag Coverage Δ
integration-tests 19.47% <0.00%> (-0.13%) ⬇️
unit-tests 67.32% <0.00%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
crates/rpc/rpc-types/src/eth/index.rs 69.01% <0.00%> (-3.05%) ⬇️
crates/rpc/rpc/src/eth/api/transactions.rs 22.45% <0.00%> (-0.63%) ⬇️
crates/rpc/rpc/src/eth/error.rs 9.17% <ø> (ø)

... and 61 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

good progress, left some suggestions and pointers.

crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

a few additional suggestions, but this is pretty close.

crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
@chirag-bgh
Copy link
Contributor Author

Option 3 (update 2017.07.28: we are going with this one): For blocks where block.number >= METROPOLIS_FORK_BLKNUM, the intermediate state root parameter in the receipt should be set to a \x01 byte if the outermost code execution succeeded, or a zero byte if the outermost code execution failed.

ref EIP-98

I think status root has been replaced with status code (ref EIP-658) which we already have in TransactionReceipt

@chirag-bgh chirag-bgh marked this pull request as ready for review March 25, 2023 06:58
@chirag-bgh chirag-bgh requested a review from Rjected as a code owner March 25, 2023 06:58
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

getting there.

left more suggestions and requests

crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/transactions.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice work!

@mattsse mattsse merged commit 9fc3e6f into paradigmxyz:main Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fill missing Tx receipt info
3 participants