test: add system-tests #189
Conversation
|
Warning Rate limit exceeded@JulianToledano has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a comprehensive system testing framework for the Rosetta blockchain integration project. The changes include adding a new GitHub Actions workflow job for system tests, creating a dedicated system tests package with multiple test files, updating the Makefile to support system testing, and modifying various configuration files. The system tests cover critical functionality such as account transactions, block interactions, network endpoints, mempool operations, and Rosetta API construction methods. Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Test as System Test Runner
participant Rosetta as Rosetta API
participant Blockchain as Blockchain Node
GHA->>Test: Trigger system tests
Test->>Blockchain: Reset and start chain
Test->>Rosetta: Start Rosetta service
Test->>Rosetta: Perform network checks
Test->>Blockchain: Execute transactions
Test->>Rosetta: Verify transaction details
Test->>Rosetta: Check mempool and block endpoints
Rosetta-->>Test: Return test results
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
Makefile (1)
Fix path and environment configuration in test-rosetta-ci target
The pipeline failure is due to incorrect configuration:
- The rosetta binary path in test-rosetta-ci (
./rosetta) doesn't match the build output path (./build/rosetta)- Required environment variable
COSMOS_SDK_VERSIONis not set in simapp-start-node.shSuggested fixes:
- Update the rosetta binary path in test-rosetta-ci target to use
./build/rosetta- Add
COSMOS_SDK_VERSIONenvironment variable before running simapp-start-node.sh🔗 Analysis chain
Line range hint
32-37: Investigate test-rosetta-ci pipeline failure.The test-rosetta-ci target is failing in the pipeline. Let's verify the setup and dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues in test-rosetta-ci setup # Check if simd binary exists in the expected location echo "Checking for simd binary..." fd simd # Check if rosetta binary exists and is executable echo "Checking for rosetta binary..." fd rosetta # Check if required ports are available echo "Checking if required ports are available..." netstat -tuln | grep -E '26657|8080|9090' # Check cosmos-sdk directory structure echo "Checking cosmos-sdk directory..." fd . -t d -d 2 | grep cosmos-sdkLength of output: 657
Script:
#!/bin/bash # Check content of required shell scripts and Makefile targets echo "=== Checking shell scripts ===" echo "simapp-start-node.sh content:" cat ./scripts/simapp-start-node.sh echo -e "\n=== rosetta-cli-test.sh content:" cat ./tests/rosetta-cli/rosetta-cli-test.sh echo -e "\n=== Checking Makefile targets ===" echo "build and plugin targets:" rg "^(build|plugin):" Makefile -A 3 echo -e "\n=== Checking rosetta build process ===" rg "rosetta" MakefileLength of output: 1733
🧰 Tools
🪛 GitHub Actions: rosetta-cli-test
[error] test-rosetta-ci target failed with exit code 2
🧹 Nitpick comments (8)
tests/systemtests/accounts_test.go (1)
18-19: Consider extracting magic numbers into named constants.The token amount "10000000stake" should be defined as a constant at the package level for better maintainability and reusability across tests.
+const ( + initialStakeAmount = "10000000stake" + transferAmount = "1000000stake" +)tests/systemtests/network_test.go (1)
35-35: Replace magic number with a named constant and enhance validation.The assertion for operation statuses length uses a magic number and only checks the length. Consider validating the actual status values.
+const expectedOperationStatusCount = 2 -assert.Equal(t, len(gjson.GetBytes(res, "allow.operation_statuses").Array()), 2) +statuses := gjson.GetBytes(res, "allow.operation_statuses").Array() +assert.Equal(t, expectedOperationStatusCount, len(statuses)) + +// Validate expected status values +statusMap := make(map[string]bool) +for _, status := range statuses { + statusMap[status.Get("status").String()] = true +} +assert.True(t, statusMap["SUCCESS"]) +assert.True(t, statusMap["FAILURE"])tests/systemtests/block_test.go (1)
27-27: Extract token amount to a constant.The token amount "10stake" should be defined as a constant for consistency and maintainability.
+const testStakeAmount = "10stake" -rsp := cli.RunAndWait("tx", "bank", "send", fromAddr, toAddr, "10stake") +rsp := cli.RunAndWait("tx", "bank", "send", fromAddr, toAddr, testStakeAmount)tests/systemtests/construction_test.go (1)
77-78: Document the significance of hardcoded gas values.The gas price and limit values appear to be arbitrary. Consider adding comments explaining their significance or using constants with meaningful names.
+// Example gas values for testing. In production, these would be calculated based on transaction complexity metadata["gas_price"] = `"123uatom"` metadata["gas_limit"] = 423tests/systemtests/test_runner.go (2)
95-95: Document the magic number for file handlers.The value 260 appears to be arbitrarily chosen based on local testing.
-expFH := nodesCount * 260 // random number that worked on my box +const fileHandlersPerNode = 260 // Empirically determined: Each node requires ~260 file handlers for logs, connections, etc. +expFH := nodesCount * fileHandlersPerNode
41-45: Potential race condition in WorkDir assignment.The global WorkDir variable is assigned without synchronization, which could lead to race conditions in concurrent tests.
Consider using sync.Once or initializing WorkDir during package initialization.
tests/systemtests/Makefile (1)
1-6: Add essential Makefile improvements.The Makefile needs several improvements for better maintainability and functionality:
- Add .PHONY declaration
- Add clean target
- Make timeout configurable
#!/usr/bin/make -f WAIT_TIME ?= 45s +TIMEOUT ?= 15m + +.PHONY: test clean test: - go test -mod=readonly -failfast -timeout=15m -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose + go test -mod=readonly -failfast -timeout=$(TIMEOUT) -tags='system_test' ./... --wait-time=$(WAIT_TIME) --verbose + +clean: + rm -f tests/systemtests/bin/*🧰 Tools
🪛 GitHub Actions: rosetta-cli-test
[error] test-rosetta-ci target failed with exit code 2
.github/workflows/test.yml (1)
65-65: Add newline at end of file.Add a newline character at the end of the file to comply with POSIX standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 65-65: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.github/workflows/test.yml(1 hunks).gitignore(1 hunks)Makefile(2 hunks)codec.go(1 hunks)go.mod(0 hunks)tests/systemtests/Makefile(1 hunks)tests/systemtests/accounts_test.go(1 hunks)tests/systemtests/block_test.go(1 hunks)tests/systemtests/construction_test.go(1 hunks)tests/systemtests/go.mod(1 hunks)tests/systemtests/main_test.go(1 hunks)tests/systemtests/mempool_test.go(1 hunks)tests/systemtests/network_test.go(1 hunks)tests/systemtests/rest_client.go(1 hunks)tests/systemtests/rosetta.go(1 hunks)tests/systemtests/test_runner.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
✅ Files skipped from review due to trivial changes (1)
- tests/systemtests/go.mod
🧰 Additional context used
🪛 GitHub Actions: rosetta-cli-test
Makefile
[error] test-rosetta-ci target failed with exit code 2
tests/systemtests/Makefile
[error] test-rosetta-ci target failed with exit code 2
tests/systemtests/rosetta.go
[error] Failed to start Rosetta server: './rosetta' command not found
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 65-65: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: gosec
tests/systemtests/test_runner.go
[failure] 85-85: Subprocess launched with variable
Subprocess launched with variable
tests/systemtests/rosetta.go
[failure] 84-84: Subprocess launched with variable
Subprocess launched with a potential tainted input or cmd arguments
🪛 GitHub Actions: Tests / Code Coverage
tests/systemtests/construction_test.go
[error] 51-51: Chain ID required but not specified when running tx bank send command
[error] 53-53: Invalid transaction file format: invalid character 'c' looking for beginning of value
[error] 55-55: Invalid transaction file format: invalid character 'U' looking for beginning of value
[error] 58-58: Illegal base64 data at input byte 5
🪛 golangci-lint (1.62.2)
tests/systemtests/rosetta.go
[medium] 84-84: G204: Subprocess launched with a potential tainted input or cmd arguments
(gosec)
98-98: SA1019: resty.New().SetHostURL is deprecated: use [Client.SetBaseURL] instead. To be removed in the v3.0.0 release.
(staticcheck)
tests/systemtests/rest_client.go
10-10: type restClient is unused
(unused)
15-15: func newRestClient is unused
(unused)
25-25: func (*restClient).networkList is unused
(unused)
34-34: func (*restClient).networkStatus is unused
(unused)
113-113: func buildBody is unused
(unused)
43-43: func (*restClient).networkOptions is unused
(unused)
52-52: func (*restClient).block is unused
(unused)
107-107: func withBlockIdentifier is unused
(unused)
63-63: func (*restClient).blockTransaction is unused
(unused)
76-76: func (*restClient).mempool is unused
(unused)
16-16: SA1019: resty.New().
SetHostURL is deprecated: use [Client.SetBaseURL] instead. To be removed in the v3.0.0 release.
(staticcheck)
🪛 GitHub Actions: Lint
tests/systemtests/rosetta.go
[error] 84-84: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
🔇 Additional comments (5)
tests/systemtests/main_test.go (1)
1-11: LGTMThe
TestMainfunction is correctly set up to run the system tests usingRunTests(m). The build constraint ensures that this test file is included only when thesystem_testbuild tag is specified.codec.go (1)
Line range hint
34-38: LGTM! Interface registrations are correctly ordered.The SDK interfaces are registered in a logical order, starting with core SDK interfaces followed by module-specific ones.
tests/systemtests/construction_test.go (1)
74-74: Replace fragile string manipulation with robust key extraction.The same fragile string manipulation is used here as in TestDerive.
.gitignore (1)
42-43: LGTM! Appropriate entries added to .gitignore.The new entries properly exclude system test binaries and testnet data from version control.
Makefile (1)
6-7: LGTM! Build directory creation and output path specification.Good practice to explicitly create the build directory and specify the output path.
🧰 Tools
🪛 GitHub Actions: rosetta-cli-test
[error] test-rosetta-ci target failed with exit code 2
| cmd := exec.Command(ulimit, "-n") | ||
| cmd.Dir = systemtests.WorkDir |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Secure the ulimit command execution.
The current implementation could be vulnerable to command injection. While this is a test environment, it's good practice to use secure coding patterns.
Use a fixed path or validate the command:
-cmd := exec.Command(ulimit, "-n")
+if !strings.HasSuffix(ulimit, "ulimit") {
+ return
+}
+cmd := exec.Command(ulimit, "-n")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: gosec
[failure] 85-85: Subprocess launched with variable
Subprocess launched with variable
| func (c *restClient) constructionMetadata(hexPk string, options map[string]interface{}) ([]byte, error) { | ||
| optParts := make([]string, 0, len(options)) | ||
| for k, v := range options { | ||
| optParts = append(optParts, fmt.Sprintf(`"%s":%v`, k, v)) | ||
| } | ||
| optionsStr := strings.Join(optParts, ",") | ||
| body := fmt.Sprintf(`{%s, "options":{%s}, %s}`, c.networkIdentifier, optionsStr, publicKeyBody(hexPk, "secp256k1")) | ||
| res, err := c.client.R().SetBody( | ||
| body, | ||
| ).Post("/construction/metadata") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper JSON marshaling instead of string concatenation.
The current approach of building JSON through string concatenation is error-prone and vulnerable to malformed input.
Consider using proper JSON marshaling:
-optParts := make([]string, 0, len(options))
-for k, v := range options {
- optParts = append(optParts, fmt.Sprintf(`"%s":%v`, k, v))
-}
-optionsStr := strings.Join(optParts, ",")
-body := fmt.Sprintf(`{%s, "options":{%s}, %s}`, c.networkIdentifier, optionsStr, publicKeyBody(hexPk, "secp256k1"))
+type metadataRequest struct {
+ NetworkIdentifier map[string]string `json:"network_identifier"`
+ Options map[string]interface{} `json:"options"`
+ PublicKey map[string]string `json:"public_key"`
+}
+req := metadataRequest{
+ NetworkIdentifier: parseNetworkIdentifier(c.networkIdentifier),
+ Options: options,
+ PublicKey: parsePublicKey(hexPk, "secp256k1"),
+}
+body, err := json.Marshal(req)
+if err != nil {
+ return nil, fmt.Errorf("failed to marshal request: %w", err)
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/go-lint-all.bash (3)
11-17: Add error handling for module detection.While the conditional directory navigation is a good improvement, the
grepcommand for module detection could fail silently ifgo.modis malformed.Apply this diff to add error handling:
- echo "linting $(grep "^module" go.mod) [$(date -Iseconds -u)]" + module_name=$(grep "^module" go.mod) || { echo "Error: Failed to detect module name in go.mod"; exit 1; } + echo "linting ${module_name} [$(date -Iseconds -u)]"
30-30: Improve Git diff processing robustness.The current implementation might break with paths containing spaces or quotes. Additionally, the complex pipe operations would benefit from documentation.
Apply this diff to improve robustness and add documentation:
-GIT_DIFF=$(echo $GIT_DIFF | tr -d "'" | tr ' ' '\n' | grep '\.go$' | grep -v '\.pb\.go$' | grep -Eo '^[^/]+\/[^/]+' | uniq) +# Process Git diff to: +# 1. Find .go files (excluding .pb.go) +# 2. Extract top-level directory paths +# 3. Remove duplicates +GIT_DIFF=$(echo "$GIT_DIFF" | while IFS= read -r file; do + [[ $file == *.go && $file != *.pb.go ]] && echo "${file%%/*}/${file#*/}" +done | sort -u)
41-43: Add error handling for directory change and build tags.The final commands lack error handling for the directory change and LINT_TAGS variable.
Apply this diff to add proper error handling:
-cd "$REPO_ROOT" +cd "$REPO_ROOT" || { echo "Error: Failed to change to repository root"; exit 1; } +LINT_TAGS=${LINT_TAGS:-} # Set default if unset echo "linting github.com/cosmos/rosetta [$(date -Iseconds -u)]" -golangci-lint run ./... -c "${REPO_ROOT}/.golangci.yml" "$@" --build-tags=${LINT_TAGS} +golangci-lint run ./... -c "${REPO_ROOT}/.golangci.yml" "$@" ${LINT_TAGS:+--build-tags="${LINT_TAGS}"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.golangci.yml(1 hunks)CHANGELOG.md(1 hunks)scripts/go-lint-all.bash(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/go-lint-all.bash
[error] 33-33: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 35-35: lint_sdk appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-system
- GitHub Check: test-rosetta
- GitHub Check: build (amd64)
- GitHub Check: rosetta-cli-test
- GitHub Check: golangci-lint
- GitHub Check: Gosec
🔇 Additional comments (2)
CHANGELOG.md (1)
39-42: LGTM! The changelog entry follows the guidelines.The entry is correctly placed in the "Features" section under "Unreleased" and follows the proper format with PR link.
.golangci.yml (1)
55-57: 🛠️ Refactor suggestionConsider scoping the G204 exclusion to test files only.
While disabling the G204 (command execution) check is understandable for system tests, a global exclusion could miss potential security issues in production code. Consider either:
- Scoping the exclusion to test files:
- text: "G204:" linters: - "gosec" + path: "_test.go$"
- Or preferably, use inline
//nolint:gosecdirectives only where needed in the test files.Let's verify the usage of command execution in the codebase:
✅ Verification successful
Verified: G204 exclusion should be scoped to test files
The codebase scan confirms that all command executions (
exec.Command,syscall) are exclusively within thetests/systemtests/directory. Scoping the G204 exclusion to test files as suggested would effectively cover all current usage while maintaining security checks for production code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for patterns that might trigger G204 # This helps assess the impact of the exclusion # Look for exec.Command usage rg "exec\.Command\(" -A 2 # Look for system.* calls rg "syscall\." -A 2 # Look for os.StartProcess rg "os\.StartProcess" -A 2Length of output: 673
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
52-52: Document the separate dependency path for system tests.Using a different
cache-dependency-pathsuggests separate dependencies for system tests. Consider:
- Documenting why system tests need separate dependencies
- Adding a README in the systemtests directory explaining the setup
64-67: Enhance system test job configuration.The system test step might need additional configuration:
- Consider adding environment variables required for system tests
- Add timeout for the test command itself
- Consider adding test result artifacts
Example enhancement:
- name: system tests if: env.GIT_DIFF run: | make test-system + timeout: 20m + - name: Upload test results + if: always() + uses: actions/upload-artifact@v3 + with: + name: system-test-results + path: tests/systemtests/results/🧰 Tools
🪛 yamllint (1.35.1)
[error] 67-67: no new line character at the end of file
(new-line-at-end-of-file)
67-67: Add newline at end of file.Add a newline character at the end of the file to comply with YAML formatting standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 67-67: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/rosetta-cli-test.yaml(1 hunks).github/workflows/test.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 67-67: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
.github/workflows/rosetta-cli-test.yaml (1)
24-24: Verify compatibility and consider risks of using RC version.Using a release candidate version (
v0.52.0-rc.1) in CI could introduce instability. Please ensure:
- This version is compatible with other dependencies
- There are no breaking changes that could affect the tests
- Consider using a stable release if available
Run this script to check version compatibility and availability:
✅ Verification successful
Version choice is appropriate for testing purposes
The selected version
v0.52.0-rc.1is:
- The latest pre-release for the v0.52.x series
- Has documented upgrade path from v0.50
- Appropriate for CI testing to ensure compatibility with upcoming SDK version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this version exists and compare with latest stable curl -s https://api.github.com/repos/cosmos/cosmos-sdk/releases | jq -r '.[] | select(.prerelease==false) | .tag_name' | head -n 1 # Check go.mod in the repository for other Cosmos SDK dependencies rg -l "github.com/cosmos/cosmos-sdk" --type modLength of output: 217
Script:
#!/bin/bash # Check if RC version exists and get all recent releases curl -s https://api.github.com/repos/cosmos/cosmos-sdk/releases | jq -r '.[] | "\(.tag_name) (prerelease: \(.prerelease))"' | head -n 5 # Find go.mod files fd "go.mod$" # Look for migration docs in the SDK repo for v0.52 curl -s https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/UPGRADING.md | grep -A 5 "v0.52"Length of output: 2428
.github/workflows/test.yml (2)
49-49: Align Go versions across jobs.The
test-systemjob uses Go 1.23 whiletest-rosettauses 1.23.4.
40-41: Verify the necessity of building plugin before tests.The addition of
make pluginbeforemake testsuggests a new dependency. Please ensure this is necessary and document why the plugin needs to be built before running tests.Run this script to understand the plugin dependency:
✅ Verification successful
Plugin build step is necessary for Rosetta tests
The plugin build step is required as the Rosetta tests explicitly depend on and load the plugin for API functionality. This is evidenced by:
- Plugin path being a mandatory parameter in
newRosettaRunner- Rosetta test configuration requiring the plugin via command line flags
- Plugin being built from the cosmos-hub source for test execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Makefile for plugin target dependencies rg "plugin:" -A 5 --type make # Check if tests actually require the plugin rg -l "plugin" tests/Length of output: 604
Script:
#!/bin/bash # Check how plugin is used in test files rg "plugin" tests/systemtests/rosetta.go -B 2 -A 2 rg "plugin" tests/systemtests/test_runner.go -B 2 -A 2 # Check if there are any plugin loading patterns ast-grep --pattern 'plugin.Open($_)' ast-grep --pattern 'plugin.Lookup($_)'Length of output: 1410
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/systemtests/rosetta.go (4)
28-47: Consider grouping related fields and adding missing comments.The struct fields could be organized better by grouping them into logical categories (e.g., configuration, runtime state, logging). Also, some fields (
pid,outputDir) lack documentation.Consider reorganizing the fields like this:
type rosettaRunner struct { + // Configuration execBinary string Addr string // the address rosetta will bind to (default ":8080") Blockchain string // the blockchain type (default "app") DenomToSuggest string // default denom for fee suggestion (default "uatom") GRPC string // the app gRPC endpoint (default "localhost:9090") GRPCTypesServer string // the app gRPC Server endpoint for proto messages types and reflection Network string // the network name (default "network") Plugin string // plugin folder name Tendermint string // CometBFT rpc endpoint Offline bool // run rosetta only with construction API + + // Logging verbose bool out io.Writer outBuff *ring.Ring errBuff *ring.Ring + + // Runtime state cleanupFn []cleanUpFn pid int // process ID of the running Rosetta server outputDir string // directory for output files and logs }
49-69: Consider making the ring buffer size configurable.The ring buffer size for logs is hardcoded to 100 lines. Consider making this configurable to allow adjusting the log history based on debugging needs.
+const defaultRingBufferSize = 100 + func newRosettaRunner(binary, denom, grpcTypesServer, plugin string, offline, verbose bool) rosettaRunner { execBinary := filepath.Join(systemtests.WorkDir, "binaries", binary) return rosettaRunner{ // ... other fields ... - outBuff: ring.New(100), - errBuff: ring.New(100), + outBuff: ring.New(defaultRingBufferSize), + errBuff: ring.New(defaultRingBufferSize), // ... other fields ... } }
136-136: Consider making the sleep duration configurable.The hardcoded sleep duration of 2 seconds in the stop method might not be suitable for all environments.
+const defaultShutdownTimeout = 2 * time.Second + func (r *rosettaRunner) stop() error { // ... existing code ... - time.Sleep(time.Second * 2) + time.Sleep(defaultShutdownTimeout) return nil }
150-173: Consider using context for managing goroutine lifecycle.The
watchLogsmethod uses a channel for stopping goroutines, but it could benefit from using context for better lifecycle management.-func (r *rosettaRunner) watchLogs(cmd *exec.Cmd) { +func (r *rosettaRunner) watchLogs(ctx context.Context, cmd *exec.Cmd) { // ... existing setup code ... - stopRingBuffer := make(chan struct{}) - go appendToBuf(io.TeeReader(errReader, logfile), r.errBuff, stopRingBuffer) + go appendToBuf(ctx, io.TeeReader(errReader, logfile), r.errBuff) // ... existing stdout setup ... - go appendToBuf(io.TeeReader(outReader, logfile), r.outBuff, stopRingBuffer) + go appendToBuf(ctx, io.TeeReader(outReader, logfile), r.outBuff) r.cleanupFn = append(r.cleanupFn, func() { - close(stopRingBuffer) _ = logfile.Close() }) } -func appendToBuf(r io.Reader, b *ring.Ring, stop <-chan struct{}) { +func appendToBuf(ctx context.Context, r io.Reader, b *ring.Ring) { scanner := bufio.NewScanner(r) for scanner.Scan() { select { - case <-stop: + case <-ctx.Done(): return default: } // ... existing code ... } }tests/systemtests/rest_client.go (1)
16-18: Add request timeout configuration.Consider adding timeout configuration to prevent hanging requests in case of network issues.
client := resty.New(). SetBaseURL("http://" + rosetta.Addr). + SetTimeout(30 * time.Second). SetHeaders(map[string]string{"Content-Type": "application/json", "Accept": "application/json"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
scripts/go-lint-all.bash(1 hunks)tests/systemtests/accounts_test.go(1 hunks)tests/systemtests/block_test.go(1 hunks)tests/systemtests/mempool_test.go(1 hunks)tests/systemtests/rest_client.go(1 hunks)tests/systemtests/rosetta.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/systemtests/accounts_test.go
- tests/systemtests/mempool_test.go
- tests/systemtests/block_test.go
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/go-lint-all.bash
[error] 37-37: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🪛 GitHub Check: gosec
tests/systemtests/rosetta.go
[failure] 85-85: Subprocess launched with variable
Subprocess launched with a potential tainted input or cmd arguments
🪛 golangci-lint (1.62.2)
tests/systemtests/rest_client.go
10-10: type restClient is unused
(unused)
15-15: func newRestClient is unused
(unused)
25-25: func (*restClient).networkList is unused
(unused)
34-34: func (*restClient).networkStatus is unused
(unused)
113-113: func buildBody is unused
(unused)
43-43: func (*restClient).networkOptions is unused
(unused)
52-52: func (*restClient).block is unused
(unused)
107-107: func withBlockIdentifier is unused
(unused)
63-63: func (*restClient).blockTransaction is unused
(unused)
76-76: func (*restClient).mempool is unused
(unused)
🔇 Additional comments (10)
tests/systemtests/rosetta.go (4)
1-22: LGTM! Clean package structure and imports.The package name aligns with the directory structure, and all imports are relevant for the functionality.
85-85: Address the security risk in subprocess execution.The use of
exec.Commandwith variable inputs needs to be secured against command injection.🧰 Tools
🪛 GitHub Check: gosec
[failure] 85-85: Subprocess launched with variable
Subprocess launched with a potential tainted input or cmd arguments
106-107: Prevent potential panic by checking array length before access.Accessing
Array()[0]without checking if the array is non-empty could cause a runtime panic.
190-202: Fix executable lookup to handle absolute and relative paths correctly.The current implementation may not find executables specified with absolute or relative paths.
scripts/go-lint-all.bash (3)
3-3: Restore stricter error handling options.The removal of
-uand-o pipefailfromsetmakes the script less robust. These options help catch errors early by failing on unset variables and pipeline failures.Apply this diff to restore strict error handling:
-set -e +set -eu -o pipefail
45-49: 🛠️ Refactor suggestionFix conditional test for lint_sdk flag.
The current test
[[ $lint_sdk ]]doesn't properly check the boolean value.Apply this diff:
-if [[ $lint_sdk ]]; then +if [[ $lint_sdk == true ]]; thenLikely invalid or redundant comment.
37-43:⚠️ Potential issueFix array expansion and quoting issues.
The array expansion could cause word splitting, and the
lint_modulecall has unquoted arguments.Apply this diff to fix these issues:
-for dir in ${GIT_DIFF[@]}; do +while IFS= read -r dir; do if [[ ! -f "$REPO_ROOT/$dir/go.mod" ]]; then lint_sdk=true else - lint_module $dir "$@" + lint_module "$dir" "$@" fi -done +done <<< "$GIT_DIFF"Likely invalid or redundant comment.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 37-37: Double quote array expansions to avoid re-splitting elements.
(SC2068)
tests/systemtests/rest_client.go (3)
143-158: Use proper JSON marshaling for request body.The current string concatenation approach for building JSON is error-prone and difficult to maintain.
This issue was previously identified in a past review. Please refer to the previous review comment for the suggested solution using proper JSON marshaling.
87-105: 🛠️ Refactor suggestionAdd address validation and use proper JSON marshaling.
Consider:
- Validating the address format
- Using proper JSON marshaling instead of string concatenation
- Adding response validation
+type AccountRequest struct { + NetworkIdentifier *NetworkIdentifier `json:"network_identifier"` + AccountIdentifier *AccountIdentifier `json:"account_identifier"` + BlockIdentifier *BlockIdentifier `json:"block_identifier,omitempty"` +} + func (c *restClient) accountBalance(address string, opts ...func() string) ([]byte, error) { + if address == "" { + return nil, fmt.Errorf("address cannot be empty") + } + - bodyOpts := make([]string, len(opts)) - for i, opt := range opts { - bodyOpts[i] = opt() - } + req := &AccountRequest{ + AccountIdentifier: &AccountIdentifier{ + Address: address, + }, + }Likely invalid or redundant comment.
34-41: 🛠️ Refactor suggestionUse proper JSON marshaling instead of string concatenation.
Current string concatenation approach for JSON is error-prone. Consider using proper structs and JSON marshaling.
+type NetworkRequest struct { + NetworkIdentifier struct { + Network string `json:"network"` + Blockchain string `json:"blockchain"` + } `json:"network_identifier"` +} + func (c *restClient) networkStatus() ([]byte, error) { - res, err := c.client.R().SetBody(buildBody(c.networkIdentifier)).Post("/network/status") + req := NetworkRequest{} + req.NetworkIdentifier.Network = rosetta.Network + req.NetworkIdentifier.Blockchain = rosetta.Blockchain + + res, err := c.client.R().SetBody(req).Post("/network/status") if err != nil { return nil, err }Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
34-34: func
(*restClient).networkStatusis unused(unused)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/systemtests/construction_test.go (3)
31-32: 🛠️ Refactor suggestionReplace fragile string manipulation with robust key extraction.
The current approach to extract the public key using
strings.Splitand array indexing is fragile and could break if the format changes.Use proper serialization methods:
-hexPk := strings.Split(pubKey.String(), "{")[1] -res, err := rosettaRest.constructionDerive(hexPk[:len(hexPk)-1]) +hexPk := hex.EncodeToString(pubKey.Bytes()) +res, err := rosettaRest.constructionDerive(hexPk)
52-55: 🛠️ Refactor suggestionHandle temporary files cleanup.
The temporary files created are not being cleaned up, which could lead to resource leaks.
Add cleanup:
tempFile := systemtests.StoreTempFile(t, []byte(rsp)) +defer os.Remove(tempFile.Name()) rsp = cli.RunCommandWithArgs(cli.WithTXFlags("tx", "sign", tempFile.Name(), "--from", fromAddr)...) tempFile = systemtests.StoreTempFile(t, []byte(rsp)) +defer os.Remove(tempFile.Name())
74-74: 🛠️ Refactor suggestionReplace fragile string manipulation with robust key extraction.
The same fragile public key extraction is used here as in TestDerive.
Use proper serialization methods:
-hexPk := strings.Split(pubKey.String(), "{")[1] +hexPk := hex.EncodeToString(pubKey.Bytes())
🧹 Nitpick comments (3)
tests/systemtests/construction_test.go (3)
51-51: Consider making the test more robust.The test uses a hardcoded stake amount which could make it flaky if gas prices change. Consider calculating the minimum required amount based on current gas prices.
76-79: Improve type safety of metadata construction.The current metadata construction using string interface map could be error-prone.
Consider creating a typed struct for metadata:
type ConstructionMetadata struct { GasPrice string `json:"gas_price"` GasLimit int `json:"gas_limit"` } metadata := ConstructionMetadata{ GasPrice: "123uatom", GasLimit: 423, }
77-78: Consider making gas values configurable.Hardcoded gas values could make the test flaky if chain parameters change.
Consider fetching these values from chain parameters or making them configurable through test parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/systemtests/construction_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-system
- GitHub Check: rosetta-cli-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
35-37: Improve test robustness and process management.Consider the following improvements:
- Replace the hardcoded sleep with proper health checking
- Add proper cleanup of background processes
- Consider using a trap for cleanup on script exit
test-rosetta-ci: sh ./scripts/simapp-start-node.sh make build && make plugin - ./build/rosetta --blockchain "cosmos" --network "cosmos" --tendermint "tcp://localhost:26657" --addr "localhost:8080" --grpc "localhost:9090" & - sleep 30 - export SIMD_BIN=./cosmos-sdk/build/simd && sh ./tests/rosetta-cli/rosetta-cli-test.sh + ROSETTA_PID="" && \ + trap 'kill $$ROSETTA_PID 2>/dev/null' EXIT && \ + ./build/rosetta --blockchain "cosmos" --network "cosmos" --tendermint "tcp://localhost:26657" --addr "localhost:8080" --grpc "localhost:9090" & ROSETTA_PID=$$! && \ + until curl -s http://localhost:8080/network/list > /dev/null; do \ + echo "Waiting for rosetta to start..." && \ + sleep 2; \ + done && \ + export SIMD_BIN=./cosmos-sdk/build/simd && sh ./tests/rosetta-cli/rosetta-cli-test.sh
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-rosetta
- GitHub Check: test-system
- GitHub Check: build (amd64)
- GitHub Check: Gosec
- GitHub Check: golangci-lint
- GitHub Check: rosetta-cli-test
🔇 Additional comments (2)
Makefile (2)
6-7: LGTM! Build directory structure improvements.Good improvement to ensure consistent binary location and proper directory structure.
21-31: Enhance error handling and simplify version extraction.The test-system target implementation needs improvements in error handling and maintainability.
This PR adds system-tests for various Rosetta endpoints.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores