feat(zetaclient): SUI withdrawals #3562
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 refactoring and enhancements to the Sui integration and scheduler components. Updates include parameter additions for scheduler initialization, renaming and restructuring of deposit/withdrawal data types, enhanced gateway construction and event parsing, new cryptographic utility functions, and significant extensions to signer and observer functionalities for outbound transaction processing. In addition, obsolete Sui signer utilities and associated tests have been removed, while new tests and mock methods have been added to improve error handling and concurrency. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Signer
participant SR as Sui RPC
participant OT as Outbound Tracker
participant Z as Zetacore
S->>SR: MoveCall to build withdrawal transaction
SR-->>S: Return txn metadata
S->>SR: signTx (execute transaction block)
SR-->>S: Return tx block response & digest
S->>S: (Optional) getWithdrawCapID check
S->>OT: reportOutboundTracker(nonce, digest)
OT-->>S: Acknowledgment
S->>Z: postTrackerVote(nonce, digest)
Z-->>S: Vote acknowledged
sequenceDiagram
participant O as Orchestrator (V2)
participant G as Gateway
participant Ob as Observer
O->>G: NewGatewayFromPairID(packageID, gatewayObjectID)
G-->>O: Initialized Gateway with objectID
O->>Ob: Create Observer with Gateway instance
Ob-->>Ob: Initialize txMap & gasPrice fields (with locking)
Ob->>G: Request event parsing (Deposit/Withdrawal)
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
|
|
!!!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 |
swift1337
force-pushed
the
feat/sui/withdrawals
branch
from
a6f3c4b to
ce8a8b5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (33)
zetaclient/testutils/constant.go (1)
47-47: Updated gateway address format requires documentation.The gateway address format for Sui has been changed to include two components separated by a comma (likely package ID and gateway object ID). Consider adding a comment explaining the new format and purpose of each component to improve maintainability and developer onboarding.
+ // SuiMainnet gateway address format: "$packageID,$gatewayObjectID" chains.SuiMainnet.ChainId: "0x5d4b302506645c37ff133b98fff50a5ae14841659738d6d733d59d0d217a9fff,0xffff302506645c37ff133b98fff50a5ae14841659738d6d733d59d0d217a9aaa",zetaclient/orchestrator/v2_orchestrator_test.go (1)
126-126: Inconsistent default interval between tests and production code.The test uses
time.Secondas the default interval while the production code uses0. Consider using a constant to ensure consistency, or document why different values are appropriate in different contexts.- schedulerService = scheduler.New(logger.Logger, time.Second) + // Use same default interval as production code (0) or document why a different value is needed + schedulerService = scheduler.New(logger.Logger, 0)zetaclient/chains/sui/signer/signer_tracker.go (1)
12-61: Well-structured transaction tracking implementationThe
reportOutboundTrackerfunction employs effective timeout and retry mechanisms with proper error handling. The flag system to prevent duplicate reports is a good safeguard.Consider extracting the timeout and interval constants to package-level variables if they may be reused elsewhere in the package:
- // approx Sui checkpoint interval - const interval = 3 * time.Second - - // some sanity timeout - const maxTimeout = time.Minute + // defined at package level + // const suiCheckpointInterval = 3 * time.Second + // const maxTrackingTimeout = time.MinuteAlso, enhance error context by wrapping errors with additional information:
- return errors.Errorf("timeout reached (%s)", maxTimeout.String()) + return errors.Errorf("timeout reached (%s) while tracking tx %s for nonce %d", + maxTimeout.String(), digest, nonce)pkg/contracts/sui/withdrawal.go (1)
24-73: Consider refactoring repetitive parsing logic.The parsing function follows a repetitive pattern of extracting fields and checking for errors. This could be refactored for more concise code.
You could define a helper function to reduce repetition:
+func extractAndParse[T any]( + parsedJSON map[string]any, + field string, + parser func(string) (T, error), + errorMsg string, +) (T, error) { + var zero T + raw, err := extractStr(parsedJSON, field) + if err != nil { + return zero, err + } + + val, err := parser(raw) + if err != nil { + return zero, errors.Wrap(err, errorMsg) + } + + return val, nil +}Then use it to simplify the parsing code:
- amountRaw, err := extractStr(parsedJSON, "amount") - if err != nil { - return WithdrawData{}, err - } - - amount, err := math.ParseUint(amountRaw) - if err != nil { - return WithdrawData{}, errors.Wrap(err, "unable to parse amount") - } + amount, err := extractAndParse(parsedJSON, "amount", math.ParseUint, "unable to parse amount") + if err != nil { + return WithdrawData{}, err + }zetaclient/chains/sui/signer/signer_withdrawcap.go (2)
41-59: Be aware of potential concurrent fetch scenarios.
If multiple goroutines callgetWithdrawCapIDCachedaround the same time when the cache is invalid, each might initiate a fetch. Consider a “double-checked locking” approach or an atomic flag to avoid redundant fetches.
61-72: Consider adding test coverage for 'getWithdrawCapID'.
Proactively create unit tests verifying correct error wrapping and the returned object ID in normal and edge cases (e.g., no owned objects).zetaclient/chains/sui/sui.go (2)
28-34: Consider making 'outboundLookbackFactor' configurable.
Hardcoding the factor to 1.0 might be too rigid for some scenarios. Using a configurable value could allow future flexibility without changing the code.
124-204: Complex scheduling logic - good approach, but scrutinize potential edge cases.
- You use
time.Sleep(delay)before listing pending CCTX. Ensure that sleeping does not inadvertently mask time-sensitive tasks.- The logic in the
forloop accounts for skipping CCTX based onnoncechecks, but double-check concurrency withProcessCCTX.- Updating the chain params immediately before scheduling is a nice move—just ensure that partial updates or error paths do not lock the system out of future scheduling attempts.
Otherwise, your outward flow looks coherent, withVoteOutboundin the error path to keep the ledger informed.zetaclient/chains/sui/client/client.go (1)
133-156: Add dedicated tests to ensure retrieving a single object ID behaves correctly.
The code logic looks good, but static analysis indicates no coverage. Testing both the “0 objects found” and “multiple objects found” error cases, as well as success scenarios, will boost reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 135-152: zetaclient/chains/sui/client/client.go#L135-L152
Added lines #L135 - L152 were not covered by tests
[warning] 155-155: zetaclient/chains/sui/client/client.go#L155
Added line #L155 was not covered by testszetaclient/chains/sui/signer/signer_tx.go (3)
19-23: Validate nil references before usage.
Kindly confirm that neithercctxnorparamsis evernil, as these are immediately dereferenced. A precautionary check would make the function more robust.
24-40: Validate coin types more comprehensively.
The switch statement outlines valid coin types, but adding explicit logging or metrics for unsupported coin types can help in debugging future issues.
67-85: Enhance error handling for broadcast failures.
Consider explicitly retrying or adding structured logging ifSuiExecuteTransactionBlockreturns known transient errors.zetaclient/chains/sui/observer/observer.go (2)
105-106: Update gas price after validation.
SettinglatestGasPricehere is logical; consider logging the new price to aid in operational visibility.
122-126: Write-lock usage aligns well.
This method ensures correct concurrency control; consider adding an optional upper-bound check on gas price before assignment if applicable.zetaclient/chains/sui/signer/signer_test.go (1)
25-102: Comprehensive coverage
The sub-test “ProcessCCTX” covers the typical success path well. Consider adding negative test scenarios (e.g., invalid or missingcctxfields) to validate error-handling logic.zetaclient/chains/sui/observer/observer_test.go (4)
27-27: Empty map usage
someArgStubis a suitable placeholder for dynamic test arguments. Ensure it’s used consistently or consider populating it for clarity.
140-140: Embedded offset
Storing the offset,0within the same string works but can reduce clarity. Consider tracking the offset separately for maintainability.
227-289: ProcessOutboundTrackers
Test coverage is thorough. Including a broken scenario (e.g., missing OutboundTracker or incomplete Sui TX) would improve resilience.
291-370: VoteOutbound scenario
The test efficiently checks final statuses and block heights. Adding an edge-case test (e.g., invalid event fields) can enhance reliability.pkg/contracts/sui/crypto.go (3)
20-34: Digest function
Implementation aligns with Sui’s cryptographic guidelines for the initial Blake2b hashing. Consider including unit tests that validate boundary conditions.
67-87: Signature serialization
This logic follows the required R|S format, omitting the V byte. Confirm usage across the code aligns with Sui’s spec.
119-156: SignerSecp256k1
Double-hashing for ECDSA compliance is well-implemented. Consider detecting invalid private keys (empty or malformed) for safer error handling.zetaclient/chains/sui/signer/signer.go (3)
3-16: Imports look consistent and well-structured.
All newly introduced imports, includingsha256, align with the current functionality. Also note thatcf-workers-proxy-enn.pages.dev/pkg/errorsis used widely, but consider migrating to the Go standard library’s error wrapping features for a unified approach.
21-26: Document newly added fields for clarity.
The fieldsclient,gateway,withdrawCap, andzetacoreappear essential for Sui-specific logic. For maintainability, consider adding inline comments or docstrings on how each is intended to be used, especially since they hold pointers.
44-57: Constructor sets up needed fields successfully.
TheNewfunction thoroughly initializes each field. IfwithdrawCapis not used in certain scenarios, consider deferring its creation or documenting why it is always allocated.pkg/contracts/sui/crypto_test.go (2)
15-43: Digest sub-test offers solid coverage.
Verifying the digest against an expected base64 output is a strong approach. Consider adding negative scenarios (e.g., malformedTxBytes) to robustly test error handling.
45-80: Address derivation logic is verified thoroughly.
Tests confirm that decompression and expected address values match. To strengthen confidence, you could add tests for invalid or corrupted public keys to ensure graceful failure.pkg/contracts/sui/gateway.go (2)
56-59: Constructor for Gateway is straightforward.
This is sufficiently minimal. If additional validation of IDs is needed, consider consolidating it here.
74-82: Robust deposit data extraction.
The cast toDepositDataensures typed content. Consider also validating content fields for completeness.zetaclient/chains/sui/observer/outbound.go (4)
28-33: Consider adding coverage for memory checks.
Even though the concurrency usage ofob.getTx(nonce)is safe, adding test cases for scenarios where the transaction does not exist or is invalid would improve robustness and maintain confidence inOutboundCreated.I can provide sample tests if needed to verify memory lookups in varied conditions.
35-77: Enhance test coverage for error paths.
Error returns (e.g. emptyHashListor failure to get cctx) are not covered by unit tests. Testing these code paths ensures the observer recovers gracefully instead of silently failing.I can help by outlining test scenarios that induce each error condition for thorough validation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-43: zetaclient/chains/sui/observer/outbound.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 50-51: zetaclient/chains/sui/observer/outbound.go#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 55-55: zetaclient/chains/sui/observer/outbound.go#L55
Added line #L55 was not covered by tests
[warning] 62-63: zetaclient/chains/sui/observer/outbound.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 66-73: zetaclient/chains/sui/observer/outbound.go#L66-L73
Added lines #L66 - L73 were not covered by tests
79-163: Expand coverage forVoteOutbounderror handling.
Multiple branches (missing transaction, checkpoint parse failure, zero gas price, post vote failure) lack explicit test coverage. Without test scenarios exercising these branches, unexpected behaviors could surface in production.I can assist in creating negative test cases to ensure each error path is well-handled.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-90: zetaclient/chains/sui/observer/outbound.go#L89-L90
Added lines #L89 - L90 were not covered by tests
[warning] 95-96: zetaclient/chains/sui/observer/outbound.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 111-112: zetaclient/chains/sui/observer/outbound.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 115-116: zetaclient/chains/sui/observer/outbound.go#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 128-129: zetaclient/chains/sui/observer/outbound.go#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 133-134: zetaclient/chains/sui/observer/outbound.go#L133-L134
Added lines #L133 - L134 were not covered by tests
[warning] 157-158: zetaclient/chains/sui/observer/outbound.go#L157-L158
Added lines #L157 - L158 were not covered by tests
271-290: Perform type-safe parsing forvalue.
Relying onraw["value"].(string)without validating the type can lead to runtime panics if the JSON shape changes. Creating a small helper that checks for the string type before parsing would make the code more robust.func parseNonceFromWithdrawInputs(inputs []models.SuiCallArg) (uint64, error) { + val, ok := raw["value"].(string) + if !ok { + return 0, errors.New("invalid nonce format; expected string") + } return strconv.ParseUint(val, 10, 64) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 273-274: zetaclient/chains/sui/observer/outbound.go#L273-L274
Added lines #L273 - L274 were not covered by tests
[warning] 286-287: zetaclient/chains/sui/observer/outbound.go#L286-L287
Added lines #L286 - L287 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
cmd/zetaclientd/start.go(1 hunks)e2e/config/config.go(2 hunks)e2e/runner/setup_sui.go(2 hunks)e2e/utils/sui/signer.go(0 hunks)e2e/utils/sui/signer_test.go(0 hunks)pkg/contracts/sui/crypto.go(1 hunks)pkg/contracts/sui/crypto_test.go(1 hunks)pkg/contracts/sui/deposit.go(3 hunks)pkg/contracts/sui/gateway.go(7 hunks)pkg/contracts/sui/gateway_test.go(5 hunks)pkg/contracts/sui/withdrawal.go(1 hunks)pkg/scheduler/scheduler.go(3 hunks)pkg/scheduler/scheduler_test.go(2 hunks)zetaclient/chains/sui/client/client.go(2 hunks)zetaclient/chains/sui/client/client_live_test.go(1 hunks)zetaclient/chains/sui/observer/inbound.go(3 hunks)zetaclient/chains/sui/observer/observer.go(5 hunks)zetaclient/chains/sui/observer/observer_test.go(8 hunks)zetaclient/chains/sui/observer/outbound.go(1 hunks)zetaclient/chains/sui/signer/signer.go(1 hunks)zetaclient/chains/sui/signer/signer_test.go(1 hunks)zetaclient/chains/sui/signer/signer_tracker.go(1 hunks)zetaclient/chains/sui/signer/signer_tx.go(1 hunks)zetaclient/chains/sui/signer/signer_withdrawcap.go(1 hunks)zetaclient/chains/sui/sui.go(5 hunks)zetaclient/orchestrator/v2_bootstrap.go(1 hunks)zetaclient/orchestrator/v2_bootstrap_test.go(1 hunks)zetaclient/orchestrator/v2_orchestrator_test.go(1 hunks)zetaclient/testutils/constant.go(1 hunks)zetaclient/testutils/mocks/sui_client.go(3 hunks)zetaclient/testutils/mocks/sui_gen.go(2 hunks)zetaclient/tss/crypto.go(4 hunks)
💤 Files with no reviewable changes (2)
- e2e/utils/sui/signer.go
- e2e/utils/sui/signer_test.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/orchestrator/v2_orchestrator_test.gocmd/zetaclientd/start.gozetaclient/testutils/constant.gozetaclient/chains/sui/client/client_live_test.gopkg/contracts/sui/gateway_test.gozetaclient/orchestrator/v2_bootstrap_test.gozetaclient/chains/sui/signer/signer_tracker.gozetaclient/tss/crypto.gozetaclient/testutils/mocks/sui_gen.gozetaclient/orchestrator/v2_bootstrap.gopkg/contracts/sui/deposit.gopkg/scheduler/scheduler.goe2e/config/config.gozetaclient/chains/sui/sui.gozetaclient/chains/sui/signer/signer_test.gopkg/scheduler/scheduler_test.gopkg/contracts/sui/withdrawal.gozetaclient/testutils/mocks/sui_client.gozetaclient/chains/sui/signer/signer.gozetaclient/chains/sui/observer/observer.gopkg/contracts/sui/gateway.gozetaclient/chains/sui/observer/observer_test.gozetaclient/chains/sui/signer/signer_withdrawcap.gozetaclient/chains/sui/observer/outbound.gozetaclient/chains/sui/client/client.goe2e/runner/setup_sui.gozetaclient/chains/sui/observer/inbound.gopkg/contracts/sui/crypto_test.gopkg/contracts/sui/crypto.gozetaclient/chains/sui/signer/signer_tx.go
🧠 Learnings (1)
pkg/contracts/sui/crypto.go (1)
Learnt from: gartnera
PR: zeta-chain/node#3485
File: e2e/utils/sui/signer.go:24-35
Timestamp: 2025-02-06T16:29:58.925Z
Learning: The Sui blockchain requires signatures in [R || S || V] format, which is best generated using go-ethereum's crypto.Sign function that takes an ecdsa.PrivateKey.
🪛 GitHub Check: codecov/patch
pkg/scheduler/scheduler.go
[warning] 74-75: pkg/scheduler/scheduler.go#L74-L75
Added lines #L74 - L75 were not covered by tests
zetaclient/chains/sui/observer/outbound.go
[warning] 42-43: zetaclient/chains/sui/observer/outbound.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 50-51: zetaclient/chains/sui/observer/outbound.go#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 55-55: zetaclient/chains/sui/observer/outbound.go#L55
Added line #L55 was not covered by tests
[warning] 62-63: zetaclient/chains/sui/observer/outbound.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 66-73: zetaclient/chains/sui/observer/outbound.go#L66-L73
Added lines #L66 - L73 were not covered by tests
[warning] 89-90: zetaclient/chains/sui/observer/outbound.go#L89-L90
Added lines #L89 - L90 were not covered by tests
[warning] 95-96: zetaclient/chains/sui/observer/outbound.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 111-112: zetaclient/chains/sui/observer/outbound.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 115-116: zetaclient/chains/sui/observer/outbound.go#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 128-129: zetaclient/chains/sui/observer/outbound.go#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 133-134: zetaclient/chains/sui/observer/outbound.go#L133-L134
Added lines #L133 - L134 were not covered by tests
[warning] 157-158: zetaclient/chains/sui/observer/outbound.go#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 177-178: zetaclient/chains/sui/observer/outbound.go#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 181-182: zetaclient/chains/sui/observer/outbound.go#L181-L182
Added lines #L181 - L182 were not covered by tests
[warning] 198-199: zetaclient/chains/sui/observer/outbound.go#L198-L199
Added lines #L198 - L199 were not covered by tests
[warning] 203-204: zetaclient/chains/sui/observer/outbound.go#L203-L204
Added lines #L203 - L204 were not covered by tests
[warning] 207-208: zetaclient/chains/sui/observer/outbound.go#L207-L208
Added lines #L207 - L208 were not covered by tests
[warning] 211-212: zetaclient/chains/sui/observer/outbound.go#L211-L212
Added lines #L211 - L212 were not covered by tests
[warning] 216-217: zetaclient/chains/sui/observer/outbound.go#L216-L217
Added lines #L216 - L217 were not covered by tests
[warning] 220-221: zetaclient/chains/sui/observer/outbound.go#L220-L221
Added lines #L220 - L221 were not covered by tests
[warning] 231-232: zetaclient/chains/sui/observer/outbound.go#L231-L232
Added lines #L231 - L232 were not covered by tests
[warning] 236-242: zetaclient/chains/sui/observer/outbound.go#L236-L242
Added lines #L236 - L242 were not covered by tests
[warning] 273-274: zetaclient/chains/sui/observer/outbound.go#L273-L274
Added lines #L273 - L274 were not covered by tests
[warning] 286-287: zetaclient/chains/sui/observer/outbound.go#L286-L287
Added lines #L286 - L287 were not covered by tests
[warning] 297-298: zetaclient/chains/sui/observer/outbound.go#L297-L298
Added lines #L297 - L298 were not covered by tests
zetaclient/chains/sui/client/client.go
[warning] 135-152: zetaclient/chains/sui/client/client.go#L135-L152
Added lines #L135 - L152 were not covered by tests
[warning] 155-155: zetaclient/chains/sui/client/client.go#L155
Added line #L155 was not covered by tests
[warning] 160-160: zetaclient/chains/sui/client/client.go#L160
Added line #L160 was not covered by tests
[warning] 169-169: zetaclient/chains/sui/client/client.go#L169
Added line #L169 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (80)
cmd/zetaclientd/start.go (1)
136-136:✅ Verification successful
Verify scheduler behavior with zero default interval.
The scheduler is initialized with a default interval of 0, which could be problematic if the scheduler implementation doesn't properly handle this special case.
🏁 Script executed:
#!/bin/bash # Check how the default interval is used in the scheduler implementation rg -A 10 "func New\(" pkg/scheduler/scheduler.go rg "defaultInterval" pkg/scheduler/scheduler.goLength of output: 657
Verified Default Interval Handling
The scheduler implementation properly checks if the provided interval is less than or equal to zero and substitutes it with a 10-second interval. Passing
0toscheduler.New(as intaskScheduler := scheduler.New(logger.Std, 0)) is safe and will result in an initialized scheduler with a default interval of 10 seconds. No changes are needed.zetaclient/orchestrator/v2_bootstrap_test.go (1)
79-84: Consistent test isolation pattern.The approach to disable other chains is good for test isolation. This pattern follows the same approach used in other test cases like the Solana test.
e2e/runner/setup_sui.go (3)
26-26: Variable name change improves clarityThe renaming from
publishReqtopublishTxbetter reflects that this is a transaction block rather than a request, enhancing code readability.
37-37: Method name refactoring enhances consistencyThe change from
SignTransactionBlocktoSignTxBlockfollows a more concise naming convention while maintaining semantic clarity. This is a positive improvement for API consistency.
41-41: Consistent variable usageThe updated usage of
publishTx.TxBytesproperly reflects the variable renaming and maintains functional equivalence.pkg/contracts/sui/gateway_test.go (6)
18-18: Addition of gatewayID constant improves test configurationThe addition of this constant enhances the test configurability and aligns with the changes to the
NewGatewayfunction signature.
27-27: Updated function call with additional parameterThe updated function call properly includes the new
gatewayIDparameter, reflecting changes in the function signature ofNewGateway.
57-66: API refinement from Inbound to DepositThe change from
out.Inbound()toout.Deposit()represents a semantic improvement, using more specific terminology that better describes the actual operation.
88-97: Consistent use of refined APIThe renamed method is consistently used throughout the test suite, maintaining coherence with the architectural changes.
119-128: Uniform API usage patternThe continued consistent usage of the renamed API method enhances code readability and maintains uniformity across the test suite.
131-158: Comprehensive test coverage for withdrawal functionalityThe addition of a test case for "withdraw" events ensures proper coverage of the new withdrawal functionality, which is essential for maintaining code quality and preventing regressions.
zetaclient/chains/sui/signer/signer_tracker.go (1)
63-67: Concise tracker vote posting implementationThe
postTrackerVotemethod is appropriately focused on a single responsibility and includes a helpful comment explaining that transaction success status is irrelevant at this stage.zetaclient/tss/crypto.go (3)
91-93: Useful accessor method for ECDSA public keyThe addition of the
AsECDSA()method provides a clean way to access the underlying ECDSA public key, facilitating operations that require direct access to this key.
103-103: Enhanced public key compression methodThe switch from
elliptic.MarshalCompressedtocrypto.CompressPubkeyleverages the Ethereum crypto package's functionality, which may offer additional benefits or consistency with other parts of the codebase.
140-142: Support for Sui blockchain addressesThe addition of the
AddressSui()method extends thePubKeystruct's capabilities to support Sui blockchain operations, which is essential for implementing SUI withdrawals as specified in the PR objectives.zetaclient/chains/sui/client/client_live_test.go (1)
97-118:❓ Verification inconclusive
Good test structure with clear ARRANGE-ACT-ASSERT pattern
The test case for
GetOwnedObjectIDfollows best practices with a clear separation of test phases and good comments explaining the purpose of each section.However, I do have some concerns about the test's long-term maintainability:
- // Given admin wallet us Cetus DEX team + // Given admin wallet of Cetus DEX teamAlso, consider making this test more robust against future changes:
- The test relies on a third-party wallet (Cetus DEX) that might change
- The hardcoded object ID might need updating if the referenced object changes
🏁 Script executed:
#!/bin/bash # Verify if the object ID still exists and matches the expected type curl -s "https://sui-mainnet.public.blastapi.io" \ -H "Content-Type: application/json" \ -d '{ "jsonrpc": "2.0", "id": 1, "method": "sui_getObject", "params": [ "0x89c1a321291d15ddae5a086c9abc533dff697fde3d89e0ca836c41af73e36a75", { "showType": true, "showOwner": true } ] }' | jq '.result.data | {owner: .owner, type: .type}'Length of output: 564
Attention: Maintain Test Clarity & Future Robustness
The test for
GetOwnedObjectIDis well-structured with a clear ARRANGE-ACT-ASSERT pattern, and our verification confirms that the hardcoded object ID currently returns the expected owner and type. However, note the following points for long-term maintainability:
- Update the inline comment to correct the typo:
- // Given admin wallet us Cetus DEX team + // Given admin wallet of Cetus DEX team- The test relies on a specific third-party wallet (Cetus DEX), and the hardcoded object ID may become outdated if the wallet details or the referenced object change. Consider parameterizing these values or decoupling them from the test (e.g., through configuration or mocking) to safeguard against future changes.
pkg/scheduler/scheduler_test.go (2)
37-40: Updated test assertion to match new behaviorThe comment and assertion have been properly updated to reflect the new behavior where the task runs at start and after 1 second interval.
364-364: Scheduler initialization updated to use new parameterThe test suite now initializes the scheduler with a 1-second default interval, which aligns with the updated constructor signature.
pkg/scheduler/scheduler.go (2)
22-27: Improved struct field ordering and added configurable default intervalThe Scheduler struct fields have been reordered for better readability, and a new
defaultIntervalfield has been added to make the scheduler more configurable.
96-96: Using configurable default interval instead of hardcoded valueThe
Registermethod now properly uses the configureddefaultIntervalinstead of a hardcoded value, which is more flexible and consistent with the new design.zetaclient/testutils/mocks/sui_gen.go (2)
21-21: Added interface method for retrieving owned object IDsThe new
GetOwnedObjectIDmethod extends the interface to support retrieving object IDs by owner and struct type, which aligns with the PR objectives for SUI withdrawals.
30-34: Added transaction execution methods to support withdrawalsThese new interface methods (
MoveCallandSuiExecuteTransactionBlock) provide the necessary functionality for building and executing transactions for SUI withdrawals.These additions complete the necessary interface for implementing the withdrawal functionality described in the PR objectives.
e2e/config/config.go (2)
18-18: Clean import update.The import has been appropriately updated to use the centralized package for Sui contracts rather than a utility package, which aligns with better code organization.
406-413: Improved signer implementation.The SuiSigner method has been refactored to use the consolidated signing functionality from the centralized Sui package. This change simplifies the implementation and eliminates code duplication.
pkg/contracts/sui/withdrawal.go (2)
11-18: Well-structured data model for withdrawal events.The WithdrawData struct clearly captures all necessary fields for a Sui withdrawal event, following the same pattern as other event data models in the codebase.
20-22: Simple utility method for gas identification.The IsGas method provides a clean way to identify gas coin transactions.
pkg/contracts/sui/deposit.go (3)
10-11: Good semantic renaming of struct.Renaming from Inbound to DepositData makes the purpose clearer and better represents the data structure's functionality.
21-21: Consistent method renaming.The method name change from IsGasDeposit to IsGas aligns with the struct's new name and creates consistency with the WithdrawData implementation.
25-25: Function name now better reflects its purpose.Renaming parseInbound to parseDeposit provides clarity about the function's specific role in handling deposit events.
zetaclient/orchestrator/v2_bootstrap.go (3)
203-207: Improved gateway initialization with clear documentation.The comment clearly indicates the expected format for the gateway address, and the code now uses the appropriate constructor function that accepts this format.
211-214: Proper sequencing of operations.The base observer is now created after the gateway, ensuring dependencies are properly established before they're needed.
218-218: Enhanced signer initialization with required dependencies.The signer creation now includes all necessary dependencies (client, gateway, and core), which aligns with the PR's goal of enhancing SUI withdrawal functionality.
zetaclient/chains/sui/signer/signer_withdrawcap.go (3)
13-21: Well-structured concurrency usage in this struct.
You’ve wrapped the data in a setup that correctly uses a mutex and timestamp to handle cache validity, making it easy to reason about.
22-31: Good thread-safe implementation of 'valid'.
Locking withRLockensures safe concurrent reads, and the TTL comparison is straightforward.
33-40: Efficient synchronization in 'set'.
Using the write lock before updating state avoids race conditions. This is concise and clear.zetaclient/chains/sui/sui.go (4)
85-87: Verify that 'OutboundTicker' is non-zero to avoid potential zero-interval scheduling.
IfOutboundTickeris zero, the scheduler might spin continuously or never run. Confirm that this configuration is set appropriately.
105-105: Skipping comment.
No real changes apart from job registration.
206-227: Appropriate place for chain param updates.
Since these updates can affect the entire scheduling logic downstream, the centralized approach reduces confusion. Ensure no concurrency conflicts arise if this is called repeatedly in overlapping intervals.
229-233: Well-structured 'outboundLogger'.
Attaching the outbound ID in the context is a concise way to track logs for each transaction flow.zetaclient/chains/sui/client/client.go (2)
160-160: Consider verifying the delimiter change in all dependent code.
Switching from ‘#’ to ‘,’ might break existing consumers if they rely on the old format. Check references toEncodeCursorto confirm no regressions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: zetaclient/chains/sui/client/client.go#L160
Added line #L160 was not covered by tests
169-169: Handle legacy cursor formats or confirm no backward compatibility issues.
The updatedstrings.Split(cursor, ",")might fail for older cursors. If backward compatibility is necessary, handle both delimiters gracefully or ensure old cursors don’t exist in the wild.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 169-169: zetaclient/chains/sui/client/client.go#L169
Added line #L169 was not covered by testszetaclient/chains/sui/signer/signer_tx.go (2)
49-53: Confirm concurrency behavior for cached data.
Ifs.getWithdrawCapIDCachedreturns shared data, consider whether concurrent calls require locks or synchronization to avoid race conditions.
54-64: Check numeric conversion safety.
Parameters likegasBudgetare derived viastrconv.FormatUint; ensure these values remain within Sui's allowed gas limits.zetaclient/chains/sui/observer/inbound.go (1)
162-201: Verify deposit extraction correctness.
Switching frominboundtodepositis consistent with the broader refactoring. Confirm that all dependencies (e.g., deposit object, cross-chain call flags) remain intact.zetaclient/chains/sui/observer/observer.go (2)
24-29: Ensure thread-safe access to txMap.
UsingtxMapwithtxMuis a good start. Confirm all interactions, including reads and writes, occur under the lock.
115-120: Read-lock usage looks clean.
This locking pattern is straightforward and safe, ensuring minimal overhead for gas price reads.zetaclient/testutils/mocks/sui_client.go (3)
50-76: Stable mock setup
These lines follow the expected usage of testify's mocking. The function structure is conventional and should be reliable.
106-132: No improvements needed
The mocking approach is consistent, and argument handling appears correct for MoveCall.
171-197: Mock structure looks good
SuiExecuteTransactionBlock uses standard testify pattern for returning mock values, with no obvious issues.zetaclient/chains/sui/signer/signer_test.go (7)
104-119: Well-structured testSuite
ThetestSuitestruct offers a clear blueprint for managing mocks and test dependencies.
120-159: Solid overall setup
No issues identified. Environment initialization is straightforward and aligns with best practices.
161-164: Mock helper
Consistently follows the testify approach to define custom return values.
166-174: Adequate coverage
The mocked MoveCall helper is succinct and effectively captures arguments for verification.
175-185: MockExec is appropriate
This function aligns with the style and usage in the rest of the codebase, returning well-defined mock responses.
187-190: Minimal and clear
ThetestTrackerstruct is straightforward, with no apparent improvements needed.
192-211: Tracker bag logic
This function nicely appends newly identified trackers. The approach is simple yet effective.zetaclient/chains/sui/observer/observer_test.go (1)
91-91: Cursor separator check
The change to comma-based cursor"ABC123_first_tx,0"should be verified in related logic to avoid unexpected mismatches with older cursor formats.pkg/contracts/sui/crypto.go (4)
36-41: Static intent
defaultIntentcorrectly encodes the standard Sui transaction scope/version/app ID.
42-50: Prefix logic
messageWithIntentPrefixsystematically prepends the default intent. No issues identified.
52-65: Sui address derivation
Combining secp256k1 flag and compressed pubKey is consistent with official Sui documentation.
89-117: Signature parsing
Validation steps and error messages are methodical, ensuring easier debugging.zetaclient/chains/sui/signer/signer.go (2)
28-41: RPC interface is cohesive.
Having a typed interface with context-based calls clarifies usage and error handling. Ensure all implementations handle context cancellation properly to avoid lengthy blocking operations.
106-124: Signature flow is well-aligned with Sui’s ECDSA requirements.
Double-hashing with SHA-256 for Sui transactions appears correct per the documentation. The step to unify TSS signature logic underTSS().Sign()is also clear.pkg/contracts/sui/crypto_test.go (3)
1-13: Test package and imports are well-organized.
Usingtestify/assertandrequireis an effective choice for streamlined test assertions.
82-112: Serialization and deserialization of ECDSA signatures is validated.
The test is comprehensive, checking signature length and re-validating the public key. This ensures correct round-tripping.
114-159: Secp256k1 signer tests are well-structured.
Private key handling and final address checks look correct. Good job verifying sign operations do not fail.pkg/contracts/sui/gateway.go (12)
21-28: Added concurrency control with RWMutex.
TheGatewaystruct’smueffectively guardspackageIDandobjectID. Ensure that all getters and updaters consistently acquire locks to maintain data integrity.
45-54: New constructor promotes structured input.
NewGatewayFromPairIDconveniently parses the comma-separated pair to setpackageIDandobjectID. Handle unexpected formats gracefully, as you already do withparsePair.
70-72:IsDepositcheck fosters clarity.
This explicit method clarifies event intention and ensures easy identification of deposit-type events.
84-86:IsWithdrawelegantly segregates withdraw events.
No issues noted; simple and direct.
88-96: Withdrawal data retrieval is consistent.
Similar toDeposit(), the type assertion is well-structured. Ensure that external code sets the correct event content.
98-101: Thread-safe access forPackageID().
Good use of read lock to prevent data races onpackageID.
105-110: Thread-safe getter forObjectID().
Matches the read-lock pattern used inPackageID(). No concerns here.
117-120:WithdrawCapType()uses existingPackageID().
Constructing the string with module name is straightforward.
122-136:UpdateIDsensures concurrency safety.
The function properly locks before modifying both fields. Also, returning an explicit error when parsing fails is beneficial for debugging.
179-182: Withdrawal event parsing integrated with deposit logic.
The switch statement systematically handles recognized event types. This keeps the code well-organized for future expansions.
200-223:ParseTxWithdrawalgracefully handles missing or invalid events.
Filtering out older events or lacking events with explicit error returns helps identify upstream data issues. The function’s chain of calls (includingevent.Withdrawal()) is coherent.
275-283:parsePairproperly extracts package and gateway IDs.
Error messaging is clear if the format is invalid.zetaclient/chains/sui/observer/outbound.go (2)
189-224: Validate multiple signatures and improve coverage.
Currently, only the first signature inTxSignaturesis checked. Should additional signatures appear, clarifying how they are handled or rejected might help. Also, many validation failure paths remain untested.Please confirm whether multi-signature scenarios are expected and, if so, consider extending the logic or clarifying that only one signature is allowed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 198-199: zetaclient/chains/sui/observer/outbound.go#L198-L199
Added lines #L198 - L199 were not covered by tests
[warning] 203-204: zetaclient/chains/sui/observer/outbound.go#L203-L204
Added lines #L203 - L204 were not covered by tests
[warning] 207-208: zetaclient/chains/sui/observer/outbound.go#L207-L208
Added lines #L207 - L208 were not covered by tests
[warning] 211-212: zetaclient/chains/sui/observer/outbound.go#L211-L212
Added lines #L211 - L212 were not covered by tests
[warning] 216-217: zetaclient/chains/sui/observer/outbound.go#L216-L217
Added lines #L216 - L217 were not covered by tests
[warning] 220-221: zetaclient/chains/sui/observer/outbound.go#L220-L221
Added lines #L220 - L221 were not covered by tests
248-269: Good concurrency approach for transaction storage.
The read-write locks ingetTx,setTx, andunsetTxcorrectly protect the sharedtxMap. This design prevents data races and ensures consistent state. No immediate changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (22)
cmd/zetaclientd/start.go (1)
136-136: Default interval parameter requires clarification.The addition of
0as the default interval parameter lacks context. While functional, this could benefit from a named constant or comment explaining the significance of the zero value in this context.Consider using a named constant for improved self-documentation:
-taskScheduler := scheduler.New(logger.Std, 0) +const defaultSchedulerInterval time.Duration = 0 // Use system default interval +taskScheduler := scheduler.New(logger.Std, defaultSchedulerInterval)zetaclient/testutils/constant.go (1)
47-47: The Gateway address format change needs documentation.The value has been updated to use a comma-separated format containing two addresses (
$packageID,$gatewayObjectID), but this change in format is not documented in the code comments. This modification aligns with the PR objective of adding Gateway's object intochainParams.GatewayAddress.Consider adding a comment that explains the new format:
- // stub, will be replaced with real address later + // Format: $packageID,$gatewayObjectID - where the second part is the Gateway's WithdrawCap object chains.SuiMainnet.ChainId: "0x5d4b302506645c37ff133b98fff50a5ae14841659738d6d733d59d0d217a9fff,0xffff302506645c37ff133b98fff50a5ae14841659738d6d733d59d0d217a9aaa",pkg/contracts/sui/withdrawal.go (1)
24-73: Comprehensive withdrawal event parsing with robust error handling.The
parseWithdrawalfunction follows best practices:
- Validates the event type upfront
- Extracts fields with proper error handling for each extraction
- Performs necessary type conversions with clear error wrapping
- Returns a well-formed WithdrawData struct
Consider adding some validation for extreme values, such as checking if the amount is within reasonable bounds, especially since the function is dealing with external data.
func parseWithdrawal(event models.SuiEventResponse, eventType EventType) (WithdrawData, error) { if eventType != Withdraw { return WithdrawData{}, errors.Errorf("invalid event type %q", eventType) } parsedJSON := event.ParsedJson coinType, err := extractStr(parsedJSON, "coin_type") if err != nil { return WithdrawData{}, err } amountRaw, err := extractStr(parsedJSON, "amount") if err != nil { return WithdrawData{}, err } amount, err := math.ParseUint(amountRaw) if err != nil { return WithdrawData{}, errors.Wrap(err, "unable to parse amount") } + + // Validate amount is within reasonable bounds + if amount.IsZero() { + return WithdrawData{}, errors.New("amount cannot be zero") + } sender, err := extractStr(parsedJSON, "sender") if err != nil { return WithdrawData{}, err } receiver, err := extractStr(parsedJSON, "receiver") if err != nil { return WithdrawData{}, err } nonceRaw, err := extractStr(parsedJSON, "nonce") if err != nil { return WithdrawData{}, err } nonce, err := strconv.ParseUint(nonceRaw, 10, 64) if err != nil { return WithdrawData{}, errors.Wrap(err, "unable to parse nonce") } return WithdrawData{ CoinType: CoinType(coinType), Amount: amount, Sender: sender, Receiver: receiver, Nonce: nonce, }, nil }zetaclient/orchestrator/v2_bootstrap.go (1)
203-207: Improve gateway creation comment style and error handling.The comment about the gateway address format is informative, but could follow Go standard documentation style.
- // note that gw address should be in format of `$packageID,$gatewayObjectID` + // Note: Gateway address should be in format of `$packageID,$gatewayObjectID`zetaclient/chains/sui/signer/signer_withdrawcap.go (3)
11-20: Consider making withdrawCapTTL configurable.The TTL is hardcoded to 5 minutes, which might not be optimal for all network conditions or withdrawal patterns.
Consider making this value configurable through a config parameter or environment variable to allow tuning without code changes.
41-59: Enhance cache expiration log message.The current log message is somewhat ambiguous.
- s.Logger().Std.Info().Msg("WithdrawCap cache expired, fetching new objectID") + s.Logger().Std.Info().Str("ttl", withdrawCapTTL.String()).Msg("WithdrawCap cache expired or not initialized, fetching new objectID")
61-72: Additional context in error messages for improved debugging.The error message could provide more context for easier debugging.
- return "", errors.Wrap(err, "unable to get owned object ID") + return "", errors.Wrapf(err, "unable to get owned object ID for TSS address %s with struct type %s", owner, structType)zetaclient/chains/sui/sui.go (2)
28-34: Use of Float-Based Lookback Factor
DefiningoutboundLookbackFactoras afloat64is flexible but introduces floating-point rounding concerns. If precise arithmetic is desired, consider using integers or rationals.
129-134: Blocking the Scheduler with time.Sleep
Usingtime.Sleep(delay)can stall the entire scheduling process. For production-grade concurrency, consider scheduling asynchronous or parallel tasks rather than pausing the loop if the delay is significant.zetaclient/chains/sui/client/client.go (1)
133-156: Add Unit Tests for GetOwnedObjectID
IntroducingGetOwnedObjectIDis helpful for retrieving object ownership. But from code coverage hints, lines 135-152 are not tested:
• Ensure you add a dedicated unit test to validate the “single object,” “none found,” and “multiple objects” scenarios.Would you like help creating a test suite that verifies each branch of this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 135-152: zetaclient/chains/sui/client/client.go#L135-L152
Added lines #L135 - L152 were not covered by tests
[warning] 155-155: zetaclient/chains/sui/client/client.go#L155
Added line #L155 was not covered by testspkg/contracts/sui/crypto_test.go (4)
15-44: Consider Adding Error Scenarios forDigestSubtestWhile the positive path is tested thoroughly by comparing the digest with the expected value, consider adding a negative test for malformed or empty
TxBytesto ensure robust error handling in theDigestfunction.
45-80: Positive Coverage forAddressFromPubKeyECDSAThis subtest provides solid verification for several known base64-encoded public keys. As an enhancement, consider adding a test case for invalid or corrupted public keys to ensure the function handles such cases gracefully.
82-113: Serialization and Deserialization Tests Are ComprehensiveThe test adequately checks both serialization and deserialization of ECDSA signatures. A minor improvement consideration is verifying erroneous input, such as truncated or badly formatted signatures, to fully validate error handling paths.
114-160: StrengthenSignerSecp256k1Test CoverageThe test checks addresses derived from known private keys and ensures sign operations do not fail. For increased confidence in the signing process, consider verifying the produced signature's validity using a library-based ECDSA verification, thereby providing end-to-end assurance.
zetaclient/chains/sui/signer/signer_tx.go (2)
17-65: ValidatebuildWithdrawalResults More RigorouslyThe function performs essential checks on chain ID and protocol version, and properly derives
coinType. However, consider adding validation or logging of the returnedTxnMetaDatafromMoveCallto confirm success or partial failures. Also, adding unit tests for unsupported coin types or unexpected inbound parameters can improve coverage.
67-85: Broadcast Logic is Straightforward But May Need Expanded LoggingThe function correctly serializes and broadcasts the transaction. As a refinement, consider logging the transaction digest or other metadata on success for easier debugging and traceability.
zetaclient/chains/sui/signer/signer_tracker.go (1)
63-67:postTrackerVoteShould Provide Additional VisibilityWhile posting the vote, consider logging success or returning richer data to confirm correct tracking. This helps operators monitor outbound trackers without diving into external logs.
pkg/contracts/sui/gateway.go (1)
276-283: String Pair Parsing Seems Reliable.
parsePairhandles the$packageID,$gatewayObjectIDformat well. Consider trimming whitespace if needed.zetaclient/chains/sui/signer/signer.go (1)
59-105: Comprehensive Outbound Steps inProcessCCTX.
This method concisely orchestrates building, signing, broadcasting, and asynchronous reporting. Consider adding a dedicated branch to handle duplicate broadcasts if concurrency among clients is common.zetaclient/chains/sui/observer/outbound.go (2)
49-51: Consider proceeding with other trackers even if a tracker has an empty hash list.
Currently, returning an error here stops the processing of subsequent trackers. If partial processing is acceptable in this scenario, you may want to log and skip the offending tracker instead.Example adjustment:
- if len(tracker.HashList) == 0 { - return errors.Errorf("empty outbound tracker hash for nonce %d", nonce) - } + if len(tracker.HashList) == 0 { + ob.Logger().Outbound.Warn(). + Uint64("nonce", nonce). + Msg("Skipping outbound tracker with empty hash list") + continue + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 50-51: zetaclient/chains/sui/observer/outbound.go#L50-L51
Added lines #L50 - L51 were not covered by tests
153-154: Follow up on the TODO for compliance checks.
Ensuring proper compliance checks is critical for production-grade code. If you require help implementing these checks, let me know.zetaclient/chains/sui/signer/signer_test.go (1)
25-27: Add negative test scenarios.
While the current test covers the happy path, consider adding negative tests for various error conditions (e.g., invalid nonce, failing MoveCall, failing transaction execution) to increase confidence in your error-handling logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
cmd/zetaclientd/start.go(1 hunks)e2e/config/config.go(2 hunks)e2e/runner/setup_sui.go(2 hunks)e2e/utils/sui/signer.go(0 hunks)e2e/utils/sui/signer_test.go(0 hunks)pkg/contracts/sui/crypto.go(1 hunks)pkg/contracts/sui/crypto_test.go(1 hunks)pkg/contracts/sui/deposit.go(3 hunks)pkg/contracts/sui/gateway.go(7 hunks)pkg/contracts/sui/gateway_test.go(5 hunks)pkg/contracts/sui/withdrawal.go(1 hunks)pkg/scheduler/scheduler.go(3 hunks)pkg/scheduler/scheduler_test.go(2 hunks)zetaclient/chains/sui/client/client.go(2 hunks)zetaclient/chains/sui/client/client_live_test.go(1 hunks)zetaclient/chains/sui/observer/inbound.go(3 hunks)zetaclient/chains/sui/observer/observer.go(5 hunks)zetaclient/chains/sui/observer/observer_test.go(8 hunks)zetaclient/chains/sui/observer/outbound.go(1 hunks)zetaclient/chains/sui/signer/signer.go(1 hunks)zetaclient/chains/sui/signer/signer_test.go(1 hunks)zetaclient/chains/sui/signer/signer_tracker.go(1 hunks)zetaclient/chains/sui/signer/signer_tx.go(1 hunks)zetaclient/chains/sui/signer/signer_withdrawcap.go(1 hunks)zetaclient/chains/sui/sui.go(5 hunks)zetaclient/orchestrator/v2_bootstrap.go(1 hunks)zetaclient/orchestrator/v2_bootstrap_test.go(1 hunks)zetaclient/orchestrator/v2_orchestrator_test.go(1 hunks)zetaclient/testutils/constant.go(1 hunks)zetaclient/testutils/mocks/sui_client.go(3 hunks)zetaclient/testutils/mocks/sui_gen.go(2 hunks)zetaclient/tss/crypto.go(4 hunks)
💤 Files with no reviewable changes (2)
- e2e/utils/sui/signer_test.go
- e2e/utils/sui/signer.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/chains/sui/client/client_live_test.gozetaclient/orchestrator/v2_orchestrator_test.gozetaclient/orchestrator/v2_bootstrap_test.gocmd/zetaclientd/start.gopkg/contracts/sui/crypto_test.gopkg/contracts/sui/deposit.gozetaclient/testutils/mocks/sui_gen.gopkg/scheduler/scheduler.gozetaclient/chains/sui/signer/signer_withdrawcap.gozetaclient/chains/sui/observer/inbound.gopkg/scheduler/scheduler_test.gozetaclient/chains/sui/signer/signer_tx.gopkg/contracts/sui/gateway_test.goe2e/config/config.gopkg/contracts/sui/withdrawal.gozetaclient/testutils/constant.gozetaclient/orchestrator/v2_bootstrap.gozetaclient/chains/sui/signer/signer_tracker.gozetaclient/chains/sui/client/client.gozetaclient/chains/sui/observer/observer.gozetaclient/testutils/mocks/sui_client.gopkg/contracts/sui/gateway.goe2e/runner/setup_sui.gopkg/contracts/sui/crypto.gozetaclient/chains/sui/observer/outbound.gozetaclient/chains/sui/observer/observer_test.gozetaclient/tss/crypto.gozetaclient/chains/sui/sui.gozetaclient/chains/sui/signer/signer_test.gozetaclient/chains/sui/signer/signer.go
🧠 Learnings (1)
pkg/contracts/sui/crypto.go (1)
Learnt from: gartnera
PR: zeta-chain/node#3485
File: e2e/utils/sui/signer.go:24-35
Timestamp: 2025-02-06T16:29:58.925Z
Learning: The Sui blockchain requires signatures in [R || S || V] format, which is best generated using go-ethereum's crypto.Sign function that takes an ecdsa.PrivateKey.
🪛 GitHub Check: codecov/patch
pkg/scheduler/scheduler.go
[warning] 74-75: pkg/scheduler/scheduler.go#L74-L75
Added lines #L74 - L75 were not covered by tests
zetaclient/chains/sui/client/client.go
[warning] 135-152: zetaclient/chains/sui/client/client.go#L135-L152
Added lines #L135 - L152 were not covered by tests
[warning] 155-155: zetaclient/chains/sui/client/client.go#L155
Added line #L155 was not covered by tests
[warning] 160-160: zetaclient/chains/sui/client/client.go#L160
Added line #L160 was not covered by tests
[warning] 169-169: zetaclient/chains/sui/client/client.go#L169
Added line #L169 was not covered by tests
zetaclient/chains/sui/observer/outbound.go
[warning] 42-43: zetaclient/chains/sui/observer/outbound.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 50-51: zetaclient/chains/sui/observer/outbound.go#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 55-55: zetaclient/chains/sui/observer/outbound.go#L55
Added line #L55 was not covered by tests
[warning] 62-63: zetaclient/chains/sui/observer/outbound.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 66-73: zetaclient/chains/sui/observer/outbound.go#L66-L73
Added lines #L66 - L73 were not covered by tests
[warning] 89-90: zetaclient/chains/sui/observer/outbound.go#L89-L90
Added lines #L89 - L90 were not covered by tests
[warning] 95-96: zetaclient/chains/sui/observer/outbound.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 111-112: zetaclient/chains/sui/observer/outbound.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 115-116: zetaclient/chains/sui/observer/outbound.go#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 128-129: zetaclient/chains/sui/observer/outbound.go#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 133-134: zetaclient/chains/sui/observer/outbound.go#L133-L134
Added lines #L133 - L134 were not covered by tests
[warning] 157-158: zetaclient/chains/sui/observer/outbound.go#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 177-178: zetaclient/chains/sui/observer/outbound.go#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 181-182: zetaclient/chains/sui/observer/outbound.go#L181-L182
Added lines #L181 - L182 were not covered by tests
[warning] 198-199: zetaclient/chains/sui/observer/outbound.go#L198-L199
Added lines #L198 - L199 were not covered by tests
[warning] 203-204: zetaclient/chains/sui/observer/outbound.go#L203-L204
Added lines #L203 - L204 were not covered by tests
[warning] 207-208: zetaclient/chains/sui/observer/outbound.go#L207-L208
Added lines #L207 - L208 were not covered by tests
[warning] 211-212: zetaclient/chains/sui/observer/outbound.go#L211-L212
Added lines #L211 - L212 were not covered by tests
[warning] 216-217: zetaclient/chains/sui/observer/outbound.go#L216-L217
Added lines #L216 - L217 were not covered by tests
[warning] 220-221: zetaclient/chains/sui/observer/outbound.go#L220-L221
Added lines #L220 - L221 were not covered by tests
[warning] 231-232: zetaclient/chains/sui/observer/outbound.go#L231-L232
Added lines #L231 - L232 were not covered by tests
[warning] 236-242: zetaclient/chains/sui/observer/outbound.go#L236-L242
Added lines #L236 - L242 were not covered by tests
[warning] 273-274: zetaclient/chains/sui/observer/outbound.go#L273-L274
Added lines #L273 - L274 were not covered by tests
[warning] 286-287: zetaclient/chains/sui/observer/outbound.go#L286-L287
Added lines #L286 - L287 were not covered by tests
[warning] 297-298: zetaclient/chains/sui/observer/outbound.go#L297-L298
Added lines #L297 - L298 were not covered by tests
🔇 Additional comments (83)
zetaclient/orchestrator/v2_bootstrap_test.go (1)
78-83: Proper test isolation for EVM chain configurations.The addition of these lines ensures proper isolation of the EVM test environment by explicitly disabling other chain configurations. This promotes test reliability by preventing potential cross-chain interference.
e2e/runner/setup_sui.go (1)
26-26: Improved variable naming for better semantic clarity.Renaming from
publishReqtopublishTxmore accurately reflects the variable's role as a transaction rather than just a request. The change is consistently applied throughout the function, enhancing code readability.Also applies to: 37-37, 41-41
zetaclient/orchestrator/v2_orchestrator_test.go (1)
126-126: Clear interval specification for test scheduler.Using
time.Secondas the default interval parameter provides clear intent in the test environment. This is consistent with the architecture changes to the scheduler initialization.zetaclient/chains/sui/client/client_live_test.go (1)
97-118: The test structure is well-designed and comprehensive.The new test case follows the ARRANGE-ACT-ASSERT pattern which is a robust testing practice. The constants are well documented with clear comments explaining their purpose, and the test includes proper assertions. The verification of object ID retrieval functionality against a live mainnet example is especially valuable.
e2e/config/config.go (1)
18-18: Clean refactoring to use consolidated implementation.The code has been appropriately updated to use the implementation from
pkg/contracts/suiinstead ofe2e/utils/sui. This change contributes to the PR objective of deduplicating ECDSA x Sui code that was previously spread across the codebase.Also applies to: 406-413
pkg/scheduler/scheduler.go (2)
22-27: Field addition enhances scheduler flexibility.Adding the
defaultIntervalfield to theSchedulerstruct provides better customizability for task scheduling across different use cases.
95-97: Proper usage of the new defaultInterval setting.The code correctly uses the scheduler's
defaultIntervalfield instead of the previously hardcoded value, maintaining consistent behavior with the added flexibility.zetaclient/tss/crypto.go (3)
91-93: Good addition of AsECDSA accessor method.This method provides a clean way to access the underlying ECDSA public key, which will be useful for the Sui integration.
103-103: Improved implementation using crypto.CompressPubkey.The switch to using
crypto.CompressPubkeyinstead of directly callingelliptic.MarshalCompressedenhances maintainability by leveraging established library functions.
139-142: Well-implemented Sui address derivation.The
AddressSui()method follows the same pattern as the existingAddressEVM()andAddressBTC()methods, providing a consistent interface for address derivation across different chains.pkg/scheduler/scheduler_test.go (2)
37-40: Updated test assertions to match new scheduler behavior.The comments and assertion correctly reflect the changed behavior where tasks are now executed once at start and once after the 1-second interval.
364-364: Added configurable default interval parameter.Good enhancement to make the scheduler more flexible by accepting a default interval parameter. This aligns with the PR objective of establishing scheduler boilerplate and customizable default intervals.
zetaclient/testutils/mocks/sui_gen.go (1)
21-21: Enhanced SuiClient interface with necessary methods for withdrawals.The addition of these methods to the mock interface properly aligns with the implementation requirements for Sui withdrawals. This ensures comprehensive testing capabilities for the new functionality.
Also applies to: 30-34
pkg/contracts/sui/withdrawal.go (1)
12-18: Well-structured WithdrawData struct.The struct properly captures all necessary fields for a Sui withdrawal event with appropriate types.
zetaclient/orchestrator/v2_bootstrap.go (1)
218-218: LGTM! Good refactoring of Signer initialization.The Signer now receives the proper dependencies (suiClient, gateway, Zetacore) needed for withdrawal functionality.
pkg/contracts/sui/deposit.go (3)
10-19: Well-structured DepositData refactoring.Good refactoring from
Inboundto the more explicitDepositDatatype, which clearly indicates the purpose of this structure.
21-23: Clean method renaming.Appropriate renaming from
IsGasDepositto the simplerIsGasthat aligns with the new struct name.
25-83: Good function refactoring with consistent error handling.The function renaming from
parseInboundtoparseDepositis consistent with the struct name change. All error return values have been properly updated.zetaclient/chains/sui/signer/signer_withdrawcap.go (2)
22-31: Efficient validity check with proper mutex usage.Good implementation of the validity check with proper read lock usage.
33-39: Thread-safe value updates with write mutex.Correct implementation of the setter with proper write lock protection.
pkg/contracts/sui/gateway_test.go (2)
130-158: Well-structured withdrawal test case.Good addition of a test case for the withdrawal event parsing, with appropriate assertions for all properties.
17-19: Clear constants definition with good separation.The addition of the
gatewayIDconstant makes the test more readable and maintainable.zetaclient/chains/sui/observer/inbound.go (4)
162-165: Looks Good for Deposits Extraction
The new call toevent.Deposit()and the associated error check appear correct. No issues stand out, as error handling is properly delegated.
168-172: Conditional Asset Initialization
Assigningassetas an empty string for gas deposits and setting it fromdeposit.CoinTypeotherwise is clear. Ensure thatdeposit.CoinTypeis always valid whenIsGas()returns false.
185-185: Receiver Address Validation
Referencingdeposit.Receiver.String()directly can be risky ifReceiveris uninitialized. Consider confirmingReceiveris never nil or missing in upstream code to prevent unexpected panics.
199-199: Cross-Chain Call Toggle
Usingcctypes.WithCrossChainCall(deposit.IsCrossChainCall)neatly toggles cross-chain logic. This is concise and maintainable.zetaclient/chains/sui/sui.go (6)
85-88: Dynamic Outbound Interval
Usingscheduler.IntervalUpdaterto adjust the outbound ticker dynamically is a good approach. This ensures flexibility ifOutboundTickerchanges at runtime.
125-127: Up-To-Date ChainParams
Fetching updated chain parameters before scheduling outbound transactions is essential to align with the latest config. This also provides a clear boundary for reloading potential changes.
136-144: Graceful Exit on Empty Pending CCTX
Returning early if there are no pending cross-chain transactions is a sensible optimization, preventing unnecessary processing load.
146-201: Concurrent Scheduling of CCTX
Looping over the list of pending CCTX and spawning background workers withbg.Workis effective for concurrent handling. However, ensure that:
- You do not spawn an unbounded number of goroutines on very large CCTX lists.
- Any shared data structures are handled safely.
205-227: Chain Parameter and Gateway Updates
updateChainParamsproperly retrieves new parameters and updates gateway IDs. This ensures that the observer and signer share synchronized configuration data. Logging any changed parameters could help debugging.
229-233: Contextual Outbound Logging
Attaching contextual metadata (outbound.id) to logs improves traceability. This is a clean pattern for grouping related logs.zetaclient/chains/sui/client/client.go (1)
160-160: Verify Backward Compatibility of Cursor Format
Changing the cursor delimiter from#to,is straightforward; however, confirm that older cursors stored with#do not break your system. This might require a migration or fallback decoding logic if existing cursors are in use.Also applies to: 169-169
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: zetaclient/chains/sui/client/client.go#L160
Added line #L160 was not covered by testspkg/contracts/sui/crypto_test.go (1)
1-13: Imports and Package Declaration Look GoodThe use of testify for assertions and the well-structured import grouping provide clarity and consistency.
zetaclient/chains/sui/signer/signer_tx.go (1)
1-16: Initial Declarations Appear SoundThe package declaration and import statements are well-structured, and constants like
funcWithdrawimprove readability.zetaclient/testutils/mocks/sui_client.go (3)
50-76: Mocks for 'GetOwnedObjectID' look consistent.This function cleanly follows the typical Mockery pattern, storing return values and simulating the real call. No concurrency concerns or logic issues appear here.
106-132: Well-structured 'MoveCall' mock.The return values and type assertions are handled correctly. The panic upon missing return values ensures test integrity by surfacing invalid mock setups.
171-197: 'SuiExecuteTransactionBlock' mock is appropriately implemented.This function mirrors the pattern of other mocks in this file, with a clear approach to simulating expected responses and error paths.
zetaclient/chains/sui/observer/observer.go (5)
23-29: Concurrent fields introduced for transaction and gas price management.The addition of
txMapandlatestGasPricealong with their corresponding locks (txMu,gasPriceMu) is a prudent approach to ensure thread-safe operations. Keep referencing the locks consistently for both reads and writes.
52-52: Proper initialization of 'txMap'.Explicitly allocating the map in the constructor prevents inadvertent nil-map usage.
105-105: Updated PostGasPrice with safe concurrency management.Storing the freshly fetched gas price by calling
ob.setLatestGasPriceensures the value is accessed and updated safely.
115-120: 'getLatestGasPrice' implements correct read locking.Using a read lock for retrieval avoids blocking concurrent readers, aligning with best practices for lock granularity.
122-126: 'setLatestGasPrice' implements correct write locking.The write lock usage ensures exclusive access when mutating shared data, preventing lost updates or data races.
pkg/contracts/sui/crypto.go (9)
1-9: Package and imports: cryptographic libraries appear suitably chosen.The combination of secp256k1, Ethereum crypto, and Blake2b meets Sui requirements. Dependencies are succinct and well-organized.
10-16: Constants properly define scope and usage.Using a separate constant for the key flag helps keep the code consistent and avoids magic numbers.
20-34: 'Digest' method accurately implements Sui's cryptographic specs.Decoding the transaction bytes, prefixing them, then applying Blake2b is consistent with the official Sui documentation.
36-51: Intent prefix construction is straightforward.The approach to build the final message by concatenating defaultIntent ensures alignment with the documented signing flow.
52-66: ECDSA public key to Sui address conversion is well-structured.Marshaling to compressed form, appending a flag, then hashing with Blake2b matches the Sui reference.
67-88: Signature serialization is cohesive and well-validated.The function carefully checks lengths, compresses the public key, and encodes to base64, effectively avoiding inconsistent signature states.
89-117: Signature deserialization logic is robust.Validating length, checking the flag, and decompressing the public key collectively reduce parsing errors.
119-131: SignerSecp256k1 struct fosters clear design for Sui signing.Storing both the private key and derived address keeps the code expressive and neat.
133-156: 'SignTxBlock' fulfills Sui's hashing guidelines effectively.The final SHA256 wrap on the Blake2b digest, plus rearranging the signature bytes, complies with Sui's ECDSA signature format.
pkg/contracts/sui/gateway.go (15)
4-4: No Issues Found in Additional Import.
The addition of"fmt"appears consistent with the usage in this file.
7-7: Concurrency Support Import Looks Good.
The introduction of"sync"and the subsequent use of async.RWMutexis a good pattern for concurrent data protection.
21-27: Appropriate Use of Mutex for Concurrent Access.
StoringpackageID,objectID, and using an RWMutex suggests proper concurrency handling. Ensure each method that accesses these fields holds the lock consistently.
37-37: Event Type Addition is Clear.
The newWithdrawevent type extends coverage without conflict.
45-54: New Constructor is Readable.
NewGatewayFromPairIDeffectively parses the$packageID,$gatewayObjectIDpair. It simplifies adopting the new format.
56-58: Constructor Logic is Straightforward.
NewGatewaycleanly initializes the struct fields. No immediate concerns.
70-72: Deposit Check Method is Fine.
IsDeposit()is explicit, improves readability, and pairs well with the new event definitions.
74-82: Deposit Data Extraction is Correct.
Ensuring the content is cast toDepositDatais a safe approach to event parsing.
84-85: Withdrawal Indicator is Properly Implemented.
IsWithdraw()is consistent with your event naming. No issues.
88-96: Withdrawal Data Extraction Mirrors Deposit Parsing.
TheWithdrawal()implementation aligns with the deposit approach, ensuring type safety.
98-101: Getter With RWMutex is Good.
Lock usage inPackageID()is correct for safe read access.
105-110: Protected ObjectID Getter is Aligned with Concurrency Goals.
Basic synchronous read access is properly guarded.
117-120: Inline Computation for WithdrawCapType is Straightforward.
Formatting the type string is concise and clear.
122-137: UpdateIDs Method Properly Synchronizes State.
The function locks before reassigning IDs. This pattern reduces race conditions effectively.
181-183: Case Handling Augments Parsing Logic.
The scenario forWithdrawEventmirrors the deposit approach, maintaining consistency.zetaclient/chains/sui/observer/observer_test.go (10)
27-28: Stub Variable for Reusability.
The introduction ofsomeArgStubsimplifies repeated usage in test scenarios.
91-91: Cursor Format Updated.
Switching to use a comma,instead of another delimiter is consistent with how Sui represents transaction sequences.
141-141: Cursor Validation.
Verifying"TX_4_invalid_data,0"ensures correct handling even for invalid data scenarios. This strengthens test coverage.
227-289: Thorough Outbound Tracker Test.
YourProcessOutboundTrackerstest covers nonce retrieval, Sui transaction mocking, and checks the final status. Good job verifying multiple paths and ensuringOutboundCreatedis set appropriately.
291-370: Outbound Voting Test is Comprehensive.
TheVoteOutboundtest verifies status, block height, and gas usage. This helps confirm end-to-end correctness of the outbound flow.
382-383: Separate Slices for Votes.
IntroducinginboundVotesBagandoutboundVotesBagclarifies inbound vs. outbound vote handling within tests.
417-418: Neat Integration withNewGatewayFromPairID.
Instantiating the gateway using the updated constructor ensures test setup consistency.
465-466: Utility Function for Single Tx Mocks.
MockGetTxOncehelps isolate unique transaction responses when testing, improving maintainability.
481-485: Flexible Outbound Votes Capture.
CatchOutboundVotesensures dynamic verification of posted outbound votes without repeated boilerplate.
493-505: Mocking CCTX By Nonce is Straightforward.
MockCCTXByNonceandMockOutboundTrackersfunctions keep test logic decoupled from how CCTX data is retrieved.zetaclient/chains/sui/signer/signer.go (5)
3-16: New Imports are Relevant.
Added packages (context,crypto/sha256, Sui packages, etc.) align well with the extended functionality for transaction signing and broadcasting.
21-26: Extended Signer Fields Bolster Sui Transaction Support.
By embeddingclient,gateway,withdrawCap, andzetacore, you centralize the necessary components for outbound transaction handling.
28-41: RPC Interface is Well-Defined.
Providing explicit methods (GetOwnedObjectID,MoveCall, etc.) fosters a clear contract for Sui interactions, aiding in testing and mocking.
44-57: Refactored Signer Constructor.
Acceptingclient,gateway, andzetacoreinNewensures modularity and promotes a more manageable code structure.
106-124: Digest and Signing Approach Matches Sui's Requirements.
Double hashing for ECDSA is aligned with Sui's cryptographic standards. The error wrapping is also consistent.zetaclient/chains/sui/observer/outbound.go (1)
66-73: Increase test coverage for error-handling paths.
The logic correctly logs errors for individual transactions without halting the entire loop. However, these lines are not covered by tests, according to static analysis. Consider adding unit test scenarios that simulateloadOutboundTxfailures to ensure robust and verifiable error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-73: zetaclient/chains/sui/observer/outbound.go#L66-L73
Added lines #L66 - L73 were not covered by tests
chainParams.GatewayAddressin the format of$packageID,$gatewayObjectIDWithdrawalCapWithdrawEventSummary by CodeRabbit
New Features
Improvements