refactor(zetaclient): make EVM getLogs call once #3570
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the log processing logic within the Observer class for handling Ethereum gateway events. The Changes
Sequence Diagram(s)sequenceDiagram
participant O as Observer
participant FG as fetchGatewayLogs
participant D as observeGatewayDeposit
participant C as observeGatewayCall
participant DC as observeGatewayDepositAndCall
O->>FG: fetchGatewayLogs(ctx, startBlock, toBlock)
FG-->>O: Return gatewayLogs / error
alt gatewayLogs available
O->>D: observeGatewayDeposit(ctx, startBlock, toBlock, gatewayLogs)
O->>C: observeGatewayCall(ctx, startBlock, toBlock, gatewayLogs)
O->>DC: observeGatewayDepositAndCall(ctx, startBlock, toBlock, gatewayLogs)
else Error encountered
O->>O: Log error message
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3570 +/- ##
===========================================
+ Coverage 64.71% 64.76% +0.05%
===========================================
Files 460 460
Lines 31900 31873 -27
===========================================
Hits 20643 20643
+ Misses 10353 10326 -27
Partials 904 904
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zetaclient/chains/evm/observer/v2_inbound.go (3)
64-83: Consider adding dedicated unit tests forfetchGatewayLogs.These lines introduce a new method to retrieve logs from the Gateway contract but the static analysis indicates insufficient test coverage. Adding targeted unit tests (e.g., mocking
ob.evmClient.FilterLogs) will help ensure correct behavior, particularly around error handling for edge cases.Would you like me to propose a minimal test structure using a mock EVM client?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-68: zetaclient/chains/evm/observer/v2_inbound.go#L64-L68
Added lines #L64 - L68 were not covered by tests
[warning] 70-77: zetaclient/chains/evm/observer/v2_inbound.go#L70-L77
Added lines #L70 - L77 were not covered by tests
[warning] 80-82: zetaclient/chains/evm/observer/v2_inbound.go#L80-L82
Added lines #L80 - L82 were not covered by tests
140-154: Factor out repeated logic across parse-and-validate methods.This deposit-event parsing loop shares similarities with the call and deposit-and-call loops. Combining them under a generic or parameterized approach (as hinted in the TODO) would reduce duplication, centralize validation logic, and mitigate potential maintenance overhead.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 140-143: zetaclient/chains/evm/observer/v2_inbound.go#L140-L143
Added lines #L140 - L143 were not covered by tests
[warning] 145-151: zetaclient/chains/evm/observer/v2_inbound.go#L145-L151
Added lines #L145 - L151 were not covered by tests
[warning] 154-154: zetaclient/chains/evm/observer/v2_inbound.go#L154
Added line #L154 was not covered by tests
352-359: Consolidate usage offetchGatewayLogs.Here, deposit-and-call events are also using the single log fetch approach. Validate that passing all logs to every observe function does not significantly degrade performance when multiple events occur in the same block. You may optionally consider partial filtering prior to each parse-and-validate function for improved efficiency.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 352-352: zetaclient/chains/evm/observer/v2_inbound.go#L352
Added line #L352 was not covered by tests
[warning] 359-359: zetaclient/chains/evm/observer/v2_inbound.go#L359
Added line #L359 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zetaclient/chains/evm/observer/inbound.go(1 hunks)zetaclient/chains/evm/observer/v2_inbound.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/inbound.gozetaclient/chains/evm/observer/v2_inbound.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/evm/observer/inbound.go
[warning] 165-171: zetaclient/chains/evm/observer/inbound.go#L165-L171
Added lines #L165 - L171 were not covered by tests
[warning] 173-186: zetaclient/chains/evm/observer/inbound.go#L173-L186
Added lines #L173 - L186 were not covered by tests
zetaclient/chains/evm/observer/v2_inbound.go
[warning] 64-68: zetaclient/chains/evm/observer/v2_inbound.go#L64-L68
Added lines #L64 - L68 were not covered by tests
[warning] 70-77: zetaclient/chains/evm/observer/v2_inbound.go#L70-L77
Added lines #L70 - L77 were not covered by tests
[warning] 80-82: zetaclient/chains/evm/observer/v2_inbound.go#L80-L82
Added lines #L80 - L82 were not covered by tests
[warning] 91-91: zetaclient/chains/evm/observer/v2_inbound.go#L91
Added line #L91 was not covered by tests
[warning] 100-100: zetaclient/chains/evm/observer/v2_inbound.go#L100
Added line #L100 was not covered by tests
[warning] 140-143: zetaclient/chains/evm/observer/v2_inbound.go#L140-L143
Added lines #L140 - L143 were not covered by tests
[warning] 145-151: zetaclient/chains/evm/observer/v2_inbound.go#L145-L151
Added lines #L145 - L151 were not covered by tests
[warning] 154-154: zetaclient/chains/evm/observer/v2_inbound.go#L154
Added line #L154 was not covered by tests
[warning] 235-235: zetaclient/chains/evm/observer/v2_inbound.go#L235
Added line #L235 was not covered by tests
[warning] 242-242: zetaclient/chains/evm/observer/v2_inbound.go#L242
Added line #L242 was not covered by tests
[warning] 276-279: zetaclient/chains/evm/observer/v2_inbound.go#L276-L279
Added lines #L276 - L279 were not covered by tests
[warning] 281-287: zetaclient/chains/evm/observer/v2_inbound.go#L281-L287
Added lines #L281 - L287 were not covered by tests
[warning] 290-290: zetaclient/chains/evm/observer/v2_inbound.go#L290
Added line #L290 was not covered by tests
[warning] 352-352: zetaclient/chains/evm/observer/v2_inbound.go#L352
Added line #L352 was not covered by tests
[warning] 359-359: zetaclient/chains/evm/observer/v2_inbound.go#L359
Added line #L359 was not covered by tests
[warning] 399-402: zetaclient/chains/evm/observer/v2_inbound.go#L399-L402
Added lines #L399 - L402 were not covered by tests
[warning] 404-410: zetaclient/chains/evm/observer/v2_inbound.go#L404-L410
Added lines #L404 - L410 were not covered by tests
[warning] 413-413: zetaclient/chains/evm/observer/v2_inbound.go#L413
Added line #L413 was not covered by tests
🔇 Additional comments (7)
zetaclient/chains/evm/observer/v2_inbound.go (5)
85-91: Check for overlapping calls inobserveGatewayDeposit.The method now accepts logs directly instead of using an event iterator. Verify that passing unfiltered logs from other event types does not cause unintended parsing or side effects. It may be helpful to add a quick pre-filter or a graceful fallback for logs that do not match deposit events.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 91-91: zetaclient/chains/evm/observer/v2_inbound.go#L91
Added line #L91 was not covered by tests
100-100: Improve test coverage for parsing deposit events.This line calls
parseAndValidateDepositEvents, which is new or refactored code. Consider shaping a test that feeds various log combinations (valid deposit logs, invalid logs, etc.) to confirm robust parsing and validation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 100-100: zetaclient/chains/evm/observer/v2_inbound.go#L100
Added line #L100 was not covered by tests
235-242: Ensure consistent naming across function signatures.Observe that the
observeGatewayCallfunction now takes arawLogs []ethtypes.Log. Confirm that any references or docstrings outside this file (e.g., import usage, tests, or README) match this revised argument signature and naming convention.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 235-235: zetaclient/chains/evm/observer/v2_inbound.go#L235
Added line #L235 was not covered by tests
[warning] 242-242: zetaclient/chains/evm/observer/v2_inbound.go#L242
Added line #L242 was not covered by tests
276-290: Add negative tests for call-event parsing.The new logic processes logs for call events and discards invalid entries or suspicious multiple events in the same transaction. Write negative tests ensuring that invalid logs and multi-event logs are handled correctly and do not slip through unintended.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 276-279: zetaclient/chains/evm/observer/v2_inbound.go#L276-L279
Added lines #L276 - L279 were not covered by tests
[warning] 281-287: zetaclient/chains/evm/observer/v2_inbound.go#L281-L287
Added lines #L281 - L287 were not covered by tests
[warning] 290-290: zetaclient/chains/evm/observer/v2_inbound.go#L290
Added line #L290 was not covered by tests
399-413: Test coverage for deposit-and-call event parsing.Similar to deposit and call event parsing, deposit-and-call parsing is newly introduced or refactored. Because these events can be more complex (containing both deposit and call data), carefully confirm that unexpected logs are not silently dropped and valid logs are processed correctly in tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 399-402: zetaclient/chains/evm/observer/v2_inbound.go#L399-L402
Added lines #L399 - L402 were not covered by tests
[warning] 404-410: zetaclient/chains/evm/observer/v2_inbound.go#L404-L410
Added lines #L404 - L410 were not covered by tests
[warning] 413-413: zetaclient/chains/evm/observer/v2_inbound.go#L413
Added line #L413 was not covered by testszetaclient/chains/evm/observer/inbound.go (2)
165-171: Enforce separate tracking for gateway events.The newly introduced
lastScannedGatewayDeposit,lastScannedGatewayCall, andlastScannedGatewayDepositAndCallall default tostartBlock - 1. This approach ensures each event category is tracked independently. Verify that re-scanning fromstartBlock - 1does not lead to double-counting events or indefinite retries if a persistent error occurs.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-171: zetaclient/chains/evm/observer/inbound.go#L165-L171
Added lines #L165 - L171 were not covered by tests
173-186: Augment test coverage for consolidated gateway logs.These lines integrate the freshly fetched logs, passing them to each observation method. At least one test scenario should simulate a single block with multiple gateway events for deposit, call, and deposit-and-call, ensuring that each method processes only its relevant subset without collisions or missed events.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 173-186: zetaclient/chains/evm/observer/inbound.go#L173-L186
Added lines #L173 - L186 were not covered by tests
Make the
getLogscall once then filter the results for each event type. Now instead of making 3getLogscalls we only make 1!This could probably use a bit more refactoring but I think it's a good demo at least.
Related to #3549 #2493 #3525
Summary by CodeRabbit