Skip to content

Conversation

akaladarshi
Copy link
Collaborator

@akaladarshi akaladarshi commented Oct 8, 2025

Summary of changes

Changes introduced in this pull request:

  • Make the EthFilterSpec address as optional field so the EthGetLogs API can accept address as None.
  • Update the ethgetLogs snapshot

Reference issue to close (if applicable)

Closes #6134

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • New Features

    • eth_getLogs and newFilter now accept an optional address field; omitting it matches all addresses.
  • Bug Fixes

    • Clarified address-matching semantics: empty address lists act as wildcards and omitted vs provided values are distinguished to prevent unintended matches.
  • Tests / Chores

    • Tests and snapshots updated to reflect the new address semantics; changelog entry added.

@akaladarshi akaladarshi requested a review from a team as a code owner October 8, 2025 11:58
@akaladarshi akaladarshi requested review from elmattic and hanabi1224 and removed request for a team October 8, 2025 11:58
@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Oct 8, 2025
Copy link
Contributor

coderabbitai bot commented Oct 8, 2025

Walkthrough

EthGetLogs filter address became optional: EthFilterSpec.address is now Option<EthAddressList>. Parsing and matching updated to handle None (wildcard), Some(non-empty) (explicit match), and Some(empty) (no-match). Tests and one snapshot were updated; changelog added.

Changes

Cohort / File(s) Summary
Filter parsing & matching
src/rpc/methods/eth/filter/mod.rs
parse_eth_filter_spec and matcher logic updated to treat address as Option: None = wildcard, Some(non-empty) = match list, Some(empty) = no matches. Tests adjusted/expanded for those cases.
Types & public API
src/rpc/methods/eth/types.rs
EthFilterSpec.address changed from EthAddressList to Option<EthAddressList> with serde default/skip rules. From<LogFilter> for EthFilterSpec now wraps addresses in Some(...). Public signature updated.
API compare tests
src/tool/subcommands/api_cmd/api_compare_tests.rs
Test cases updated to wrap address variants in Some(...), added None cases, and adjusted EthGetLogs request constructions to use optional addresses.
Snapshots
src/tool/subcommands/api_cmd/test_snapshots.txt
Replaced one EthGetLogs snapshot artifact; other snapshots unchanged.
Changelog
CHANGELOG.md
Added entry documenting eth_getLogs address parameter is now optional.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant R as RPC Server
  participant H as EthGetLogs Handler
  participant P as EthFilterSpec Parser
  participant M as Log Matcher

  C->>R: eth_getLogs({ address?, topics, ... })
  R->>H: Dispatch request
  H->>P: Parse EthFilterSpec
  alt address omitted (None)
    Note right of P #DDEBF7: address = None (wildcard)
  else address provided & non-empty
    Note right of P #E6F4EA: address = Some(list) (explicit match)
  else address provided & empty
    Note right of P #FFF4E6: address = Some([]) (no matches)
  end
  H->>M: Match logs with parsed spec
  alt None (wildcard)
    M-->>H: Match any address
  else Some(list)
    M-->>H: Match listed addresses
  else Some([])
    M-->>H: No matches
  end
  H-->>R: Logs result
  R-->>C: Response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • elmattic
  • LesnyRumcajs

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely and accurately summarizes the primary change of making the address parameter in the EthFilterSpec optional to support the EthGetLogs API, which aligns with the code modifications and linked issue objective.
Linked Issues Check ✅ Passed The changes convert the EthFilterSpec.address field to an optional type, update parsing and matching logic to handle None as a wildcard, adjust From implementations, and add tests and snapshots to validate requests without an address, fully satisfying the objective of making the address parameter optional as described in issue #6134.
Out of Scope Changes Check ✅ Passed All modifications are confined to making the EthFilterSpec.address field optional, updating related parsing, matching logic, tests, snapshots, and changelog; no unrelated or out-of-scope changes are present.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch akaladarshi/fix-eth-get-logs-api

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ebac6f9 and d5295b8.

📒 Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

Comment @coderabbitai help to get the list of available commands and usage tips.

@akaladarshi akaladarshi force-pushed the akaladarshi/fix-eth-get-logs-api branch from 081e209 to 659a403 Compare October 8, 2025 15:17
elmattic
elmattic previously approved these changes Oct 8, 2025
hanabi1224
hanabi1224 previously approved these changes Oct 9, 2025
@akaladarshi akaladarshi added this pull request to the merge queue Oct 9, 2025
@akaladarshi akaladarshi removed this pull request from the merge queue due to a manual request Oct 9, 2025
@akaladarshi akaladarshi dismissed stale reviews from hanabi1224 and elmattic via 5559ee1 October 9, 2025 05:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 659a403 and 5559ee1.

📒 Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: tests-release
  • GitHub Check: All lint checks
  • GitHub Check: tests
  • GitHub Check: Build forest binaries on Linux AMD64

@akaladarshi akaladarshi force-pushed the akaladarshi/fix-eth-get-logs-api branch from 5559ee1 to ebac6f9 Compare October 9, 2025 06:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5559ee1 and ebac6f9.

📒 Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks

@akaladarshi akaladarshi force-pushed the akaladarshi/fix-eth-get-logs-api branch from ebac6f9 to d5295b8 Compare October 9, 2025 06:05
@akaladarshi akaladarshi enabled auto-merge October 9, 2025 06:29
@akaladarshi akaladarshi added this pull request to the merge queue Oct 9, 2025
Merged via the queue into main with commit 2d24c11 Oct 9, 2025
50 checks passed
@akaladarshi akaladarshi deleted the akaladarshi/fix-eth-get-logs-api branch October 9, 2025 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Address param should be optional in EthGetLogs API

3 participants