feat(cmd/testnetify): backport testnetify from live blockchain state #1995
Conversation
WalkthroughThis update refactors and replaces the previous "testnetify" CLI tool and its supporting code with a new, integrated mechanism for initializing and running a testnet instance for the Akash blockchain. It introduces new configuration structures, application initialization logic, and command-line utilities, while removing the old testnetify state manipulation code and related utilities. Dependencies are updated, and a sample testnet configuration file is added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant App
participant DB
participant Node
User->>CLI: Run testnetify command with config
CLI->>Config: Load testnet config (validators, accounts, gov, upgrade)
CLI->>DB: Open local database and state
CLI->>App: Initialize AkashApp with testnet config
App->>App: Reset validators, set up accounts, apply gov/upgrade
CLI->>Node: Start blockchain node with modified state
Node-->>User: Node runs testnet with new parameters
Estimated code review effort5 (~150 minutes) Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
💤 Files with no reviewable changes (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code Graph Analysis (2)cmd/akash/cmd/testnetify/config.go (1)
app/testnet.go (4)
⏰ 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). (7)
🔇 Additional comments (13)
✨ Finishing Touches
🧪 Generate unit tests
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
app/testnet.go (1)
52-55: Duplicate function should be refactored.This is a duplicate of the
TrimQuotesfunction incmd/akash/cmd/testnetify/config.go.
🧹 Nitpick comments (7)
tests/upgrade/testnet.json (1)
1-25: Appropriate testnet configuration for local testing.This configuration file provides a sensible setup for local testnet testing:
- Short 60-second voting period enables rapid governance testing
- Validator commission rates are within reasonable bounds
- Single validator setup is sufficient for local development
Consider adding a comment at the top of the file explaining its purpose and usage context.
+// Local testnet configuration for upgrade testing +// Used by the testnetify command to create a local testnet environment { "chain_id": "localakash",cmd/akash/cmd/app_creator.go (1)
16-31: Well-structured testnet app creator with clear separation of concerns.The function properly implements the testnet app creation pattern by:
- Leveraging the existing
newAppfunction- Type-safe casting with appropriate validation
- Extracting configuration and initializing testnet-specific state
Consider improving the panic messages for better debugging:
- panic("app created from newApp is not of type AkashApp") + panic("failed to cast app to *akash.AkashApp - ensure newApp returns correct type")- panic("cflags.KeyTestnetConfig is not of type akash.TestnetConfig") + panic("testnet config not found or invalid type - ensure KeyTestnetConfig contains *akash.TestnetConfig")cmd/akash/cmd/testnetify/config.go (1)
67-70: Extract TrimQuotes to a common utility package.This function is duplicated in
app/testnet.go. Consider moving it to a shared utility package to follow DRY principles.Would you like me to create a common utility package and refactor both occurrences to use the shared function?
app/testnet.go (2)
133-134: Consider making validator parameters configurable.Hardcoded values for tokens, delegator shares, and commission rates should be configurable through the TestnetValidator struct.
- Tokens: sdk.NewInt(900000000000000), - DelegatorShares: sdk.MustNewDecFromStr("10000000"), + Tokens: val.Tokens, // Add to TestnetValidator + DelegatorShares: val.DelegatorShares, // Add to TestnetValidator- Rate: sdk.MustNewDecFromStr("0.05"), - MaxRate: sdk.MustNewDecFromStr("0.1"), - MaxChangeRate: sdk.MustNewDecFromStr("0.05"), + Rate: val.Commission.CommissionRates.Rate, + MaxRate: val.Commission.CommissionRates.MaxRate, + MaxChangeRate: val.Commission.CommissionRates.MaxChangeRate,Also applies to: 140-142
76-249: Consider refactoring this function into smaller, focused functions.The function is quite long (170+ lines) and handles multiple responsibilities. Consider breaking it down into smaller functions like
clearValidators,setupValidators,applyGovernanceConfig,mintAccountCoins, andscheduleUpgrade.cmd/akash/cmd/testnetify/testnetify.go (2)
176-176: Make timeouts configurable.Hardcoded 1-minute timeouts might not be sufficient for all scenarios.
Consider adding command flags for these timeout values:
cmd.Flags().Duration("block-wait-timeout", 1*time.Minute, "Timeout for waiting for first block") cmd.Flags().Duration("block-commit-timeout", 1*time.Minute, "Timeout for waiting for block commit")Also applies to: 196-196
452-452: Make validator voting power configurable.The hardcoded voting power value should be configurable.
- VotingPower: 900000000000000, + VotingPower: val.VotingPower, // Add to TestnetValidator struct
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
app/testnet.go(1 hunks)cmd/akash/cmd/app_creator.go(1 hunks)cmd/akash/cmd/root.go(1 hunks)cmd/akash/cmd/testnetify/accounts.go(0 hunks)cmd/akash/cmd/testnetify/cmd.go(0 hunks)cmd/akash/cmd/testnetify/config.go(2 hunks)cmd/akash/cmd/testnetify/escrow.go(0 hunks)cmd/akash/cmd/testnetify/state.go(0 hunks)cmd/akash/cmd/testnetify/testnetify.go(1 hunks)cmd/akash/cmd/testnetify/util.go(0 hunks)cmd/akash/cmd/testnetify/utils.go(1 hunks)cmd/akash/cmd/testnetify/validators.go(0 hunks)go.mod(3 hunks)tests/upgrade/testnet.json(1 hunks)util/server/server.go(1 hunks)
💤 Files with no reviewable changes (6)
- cmd/akash/cmd/testnetify/accounts.go
- cmd/akash/cmd/testnetify/escrow.go
- cmd/akash/cmd/testnetify/util.go
- cmd/akash/cmd/testnetify/validators.go
- cmd/akash/cmd/testnetify/cmd.go
- cmd/akash/cmd/testnetify/state.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
cmd/akash/cmd/root.go (1)
cmd/akash/cmd/testnetify/testnetify.go (1)
GetCmd(52-237)
cmd/akash/cmd/testnetify/utils.go (1)
cmd/akash/cmd/testnetify/testnetify.go (1)
FlagTraceStore(40-40)
cmd/akash/cmd/testnetify/config.go (1)
app/testnet.go (4)
TestnetValidator(22-29)TestnetGovConfig(39-43)TestnetUpgrade(31-33)TrimQuotes(52-55)
⏰ 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). (8)
- GitHub Check: sims
- GitHub Check: release-dry-run
- GitHub Check: coverage
- GitHub Check: lint
- GitHub Check: network-upgrade-names
- GitHub Check: tests
- GitHub Check: build-macos
- GitHub Check: build-bins
🔇 Additional comments (12)
cmd/akash/cmd/root.go (1)
160-160: Good architectural improvement: Moving testnetify to root level.This change appropriately elevates the testnetify command from a debug subcommand to a top-level command, reflecting its significance as a primary testnet management tool. The use of dependency injection with
newTestnetAppfollows good software design principles.util/server/server.go (1)
194-199: Well-designed utility function for testnet creation.This function provides a clean, reusable interface for adding testnet creation capabilities to CLI applications. The implementation follows established patterns in the codebase and properly integrates with the Cosmos SDK's
InPlaceTestnetCreator.go.mod (2)
55-55: Good dependency updates to newer akash fork versions.The updates to
akash.5versions of both cosmos-sdk and tendermint forks indicate improved compatibility and likely include important patches. This aligns well with the testnet functionality improvements.Also applies to: 71-71
15-15: JWT Dependency Version Security – ApprovedThe specified version
github.com/golang-jwt/jwt/v5 v5.2.2includes the patch for CVE-2025-30204 (excessive memory allocation in header parsing) and is not vulnerable to that issue.
- Ensure your code properly handles errors returned by
ParseWithClaimsto mitigate risks related to CVE-2024-51744.- Continue to update this dependency to the latest stable release and monitor the project’s security advisories for new vulnerabilities.
No further action required.
cmd/akash/cmd/testnetify/utils.go (2)
13-16: LGTM!The function correctly creates a LevelDB instance with proper path construction and error handling.
31-31: Avoid variable shadowing in cleanup function.The error variable
errin the cleanup function shadows the function parametererr. This could lead to confusion.- if err = traceWriter.Close(); err != nil { + if closeErr := traceWriter.Close(); closeErr != nil { - svrCtx.Logger.Error("failed to close trace writer", "err", err) + svrCtx.Logger.Error("failed to close trace writer", "err", closeErr)Likely an incorrect or invalid review comment.
cmd/akash/cmd/testnetify/config.go (4)
17-30: LGTM!The key types are well-structured for handling Tendermint validator and node keys.
43-55: LGTM!The TestnetValidator struct appropriately combines configuration fields with private runtime state fields.
72-79: LGTM!The custom JSON unmarshaling correctly uses Tendermint's JSON library for proper key deserialization.
81-106: LGTM!The address unmarshaling methods correctly handle JSON string parsing with quote trimming and proper error propagation.
app/testnet.go (1)
22-50: LGTM!The testnet configuration types are well-structured with appropriate field types and clear purposes.
cmd/akash/cmd/testnetify/testnetify.go (1)
393-396: Key prefix confirmed in Tendermint blockstoreWe’ve verified that
"SC:%v"is the exact format used by Tendermint’s owncalcSeenCommitKeyinstore/store.go, so the key schema is stable:• Definition (store/store.go):
func calcSeenCommitKey(height int64) []byte { return []byte(fmt.Sprintf("SC:%v", height)) }You can safely delete keys constructed with
fmt.Appendf(nil, "SC:%v", …).
Optional improvement: import and callcalcSeenCommitKey(height)instead of inliningfmt.Appendf, to stay aligned with upstream.
| if !strings.HasSuffix(val, "s") { | ||
| return fmt.Errorf("invalid format of voting period. must contain time unit. Valid time units are ns|us(µs)|ms|s|m|h") // nolint: goerr113 | ||
| } |
There was a problem hiding this comment.
Remove overly restrictive time unit validation.
The validation only allows durations ending with 's', but Go's time.ParseDuration supports multiple units (ns, us, µs, ms, s, m, h). The error message mentions these units but the code rejects them.
- if !strings.HasSuffix(val, "s") {
- return fmt.Errorf("invalid format of voting period. must contain time unit. Valid time units are ns|us(µs)|ms|s|m|h") // nolint: goerr113
- }📝 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.
| if !strings.HasSuffix(val, "s") { | |
| return fmt.Errorf("invalid format of voting period. must contain time unit. Valid time units are ns|us(µs)|ms|s|m|h") // nolint: goerr113 | |
| } |
🤖 Prompt for AI Agents
In app/testnet.go around lines 60 to 62, the code restricts the voting period
duration to only those ending with 's', which conflicts with the error message
and Go's time.ParseDuration support for multiple units. Remove the check that
enforces the suffix 's' so that all valid time units supported by
time.ParseDuration (ns, us, µs, ms, s, m, h) are accepted, and keep the error
message as is to reflect the valid units.
| defer func() { | ||
| if err != nil { | ||
| tmos.Exit(err.Error()) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Avoid using os.Exit in library functions.
Using tmos.Exit in a defer block makes this function unsuitable for use in tests or as a library function. It prevents proper error handling by the caller.
- defer func() {
- if err != nil {
- tmos.Exit(err.Error())
- }
- }()Instead, return the error and let the caller decide how to handle it.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/testnet.go around lines 86 to 90, the defer function calls tmos.Exit on
error, which abruptly terminates the program and prevents callers from handling
errors properly. Remove the defer block that calls tmos.Exit and instead modify
the function to return the error to the caller, allowing them to decide how to
handle it.
| // | ||
|
|
||
| voteParams := app.Keepers.Cosmos.Gov.GetVotingParams(ctx) | ||
| voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration |
There was a problem hiding this comment.
Add nil check for VotingParams.
Accessing tcfg.Gov.VotingParams.VotingPeriod without checking if VotingParams is nil could cause a panic.
+ if tcfg.Gov.VotingParams != nil {
voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration
app.Keepers.Cosmos.Gov.SetVotingParams(ctx, voteParams)
+ }📝 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.
| voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration | |
| if tcfg.Gov.VotingParams != nil { | |
| voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration | |
| app.Keepers.Cosmos.Gov.SetVotingParams(ctx, voteParams) | |
| } |
🤖 Prompt for AI Agents
In app/testnet.go at line 197, add a nil check for tcfg.Gov.VotingParams before
accessing VotingPeriod to prevent a potential panic. Ensure you verify that
VotingParams is not nil, and only then assign VotingPeriod from its Duration
field.
| cctx, err := client.GetClientQueryContext(cmd) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log errors in monitoring goroutine.
Errors in the goroutine are silently ignored, making debugging difficult.
cctx, err := client.GetClientQueryContext(cmd)
if err != nil {
+ sctx.Logger.Error("failed to get client context in monitor", "err", err)
return
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/akash/cmd/testnetify/testnetify.go around lines 170 to 173, the error
returned by client.GetClientQueryContext(cmd) is currently ignored without
logging. Modify the code to log the error before returning, using an appropriate
logger or fmt to output the error details, so that errors in this goroutine are
visible for debugging.
| return os.OpenFile( | ||
| traceWriterFile, | ||
| os.O_WRONLY|os.O_APPEND|os.O_CREATE, | ||
| 0o666, |
There was a problem hiding this comment.
Use more restrictive file permissions for trace files.
The permission 0o666 allows all users to read and write the trace file, which could expose sensitive information.
- 0o666,
+ 0o600,🤖 Prompt for AI Agents
In cmd/akash/cmd/testnetify/utils.go at line 47, the file permission 0o666 is
too permissive, allowing all users to read and write the trace file. Change the
permission to a more restrictive setting such as 0o600 to ensure only the owner
can read and write the file, protecting sensitive information.
Signed-off-by: Artur Troian <[email protected]>
No description provided.