fix(zetaclient): add zetaclient evm outbound tx index by nonce to supplement outtx tracker
#2735
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes encompass the addition of a new entry in the changelog for version 19.0.0, detailing a fix for the outbound tracker affecting confirmation and processing on EVM chains. Furthermore, the Changes
Sequence Diagram(s)sequenceDiagram
participant Observer
participant TransactionHandler
participant EVMChain
Observer->>EVMChain: ObserveTSSReceiveInBlockAndOutbound(blockNumber)
EVMChain->>TransactionHandler: Check for confirmed transactions
TransactionHandler-->>EVMChain: Confirmed transactions found
EVMChain-->>Observer: Send transaction receipt
Observer->>Observer: Process inbound and outbound transactions
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 #2735 +/- ##
===========================================
+ Coverage 66.80% 66.93% +0.13%
===========================================
Files 373 373
Lines 20993 21028 +35
===========================================
+ Hits 14025 14076 +51
+ Misses 6315 6288 -27
- Partials 653 664 +11
|
Co-authored-by: Lucas Bertrand <lucas.bertrand.22@gmail.com>
brewmaster012
changed the title
zetaclient): add zetaclient evm outbound tx index by nonce to supplement outtx tracker
brewmaster012
requested review from
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- changelog.md (1 hunks)
- zetaclient/chains/evm/observer/inbound.go (3 hunks)
- zetaclient/chains/evm/observer/inbound_test.go (1 hunks)
Additional context used
Path-based instructions (2)
zetaclient/chains/evm/observer/inbound_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/observer/inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
zetaclient/chains/evm/observer/inbound.go
[warning] 449-449: zetaclient/chains/evm/observer/inbound.go#L449
Added line #L449 was not covered by tests
[warning] 455-455: zetaclient/chains/evm/observer/inbound.go#L455
Added line #L455 was not covered by tests
[warning] 798-800: zetaclient/chains/evm/observer/inbound.go#L798-L800
Added lines #L798 - L800 were not covered by tests
Additional comments not posted (5)
zetaclient/chains/evm/observer/inbound_test.go (3)
484-484: Ensure comprehensive test coverage forObserveTSSReceiveInBlockAndOutbound.The test case verifies the successful observation of TSS receive in a block. Ensure that the new outbound functionality is adequately tested.
Consider adding specific test cases to cover outbound transaction scenarios.
489-489: Verify error handling for block retrieval.The test case simulates an error in getting the block. Ensure that the error handling logic in
ObserveTSSReceiveInBlockAndOutboundis robust.Consider testing additional edge cases related to block retrieval errors.
496-496: Verify error handling for receipt retrieval.The test case simulates an error in getting the receipt. Ensure that the error handling logic in
ObserveTSSReceiveInBlockAndOutboundis robust.Consider testing additional edge cases related to receipt retrieval errors.
zetaclient/chains/evm/observer/inbound.go (1)
Line range hint
773-802: Enhance test coverage forObserveTSSReceiveInBlockAndOutbound.The method now includes logic for handling outbound transactions. Ensure that this new functionality is adequately tested.
Consider adding test cases to cover scenarios where outbound transactions are processed.
Tools
GitHub Check: codecov/patch
[warning] 798-800: zetaclient/chains/evm/observer/inbound.go#L798-L800
Added lines #L798 - L800 were not covered by testschangelog.md (1)
32-32: Ensure clarity and consistency in changelog entries.The changelog entry for PR 2735 is clear and aligns with the PR objectives. It effectively communicates the fix for the outbound tracker issue on EVM chains by indexing outbound transactions locally in the
zetaclient.
Co-authored-by: Charlie Chen <34498985+ws4charlie@users.noreply.github.com>
There was a problem hiding this comment.
I see know the logic and idea. It would still be proper to define a new method like ObserveTSSOutbound, and calling this function in ObserveInbound. Ideally we should have this logic in outbound.go as it pertains to outbounds but the current state of the code doesn't allow it. ObserveInbound is fine as we iterate the blocks there.
I think the ObserveTSSReceiveInBlockAndOutbound naming is a bit confusing and the method is still called in ObserverTSSReceive which makes the control flow a bit more implicit.
Also ObserverTSSReceive will be removed in the future once we fully migrate to V2 contract and wind down v1 compatibility
Tested in localnet E2E by disabling outbound tracker in Ethereum client.
Is it possible to share the line you removed to disable the outbound trackers? Just wanted to do some checks as in the current code it seems we still rely on outbound trackers to watch the outbounds
…aclient-evm-index-by-nonce
I think this PR was intended to be a supplement to (not to disable) outbound trackers. I think it is possible to minimize our reliance on outbound trackers in future and restrict its usage to only missed outbound (similar to inbound tracker). The new observation might look like:
overall: we could switch to a block-scanning model to minimize reliance on outbound trackers while still keep them for scenarios similar to inbound trackers. I created a issue open to discussion: #2805 |
i tested in localnet by disabling reporting to outbound tracker. |
then this function will just become ObserveOutbound |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Description
Currently outbound tracker is susceptible to being filled by invalid hashes and missing entries.
Invalid hashes require manual cleanup by group proposal and missing entries require constant monitoring and filling in.
This PoC PR adds simple logic in zetaclient to index outtx from TSS address by nonce, therefore avoid relying solely on outtx tracker for discovery of outbound tx hashes on Ethereum.
Overhead (both memory and computation) added is minimal as it piggy backs on existing block by block scanner in inbound tx processing.
FilterTSSOutboundinoutbound.goto filter outbounds in blocks.Closes #2846
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Tests