refactor(zetaclient): drop `outboundprocessor` #3410

swift1337 · 2025-01-24T15:22:52Z

Summary by CodeRabbit

Release Notes

Refactor
- Removed outboundprocessor package and related dependencies across multiple chain signers
- Simplified outbound transaction processing logic
- Consolidated outbound ID generation and tracking within the Signer struct
Changes
- Removed outboundProcessor parameter from TryProcessOutbound methods
- Updated outbound ID generation to use base.OutboundIDFromCCTX
- Added MarkOutbound method to track active outbound transactions
- Removed outboundprocessor initialization from various chain implementations

coderabbitai · 2025-01-24T15:23:02Z

📝 Walkthrough

Walkthrough

The pull request introduces a comprehensive refactoring of the outbound transaction processing mechanism across multiple components in the Zetachain client. The primary focus is on removing the outboundprocessor package and integrating its functionality directly into the Signer structs. This involves simplifying method signatures, removing external processor dependencies, and introducing a more integrated approach to tracking outbound transaction states.

Changes

File	Change Summary
`zetaclient/chains/base/observer.go`	Simplified logging messages by removing chain ID references
`zetaclient/chains/base/signer.go`	Added `activeOutbounds` tracking, new methods for managing outbound states
`zetaclient/chains/bitcoin/bitcoin.go`, `zetaclient/chains/evm/evm.go`	Removed `outboundprocessor.Processor`, updated `New` method signatures
Multiple signer files	Removed `outboundProc` parameter, introduced `base.OutboundIDFromCCTX()`
`zetaclient/outboundprocessor/outbound_processor.go`	Entire package deleted

Suggested labels

breaking:cli, refactor, LEGACY_TESTS

Suggested reviewers

lumtis
kingpinXD
fbac
skosito

Possibly related PRs

feat: withdraw SOL from ZEVM to Solana #2560: Logging improvements in ZetaCore client
fix: adjust evm outbound tracker reporter to avoid submitting invalid hashes #2628: Outbound tracker reporter modifications
refactor(zetaclient): move app context update from zetacore client #3131: Application context refactoring
refactor(zetaclient): remove redundant getters/setters #3268: Removal of redundant getters/setters

The changes represent a significant architectural refactoring that simplifies the outbound transaction processing workflow by consolidating responsibilities within the Signer components and removing the external processor dependency.

✨ Finishing Touches

📝 Generate Docstrings (Beta)

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

codecov · 2025-01-24T15:32:23Z

Codecov Report

Attention: Patch coverage is 12.22222% with 79 lines in your changes missing coverage. Please review.

Project coverage is 62.99%. Comparing base (3961c42) to head (1286b2d).

Files with missing lines	Patch %	Lines
zetaclient/chains/base/signer.go	2.17%	45 Missing ⚠️
zetaclient/orchestrator/orchestrator.go	0.00%	19 Missing ⚠️
zetaclient/chains/bitcoin/signer/signer.go	0.00%	4 Missing ⚠️
zetaclient/chains/solana/signer/signer.go	0.00%	4 Missing ⚠️
zetaclient/chains/base/observer.go	25.00%	3 Missing ⚠️
zetaclient/chains/bitcoin/bitcoin.go	33.33%	2 Missing ⚠️
zetaclient/chains/evm/evm.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3410      +/-   ##
===========================================
- Coverage    63.03%   62.99%   -0.04%     
===========================================
  Files          436      435       -1     
  Lines        30660    30665       +5     
===========================================
- Hits         19325    19318       -7     
- Misses       10525    10537      +12     
  Partials       810      810

Files with missing lines	Coverage Δ
zetaclient/chains/evm/signer/signer.go	`41.56% <100.00%> (+0.53%)`	⬆️
zetaclient/chains/ton/signer/signer.go	`44.78% <100.00%> (-0.34%)`	⬇️
zetaclient/chains/bitcoin/bitcoin.go	`37.93% <33.33%> (-1.04%)`	⬇️
zetaclient/chains/evm/evm.go	`28.57% <33.33%> (-1.23%)`	⬇️
zetaclient/chains/base/observer.go	`77.01% <25.00%> (ø)`
zetaclient/chains/bitcoin/signer/signer.go	`15.14% <0.00%> (-0.17%)`	⬇️
zetaclient/chains/solana/signer/signer.go	`21.13% <0.00%> (-0.25%)`	⬇️
zetaclient/orchestrator/orchestrator.go	`22.88% <0.00%> (-0.38%)`	⬇️
zetaclient/chains/base/signer.go	`39.77% <2.17%> (-41.18%)`	⬇️

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (7)

zetaclient/chains/base/signer.go (2)

116-120: Clarify early-return logic in MarkOutbound.
When IsOutboundActive(outboundID) already matches the desired state, you silently return. This is concise but may benefit from a trace log if diagnosing unexpected inactivity.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 116-120: zetaclient/chains/base/signer.go#L116-L120
Added lines #L116 - L120 were not covered by tests

144-152: Record total active outbounds for analytics.
Removing the outbound from the map upon completion is correct. Ensure you have coverage or monitoring metrics for the time_taken field, so you can build performance dashboards around average outbound durations.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 144-152: zetaclient/chains/base/signer.go#L144-L152
Added lines #L144 - L152 were not covered by tests

zetaclient/chains/bitcoin/bitcoin.go (1)

147-147: Missing test coverage.
The static analysis indicates this line is untested. Consider adding a unit test scenario to ensure OutboundIDFromCCTX interacts correctly with subsequent processing logic.

Would you like me to propose a concise test to cover this line?

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 147-147: zetaclient/chains/bitcoin/bitcoin.go#L147
Added line #L147 was not covered by tests
zetaclient/chains/solana/signer/signer.go (1)
123-123: Include a stack trace for debugging
Capturing a stack trace in case of panics would aid in troubleshooting deeper issues.
 defer func() {
     signer.MarkOutbound(outboundID, false)
-    if err := recover(); err != nil {
+    if r := recover(); r != nil {
+        debug.PrintStack()
         signer.Logger().Std.Error().Msgf("TryProcessOutbound: %s, caught panic error: %v", cctx.Index, r)
     }
 }()
🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 123-123: zetaclient/chains/solana/signer/signer.go#L123
Added line #L123 was not covered by tests
zetaclient/chains/bitcoin/signer/signer.go (1)
288-288: Use stack traces for improved panic handling
Adding a debug stack trace would help in diagnosing issues when a panic occurs.
 defer func() {
     signer.MarkOutbound(outboundID, false)
-    if err := recover(); err != nil {
+    if r := recover(); r != nil {
+        debug.PrintStack()
         signer.Logger().Std.Error().Msgf("BTC TryProcessOutbound: %s, caught panic error: %v", cctx.Index, r)
     }
 }()
🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 288-288: zetaclient/chains/bitcoin/signer/signer.go#L288
Added line #L288 was not covered by tests
zetaclient/chains/base/observer.go (1)

134-134: Retain chain ID context in logs for easier debugging
Removing the chain ID from the log messages streamlines output, but consider whether additional context might be lost. For troubleshooting, chain-specific logs can be crucial.

Also applies to: 138-138, 145-145, 148-148
zetaclient/chains/evm/signer/signer.go (1)
249-249: Add a stack trace for debugging panics
Emitting stack traces on recovery simplifies troubleshooting deeper panic causes.
 defer func() {
     signer.MarkOutbound(outboundID, false)
-    if r := recover(); r != nil {
+    if r := recover(); r != nil {
+        debug.PrintStack()
         signer.Logger().Std.Error().Msgf("TryProcessOutbound: %s, caught panic error: %v", cctx.Index, r)
     }
 }()

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3961c42 and 1286b2d.

📒 Files selected for processing (15)

zetaclient/chains/base/observer.go (1 hunks)
zetaclient/chains/base/signer.go (4 hunks)
zetaclient/chains/bitcoin/bitcoin.go (3 hunks)
zetaclient/chains/bitcoin/signer/signer.go (1 hunks)
zetaclient/chains/evm/evm.go (4 hunks)
zetaclient/chains/evm/signer/signer.go (1 hunks)
zetaclient/chains/evm/signer/signer_test.go (1 hunks)
zetaclient/chains/interfaces/interfaces.go (0 hunks)
zetaclient/chains/solana/signer/signer.go (1 hunks)
zetaclient/chains/ton/signer/signer.go (1 hunks)
zetaclient/chains/ton/signer/signer_test.go (1 hunks)
zetaclient/orchestrator/orchestrator.go (7 hunks)
zetaclient/outboundprocessor/outbound_processor.go (0 hunks)
zetaclient/outboundprocessor/outbound_processor_test.go (0 hunks)
zetaclient/testutils/mocks/chain_signer.go (0 hunks)

💤 Files with no reviewable changes (4)

zetaclient/outboundprocessor/outbound_processor_test.go
zetaclient/testutils/mocks/chain_signer.go
zetaclient/chains/interfaces/interfaces.go
zetaclient/outboundprocessor/outbound_processor.go

🧰 Additional context used

📓 Path-based instructions (11)

zetaclient/chains/evm/signer/signer_test.go (1)