test: add deployment functional test #73
Conversation
WalkthroughAdds TypeScript functional test suites and docs, local testnet provisioning (Make targets and make/local-node.mk), CI job and scripts for running functional tests, Jest functional-runner change to serial, tooling wrappers with localStorage polyfills, and minor repo ignores and package updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI / Developer
participant Make as Make targets
participant LocalNode as Local Akash Node
participant Jest as Jest functional tests
participant SDK as Akash SDK Client
participant Chain as Akash Chain RPC
CI->>Make: make test-functional-ts-local (TEST_MNEMONIC set)
Make->>LocalNode: local-node-install-akash / local-node-init / local-node-run
LocalNode-->>Make: ready / RPC endpoint available
Make->>Jest: npm run test:functional (env: endpoints, TEST_MNEMONIC)
Jest->>SDK: initialize client & wallet from TEST_MNEMONIC
loop Per test
Jest->>Chain: Query deployments/bids/leases via SDK
Chain-->>SDK: Query responses
SDK-->>Jest: data
alt E2E deploy/lease flow
Jest->>SDK: Broadcast MsgCreateDeployment / MsgCreateLease
SDK->>Chain: Broadcast tx
Chain-->>SDK: Tx result
SDK-->>Jest: afterSign / afterBroadcast callbacks
loop Poll bids
Jest->>Chain: Query bids(dseq)
Chain-->>Jest: bids page
end
end
end
Note right of Make: afterAll cleanup -> close deployments (if TEST_MNEMONIC)
CI->>Make: make local-node-clean
Make->>LocalNode: stop & remove data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
C:ts
C:go/node
Type: CI
Type: Build
C:go/cli
C:go/sdl
C:specs
labels
* doc: adds provider authenticate details to README * Update ts/README.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
ts/test/functional/deployments.spec.ts (4)
18-18: BinaryWriter import is incompatible with ts-proto encode/decode.This issue was previously flagged. The
BinaryWriterfrom@bufbuild/protobuf/wireis incompatible with ts-proto styleencodemethods, which expect a protobufjs Writer. Remove this import and update the encode logic in lines 262-264 to let ts-proto create its own writer internally.
32-33: Network mismatch between query and tx RPC defaults.This issue was previously flagged. Using different default endpoints for
QUERY_RPC_URLandTX_RPC_URLwill cause chain-id and sequence mismatches during transaction broadcasts. Both should default to the same endpoint.
37-40: Null signer can cause runtime errors.This issue was previously flagged. Passing
signer: wallet || null as anyis a runtime hazard if the SDK touches the tx client lazily. Only include thetxconfig when a wallet is provided.
262-264: BinaryWriter usage is incorrect and will affect snapshot.This issue was previously flagged. The
BinaryWriterusage here is incompatible with ts-proto. Once fixed (by removing BinaryWriter and using ts-proto's internal writer), the expected base64 snapshot at line 272 will need to be regenerated to reflect the correct encoding.
🧹 Nitpick comments (2)
ts/test/functional/deployments.spec.ts (2)
44-113: Consider using transaction lock for cleanup operations.The
cleanupDeploymentsfunction performs multiple blockchain transactions (closing deployments) but does not acquire the transaction lock fromtestUtils. While the function includes a 6-second delay between closures, consider wrapping the cleanup logic withtestUtils.withTransactionLockto ensure proper serialization with other tests and prevent potential race conditions.Apply this approach:
const cleanupDeployments = async () => { const testMnemonic = process.env.TEST_MNEMONIC; if (!testMnemonic) { console.log("Skipping deployment cleanup - TEST_MNEMONIC not set"); return; } + await testUtils.withTransactionLock(async () => { try { const wallet = await DirectSecp256k1HdWallet.fromMnemonic(testMnemonic, { prefix: "akash" }); // ... rest of cleanup logic } catch (error) { console.log("Error during deployment cleanup:", error); } + }); };
165-177: Remove unnecessary type cast and clarify test intent.The test casts the response to
any(line 170) which weakens type safety without clear benefit. Additionally, the test name suggests it handles empty results, but the assertiontoBeGreaterThanOrEqual(0)always passes. Consider either:
- Removing the cast and testing with a filter that guarantees no results, or
- Renaming the test to better reflect that it validates response structure regardless of content.
Apply this diff to remove the cast:
- const response = await sdk.akash.deployment.v1beta4.getDeployments({ - pagination: { limit: 1 }, - }) as any; + const response = await sdk.akash.deployment.v1beta4.getDeployments({ + pagination: { limit: 1 }, + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ts/test/functional/deployments.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ts/test/functional/deployments.spec.ts (1)
ts/test/helpers/testOrchestrator.ts (2)
wait(164-166)testUtils(207-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: go
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (7)
ts/test/functional/deployments.spec.ts (7)
115-121: LGTM! Proper test orchestration initialization and cleanup.The
beforeAllhook correctly resets the test orchestrator state, and theafterAllhook appropriately sets a long timeout (300 seconds) to accommodate multiple deployment closures with delays.
123-146: LGTM! Basic deployment query test with proper assertions.The test correctly queries deployments with pagination, validates the response structure, and includes helpful console logging for debugging.
148-163: LGTM! Pagination query test with conditional assertions.The test correctly demonstrates pagination with
countTotaland appropriately handles the conditional presence of the pagination object in the response.
179-193: LGTM! Essential SDK structure verification.This test correctly validates that all expected modules and methods are present in the SDK instance, which is a good sanity check before running functional tests.
195-283: Good serialization consistency test, but snapshot will change after BinaryWriter fix.This snapshot test is a valuable practice for detecting breaking changes in protobuf serialization. However, note that once the BinaryWriter issue (lines 262-264) is resolved, the expected base64 value at line 272 must be regenerated to match the correct ts-proto encoding.
After fixing the BinaryWriter usage, regenerate the expected snapshot by:
- Running the test to get the new base64 output.
- Updating line 272 with the new expected value.
- Verifying the decode assertions still pass.
285-406: Verify that assertions in callbacks fail the test properly.The test includes assertions within
afterSign(lines 375-381) andafterBroadcast(lines 383-387) callbacks. In Jest, assertions in callbacks may not always propagate failures correctly depending on how the SDK handles these callbacks. Consider either:
- Moving critical assertions outside the callbacks (e.g., verifying the result object after the transaction completes), or
- Confirming that the SDK properly re-throws errors from callbacks.
Otherwise, the test structure is solid, with proper transaction locking, resource management, and environment-gated execution.
Confirm that assertions in
afterSignandafterBroadcastcallbacks will fail the test if they throw. If the SDK silently catches callback errors, move assertions to post-transaction checks.
408-412: LGTM! Dedicated cleanup test with transaction lock.This test ensures cleanup runs with proper transaction serialization using
withTransactionLock, which is good practice. The 300-second timeout is appropriate for multiple deployment closures.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
ts/test/functional/deployments.spec.ts (5)
18-18: [Previously flagged] BinaryWriter incompatibility persists.This issue was flagged in a previous review. The
BinaryWriterimport from@bufbuild/protobuf/wireis incompatible with ts-proto style encode/decode methods.
31-32: [Previously flagged] Network mismatch between query and TX defaults.This issue was flagged in a previous review. The defaults point to different chains (dev vs testnet).
36-39: [Previously flagged] Null signer hazard persists.This issue was flagged in a previous review. Using
null as anyfor the signer can cause runtime errors.
130-135: [Previously flagged] Duplicate assertion.This issue was flagged in a previous review. The assertion
expect(response?.deployments).toBeDefined()appears on lines 130 and 135.
258-260: [Previously flagged] BinaryWriter usage in encode.This usage of
BinaryWriterwas flagged in the previous review as incompatible with ts-proto.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ts/test/functional/deployments.spec.ts(1 hunks)ts/test/functional/leases.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: go
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (5)
ts/test/functional/deployments.spec.ts (3)
43-112: Well-structured cleanup with appropriate error handling.The cleanup function properly handles edge cases (already-closed deployments) and includes reasonable delays between operations for blockchain propagation.
191-279: Well-designed serialization test with deterministic values.The snapshot-based approach using deterministic resource values is excellent for detecting unintended serialization changes. The decode verification adds confidence in the test.
281-396: Comprehensive end-to-end test with good practices.The test includes:
- Proper environment variable gating
- Clear user instructions for funding
- Unique deployment sequence numbers via timestamp
- Transaction structure validation via callbacks
- Account structure assertions
This demonstrates good functional test design.
ts/test/functional/leases.spec.ts (2)
116-152: Well-designed bid polling with retry logic.The polling mechanism includes:
- Reasonable retry delays (6 seconds) for blockchain propagation
- Clear logging for debugging
- Graceful handling of different bid counts
- Helpful error messages for troubleshooting
This is good functional test design.
31-213: Comprehensive end-to-end lease workflow test.This test demonstrates a complete lease lifecycle:
- Deployment creation
- Bid polling with retries
- Bid selection
- Lease creation
- Verification
The environment gating and extensive logging make this test production-ready for CI/CD while remaining debuggable when run locally.
| owner: account.address, | ||
| dseq: Long.fromNumber(Date.now()) // Use timestamp for uniqueness | ||
| }, | ||
| groups: [{ |
There was a problem hiding this comment.
suggestion: use SDL.fromString instead of manually defining this groups
| console.log(` - Transaction result:`, result); | ||
|
|
||
| // Verify the response structure - these assertions are required for test to pass | ||
| expect(result).toBeDefined(); |
There was a problem hiding this comment.
question: does it make sense to check that result is defined? I guess for all transactions it's an Empty proto message and there is no point to use from the caller side
| expect(account.address).toMatch(/^akash1[a-z0-9]{38}$/); | ||
| expect(account.pubkey).toHaveLength(33); // Compressed secp256k1 pubkey |
There was a problem hiding this comment.
this is redundant because here you test cosmjs not our sdk.
| it("should cleanup all deployments for the test account", async () => { | ||
| await cleanupDeployments(); | ||
| }, 300000); |
There was a problem hiding this comment.
suggestion: you can remove it because it's redundant. You do this in afterAll if that callback fails, it will fail the test suite.
There was a problem hiding this comment.
Please change this according to the comments to deployments.spec.ts
ts/test/functional/README.md
Outdated
| **Important Security Notes:** | ||
| - Only use testnet accounts with test tokens | ||
| - Never use production mnemonics in tests | ||
| - The test will skip gracefully if TEST_MNEMONIC is not set |
There was a problem hiding this comment.
This approach is error-prone.
We may see tests passing, but only because no mnemonics is set and the tests are skipped.
Mnemonics are required for the TX tests, so let's return an error if it is missing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ts/script/protoc-gen-customtype-patches.ts (1)
3-25: Consider consolidating the localStorage polyfill implementations.This file includes an inline localStorage polyfill (lines 3-25), but the wrapper script (protoc-gen-customtype-patches-wrapper.sh) already preloads an external polyfill (localStorage-polyfill.js) via NODE_OPTIONS. This creates redundancy and maintenance burden.
Additionally, the two implementations use different approaches:
- External: Direct assignment (
globalThis.localStorage = {...})- Inline:
Object.definePropertywithwritableandconfigurableflagsConsider choosing one approach:
- Preferred: Keep only the external polyfill loaded via NODE_OPTIONS in the wrapper, and remove lines 3-25 from this file.
- Alternative: Remove the wrapper's NODE_OPTIONS preload and keep only the inline polyfill.
.github/workflows/tests.yaml (2)
65-67: Address shellcheck SC2086 warning.While the current code works, quoting the variable prevents potential issues with word splitting.
- run: | - toolchain=$(./script/tools.sh gotoolchain | sed 's/go*//') - echo "GOVERSION=${toolchain}" >> $GITHUB_ENV + toolchain="$(./script/tools.sh gotoolchain | sed 's/go*//')" + echo "GOVERSION=${toolchain}" >> "$GITHUB_ENV"
93-95: Consider addinglocal-node-cleanfor complete cleanup.The current cleanup only stops the node but doesn't remove state. For CI reproducibility, consider also cleaning node data, though this may not be strictly necessary since CI runners are ephemeral.
- name: Cleanup local node if: always() - run: make local-node-stop || true + run: make local-node-clean || truemake/local-node.mk (1)
37-39: Network call at make parse time can cause issues.The
curlto GitHub API runs during make's variable expansion phase, which can cause:
- Build failures when GitHub API is unreachable or rate-limited
- Slower make invocations even for unrelated targets
Consider making this lazy by using a target-specific recipe instead.
-LOCAL_AKASH_VERSION ?= $(shell curl -s https://api.github.com/repos/akash-network/node/releases/latest 2>/dev/null | grep -o '"tag_name": "[^"]*"' | head -1 | cut -d'"' -f4 | sed 's/^v//' || echo "0.28.0") +LOCAL_AKASH_VERSION ?= 0.38.0Alternatively, fetch the version lazily within the install target, or pin to a specific version for reproducibility. A pinned version also ensures consistent test behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ts/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/tests.yaml(1 hunks).gitignore(1 hunks)Makefile(1 hunks)make/local-node.mk(1 hunks)make/test.mk(1 hunks)ts/package.json(2 hunks)ts/script/localStorage-polyfill.js(1 hunks)ts/script/protoc-gen-customtype-patches-wrapper.sh(1 hunks)ts/script/protoc-gen-customtype-patches.ts(1 hunks)ts/script/protoc-gen-sdk-object-wrapper.sh(1 hunks)ts/script/protoc-gen-sdk-object.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- Makefile
- ts/script/protoc-gen-sdk-object.ts
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/tests.yaml
65-65: shellcheck reported issue in this script: SC2086:info:2:34: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:5:42: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: proto
- GitHub Check: go
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (11)
ts/script/protoc-gen-sdk-object-wrapper.sh (1)
1-8: LGTM!The wrapper script correctly sets up the localStorage polyfill environment before executing the TypeScript plugin. The SCRIPT_DIR calculation is robust, NODE_OPTIONS preloads the polyfill, and exec properly replaces the process while forwarding arguments.
ts/script/protoc-gen-customtype-patches-wrapper.sh (1)
1-8: LGTM!The wrapper script follows the same correct pattern as protoc-gen-sdk-object-wrapper.sh. The consistent approach across wrappers improves maintainability.
ts/script/localStorage-polyfill.js (1)
1-10: LGTM!The polyfill implementation is correct and provides a safe fallback for environments without native localStorage support. The guard logic properly checks for undefined before accessing properties, avoiding potential errors.
Note: The redundancy between this external polyfill and the inline polyfills in the TypeScript files has been flagged in the protoc-gen-customtype-patches.ts review.
ts/package.json (2)
40-40: LGTM! Serial execution is appropriate for functional tests.Using
--runInBandensures functional tests run sequentially, which is correct when tests interact with shared resources like a local blockchain node.
70-70: I'll help you verify the @bufbuild/buf version. Let me start by executing the verification script to check the latest version and any security advisories.
<function_calls>
#!/bin/bashCheck for latest version and any security advisories for @bufbuild/buf
curl -s https://registry.npmjs.org/@bufbuild/buf/latest | jq '{name: .name, version: .version, deprecated: .deprecated}'
</function_calls>I also want to verify if this package is actually being used in the codebase and check for any related buf configuration files:
<function_calls>
#!/bin/bashSearch for buf usage in the codebase
rg -i "@bufbuild" --type json --type ts -C 2
echo "---"
echo "Looking for buf configuration files:"
fd -e "buf.yaml" -e "buf.gen.yaml" -e "buf.lock"
</function_calls>Let me also check the broader context of the ts/package.json to understand if this is a dev dependency and what other protobuf-related tooling is in use:
<function_calls>
#!/bin/bashShow the section around the buf dependency
sed -n '60,80p' ts/package.json
</function_calls>.github/workflows/tests.yaml (1)
82-88: Using well-known test mnemonic is appropriate for CI.The "abandon abandon..." mnemonic is a standard BIP39 test vector. The comment clearly indicates this is intentional and not for production use, which is good documentation.
make/local-node.mk (4)
47-69: LGTM! Robust installation logic with fallbacks.The installation target properly:
- Checks for existing installation before downloading
- Falls back from wget to curl if needed
- Provides helpful manual installation instructions on failure
- Cleans up temporary files
97-104: Empty password for key creation is fine for test keyring.The
echo "" | ... keys addpattern works with--keyring-backend=test. The|| trueprevents errors if the key already exists, which is appropriate.
134-157: LGTM! Good startup logic with readiness check.The node startup properly:
- Checks if already running before starting
- Stores PID for lifecycle management
- Polls for readiness with a 30-second timeout
- Provides helpful log location on failure
159-170: LGTM! Graceful shutdown with force-kill fallback.The two-phase shutdown (SIGTERM, wait, SIGKILL) is the correct pattern for process cleanup.
make/test.mk (1)
39-51: All claims verified and accurate.The review comment correctly identifies well-structured targets. Verification confirms:
setup-local-testnet.shexists and is executable atts/test/functional/setup-local-testnet.shlocal-node-readytarget properly defined inmake/local-node.mkwith correct dependenciesAKASH_TS_NODE_MODULESis a valid target defined inmake/setup-cache.mkLOCAL_NODE_HOMEproperly exported inmake/local-node.mkTEST_MNEMONICguard clause correctly prevents execution without the required variable- Targets follow proper dependency chains and environment setup practices
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ts/test/functional/deployments.spec.ts (2)
18-18: BinaryWriter from@bufbuild/protobuf/wireis incompatible with ts-proto.The past review correctly identified this issue. ts-proto's
encodemethod expects a protobufjs-style Writer, not@bufbuild/protobuf/wireBinaryWriter. This can cause runtime errors.The issue was previously flagged. The fix is to let ts-proto create its own writer:
-import { BinaryWriter } from "@bufbuild/protobuf/wire";And update the encoding call at lines 272-274:
- const writer = new BinaryWriter(); - MsgCreateDeployment.encode(deploymentRequest, writer); - const encoded = writer.finish(); + const encoded = MsgCreateDeployment.encode(deploymentRequest).finish();
170-172: Conditional assertion makes test unreliable.The
if (response?.pagination)check allows the test to pass regardless of whether pagination is present. This was flagged in previous reviews.Either pagination should always be defined (and assert unconditionally), or the test should explicitly handle both cases with clear expectations.
🧹 Nitpick comments (10)
make/local-node.mk (2)
37-39: Network request at variable expansion time can slow down Make invocation.
LOCAL_AKASH_VERSIONuses$(shell curl ...)which executes on every Make invocation, even for unrelated targets. This can cause delays or failures if GitHub is unreachable.Consider making this a lazy evaluation or caching the result:
-LOCAL_AKASH_VERSION ?= $(shell curl -s https://api.github.com/repos/akash-network/node/releases/latest 2>/dev/null | grep -o '"tag_name": "[^"]*"' | head -1 | cut -d'"' -f4 | sed 's/^v//' || echo "0.28.0") +LOCAL_AKASH_VERSION ?= 0.36.0Alternatively, move the version fetch into the
local-node-install-akashtarget itself to avoid the network call during Make parsing.
159-170: Graceful shutdown could leave orphan processes in edge cases.The stop logic sends SIGTERM, waits 2 seconds, then SIGKILL. However, if the process spawned child processes, they may not be cleaned up. Consider using process groups.
For more robust cleanup, consider killing the process group:
@if [ -f "$(LOCAL_NODE_PID)" ]; then \ pid=$$(cat $(LOCAL_NODE_PID)); \ if kill -0 $$pid 2>/dev/null; then \ echo "Stopping local node (PID: $$pid)..."; \ - kill $$pid || true; \ + kill -TERM -$$pid 2>/dev/null || kill $$pid || true; \ sleep 2; \ - kill -9 $$pid 2>/dev/null || true; \ + kill -9 -$$pid 2>/dev/null || kill -9 $$pid 2>/dev/null || true; \ fi; \ rm -f $(LOCAL_NODE_PID); \ fi.github/workflows/tests.yaml (1)
65-67: Quote variable to prevent word splitting (shellcheck SC2086).The
toolchainvariable should be quoted to prevent word splitting issues.- run: | toolchain=$(./script/tools.sh gotoolchain | sed 's/go*//') - echo "GOVERSION=${toolchain}" >> $GITHUB_ENV + echo "GOVERSION=${toolchain}" >> "$GITHUB_ENV"ts/test/functional/deployments.spec.ts (5)
48-53: Consider logging when REST API check is skipped.The silent catch for non-balance errors could hide unexpected failures. While the comment explains the intent (gRPC-only setups), a debug log would aid troubleshooting.
} catch (error) { if (error instanceof Error && error.message.includes("Insufficient balance")) { throw error; } - // If REST API is not available, skip balance check (might be using gRPC only) + // If REST API is not available, skip balance check (might be using gRPC only) + console.debug(`Skipping balance check: ${error instanceof Error ? error.message : 'REST API unavailable'}`); }
129-131: Silent cleanup failure may hide real issues.Past reviews suggested letting errors propagate to Jest. While cleanup failures shouldn't break tests, completely silencing errors loses diagnostic information.
} catch (error) { - // Silently fail cleanup - don't break tests + // Log cleanup errors but don't fail tests + console.warn('Cleanup warning:', error instanceof Error ? error.message : error); }
178-185: Cast toanyand tautological assertion reduce test value.Line 180's
as anycast bypasses type safety. Line 185'stoBeGreaterThanOrEqual(0)is always true for array lengths (which are non-negative by definition).- const response = await sdk.akash.deployment.v1beta4.getDeployments({ - pagination: { limit: 1 }, - }) as any; + const response = await sdk.akash.deployment.v1beta4.getDeployments({ + pagination: { limit: 1 }, + }); // Should handle both empty responses and empty deployment lists expect(response?.deployments).toBeDefined(); expect(Array.isArray(response?.deployments)).toBe(true); - expect(response?.deployments?.length || 0).toBeGreaterThanOrEqual(0); + // For empty result handling, just verify the array exists
279-285: Consider using Jest snapshots for serialization consistency.Past reviews suggested using
expect(base64Encoded).toMatchSnapshot()instead of hardcoded expected values. Snapshots are easier to update when serialization legitimately changes.- // Expected base64 - this will be the reference value to detect serialization changes - // This is a snapshot test - if the serialization format changes, this test will fail - // indicating a potential breaking change in the API - const expectedBase64 = "CjIKLWFrYXNoMXRlc3QxMjM0NTY3ODlhYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ehDSCRJcCgp0ZXN0LWdyb3VwEgIKABpKCjcIARIHCgUKAzEwMBoNCgsKCTEzNDIxNzcyOCIUCgRtYWluEgwKCjIxNDc0ODM2NDgqBQoDCgEwEAEaDQoEdWFrdBIFMTAwMDAaBAECAwQiEQoPCgR1YWt0Egc1MDAwMDAw"; - - // Assert the serialization matches expected value - expect(base64Encoded).toBe(expectedBase64); + // Use snapshot testing for serialization consistency + expect(base64Encoded).toMatchSnapshot();
397-399: Cleanup test is redundant with afterAll.Past reviews noted this is redundant since
cleanupDeploymentsis called inafterAll. However, having it as an explicit test allows for manual cleanup runs, so this is a minor style preference.Consider removing if the afterAll cleanup is sufficient for your workflow.
ts/test/functional/setup-local-testnet.sh (2)
83-94: Usejqinstead ofgrepfor JSON parsing.The current grep-based parsing is fragile and may break with different JSON formatting. Since
jqis installed as a system dependency in the CI workflow, use it here for reliability.get_balance() { local address="$1" local balance=0 if response=$(curl -s "${REST_API_URL}/cosmos/bank/v1beta1/balances/${address}" 2>/dev/null); then - if uakt_balance=$(echo "$response" | grep -o '"denom":"uakt"[^}]*"amount":"[^"]*"' | grep -o '"amount":"[^"]*"' | cut -d'"' -f4); then + if uakt_balance=$(echo "$response" | jq -r '.balances[] | select(.denom=="uakt") | .amount // "0"' 2>/dev/null); then balance="${uakt_balance:-0}" fi fi echo "${balance}" }
125-163: Significant code duplication between AKASH_HOME branches.The
tx bank sendcommand is repeated with minor differences. Building the command arguments conditionally would reduce duplication and maintenance burden.# Send tokens + local home_args=() + if [ -n "${AKASH_HOME:-}" ]; then + home_args=(--home "${AKASH_HOME}") + fi + + if "${AKASH_TO_USE}" tx bank send "${from_address}" "${to_address}" "${amount}" \ + --chain-id "${CHAIN_ID}" \ + --node "${AKASH_NODE}" \ + --keyring-backend test \ + --from "${GENESIS_ACCOUNT}" \ + --yes \ + --gas auto \ + --gas-adjustment 2.0 \ + --gas-prices 0.025uakt \ + "${home_args[@]}" > /dev/null 2>&1; then + log_info "Successfully funded account" + log_info "Waiting for transaction to be included in a block..." + sleep 3 + return 0 + else + log_error "Failed to fund account" + return 1 + fi - if [ -n "${AKASH_HOME}" ]; then - if "${AKASH_TO_USE}" tx bank send "${from_address}" "${to_address}" "${amount}" \ - ... # (duplicate block removed) - else - if "${AKASH_TO_USE}" tx bank send "${from_address}" "${to_address}" "${amount}" \ - ... # (duplicate block removed) - fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/tests.yaml(1 hunks)make/local-node.mk(1 hunks)make/test.mk(1 hunks)ts/test/functional/deployments.spec.ts(1 hunks)ts/test/functional/leases.spec.ts(1 hunks)ts/test/functional/setup-local-testnet.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- make/test.mk
- ts/test/functional/leases.spec.ts
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/tests.yaml
65-65: shellcheck reported issue in this script: SC2086:info:2:34: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:5:42: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: go
🔇 Additional comments (6)
make/local-node.mk (1)
109-109: Previous governance path issue has been addressed.The jq filter now correctly uses
.app_state.gov.params.voting_periodwhich aligns with the actual Akash genesis structure..github/workflows/tests.yaml (2)
81-88: Well-known test mnemonic is appropriate for CI.Using the standard BIP39 test vector ("abandon abandon...about") is a common and safe practice for CI testing. The comment correctly notes this is not for production use.
93-95: LGTM!The cleanup step correctly uses
if: always()to ensure the local node is stopped even if tests fail, and the|| trueprevents cleanup failures from masking test results.ts/test/functional/deployments.spec.ts (1)
139-158: LGTM!The deployment query test properly validates the response structure and deployment fields.
ts/test/functional/setup-local-testnet.sh (2)
1-2: LGTM! Good use of strict mode.
set -euo pipefailensures the script fails fast on errors, unset variables, and pipeline failures.
167-223: Main flow is well-structured.The script follows a clear progression: verify node is running, derive test address, check balance, fund if needed, and confirm success. Error handling and user messaging are helpful.
| deposit: { | ||
| amount: { | ||
| denom: "uakt", | ||
| amount: "500000" // 5 AKT deposit | ||
| } as Coin, |
There was a problem hiding this comment.
Incorrect comment: 500000 uakt is 0.5 AKT, not 5 AKT.
The comment states "5 AKT deposit" but 500000 uakt equals 0.5 AKT (1 AKT = 1,000,000 uakt).
amount: {
denom: "uakt",
- amount: "500000" // 5 AKT deposit
+ amount: "500000" // 0.5 AKT deposit
} as Coin,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deposit: { | |
| amount: { | |
| denom: "uakt", | |
| amount: "500000" // 5 AKT deposit | |
| } as Coin, | |
| deposit: { | |
| amount: { | |
| denom: "uakt", | |
| amount: "500000" // 0.5 AKT deposit | |
| } as Coin, |
🤖 Prompt for AI Agents
In ts/test/functional/deployments.spec.ts around lines 361 to 365, the inline
comment incorrectly states "5 AKT deposit" for amount: "500000"; update the
comment to the correct value (0.5 AKT) or adjust the numeric amount to match the
comment. Specifically either change the comment to "0.5 AKT deposit" to reflect
500000 uakt, or change amount to "5000000" to represent 5 AKT if the intent was
5 AKT.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
make/local-node.mk (1)
84-133: Genesis mutation looks good; align funding and gas price with existing configThis block does a nice job making
local-node-initidempotent, creating keys only when missing, and mutating genesis via jq. In particular, switching to.app_state.gov.params.voting_period(Line 109) correctly matches the current Akash genesis layout and addresses the earlier review about usingvoting_params. ✅A couple of follow‑ups to keep things consistent:
LOCAL_CHAIN_ACCOUNT_DEPOSITis defined as10xthe min deposit (Line 22) but genesis accounts are currently funded with exactlyLOCAL_CHAIN_MIN_DEPOSIT(Line 118). If the intent is to give test keys more headroom (multiple proposals, fees, etc.), consider actually using the account‑level variable:$$AKASH_TO_USE genesis add-account "$$key_addr" "$(LOCAL_CHAIN_MIN_DEPOSIT)$(LOCAL_CHAIN_TOKEN_DENOM)" --home $(LOCAL_NODE_HOME) >/dev/null 2>&1 || true; \
$$AKASH_TO_USE genesis add-account "$$key_addr" "$(LOCAL_CHAIN_ACCOUNT_DEPOSIT)$(LOCAL_CHAIN_TOKEN_DENOM)" --home $(LOCAL_NODE_HOME) >/dev/null 2>&1 || true; \
In the gentx call you hard‑code
--gas-prices=0.025uakt(Line 125) instead of reusingAKASH_GAS_PRICES. To avoid denom drift if you ever changeLOCAL_CHAIN_TOKEN_DENOMor the price, it’s safer to route through the same variable:--gas-prices=0.025uakt \
--gas-prices=$(AKASH_GAS_PRICES) \These tweaks keep the whole “local chain economics” story driven from the small set of config vars at the top of the file.
🧹 Nitpick comments (3)
make/local-node.mk (3)
4-25: Tighten chain param config and remove unusedLOCAL_CHAIN_ACCOUNT_DEPOSITThe configuration block is generally clean, but there are a couple of small nits:
LOCAL_CHAIN_ACCOUNT_DEPOSIT(Line 22) is computed but never used anywhere below. Either use it when funding genesis accounts or drop it to avoid confusing future readers.- If you ever change
LOCAL_CHAIN_TOKEN_DENOMaway fromuakt,AKASH_GAS_PRICES(Line 13) and the hard‑coded0.025uaktin the gentx command (Line 125) will drift. Consider expressing gas prices in terms ofLOCAL_CHAIN_TOKEN_DENOMto keep the configuration single‑sourced.
30-40: Hardenlocal-node-install-akashfor CI/offline use and arch namingThe auto‑install flow is handy, but a few details are worth tightening:
LOCAL_AKASH_VERSION(Line 38) depends on a GitHub API call and then silently falls back to0.28.0on any failure. In CI or offline environments this can lead to surprising downgrades. Consider:
- Logging explicitly when the fallback is used, or
- Preferring a pinned default and requiring callers to override via
LOCAL_AKASH_VERSION/env when they want “latest”.- On macOS you set
AKASH_INSTALL_ARCH := all(Lines 33–35) and construct the asset URL as.../akash_$(UNAME_OS_LOWER)_$(AKASH_INSTALL_ARCH).zip(Line 56). Please double‑check that current Akash node release artifacts actually ship aakash_darwin_all.zip; if not, this will always fail and drop users into the manual install path.- This target assumes
unzipis present. If you expect contributors/CI images that may not have it, a short pre‑flight check with a clear error message would make failures easier to diagnose.Also applies to: 47-69
134-204: Process management and readiness loop look solid; minor PID safety nitThe
local-node-run,local-node-stop,local-node-status, andlocal-node-cleantargets form a coherent lifecycle:
- PID tracking, a 60‑second readiness loop that tails logs on failure, and
curl /statusprobing are all well thought‑out.local-node-stop(Lines 173–184) gracefully sendsSIGTERM, waits, thenSIGKILLs if needed.One small robustness improvement you might consider:
- Before sending
kill -9, guard against stale PID files by verifying the process really looks like an Akash node (e.g.,ps -p "$pid" -o comm= | grep -q akash). That avoids the edge case of a recycled PID pointing at an unrelated local process.Not a blocker for this PR, but a nice hardening if you expect this to be used heavily on shared dev machines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
make/local-node.mk(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: go
- GitHub Check: proto
- GitHub Check: coverage
🌟 PR Title
[Use a clear, concise title that reflects the change]
📝 Description
[Explain what this PR does in 2-3 sentences. Include context about the feature or problem being solved]
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]