test: add sui gateway upgrade E2E test#3866
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 change introduces an end-to-end (e2e) test for upgrading the Sui gateway package. It adds new configuration fields, updates Docker and shell scripts to support Sui tooling, and implements the logic for deploying, upgrading, and verifying the Sui gateway package within the test runner. Supporting cryptographic utilities and tests are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as E2ERunner
participant SuiCLI as Sui CLI
participant SuiNode as Sui Node
TestRunner->>SuiCLI: Import deployer private key
SuiCLI-->>TestRunner: Confirmation
TestRunner->>SuiCLI: Set active address
SuiCLI-->>TestRunner: Confirmation
TestRunner->>SuiCLI: Deploy Sui gateway package
SuiCLI-->>TestRunner: Deployment output (object IDs)
TestRunner->>SuiNode: Query gateway object data (before upgrade)
SuiNode-->>TestRunner: Gateway object data
TestRunner->>SuiCLI: Upgrade Sui gateway package
SuiCLI-->>TestRunner: Upgrade output (new package ID)
TestRunner->>SuiCLI: Call 'upgraded' Move method on new package
SuiCLI-->>TestRunner: Transaction result
TestRunner->>SuiNode: Query gateway object data (after upgrade)
SuiNode-->>TestRunner: Gateway object data
TestRunner->>TestRunner: Compare object data before and after
Assessment against linked issues
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
ws4charlie
added
chain:sui
SUI_TESTS
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3866 +/- ##
========================================
Coverage 64.73% 64.73%
========================================
Files 469 469
Lines 34333 34333
========================================
Hits 22224 22224
Misses 11081 11081
Partials 1028 1028 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
cmd/zetae2e/config/config.go (1)
66-66: Consistent addition to the contract export functionality.The code properly exports the Sui gateway upgrade capability to the configuration, maintaining parity between the runner state and the exported configuration.
However, consider whether this assignment should be conditional like the GatewayPackageID and GatewayObjectID assignments above:
if r.SuiGateway != nil { conf.Contracts.Sui.GatewayPackageID = config.DoubleQuotedString(r.SuiGateway.PackageID()) conf.Contracts.Sui.GatewayObjectID = config.DoubleQuotedString(r.SuiGateway.ObjectID()) + conf.Contracts.Sui.GatewayUpgradeCap = config.DoubleQuotedString(r.SuiGatewayUpgradeCap) } -conf.Contracts.Sui.GatewayUpgradeCap = config.DoubleQuotedString(r.SuiGatewayUpgradeCap)This would ensure consistent handling if the Sui gateway availability is a prerequisite for the upgrade capability.
pkg/contracts/sui/crypto.go (1)
19-25: Prefix constant belongs in a dedicatedconstblock for public prefixesYou introduced
suiPrivateKeyPrefix, which is great. Consider moving all Bech32-related constants (prefix, flag, etc.) into a small, dedicated block (or even its ownbech32.go) to keep cryptographic primitives separated from high-level digest helpers.contrib/localnet/orchestrator/Dockerfile.fastbuild (1)
16-18: COPY order leaks host-path assumptionsThe paths
contrib/localnet/sui/sui_client.yamlande2e/contracts/sui/protocol-contracts-suiare copied unconditionally. If either path goes missing (e.g. an OSS consumer disables Sui), the build fails. Consider guarding with--chownor using build-args / build-targets to make Sui support optional.e2e/runner/setup_sui.go (2)
49-63: Early return if account already imported
importAndActivateDeployerAccountis always invoked, even on repeated test runs;sui keytool importerrors if the key already exists. Handle the “already imported” case gracefully to keep the test idempotent.
112-143:suiDeployGateway– ensure deterministic object-ID selectionWhen multiple objects of the same type exist, the last match in the loop wins, which is nondeterministic. Consider breaking once a match is found or asserting a single match to avoid flakiness:
for _, change := range resp.ObjectChanges { if change.Type == changeTypeCreated && strings.Contains(change.ObjectType, filter) { if _, exists := objectIDs[filter]; exists { r.Fail("multiple objects found for type %s", filter) } objectIDs[filter] = change.ObjectId } }e2e/runner/sui_gateway_upgrade.go (2)
72-73:Infolog placeholder%fexpectsfloat64
time.Since(startTime).Seconds()already returns afloat64.
The call site is fine, but consider formatting to a fixed precision to avoid very long decimals (e.g.%.2f).
Minor, but it improves log readability.
89-105: Consider retry logic after package upgrade
moveCallUpgradedis executed right aftersui client upgrade.
On busy/remote validators the package might not be available immediately, causing an intermittentObject not founderror.A lightweight retry with back-off around
MoveCall(or aWaitForTransactionhelper) would make the E2E test deterministic.- tx, err := r.Clients.Sui.MoveCall(ctx, models.MoveCallRequest{ … }) - require.NoError(r, err) + var tx *models.TransactionResponse + err := retry.Do( + func() error { + var e error + tx, e = r.Clients.Sui.MoveCall(ctx, models.MoveCallRequest{ … }) + return e + }, + retry.Attempts(5), retry.Delay(2*time.Second)) + require.NoError(r, err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
changelog.md(1 hunks)cmd/zetae2e/config/config.go(1 hunks)cmd/zetae2e/config/contracts.go(1 hunks)cmd/zetae2e/local/local.go(1 hunks)contrib/localnet/orchestrator/Dockerfile.fastbuild(1 hunks)contrib/localnet/orchestrator/start-zetae2e.sh(1 hunks)contrib/localnet/sui/sui_client.yaml(1 hunks)e2e/config/config.go(1 hunks)e2e/contracts/sui/protocol-contracts-sui(1 hunks)e2e/runner/runner.go(3 hunks)e2e/runner/setup_sui.go(5 hunks)e2e/runner/sui.go(2 hunks)e2e/runner/sui_gateway_upgrade.go(1 hunks)pkg/contracts/sui/crypto.go(2 hunks)pkg/contracts/sui/crypto_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.gocmd/zetae2e/config/config.goe2e/config/config.goe2e/runner/sui.gopkg/contracts/sui/crypto_test.gocmd/zetae2e/config/contracts.goe2e/runner/runner.gopkg/contracts/sui/crypto.goe2e/runner/sui_gateway_upgrade.goe2e/runner/setup_sui.go
`**/*.sh`: Review the shell scripts, point out issues relative to security, performance, and maintainability.
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
contrib/localnet/orchestrator/start-zetae2e.sh
🧬 Code Graph Analysis (1)
cmd/zetae2e/config/config.go (1)
e2e/config/config.go (3)
Contracts(128-134)Sui(157-164)DoubleQuotedString(27-27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (15)
changelog.md (1)
54-54: New E2E Test Entry Looks Good
The addition of the E2E test for upgrading the Sui gateway package under Tests is accurate and follows the existing changelog format.cmd/zetae2e/local/local.go (1)
626-628: Solid addition for verifying the Sui gateway package upgrade.The placement of this verification step after the Sui tests and before the TSS migration test is logical. This enhances the test coverage for the Sui functionality within the E2E test flow.
contrib/localnet/orchestrator/start-zetae2e.sh (1)
186-190: Clear and well-structured Sui environment setup.The implementation follows the established pattern used for other environment setups in this script, correctly checking if the host is reachable before attempting to configure it.
contrib/localnet/sui/sui_client.yaml (1)
1-9: Well-configured Sui client settings.The configuration correctly defines:
- The keystore path
- A localnet environment with appropriate RPC endpoint
- The active environment setting
This provides the necessary configuration for the Sui client to connect to the local Sui node during tests.
pkg/contracts/sui/crypto_test.go (1)
114-140: Well-structured test forPrivateKeyBech32Secp256k1FromHexfunction.The test follows best practices with table-driven test cases covering both valid and invalid inputs. The validation logic is thorough, checking for both successful conversions and appropriate error handling.
e2e/runner/runner.go (3)
130-132: Good documentation and field placement.The field is well-documented, clearly explaining its purpose for upgrading the Sui gateway package.
295-295: Field properly added to copy operation.The
SuiGatewayUpgradeCapfield is appropriately added to the copy operation, maintaining consistency with other Sui-related fields.
432-432: Field correctly added to contract address logging.The integration with the address printing functionality is consistent with other Sui gateway fields.
cmd/zetae2e/config/contracts.go (1)
133-135: Configuration field handling is consistent.The code follows the established pattern for handling Sui contract configuration fields.
e2e/runner/sui.go (3)
18-18: Import added for transaction status validation.The import enables access to the
TxStatusSuccessconstant used in transaction validation.
454-458: Enhanced transaction execution with effects display.Adding
ShowEffects: truein the options enables validation of transaction status in the response.
461-461: Improved transaction validation.The addition of transaction status validation strengthens the robustness of the transaction execution process by explicitly checking for success status.
contrib/localnet/orchestrator/Dockerfile.fastbuild (1)
5-11: Pin the Sui image to a digest for reproducible buildsUsing the tag
mainnet-v1.41.1is better thanlatest, but tags can still be re-pointed. Consider pinning to a digest (@sha256:…) to guarantee byte-for-byte reproducibility.e2e/runner/setup_sui.go (1)
105-110: Trim newline withstrings.TrimSpace— good catchThe additional validation of the active address is appreciated and prevents subtle mis-configuration.
e2e/runner/sui_gateway_upgrade.go (1)
22-33: Regex is fragile to future CLI formatting changes
reGatewayPackageIDrelies on the │ box-drawing characters being present.
The Sui CLI could switch to plain ASCII in future releases, breaking the capture.A more resilient pattern:
-var reGatewayPackageID = regexp.MustCompile(`│\s*PackageID: *(0x[0-9a-fA-F]+)\s*│`) +var reGatewayPackageID = regexp.MustCompile(`PackageID:\s*(0x[0-9a-fA-F]+)`)This still matches the current output while surviving cosmetic border changes.
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
How to run the test?
Can you also share a screenshot of successful test completion?
|
!!!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 |
Shared in the description of this PR. The command is the same |
…e used in e2e test; unify the solana and sui upgrade test assertion style
…-e2e-package-upgrade
4 participants
Description
To run the upgrade test:
make start-sui-testCloses: #3805
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores