Replace custom-defined FilterCriteria type with the relevant type from Geth - #849
Conversation
WalkthroughReplace local/internal filter type and pointer usage with github.com/onflow/go-ethereum/eth/filters.FilterCriteria value across API and logs service; update NewIDFilter/NewRangeFilter signatures and call sites; adjust tests (Go and JS) to the new API and add JS cases validating max topics/addresses; minor test-runner and streaming-test cleanups. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant LogsService
Client->>API: eth_getLogs or subscribe with filters.FilterCriteria
API->>LogsService: Call NewIDFilter(criteria) or NewRangeFilter(from,to,criteria)
LogsService->>LogsService: Validate criteria (uses criteria.BlockHash for ID if ID filter)
LogsService-->>API: Return filter or validation error
API-->>Client: Return logs or RPC error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (2)tests/web3js/eth_logs_filtering_test.js (1)
tests/web3js/eth_filter_endpoints_test.js (2)
⏰ 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). (1)
🔇 Additional comments (11)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
10dfebf to
2003bf4
Compare
|
The changes are looking good. I wonder what's the reason we made the custom defined filter criteria. Was it to prevent some issue? |
| if len(topics) > maxTopics { | ||
| return nil, fmt.Errorf("max topics exceeded, only %d allowed, got %d", maxTopics, len(topics)) | ||
| } | ||
| if len(addresses) > maxAddresses { | ||
| return nil, fmt.Errorf("max addresses exceeded, only %d allowed, got %d", maxAddresses, len(addresses)) | ||
| } |
There was a problem hiding this comment.
are these limits enforced somewhere else?
There was a problem hiding this comment.
Yes, exactly, that's the main idea behind this PR.
The FilterCriteria type from Geth, enforces the validation logic inside the UnmarshalJSON method.
More specifically:
- https://github.com/ethereum/go-ethereum/blob/v1.16.1/eth/filters/api.go#L540-L542
- https://github.com/ethereum/go-ethereum/blob/v1.16.1/eth/filters/api.go#L564-L566
- https://github.com/ethereum/go-ethereum/blob/v1.16.1/eth/filters/api.go#L586-L589
Whenever a JSON-RPC call, accepts a parameter of FilterCriteria, for example:
// GetLogs returns logs matching the given argument that are stored within the state.
func (b *BlockChainAPI) GetLogs(
ctx context.Context,
criteria filters.FilterCriteria,
) ([]*types.Log, error) {Under the hood, Geth will call UnmarshalJSON on FilterCriteria, to parse the JSON object, to the appropriate FilterCriteria object, and runs any validation logic. If the validation fails, it will return the appropriate error.
@zhangchiqing Because certain Geth types are often placed under |
2003bf4 to
675a659
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/api.go(2 hunks)api/pull.go(3 hunks)api/stream.go(1 hunks)services/logs/filter.go(5 hunks)services/logs/filter_test.go(8 hunks)tests/helpers.go(1 hunks)tests/web3js/eth_filter_endpoints_test.js(1 hunks)tests/web3js/eth_logs_filtering_test.js(1 hunks)tests/web3js/eth_streaming_filters_test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/helpers.go
- tests/web3js/eth_filter_endpoints_test.js
- api/stream.go
- api/pull.go
- services/logs/filter_test.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/web3js/eth_logs_filtering_test.js (2)
tests/web3js/config.js (1)
web3(2-2)tests/web3js/helpers.js (1)
web3(5-5)
services/logs/filter.go (1)
storage/index.go (1)
ReceiptIndexer(61-82)
tests/web3js/eth_streaming_filters_test.js (3)
tests/web3js/eth_filter_endpoints_test.js (2)
web3(4-4)conf(2-2)tests/web3js/config.js (1)
web3(2-2)tests/web3js/setup_test.js (1)
conf(2-2)
api/api.go (1)
services/logs/filter.go (2)
NewIDFilter(98-113)NewRangeFilter(22-43)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (9)
tests/web3js/eth_logs_filtering_test.js (1)
194-232: LGTM! The test correctly validates max topics enforcement.The test is well-structured and properly tests both block range and block hash filters with 5 topics (exceeding the limit of 4). The error message assertion matches Geth's validation behavior.
tests/web3js/eth_streaming_filters_test.js (2)
174-174: Good cleanup of WebSocket connection.Replacing
process.exit()with proper WebSocket disconnection is the correct approach for test cleanup.
177-229: LGTM! Well-structured max topics validation test.The test properly validates the max topics limit for streaming subscriptions with appropriate error handling and cleanup.
services/logs/filter.go (4)
98-112: LGTM! Proper validation of BlockHash requirement.The implementation correctly validates that
BlockHashis non-nil and uses it as the filter ID. This aligns well with the switch to Geth'sFilterCriteriatype.
21-42: LGTM! Clean constructor with appropriate range validation.The updated signature properly accepts the
filters.FilterCriteriatype by value, and the range validation logic is preserved.
139-162: LGTM! Efficient exact matching logic.The function correctly accepts
filters.FilterCriteriaby value and maintains the proper matching logic for topics and addresses.
169-196: LGTM! Bloom filter matching properly adapted.The bloom matching function correctly uses the external
filters.FilterCriteriatype while maintaining the fast probabilistic filtering logic.api/api.go (2)
596-599: LGTM! Clean integration with the updated filter API.The code correctly passes the
criteriadirectly toNewIDFilter, eliminating the need for intermediate filter creation. Error handling is properly maintained.
639-642: LGTM! Simplified range filter creation.The direct passing of
criteriatoNewRangeFilteraligns well with the new API design and removes unnecessary intermediate steps.
daeea24 to
51c7965
Compare
3 participants
Work towards: #840
Description
By using the
FilterCriteriatype from Geth, we benefit from the parsing logic ofUnmarshalJSON, without having to duplicate it in our code-base.This removes the need for the validation checks on max topics/sub-topics/addresses etc.
In addition to that, we also get the new validations for free, such as this one: ethereum/go-ethereum#31876 , by simply upgrading the Geth version, and without having to check for code-base changes to match the JSON-RPC Ethereum API specification.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Bug Fixes
Tests
Refactor