Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request updates the changelog to document new Sui deposit & depositAndCall functionality, the adoption of ConfirmationParams, and various refactors and fixes. Major changes include revamping the Sui gateway’s event processing with new types and methods, enhancing inbound event observation with concurrency safety and gateway integration, and extending the Sui client with event query and pagination support. Test suites across components have been revised to align with these enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Sui Blockchain
participant G as Gateway
participant O as Observer
participant C as Sui Client
S->>G: Emit SuiEventResponse
G->>G: Execute ParseEvent (validate and construct Event)
G-->>O: Return Event structure
O->>C: Request Transaction Block (SuiGetTransactionBlock)
C-->>O: Provide Transaction Block
O->>O: Process inbound event (construct vote message)
sequenceDiagram
participant O as Observer
participant C as Sui Client
participant DB as Database
O->>C: QueryModuleEvents(EventQuery)
C-->>O: Return events and cursor
O->>DB: Update cursor via setCursor
DB-->>O: Acknowledge update
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3534 +/- ##
===========================================
- Coverage 64.69% 64.54% -0.16%
===========================================
Files 455 456 +1
Lines 31495 31747 +252
===========================================
+ Hits 20377 20490 +113
- Misses 10229 10353 +124
- Partials 889 904 +15
|
swift1337
force-pushed
the
feat/sui/deposits
branch
from
7978d15 to
180d8e0
Compare
swift1337
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
zetaclient/chains/sui/sui.go (1)
108-111:⚠️ Potential issueImplement scheduleCCTX functionality.
The
scheduleCCTXmethod is currently a stub. This method is critical for handling outbound cross-chain transactions.Would you like me to help implement this method or create a tracking issue for it?
🧹 Nitpick comments (14)
pkg/contracts/sui/inbound.go (1)
45-47: Potential negative or malformed amounts.
Whilemath.ParseUintwill reject negative values, consider adding additional checks if your business logic forbids non-positive amounts.pkg/contracts/sui/gateway.go (1)
73-126: Robust event parsing.
TheParseEventmethod performs comprehensive checks (empty fields, package ID mismatch) and leverages the neweventDescriptorto decode the event type. The multi-case switch for inbound logic is clear and maintainable.Consider subdividing portions of this large function (e.g., extracting common fields vs. specialized inbound logic) to improve clarity and testability.
zetaclient/chains/sui/observer/observer.go (1)
101-153: Robust cursor management for inbound scanning.
TheensureCursor,getCursor, andsetCursormethods provide a clear mechanism to track the last processed transaction. This design accommodates either environment-based overrides or an automatic fallback to the gateway package’s deployment transaction.Implement additional logging or metrics for cursor initialization and updates to simplify post-deployment monitoring and debugging.
zetaclient/chains/sui/observer/inbound.go (2)
20-65: Consider partial event processing or continuing post errors
Truncating the event processing sequence upon encountering an unconfirmed or invalid transaction may delay processing of subsequent valid events. Evaluate partial or batched reprocessing strategies to improve throughput and resilience.
138-157: Review partial processing in loop
A single invalid event causes the function to return, halting processing of subsequent events in the same transaction. Assess whether continuing after a failed event is desirable to avoid losing valid ones.zetaclient/chains/sui/client/client_live_test.go (1)
8-8: Consider adopting a local test environment
This live test relies on external RPC endpoints, which can cause non-deterministic behavior and rate limiting in CI/CD pipelines. Evaluate using a mock or local test environment to reliably simulate Sui events.Also applies to: 37-95
zetaclient/chains/sui/sui.go (1)
55-60: Address TODO comments and implement missing functionality.The TODO comments indicate several unimplemented features that are critical for the Sui integration:
- ObserveInbound
- ProcessInboundTrackers
- ProcessOutboundTrackers
- ScheduleCCTX
Would you like me to help implement these features or create tracking issues for them?
zetaclient/testutils/constant.go (1)
46-47: Document replacement timeline for stub address.The Sui Mainnet gateway address is currently a stub. Consider:
- Adding a TODO comment with a timeline for replacement
- Documenting the process for obtaining the real address
zetaclient/chains/sui/client/client.go (1)
134-153: Consider enhancing cursor encoding robustness.The cursor encoding/decoding implementation could be more robust:
func EncodeCursor(id models.EventId) string { - return fmt.Sprintf("%s#%s", id.TxDigest, id.EventSeq) + return fmt.Sprintf("%s:%s", strings.TrimSpace(id.TxDigest), strings.TrimSpace(id.EventSeq)) } func DecodeCursor(cursor string) (*models.EventId, error) { if cursor == "" { return nil, nil } - parts := strings.Split(cursor, "#") + parts := strings.Split(cursor, ":") if len(parts) != 2 { return nil, errors.New("invalid cursor format") } + txDigest := strings.TrimSpace(parts[0]) + eventSeq := strings.TrimSpace(parts[1]) + + if txDigest == "" || eventSeq == "" { + return nil, errors.New("invalid cursor components") + } return &models.EventId{ - TxDigest: parts[0], - EventSeq: parts[1], + TxDigest: txDigest, + EventSeq: eventSeq, }, nil }pkg/contracts/sui/gateway_test.go (2)
31-36: Consider adding test case descriptions.Adding descriptions to test cases would improve test documentation and make failures more understandable.
for _, tt := range []struct { + // description of what this test case validates name string event models.SuiEventResponse errContains string assert func(t *testing.T, raw models.SuiEventResponse, out Event)
51-66: Consider breaking down assertions into subtests.The assertions block could be more maintainable if broken down into logical groups.
assert: func(t *testing.T, raw models.SuiEventResponse, out Event) { + t.Run("metadata", func(t *testing.T) { assert.Equal(t, txHash, out.TxHash) assert.Equal(t, Deposit, out.EventType) assert.Equal(t, uint64(0), out.EventIndex) + }) + t.Run("inbound", func(t *testing.T) { assert.True(t, out.IsInbound()) inbound, err := out.Inbound() require.NoError(t, err) + }) + t.Run("details", func(t *testing.T) { assert.Equal(t, SUI, inbound.CoinType) assert.True(t, math.NewUint(100).Equal(inbound.Amount)) assert.Equal(t, sender, inbound.Sender) assert.Equal(t, receiverAlice, inbound.Receiver) assert.False(t, inbound.IsCrossChainCall) + }) },zetaclient/chains/sui/observer/observer_test.go (2)
57-172: Consider parameterizing test data.The test data could be moved to a separate test data structure to improve maintainability.
+type testCase struct { + name string + sender string + receiver string + amount string + coinType string + payload []any +} +var validTestCases = []testCase{ + { + name: "simple_deposit", + sender: "SUI_BOB", + receiver: evmBob.String(), + amount: "200", + coinType: string(sui.SUI), + }, + // ... more test cases +} t.Run("ObserveInbound", func(t *testing.T) { - // ... current implementation + for _, tc := range validTestCases { + events = append(events, ts.SampleEvent( + fmt.Sprintf("TX_%s", tc.name), + string(sui.Deposit), + map[string]any{ + "coin_type": tc.coinType, + "amount": tc.amount, + "sender": tc.sender, + "receiver": tc.receiver, + "payload": tc.payload, + }, + )) + }
301-314: Consider adding error handling test cases.The
OnGetTxhelper could benefit from test cases that verify error handling.func (ts *testSuite) OnGetTx(digest, checkpoint string, showEvents bool, events []models.SuiEventResponse) { + if digest == "" { + ts.suiMock.On("SuiGetTransactionBlock", mock.Anything, mock.Anything). + Return(models.SuiTransactionBlockResponse{}, fmt.Errorf("invalid digest")). + Once() + return + } + req := models.SuiGetTransactionBlockRequest{ Digest: digest, Options: models.SuiTransactionBlockOptions{ShowEvents: showEvents}, }changelog.md (1)
14-14: Consistent Changelog Entry Format:
The new changelog entry for SUI deposits and depositAndCall (line 14) clearly reflects the new feature described in the PR. However, to maintain consistency with previous entries (e.g., lines 7–13, which start with a lowercase verb such as “add”), consider changing “Add” to “add”. This small adjustment will ensure a uniform tone throughout the changelog.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
changelog.md(1 hunks)pkg/contracts/sui/gateway.go(1 hunks)pkg/contracts/sui/gateway_test.go(1 hunks)pkg/contracts/sui/inbound.go(2 hunks)pkg/contracts/sui/inbound_test.go(0 hunks)zetaclient/chains/base/observer.go(1 hunks)zetaclient/chains/sui/client/client.go(3 hunks)zetaclient/chains/sui/client/client_live_test.go(2 hunks)zetaclient/chains/sui/observer/inbound.go(1 hunks)zetaclient/chains/sui/observer/observer.go(2 hunks)zetaclient/chains/sui/observer/observer_test.go(4 hunks)zetaclient/chains/sui/sui.go(1 hunks)zetaclient/orchestrator/v2_bootstrap.go(2 hunks)zetaclient/testutils/constant.go(1 hunks)zetaclient/testutils/mocks/sui_client.go(2 hunks)zetaclient/testutils/mocks/sui_gen.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/contracts/sui/inbound_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/testutils/constant.gopkg/contracts/sui/gateway_test.gopkg/contracts/sui/inbound.gozetaclient/chains/base/observer.gozetaclient/chains/sui/sui.gozetaclient/testutils/mocks/sui_client.gozetaclient/chains/sui/observer/inbound.gozetaclient/orchestrator/v2_bootstrap.gozetaclient/testutils/mocks/sui_gen.gozetaclient/chains/sui/client/client_live_test.gozetaclient/chains/sui/observer/observer.gozetaclient/chains/sui/observer/observer_test.gopkg/contracts/sui/gateway.gozetaclient/chains/sui/client/client.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-and-test
- GitHub Check: gosec
- GitHub Check: lint
- GitHub Check: build-zetanode
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (47)
pkg/contracts/sui/inbound.go (12)
4-4: Use of cosmos math library.
The usage ofcosmossdk.io/mathformath.Uintis beneficial for large integer handling, preventing overflow issues that could arise with standarduint64.
10-11: Clear struct documentation.
The docstring succinctly states the purpose of theInboundstruct and clarifies its usage fordeposit/depositAndCallevents.
15-15: Correct transition to a big integer type.
Replacinguint64withmath.Uintensures better safety for larger amounts. Confirm that all downstream code properly handles the new type.
26-35: Efficient memo construction.
Appending the receiver’s 20-byte address followed by the payload is a clean approach. The preallocated slice capacity is appropriate, enhancing efficiency.
37-37: Refined function signature.
Using the strongerEventTypegives the parser explicit clarity and type safety.
40-43: Consistent error propagation forcoin_type.
ReturningInbound{}, errmaintains uniform error handling. This is straightforward and aligns with Go best practices.
50-52: Proper error wrapping.
Wrapping the parse error provides additional context, aiding debug efforts. Good usage oferrors.Wrap.
55-57: Consistent extraction for sender.
Extracting the"sender"field usingextractStrmatches the pattern of uniform error checking seen elsewhere.
60-62: Consistent extraction for receiver.
As with the sender field, uniform error checks keep the codebase consistent and predictable.
65-67: Robust receiver validation.
A zeroed Ethereum address is treated as invalid, which is a clear defensive measure against empty or malformed addresses.
70-83: Crosschain deposit with payload.
CheckingeventType == DepositAndCallsetsisCrosschainCall = trueand processes thepayload. This branch is well-structured, with adequate error handling for invalid payload data and out-of-range byte values.
93-93: Explicit final field assignment.
IsCrossChainCall: isCrosschainCallclarifies the event’s usage downstream.pkg/contracts/sui/gateway.go (16)
4-5: Added imports for string parsing.
Usingstrconvandstringsis straightforward for event descriptor parsing and ID conversion.
14-16: EventType declaration is concise.
Definingtype EventType stringallows a meaningful classification of deposit events.
22-24: SUI coin type constant.
Explicitly defining the SUI string constant fosters clarity for referencing the native token.
25-30: EventType constants for deposit logic.
DefiningDepositandDepositAndCallas typed constants improves code readability and correctness checks at compile time.
31-31: Self-descriptive module name.
moduleNameis a clear, centralized definition, facilitating consistent references throughout the gateway code.
39-42: Simplified gateway constructor.
Eliminating additional parameters reduces complexity. The constructor now focuses solely on instantiating the gateway with the relevant package ID.
43-52: Well-defined Event structure.
Storing event metadata in a unified struct fosters cleanliness and consistency.
53-55: Readable inbound check.
IsInbound()clarifies event direction with a simple boolean. Straightforward and efficient.
56-62: Safe inbound extraction.
Type assertion is guarded by theinboundcheck, minimizing runtime errors for non-inbound events.
65-66: Package ID accessor.
PackageID()offers a clean external handle for the gateway’s package ID without exposing internal fields directly.
69-70: Dedicated module name accessor.
Module()returnsgatewaysuccinctly, ensuring consistent usage across the codebase.
127-135: Clear final event construction.
ReturningEvent{…}with embedded content and inbound indicators ensures consistent usage in caller functions.
137-142: Structured event descriptor.
Bundling package ID, module, andEventTypeinto one struct is a concise approach to parse and represent event metadata.
143-155: Scalable descriptor parsing.
parseEventDescriptorbreaks down the fully qualified event signature into its components, with clear error handling for unexpected formats.
156-167: Helper for string extraction.
extractStrensures missing or invalid keys return errors early, preventing silent failures. This is a good pattern for robust parsing.
169-186: Converting payload data.
Extracting bytes fromfloat64values is done carefully, with boundary checks for valid byte ranges. This is a practical solution, given the Sui event format.zetaclient/chains/sui/observer/observer.go (7)
5-5: Added import for environment variables.
Leveragingosfor reading environment-based overrides is a straightforward approach to optionally set the cursor from external config.
12-12: Importing the Sui package.
Bringing inpkg/contracts/suiis consistent with the new logic referencing gateway operations.
14-14: Importing Sui client integration.
github.com/zeta-chain/node/zetaclient/chains/sui/clientis vital for extended RPC queries.
20-21: Observer struct updates.
Injecting agateway *sui.Gatewayfosters close integration with Sui inbound functionality. Ensure null checks or fallback logic if the gateway is not provided.
28-28: QueryModuleEvents extension.
Offering a dedicated method for module event queries reflects the growing Sui event-handling requirements.
31-35: Expanded RPC interface.
IncludingSuiGetObjectandSuiGetTransactionBlockbroadens the Observer’s ability to retrieve Sui blockchain data and block details.
39-43: RevisedNewconstructor.
Accepting agatewayreference at instantiation consolidates all Sui-related functionality under a single Observer.zetaclient/chains/sui/observer/inbound.go (2)
67-85: Question the error handling strategy
Currently, errors during tracker processing are only logged. Verify whether retries or error escalation are needed to avoid losing inbound transactions that fail on the first attempt.
159-201: Handle inbound status dynamically
The code setsInboundStatus_SUCCESSunconditionally. Consider mapping the actual deposit outcome to the correct status for more accurate tracking and reporting.zetaclient/testutils/mocks/sui_gen.go (1)
8-9: The mock interface additions look good
These new methods correspond well to the client’s functionality.Also applies to: 20-20, 24-28
zetaclient/chains/sui/sui.go (1)
70-72: LGTM! Well-structured interval and skipper implementation.The implementation of
optInboundIntervalandoptInboundSkipperfollows good practices:
- Uses
IntervalUpdaterfor dynamic interval updates based on chain parameters- Implements proper skipping logic based on application state
Also applies to: 78-80
zetaclient/chains/sui/client/client.go (1)
63-68: LGTM! Well-structured EventQuery with robust validation.The EventQuery struct and its validation logic are well-implemented:
- Clear field definitions
- Comprehensive validation checks
- Appropriate limit constraints
Also applies to: 97-110
pkg/contracts/sui/gateway_test.go (1)
14-66: LGTM! Well-structured test setup.The test setup is clear and comprehensive, with good use of constants and helper functions.
zetaclient/orchestrator/v2_bootstrap.go (1)
210-212: LGTM! Clean Sui gateway integration.The integration of the Sui gateway is clean and follows the established pattern of the codebase.
zetaclient/chains/base/observer.go (1)
235-237: LGTM! Critical thread safety fix.The addition of mutex locking is essential for preventing race conditions when updating
lastTxScanned.zetaclient/testutils/mocks/sui_client.go (4)
8-9: LGTM!The new import is correctly aliased and follows Go conventions.
78-113: LGTM!The implementation follows the established mock pattern with proper type assertions, error handling, and nil checks for slice return values.
115-169: LGTM!Both methods maintain consistency with the established mock patterns and include proper error handling and type assertions.
1-240: Reminder: Do not manually modify generated code.This file is auto-generated by mockery. Any changes should be made to the interface definition rather than this file directly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
3 participants
New
depositobservationsdepositAndCallobservationsNote that this PR doesn't contain E2E tests or compliance logic.
Closes #3523
Closes #3471
Closes #3475
Closes #3476
Closes #3498
Summary by CodeRabbit
New Features
Bug Fixes
Refactor