fix: apply outbound_schedule_interval and outbound_schedule_lookahead to TON scheduler
#4513
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 📝 WalkthroughWalkthroughRefactored CCTX scheduling logic across multiple chain implementations. Extracted a local constant Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
ws4charlie
added
SUI_TESTS
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #4513 +/- ##
===========================================
- Coverage 64.72% 64.68% -0.05%
===========================================
Files 477 477
Lines 28998 29017 +19
===========================================
Hits 18769 18769
- Misses 9202 9222 +20
+ Partials 1027 1026 -1
🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zetaclient/chains/sui/sui.go (1)
30-35: Inconsistency: Sui still uses local constant instead of shared package constant.While
evm.goandsolana.gowere updated to useconstant.OutboundLookbackFactor,sui.gostill defines and uses a localoutboundLookbackFactorconstant. This is inconsistent with the broader refactoring goal of centralizing the lookback factor configuration.🔎 Suggested fix to align with other chain implementations
Remove the local constant at lines 30-35:
-const ( - // outboundLookbackFactor is the factor to determine how many nonces to look back for pending cctxs - // For example, give OutboundScheduleLookahead of 120, pending NonceLow of 1000 and factor of 1.0, - // the scheduler need to be able to pick up and schedule any pending cctx with nonce < 880 (1000 - 120 * 1.0) - // NOTE: 1.0 means look back the same number of cctxs as we look ahead - outboundLookbackFactor = 1.0 -)Add the constant package import and update line 143 to use the shared constant:
+import ( + "github.com/zeta-chain/node/pkg/constant" +)- lookback = uint64(float64(lookahead) * outboundLookbackFactor) + lookback = uint64(float64(lookahead) * constant.OutboundLookbackFactor)Also applies to: 143-143
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
changelog.mdpkg/constant/constant.gozetaclient/chains/evm/evm.gozetaclient/chains/solana/solana.gozetaclient/chains/sui/sui.gozetaclient/chains/ton/ton.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
zetaclient/chains/solana/solana.gopkg/constant/constant.gozetaclient/chains/evm/evm.gozetaclient/chains/ton/ton.gozetaclient/chains/sui/sui.go
🧠 Learnings (9)
📚 Learning: 2025-03-04T22:39:58.395Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3632
File: zetaclient/chains/solana/signer/signer.go:304-304
Timestamp: 2025-03-04T22:39:58.395Z
Learning: The Solana signer implementation in zetaclient/chains/solana/signer/signer.go has limited test coverage, particularly for the transaction broadcasting logic with fallback scenarios. Adding this coverage has been acknowledged as a potential future improvement outside the scope of immediate fixes.
Applied to files:
zetaclient/chains/solana/solana.gozetaclient/chains/ton/ton.go
📚 Learning: 2024-10-08T15:34:48.217Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 2907
File: zetaclient/chains/bitcoin/observer/outbound_test.go:81-82
Timestamp: 2024-10-08T15:34:48.217Z
Learning: In the `mineTxNSetNonceMark` function within `bitcoin/observer/outbound_test.go`, it's acceptable to hardcode the chain ID, as the tests do not require varying chain IDs.
Applied to files:
zetaclient/chains/solana/solana.gozetaclient/chains/evm/evm.gozetaclient/chains/ton/ton.gozetaclient/chains/sui/sui.go
📚 Learning: 2025-09-15T13:42:17.594Z
Learnt from: lumtis
Repo: zeta-chain/node PR: 4199
File: zetaclient/chains/evm/signer/signer_admin.go:25-26
Timestamp: 2025-09-15T13:42:17.594Z
Learning: In PR 4199, the CLI references to CmdMigrateTssFunds in x/crosschain/client/cli/ files are intentionally not updated as they are out of scope for this specific refactor focused on removing ERC20 custody messages.
Applied to files:
pkg/constant/constant.go
📚 Learning: 2025-07-28T18:08:13.883Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 4053
File: e2e/e2etests/test_bitcoin_std_deposit.go:42-42
Timestamp: 2025-07-28T18:08:13.883Z
Learning: In Bitcoin deposit E2E tests for the ZetaChain project, `DepositBTCWithAmount` is preferred over `DepositBTCWithExactAmount` because depositing exact amounts to a ZEVM receiver is complex. The tests use rough amounts and then calculate the actual received amount from the raw Bitcoin transaction to verify balance changes, making the tests more robust and less flaky.
Applied to files:
pkg/constant/constant.go
📚 Learning: 2025-01-22T22:46:58.820Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3395
File: .github/workflows/reusable-sim.yml:29-30
Timestamp: 2025-01-22T22:46:58.820Z
Learning: The zeta-chain/node repository uses Go version >= 1.22. Do not suggest downgrading to earlier versions like 1.21.
Applied to files:
zetaclient/chains/evm/evm.gozetaclient/chains/ton/ton.go
📚 Learning: 2025-02-04T06:12:41.760Z
Learnt from: ws4charlie
Repo: zeta-chain/node PR: 3461
File: x/observer/types/chain_params.go:59-62
Timestamp: 2025-02-04T06:12:41.760Z
Learning: The legacy `ConfirmationCount` field in the ChainParams struct is planned to be deprecated after a full upgrade to the new confirmation count system that uses SafeInboundCount, FastInboundCount, SafeOutboundCount, and FastOutboundCount fields.
Applied to files:
zetaclient/chains/ton/ton.go
📚 Learning: 2024-10-30T17:56:16.341Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3070
File: cmd/zetae2e/init.go:0-0
Timestamp: 2024-10-30T17:56:16.341Z
Learning: In code reviews for Go files like `cmd/zetae2e/init.go` in the ZetaChain project, avoid suggesting unrelated refactoring. Focus comments on changes relevant to the PR objectives.
Applied to files:
zetaclient/chains/ton/ton.gozetaclient/chains/sui/sui.go
📚 Learning: 2024-11-27T22:01:49.732Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3228
File: zetaclient/orchestrator/orchestrator.go:388-401
Timestamp: 2024-11-27T22:01:49.732Z
Learning: When reviewing code changes in `zetaclient/orchestrator/orchestrator.go`, avoid suggesting refactoring that is unrelated to the current PR.
Applied to files:
zetaclient/chains/ton/ton.gozetaclient/chains/sui/sui.go
📚 Learning: 2024-10-31T16:19:26.038Z
Learnt from: gartnera
Repo: zeta-chain/node PR: 3071
File: e2e/e2etests/test_stress_eth_withdraw.go:58-59
Timestamp: 2024-10-31T16:19:26.038Z
Learning: In the Go code within `e2e/utils/zetacore.go`, the function `WaitCctxMinedByInboundHash` does not return an error.
Applied to files:
zetaclient/chains/ton/ton.go
🧬 Code graph analysis (2)
zetaclient/chains/evm/evm.go (1)
pkg/constant/constant.go (1)
OutboundLookbackFactor(51-51)
zetaclient/chains/ton/ton.go (3)
pkg/constant/constant.go (1)
OutboundLookbackFactor(51-51)zetaclient/chains/base/signer.go (1)
OutboundIDFromCCTX(215-218)zetaclient/chains/zrepo/zrepo.go (1)
ZetaRepo(46-52)
🔇 Additional comments (10)
zetaclient/chains/evm/evm.go (1)
13-13: LGTM! Shared constant refactoring is correctly applied.The migration from a local
outboundLookBackFactorconstant to the sharedconstant.OutboundLookbackFactormaintains the same calculation logic while improving consistency across chain implementations.Also applies to: 131-131
zetaclient/chains/sui/sui.go (1)
165-166: Verify the nonce boundary change from inclusive to exclusive.The nonce validation changed from
nonce >= maxNonce(implicit in old code) tononce > maxNonce. This now allows nonces equal tomaxNoncethat were previously rejected. Please confirm this boundary relaxation is intentional and aligns with the desired scheduling behavior.pkg/constant/constant.go (1)
47-51: LGTM! Well-documented shared constant.The
OutboundLookbackFactorconstant is clearly documented with an example calculation, making it easy to understand its purpose in the outbound scheduling logic.zetaclient/chains/solana/solana.go (2)
11-11: LGTM! Shared constant correctly applied.The migration to
constant.OutboundLookbackFactormaintains consistency with the EVM implementation.Also applies to: 132-132
142-155: LGTM! Lookahead termination correctly implemented.The addition of index tracking and the early return when
int64(i) == scheduleLookaheadcorrectly implements the lookahead limit. This ensures that the scheduler processes at mostscheduleLookaheadnumber of pending CCTXs per invocation, as intended by the PR objectives.zetaclient/chains/ton/ton.go (5)
12-12: LGTM on the import addition.The import of
constant.OutboundLookbackFactoraligns with the shared constant usage across EVM, Solana, and Sui implementations, ensuring consistent lookback factor configuration.
129-132: LGTM on the early return guard.Clean no-op pattern for empty CCTX list.
183-186: LGTM on the interval-based scheduling logic.The modulo-based distribution
nonce%interval == zetaHeight%intervaleffectively spreads keysign processing across blocks. The goroutine invocation forTryProcessOutboundensures non-blocking execution, consistent with Sui and Solana implementations.
172-181: LGTM on theVoteOutboundIfConfirmedintegration.Error handling is appropriately structured: failures are logged and processing continues to avoid blocking subsequent CCTXs, while already-processed outbounds are correctly skipped with informational logging.
134-147: Remove division by zero concern; upstream validation prevents this issue.The
OutboundScheduleIntervalis validated inx/observer/types/chain_params.go(lines 105–110) to reject zero values and enforce a range of [1, 100]. This validation is mandatory for all chain parameters before they are stored and retrieved by the observer. Consequently, the modulo operation at line 184 is safe, and the#nosec G115 positivecomment is justified.Likely an incorrect or invalid review comment.
Description
This PR wires following two chain parameters to
TONin a way similar toSuiandSolana.outbound_schedule_intervaloutbound_schedule_lookaheadHow Has This Been Tested?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.