feat(zetaclient): SUI observer–signer setup #3516
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 📝 WalkthroughWalkthroughThis pull request introduces extensive modifications to expand support for the SUI blockchain. The changes include a reorganization of the changelog with duplicated fix entries, additions of new features (such as Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant AppContext
participant Config
participant BaseObserver
participant SUIClient
participant SUIObserver
participant SUISigner
participant SUIIntegration
participant Scheduler
Orchestrator->>AppContext: Verify chain type (IsSUI())
AppContext->>Config: Retrieve SUIConfig
Config-->>AppContext: Return endpoint details
AppContext->>BaseObserver: Initialize base observer
AppContext->>SUIClient: Create client with endpoint
AppContext->>SUIObserver: Instantiate observer with BaseObserver & SUIClient
AppContext->>SUISigner: Create signer (via base signer)
AppContext->>SUIIntegration: Construct SUI integration (scheduler, observer, signer)
SUIIntegration->>Scheduler: Start scheduling tasks
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
swift1337
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3516 +/- ##
===========================================
- Coverage 65.52% 65.46% -0.07%
===========================================
Files 445 449 +4
Lines 30606 30757 +151
===========================================
+ Hits 20055 20134 +79
- Misses 9697 9758 +61
- Partials 854 865 +11
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
changelog.md (1)
17-26:⚠️ Potential issueRemove duplicate "Fixes" section.
There is a duplicate "Fixes" section containing the same entry about fixing E2E test failure caused by nil
ConfirmationParams. This redundancy should be removed to avoid confusion.Apply this diff to remove the duplicate section:
### Fixes * [3501](https://github.com/zeta-chain/node/pull/3501) - fix E2E test failure caused by nil `ConfirmationParams` for Solana and TON * [3509](https://github.com/zeta-chain/node/pull/3509) - schedule Bitcoin TSS keysign on interval to avoid TSS keysign spam * [3517](https://github.com/zeta-chain/node/pull/3517) - remove duplicate gateway event appending to fix false positive on multiple events in same tx -### Fixes - -* [3501](https://github.com/zeta-chain/node/pull/3501) - fix E2E test failure caused by nil `ConfirmationParams` for Solana and TON🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
23-23: Multiple headings with the same content
null(MD024, no-duplicate-heading)
🧹 Nitpick comments (15)
zetaclient/chains/sui/observer/observer.go (2)
36-47: Consider storing the returned block time for diagnostic or monitoring.
After callingHealthCheck, caching the block timestamp or height locally might help track RPC status trends or perform advanced diagnostic checks.
62-88: Validate potential edge cases for gas price or epoch values.
While you handle errors for invalid checkpoints and gas prices, consider logging or retrying if the values returned by RPC are unexpectedly zero or negative (if ever possible).zetaclient/chains/sui/sui.go (3)
35-85: Refine task registration logic for improved manageability.
Centralizing or grouping the scheduler tasks (e.g., inbound/outbound observation, block subscription) under a cohesive framework could ease maintenance.
87-91: Stopping tasks is effective, but confirm graceful shutdown.
Ensure thatStop()logic provides enough time for ongoing tasks to finalize, preventing abrupt interruptions.
97-101: Implement or remove the placeholder for outbound CCTX scheduling.
This stub currently returns nil. If scheduling is still in design, consider a TODO reference with details. Otherwise, provide an implementation or remove the method.zetaclient/testutils/testrpc/rpc_sui.go (1)
23-23: Address the TODO comment.The TODO comment should be addressed or removed before merging.
Would you like me to help implement the missing functionality or open an issue to track this task?
zetaclient/chains/sui/client/client_live_test.go (1)
12-14: Consider using testnet for live tests.Using mainnet RPC endpoint for tests might lead to rate limiting or unnecessary load on production infrastructure.
- RpcMainnet = "https://sui-mainnet.public.blastapi.io" + RpcTestnet = "https://sui-testnet.public.blastapi.io"pkg/contracts/sui/inbound_test.go (1)
10-42: Consider adding edge cases to the test suite.While the current test cases cover basic scenarios, consider adding edge cases:
- Empty CoinType
- Zero amount
- Empty sender/receiver addresses
}, + { + name: "empty coin type", + d: &sui.Inbound{ + TxHash: "0x123", + EventIndex: 1, + CoinType: "", + Amount: 100, + Sender: "0x456", + Receiver: sample.EthAddress(), + Payload: nil, + }, + want: false, + }, }zetaclient/chains/sui/client/client.go (2)
36-39: Use constant for base in ParseInt.Define a constant for the base parameter in
strconv.ParseIntto improve code readability and maintainability.+const ( + base10 = 10 +) -ts, err := strconv.ParseInt(checkpoint.TimestampMs, 10, 64) +ts, err := strconv.ParseInt(checkpoint.TimestampMs, base10, 64)
52-54: Use strconv.FormatInt for better performance.Replace
fmt.Sprintfwithstrconv.FormatIntfor converting sequence number to string, as it's more efficient for integer conversions.-return c.SuiGetCheckpoint(ctx, models.SuiGetCheckpointRequest{ - CheckpointID: fmt.Sprintf("%d", seqNum), -}) +return c.SuiGetCheckpoint(ctx, models.SuiGetCheckpointRequest{ + CheckpointID: strconv.FormatInt(seqNum, 10), +})e2e/runner/setup_sui.go (1)
69-70: Address TODO comment about saving IDs.The TODO comment indicates missing functionality for saving IDs in config and configuring the chain. I can help implement this requirement.
Would you like me to help implement the ID saving functionality or create an issue to track this task?
pkg/contracts/sui/inbound.go (2)
46-53: Add validation for zero amounts.The amount parsing logic should validate that the amount is greater than zero to prevent processing of zero-value transactions.
amount, err := strconv.ParseUint(parsedAmount, 10, 64) if err != nil { return Inbound{}, errors.Wrap(err, "failed to parse amount") } +if amount == 0 { + return Inbound{}, errors.New("amount cannot be zero") +}
98-99: Add function documentation.The
convertPayloadfunction lacks documentation explaining its purpose and parameters.-// convertPayload +// convertPayload converts a slice of interface{} to a byte slice. +// Each element in the input slice must be a float64 representing a byte value (0-255). +// Returns an error if any element is not a float64 or is outside the valid byte range.pkg/contracts/sui/gateway.go (1)
65-88: Return nil slice with error.When returning an error, it's idiomatic in Go to return a nil slice rather than an empty slice.
- return []Inbound{}, err + return nil, errpkg/contracts/sui/gateway_test.go (1)
18-18: Move RPC endpoint to configuration.The testnet RPC endpoint is hardcoded. Consider moving it to a configuration file or environment variable for better maintainability.
Apply this diff:
-const rpcTestnet = "https://sui-testnet-endpoint.blockvision.org" +var rpcTestnet = getEnvOrDefault("SUI_TESTNET_RPC", "https://sui-testnet-endpoint.blockvision.org") + +func getEnvOrDefault(key, defaultValue string) string { + if value := os.Getenv(key); value != "" { + return value + } + return defaultValue +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
changelog.md(1 hunks)contrib/localnet/docker-compose.yml(1 hunks)e2e/runner/setup_sui.go(2 hunks)e2e/utils/sui/gateway.go(0 hunks)pkg/chains/chain.go(1 hunks)pkg/contracts/sui/gateway.go(1 hunks)pkg/contracts/sui/gateway_test.go(1 hunks)pkg/contracts/sui/inbound.go(1 hunks)pkg/contracts/sui/inbound_test.go(1 hunks)zetaclient/chains/sui/client/client.go(1 hunks)zetaclient/chains/sui/client/client_live_test.go(1 hunks)zetaclient/chains/sui/observer/observer.go(1 hunks)zetaclient/chains/sui/observer/observer_test.go(1 hunks)zetaclient/chains/sui/signer/signer.go(1 hunks)zetaclient/chains/sui/sui.go(1 hunks)zetaclient/config/config_chain.go(2 hunks)zetaclient/config/types.go(3 hunks)zetaclient/context/chain.go(1 hunks)zetaclient/orchestrator/v2_bootstrap.go(2 hunks)zetaclient/orchestrator/v2_bootstrap_test.go(2 hunks)zetaclient/orchestrator/v2_orchestrator.go(1 hunks)zetaclient/orchestrator/v2_orchestrator_test.go(2 hunks)zetaclient/testutils/mocks/sui_client.go(1 hunks)zetaclient/testutils/mocks/sui_gen.go(1 hunks)zetaclient/testutils/testrpc/rpc_sui.go(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/utils/sui/gateway.go
🧰 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/testutils/mocks/sui_gen.gopkg/chains/chain.gozetaclient/context/chain.gozetaclient/orchestrator/v2_orchestrator.goe2e/runner/setup_sui.gozetaclient/testutils/testrpc/rpc_sui.gozetaclient/chains/sui/signer/signer.gozetaclient/config/types.gopkg/contracts/sui/inbound_test.gozetaclient/chains/sui/observer/observer_test.gozetaclient/chains/sui/client/client_live_test.gozetaclient/chains/sui/observer/observer.gozetaclient/orchestrator/v2_bootstrap.gopkg/contracts/sui/inbound.gozetaclient/chains/sui/sui.gozetaclient/chains/sui/client/client.gozetaclient/orchestrator/v2_orchestrator_test.gozetaclient/config/config_chain.gopkg/contracts/sui/gateway_test.gozetaclient/orchestrator/v2_bootstrap_test.gopkg/contracts/sui/gateway.gozetaclient/testutils/mocks/sui_client.go
🪛 markdownlint-cli2 (0.17.2)
changelog.md
23-23: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: start-e2e-test / e2e
- GitHub Check: build-and-test
🔇 Additional comments (27)
changelog.md (1)
1-893: LGTM!The changelog follows good practices with:
- Clear categorization of changes (Features, Fixes, Refactor, etc.)
- PR references for each change
- Detailed descriptions of changes
- Breaking changes clearly highlighted
- Version history well maintained
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
23-23: Multiple headings with the same content
null(MD024, no-duplicate-heading)
163-163: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
167-167: Multiple headings with the same content
null(MD024, no-duplicate-heading)
169-169: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
175-175: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
177-177: Multiple headings with the same content
null(MD024, no-duplicate-heading)
179-179: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
180-180: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
184-184: Multiple headings with the same content
null(MD024, no-duplicate-heading)
186-186: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
190-190: Multiple headings with the same content
null(MD024, no-duplicate-heading)
192-192: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
600-600: Multiple headings with the same content
null(MD024, no-duplicate-heading)
602-602: Bare URL used
null(MD034, no-bare-urls)
647-647: Multiple headings with the same content
null(MD024, no-duplicate-heading)
703-703: Multiple headings with the same content
null(MD024, no-duplicate-heading)
zetaclient/chains/sui/observer/observer.go (3)
14-18: Struct design looks clean and concise.
TheObserverstruct embedsbase.Observerand holds an RPC client. This design cleanly separates base logic from SUI-specific logic.
49-61: Documentation clarity is commendable.
The extended block comments thoroughly highlight the intricacies of gas price dynamics in SUI, giving future developers a solid reference.
90-97: Robust utility for parsing string to uint64.
Wrapping the error ensures clarity when troubleshooting invalid input values.zetaclient/chains/sui/sui.go (1)
18-23: Structured approach to observer-signer integration.
TheSUIstruct’s fields are succinctly organized, enabling seamless interplay between scheduler, observer, and signer.zetaclient/chains/sui/signer/signer.go (2)
6-8: Inheritance from base signer is straightforward.
Embeddingbase.Signerfosters reuse of core features without code duplication.
10-13: Consider validating the base signer instance.
IfbaseSignercan be nil, adding a check or returning an error would prevent potential runtime panics.zetaclient/testutils/testrpc/rpc_sui.go (2)
9-13: LGTM! Well-structured server definition.The
SUIServerstruct is well-designed with clear field definitions and appropriate embedding of the baseServertype.
15-21: LGTM! Clean constructor implementation.The
NewSUIServerfunction follows good practices:
- Clear parameter and return types
- Proper initialization of all fields
- Returns both server and config for convenience
zetaclient/testutils/mocks/sui_gen.go (2)
10-15: LGTM! Well-documented mock generation setup.The interface documentation and mock generation directives are clear and follow best practices:
- Purpose of unexported interface is explained
- Proper use of go:generate directive
- Appropriate linter directive
15-21: LGTM! Comprehensive interface definition.The interface includes all essential SUI client methods with proper context support and error handling.
zetaclient/chains/sui/client/client_live_test.go (2)
23-36: LGTM! Well-structured test implementation.The test follows best practices:
- Clear AAA pattern (Arrange, Act, Assert)
- Proper error handling and assertions
- Helpful logging of results
38-49: LGTM! Clean test suite implementation.The test suite is well-designed:
- Proper encapsulation of test dependencies
- Clear constructor function
- Good use of context
pkg/contracts/sui/inbound_test.go (1)
43-52: LGTM! Clean test implementation.The test implementation follows best practices:
- Clear table-driven test pattern
- Good use of subtests
- Proper assertions using require
zetaclient/config/config_chain.go (1)
53-57: LGTM!The SUI configuration follows the established pattern for other chains and maintains consistency with existing localnet configurations.
e2e/runner/setup_sui.go (1)
10-11: LGTM!The import path change to use the new
zetasuipackage is appropriate and follows good package organization practices.zetaclient/chains/sui/observer/observer_test.go (1)
64-65: Clarify TODO comment about zctx.The TODO comment about zctx with chain & params needs more context. Consider adding a detailed explanation or linking to relevant issues/PRs.
Could you provide more context about the planned changes for zctx in future PRs?
zetaclient/testutils/mocks/sui_client.go (1)
1-145: LGTM! Auto-generated mock implementation.The mock implementation is complete and follows the standard mockery patterns.
zetaclient/context/chain.go (1)
177-179: LGTM! Clean implementation following existing patterns.The
IsSUImethod is well-integrated and follows the established pattern of other chain type checks.zetaclient/config/types.go (2)
58-61: Update comment style to match project conventions.Based on past review feedback, "Sui" is preferred over "SUI" in comments.
Apply this diff to fix the comment:
-// SUIConfig is the config for SUI chain +// SuiConfig is the config for Sui chain
158-164: LGTM! Thread-safe implementation.The GetSUIConfig method correctly implements thread-safe access using read locks and follows the established pattern of other getter methods.
pkg/chains/chain.go (1)
167-170: Update function name and comment style to match project conventions.Based on past review feedback, "Sui" is preferred over "SUI" in function names and comments.
Apply this diff to fix the naming:
-// IsSUIChain returns true if the chain is SUI chain. -func IsSUIChain(chainID int64, additionalChains []Chain) bool { +// IsSuiChain returns true if the chain is Sui chain. +func IsSuiChain(chainID int64, additionalChains []Chain) bool {zetaclient/orchestrator/v2_orchestrator.go (1)
172-174: LGTM! Consistent implementation.The SUI case follows the established pattern of other chain types and maintains consistent error handling.
zetaclient/orchestrator/v2_bootstrap_test.go (2)
159-195: LGTM! Comprehensive test coverage.The SUI test case follows the established pattern and includes thorough assertions for observer signer bootstrapping, scheduler tasks, and logging.
307-313: Implement mock functions for comprehensive testing.Both mock functions have TODO comments without implementation, which may affect test coverage.
- Implement the mock functions to ensure thorough testing.
- Consider removing the unused
clientparameter frommockSolanaCalls.Apply this diff to fix the unused parameter:
-func mockSolanaCalls(_ *testSuite, _ *testrpc.SolanaServer) { +func mockSolanaCalls(_ *testSuite) {pkg/contracts/sui/gateway_test.go (1)
20-22: LGTM! Well-documented test setup.The comment clearly documents the test setup and provides a reference to the deployed gateway.
contrib/localnet/docker-compose.yml (1)
259-260: LGTM! Proper port mapping for SUI RPC.The port mapping correctly exposes the SUI RPC endpoint on port 9000, which aligns with the service configuration.
This is an initial work in zetaclient regarding support for SUI
Based on #3500
Closes #3473
Summary by CodeRabbit
New Features
Documentation
These updates expand cross-chain capabilities and improve system configuration, providing a smoother and more robust user experience.