refactor(filters): reduce the clones while updating the filters#7360
Conversation
akaladarshi
changed the title
WalkthroughEthereum event and tipset filters now keep cursor state behind mutexes. Polling updates that state directly, filter managers no longer receive result-limit configuration, and ChangesEthereum filter state
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/eth.rs (1)
3513-3528: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSerialize the tipset filter cursor update. Two concurrent
eth_getFilterChangescalls can read the samecollected()value before either reachesset_collected(), so both scan and return the same range. Guard the read/scan/advance sequence per filter, or make the cursor advance atomic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth.rs` around lines 3513 - 3528, Serialize the tipset filter cursor lifecycle in the eth_getFilterChanges flow surrounding tipset_filter.collected(), event scanning, and tipset_filter.set_collected(height). Ensure concurrent calls for the same filter cannot read the same cursor or scan the same range; use per-filter locking or an atomic cursor advance while preserving the existing range and update behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
src/rpc/methods/eth/filter/tipset.rs (1)
21-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
anyhow::Resulthere too.TipSetFilter::newstill exposesuuid::Error; wrappingFilterID::new()with.context(...)keeps this constructor aligned with the rest of the error handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth/filter/tipset.rs` around lines 21 - 26, Update TipSetFilter::new to return anyhow::Result, wrap FilterID::new() with contextual error information using the existing anyhow error-handling conventions, and preserve the current Arc<Self> construction on success.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3513-3528: Serialize the tipset filter cursor lifecycle in the
eth_getFilterChanges flow surrounding tipset_filter.collected(), event scanning,
and tipset_filter.set_collected(height). Ensure concurrent calls for the same
filter cannot read the same cursor or scan the same range; use per-filter
locking or an atomic cursor advance while preserving the existing range and
update behavior.
---
Nitpick comments:
In `@src/rpc/methods/eth/filter/tipset.rs`:
- Around line 21-26: Update TipSetFilter::new to return anyhow::Result, wrap
FilterID::new() with contextual error information using the existing anyhow
error-handling conventions, and preserve the current Arc<Self> construction on
success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b52443c-983a-41b3-94ed-0149f593c86e
📒 Files selected for processing (5)
src/rpc/methods/eth.rssrc/rpc/methods/eth/filter/event.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/eth/filter/store.rssrc/rpc/methods/eth/filter/tipset.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
💤 Files with no reviewable changes (1)
- src/rpc/methods/eth/filter/store.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
2 participants
Summary of changes
Changes introduced in this pull request:
updateon the filter, just to store the lastseen_positionsof the events or epochs.seen_positiondirectly by holding a lock on it.Reference issue to close (if applicable)
Closes Part of #7265
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit