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: remove transactionLogIndex from logs #2583

Merged
merged 2 commits into from
May 5, 2023
Merged

Conversation

gakonst
Copy link
Member

@gakonst gakonst commented May 5, 2023

Detected by @sslivkoff in #2457

The transactionLogIndex is not in ETH JSON RPC Spec, so clients that conform to that do not have it. However, I've seen transactionLogIndex in Parity/OpenEthereum.
Note that transactionLogIndex is not as same as logIndex (which is kind of blockLogIndex).

ethers-io/ethers.js#1721 (comment)

Fixes #2569

@codecov-commenter
Copy link

Codecov Report

Merging #2583 (b3599ba) into main (ded0896) will increase coverage by 0.01%.
The diff coverage is 50.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    #2583      +/-   ##
==========================================
+ Coverage   71.41%   71.43%   +0.01%     
==========================================
  Files         493      493              
  Lines       61977    61971       -6     
==========================================
+ Hits        44260    44266       +6     
+ Misses      17717    17705      -12     
Flag Coverage Δ
integration-tests 18.05% <0.00%> (-0.01%) ⬇️
unit-tests 66.22% <50.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
crates/rpc/rpc/src/eth/api/transactions.rs 26.74% <ø> (+0.05%) ⬆️
crates/rpc/rpc/src/eth/logs_utils.rs 48.21% <0.00%> (+0.42%) ⬆️
crates/rpc/rpc-types/src/eth/log.rs 62.85% <100.00%> (+0.69%) ⬆️

... and 9 files with indirect coverage changes

@gakonst gakonst merged commit 63d2d70 into main May 5, 2023
@gakonst gakonst deleted the fix/remove-tx-log-index branch May 5, 2023 19:26
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.

Consider removing transactionLogIndex from rpc
2 participants