refactor(cmd/testnetify): add multi-account support #2012
Conversation
WalkthroughChanged testnet init and CLI config to support per-account balances, explicit validator status and delegations, and stronger error handling; validator voting power is computed from delegations and ApplyAndReturnValidatorSetUpdates errors are now checked and panicked on. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as testnetify
participant AppInit as InitAkashAppForTestnet
participant Bank as BankKeeper
participant Staking as StakingKeeper
participant Apply as ApplyAndReturnValidatorSetUpdates
CLI->>AppInit: provide TestnetConfig (accounts, validators)
AppInit->>Bank: Mint balances for each `TestnetAccount`
Bank-->>AppInit: balances minted
AppInit->>Staking: Create validators with Status, Commission, MinSelfDelegation
Staking-->>AppInit: validators created (Tokens/DelegatorShares set from config)
AppInit->>Staking: Apply delegations from `TestnetValidator.Delegations`
Staking-->>AppInit: delegations applied
AppInit->>Apply: ApplyAndReturnValidatorSetUpdates(ctx)
alt success
Apply-->>AppInit: validator set updated
else error
Apply--xAppInit: returns error
AppInit-->>AppInit: panic (abort initialization)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/testnet.go (1)
209-212: Nil-guard GOV voting params to avoid panic.If VotingParams is omitted in config, this will nil-deref.
Apply this diff:
- voteParams := app.Keepers.Cosmos.Gov.GetVotingParams(ctx) - voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration - app.Keepers.Cosmos.Gov.SetVotingParams(ctx, voteParams) + if tcfg.Gov.VotingParams != nil { + voteParams := app.Keepers.Cosmos.Gov.GetVotingParams(ctx) + voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration + app.Keepers.Cosmos.Gov.SetVotingParams(ctx, voteParams) + }
🧹 Nitpick comments (6)
app/export.go (1)
190-193: Good: stop ignoring staking updates error; consider adding context.Capturing the error is correct. To aid debugging, wrap with context (e.g., panic(fmt.Errorf("staking.ApplyAndReturnValidatorSetUpdates: %w", err))).
cmd/akash/cmd/testnetify/testnetify.go (2)
130-140: Validate defaults: ensure tokens fit int64 and shares are non‑negative.Prevent invalid/overflow values early; fail fast with a clear error.
Apply inline checks:
for i, val := range cfg.Validators { if val.Tokens == nil { tk := sdk.NewInt(900000000000000) cfg.Validators[i].Tokens = &tk } if val.DelegatorShares == nil { shares := sdk.MustNewDecFromStr("10000000") cfg.Validators[i].DelegatorShares = &shares } + // Sanity checks + if cfg.Validators[i].Tokens.IsNegative() || cfg.Validators[i].Tokens.BigInt().BitLen() >= 63 { + return fmt.Errorf("validator %q tokens must be within [0, 2^63-1]", val.Moniker) + } + if cfg.Validators[i].DelegatorShares.IsNegative() { + return fmt.Errorf("validator %q delegator_shares must be non-negative", val.Moniker) + } }
367-377: Defensive: guard against nil Tokens/Shares before dereference.Defaults are set earlier, but add a guard to avoid future regressions if code is refactored.
Example:
- Tokens: *val.Tokens, - DelegatorShares: *val.DelegatorShares, + Tokens: func() sdk.Int { if val.Tokens == nil { return sdk.ZeroInt() }; return *val.Tokens }(), + DelegatorShares: func() sdk.Dec { if val.DelegatorShares == nil { return sdk.ZeroDec() }; return *val.DelegatorShares }(),app/testnet.go (3)
48-51: Prefer sdk.Coins for balances to avoid conversions and ensure canonicalization.sdk.Coins carries invariants (sorted, non-dup) and integrates better with bank APIs.
Apply this diff:
type TestnetAccount struct { Address sdk.AccAddress `json:"address"` - Balances []sdk.Coin `json:"balances"` + Balances sdk.Coins `json:"balances"` }
219-227: Adjust BANK mint path to new type (and avoid redundant NewCoins).If Balances is sdk.Coins, you can use it directly; fallback still uses NewCoins.
Apply this diff:
- var coins sdk.Coins - if len(account.Balances) > 0 { - coins = sdk.NewCoins(account.Balances...) - } else { - coins = sdk.NewCoins( + var coins sdk.Coins + if len(account.Balances) > 0 { + coins = account.Balances + } else { + coins = sdk.NewCoins( sdk.NewInt64Coin("uakt", 1000000000000), sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), // axlUSDC ) }
194-197: Call ApplyAndReturnValidatorSetUpdates once after inserting all validators.Invoking it inside the loop is unnecessary and increases churn; move it after the loop for clarity and fewer state transitions.
Apply this diff:
- _, err = app.Keepers.Cosmos.Staking.ApplyAndReturnValidatorSetUpdates(ctx) - if err != nil { - panic(err) - } @@ } +// After all validators are created, update the validator set once. +_, err = app.Keepers.Cosmos.Staking.ApplyAndReturnValidatorSetUpdates(ctx) +if err != nil { + panic(err) +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/export.go(1 hunks)app/testnet.go(5 hunks)cmd/akash/cmd/testnetify/config.go(1 hunks)cmd/akash/cmd/testnetify/testnetify.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/testnet.go (1)
cmd/akash/cmd/testnetify/config.go (1)
AccAddress(31-33)
cmd/akash/cmd/testnetify/config.go (1)
app/testnet.go (2)
TestnetValidator(22-32)TestnetAccount(48-51)
⏰ 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). (15)
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: release-dry-run
- GitHub Check: shellcheck
- GitHub Check: sims
- GitHub Check: lint
- GitHub Check: build-macos
- GitHub Check: tests
- GitHub Check: network-upgrade
- GitHub Check: lint
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: build-bins
- GitHub Check: coverage
- GitHub Check: build-macos
🔇 Additional comments (1)
cmd/akash/cmd/testnetify/testnetify.go (1)
324-324: LGTM: hex-encoding updated genesis checksum for LoadStateFromDBOrGenesisDocProvider.
| for i, acc := range cfg.Accounts { | ||
| if len(acc.Balances) > 0 { | ||
| cfg.Accounts[i].Balances = sdk.NewCoins(acc.Balances...) | ||
| } else { | ||
| cfg.Accounts[i].Balances = sdk.NewCoins( | ||
| sdk.NewInt64Coin("uakt", 1000000000000), | ||
| sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), // axlUSDC | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix type mismatch when assigning balances.
cfg.Accounts[i].Balances is []sdk.Coin, while sdk.NewCoins(...) returns sdk.Coins. Use an explicit conversion to avoid compile-time type errors.
Apply this diff:
for i, acc := range cfg.Accounts {
if len(acc.Balances) > 0 {
- cfg.Accounts[i].Balances = sdk.NewCoins(acc.Balances...)
+ cfg.Accounts[i].Balances = []sdk.Coin(sdk.NewCoins(acc.Balances...))
} else {
- cfg.Accounts[i].Balances = sdk.NewCoins(
- sdk.NewInt64Coin("uakt", 1000000000000),
- sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), // axlUSDC
- )
+ cfg.Accounts[i].Balances = []sdk.Coin(sdk.NewCoins(
+ sdk.NewInt64Coin("uakt", 1000000000000),
+ sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), // axlUSDC
+ ))
}
}Optional: standardize on sdk.Coins in TestnetAccount to avoid future conversions.
📝 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.
| for i, acc := range cfg.Accounts { | |
| if len(acc.Balances) > 0 { | |
| cfg.Accounts[i].Balances = sdk.NewCoins(acc.Balances...) | |
| } else { | |
| cfg.Accounts[i].Balances = sdk.NewCoins( | |
| sdk.NewInt64Coin("uakt", 1000000000000), | |
| sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), // axlUSDC | |
| ) | |
| } | |
| } | |
| for i, acc := range cfg.Accounts { | |
| if len(acc.Balances) > 0 { | |
| cfg.Accounts[i].Balances = []sdk.Coin(sdk.NewCoins(acc.Balances...)) | |
| } else { | |
| cfg.Accounts[i].Balances = []sdk.Coin(sdk.NewCoins( | |
| sdk.NewInt64Coin("uakt", 1000000000000), | |
| sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), // axlUSDC | |
| )) | |
| } | |
| } |
🤖 Prompt for AI Agents
In cmd/akash/cmd/testnetify/testnetify.go around lines 119-128, the code assigns
sdk.NewCoins(...), which returns sdk.Coins, to cfg.Accounts[i].Balances which is
of type []sdk.Coin; perform an explicit conversion to the slice type when
assigning (for example: cfg.Accounts[i].Balances =
[]sdk.Coin(sdk.NewCoins(...))). Update both branches (the existing balances
branch and the default branch) accordingly; optionally consider changing
TestnetAccount.Balances to sdk.Coins to avoid repeated conversions in future.
| Address: val.validatorAddress, | ||
| PubKey: val.pubKey, | ||
| VotingPower: 900000000000000, | ||
| VotingPower: val.Tokens.Int64(), | ||
| }) |
There was a problem hiding this comment.
Make VotingPower conversion int64‑safe (avoid overflow/negative).
Tokens may exceed int64 or be negative; clamp or normalize before assigning to Validator.VotingPower.
Apply this diff (and add import "math"):
@@
- newValidators = append(newValidators, &cmttypes.Validator{
- Address: val.validatorAddress,
- PubKey: val.pubKey,
- VotingPower: val.Tokens.Int64(),
- })
+ // Ensure int64-safe voting power
+ var vp int64
+ if val.Tokens.IsNegative() {
+ vp = 0
+ } else if val.Tokens.BigInt().BitLen() >= 63 {
+ vp = math.MaxInt64
+ } else {
+ vp = val.Tokens.Int64()
+ }
+ newValidators = append(newValidators, &cmttypes.Validator{
+ Address: val.validatorAddress,
+ PubKey: val.pubKey,
+ VotingPower: vp,
+ })Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/akash/cmd/testnetify/testnetify.go around lines 479 to 482, the
VotingPower is set using val.Tokens.Int64() which can overflow or be negative;
clamp the value into the int64 range and ensure non‑negative before assigning to
Validator.VotingPower. Replace the direct Int64() call with logic that converts
safely: compute the token value as an int64 (capping at math.MaxInt64 if it
exceeds that, and using 0 if negative), then assign that clamped int64 to
VotingPower; add import "math" to the file.
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/akash/cmd/testnetify/testnetify.go (2)
356-365: Fix operator/consensus address types in appConfig.Use typed addresses, not raw bytes. This also relies on Operator being ValAddress in config.go.
- appConfig.Validators = append(appConfig.Validators, akash.TestnetValidator{ - OperatorAddress: val.Operator.Bytes(), - ConsensusAddress: pubKey.Address().Bytes(), + appConfig.Validators = append(appConfig.Validators, akash.TestnetValidator{ + OperatorAddress: val.Operator.ValAddress, + ConsensusAddress: sdktypes.ConsAddress(pubKey.Address().Bytes()), ConsensusPubKey: consensusPubkey, Moniker: val.Moniker, Commission: val.Commission, MinSelfDelegation: val.MinSelfDelegation, Status: val.Status, Delegations: val.Delegations, })
479-483: Guard against empty validator set before selecting proposer.Avoid panic when no validators are configured.
- newValSet := &cmttypes.ValidatorSet{ - Validators: newValidators, - Proposer: newValidators[0], - } + if len(newValidators) == 0 { + return nil, fmt.Errorf("no validators configured") + } + newValSet := &cmttypes.ValidatorSet{ + Validators: newValidators, + Proposer: newValidators[0], + }app/testnet.go (1)
239-241: Nil guard for optional GOV voting params.tcfg.Gov.VotingParams can be nil; dereferencing will panic.
- voteParams := app.Keepers.Cosmos.Gov.GetVotingParams(ctx) - voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration - app.Keepers.Cosmos.Gov.SetVotingParams(ctx, voteParams) + if tcfg.Gov.VotingParams != nil { + voteParams := app.Keepers.Cosmos.Gov.GetVotingParams(ctx) + voteParams.VotingPeriod = tcfg.Gov.VotingParams.VotingPeriod.Duration + app.Keepers.Cosmos.Gov.SetVotingParams(ctx, voteParams) + }
♻️ Duplicate comments (3)
cmd/akash/cmd/testnetify/config.go (1)
45-45: Align operator address with staking semantics: use ValAddress.Operator is a validator operator address, not an account address. Switch to ValAddress to avoid accidental Acc/Val mixups and to match app.TestnetValidator.OperatorAddress.
type TestnetValidator struct { Moniker string `json:"moniker"` - Operator AccAddress `json:"operator"` + Operator ValAddress `json:"operator"` Status stakingtypes.BondStatus `json:"status"` Commission stakingtypes.Commission `json:"commission"` MinSelfDelegation sdk.Int `json:"min_self_delegation"` Home string `json:"home"` Delegations []akash.TestnetDelegation `json:"delegations"` }cmd/akash/cmd/testnetify/testnetify.go (2)
120-129: Normalize Balances type usage; avoid sdk.Coins → []sdk.Coin mismatch.Current code assigns sdk.Coins to a []sdk.Coin field; this mismatches types and was flagged earlier.
Preferred fix (paired with changing app.TestnetAccount.Balances to sdk.Coins; see app/testnet.go comment):
for i, acc := range cfg.Accounts { - if len(acc.Balances) > 0 { - cfg.Accounts[i].Balances = sdk.NewCoins(acc.Balances...) - } else { - cfg.Accounts[i].Balances = sdk.NewCoins( + if len(acc.Balances) > 0 { + cfg.Accounts[i].Balances = acc.Balances + } else { + cfg.Accounts[i].Balances = sdk.NewCoins( sdk.NewInt64Coin("uakt", 10000000000000), sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), - ) + ) } }If you cannot change app.TestnetAccount yet, perform explicit conversion here as a stopgap:
- cfg.Accounts[i].Balances = sdk.NewCoins(acc.Balances...) + cfg.Accounts[i].Balances = []sdk.Coin(sdk.NewCoins(acc.Balances...)) @@ - cfg.Accounts[i].Balances = sdk.NewCoins( + cfg.Accounts[i].Balances = []sdk.Coin(sdk.NewCoins( sdk.NewInt64Coin("uakt", 10000000000000), sdk.NewInt64Coin("ibc/12C6A0C374171B595A0A9E18B83FA09D295FB1F2D8C6DAA3AC28683471752D84", 1000000000000), - ) + ))
466-475: Make VotingPower int64‑safe; clamp and ignore negatives.Summing Int64() can overflow or go negative. Clamp to [0, math.MaxInt64].
- var vp int64 - for _, del := range val.Delegations { - vp += del.Amount.Amount.Quo(sdktypes.DefaultPowerReduction).Int64() - } + var vp int64 + for _, del := range val.Delegations { + if del.Amount.Amount.IsNegative() { + continue + } + amt := del.Amount.Amount.Quo(sdktypes.DefaultPowerReduction) + if amt.IsZero() { + continue + } + if amt.BigInt().BitLen() >= 63 { + vp = math.MaxInt64 + break + } + a := amt.Int64() + if vp > math.MaxInt64-a { + vp = math.MaxInt64 + break + } + vp += a + }Also add:
import ( "bufio" + "math" "encoding/hex"
🧹 Nitpick comments (2)
cmd/akash/cmd/testnetify/testnetify.go (1)
17-17: Import hygiene: pick one alias for cosmos types.Both sdk and sdktypes point to the same package. Prefer a single alias for clarity.
Proposed:
- Keep only
sdk "github.com/cosmos/cosmos-sdk/types"and dropsdktypes, updating call sites (DefaultPowerReduction, ConsAddress) accordingly.Also applies to: 35-35
app/testnet.go (1)
52-56: Optional: add JSON bech32 parsing for Accounts/Delegations.To allow human‑friendly configs, implement UnmarshalJSON to parse bech32 addresses.
Example (add imports: encoding/json):
func (t *TestnetAccount) UnmarshalJSON(data []byte) error { var aux struct { Address string `json:"address"` Balances sdk.Coins `json:"balances"` } if err := json.Unmarshal(data, &aux); err != nil { return err } addr, err := sdk.AccAddressFromBech32(aux.Address); if err != nil { return err } t.Address, t.Balances = addr, aux.Balances return nil } func (t *TestnetDelegation) UnmarshalJSON(data []byte) error { var aux struct { Address string `json:"address"` Amount sdk.Coin `json:"amount"` } if err := json.Unmarshal(data, &aux); err != nil { return err } addr, err := sdk.AccAddressFromBech32(aux.Address); if err != nil { return err } t.Address, t.Amount = addr, aux.Amount return nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/export.go(1 hunks)app/testnet.go(4 hunks)cmd/akash/cmd/testnetify/config.go(2 hunks)cmd/akash/cmd/testnetify/testnetify.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/export.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/akash/cmd/testnetify/config.go (1)
app/testnet.go (2)
TestnetDelegation(52-55)TestnetAccount(47-50)
cmd/akash/cmd/testnetify/testnetify.go (1)
testutil/network/network.go (1)
Validator(134-153)
⏰ 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). (14)
- GitHub Check: network-upgrade-names
- GitHub Check: lint
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: build-bins
- GitHub Check: build-macos
- GitHub Check: release-dry-run
- GitHub Check: tests
- GitHub Check: sims
- GitHub Check: lint
- GitHub Check: coverage
- GitHub Check: build-bins
| type TestnetAccount struct { | ||
| Address sdk.AccAddress `json:"address"` | ||
| Balances []sdk.Coin `json:"balances"` | ||
| } |
There was a problem hiding this comment.
Use sdk.Coins for Balances to match bank APIs.
Bank.MintCoins/SendCoinsFromModuleToAccount accept sdk.Coins. Using []sdk.Coin here causes friction and type mismatches elsewhere.
type TestnetAccount struct {
Address sdk.AccAddress `json:"address"`
- Balances []sdk.Coin `json:"balances"`
+ Balances sdk.Coins `json:"balances"`
}📝 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.
| type TestnetAccount struct { | |
| Address sdk.AccAddress `json:"address"` | |
| Balances []sdk.Coin `json:"balances"` | |
| } | |
| type TestnetAccount struct { | |
| Address sdk.AccAddress `json:"address"` | |
| Balances sdk.Coins `json:"balances"` | |
| } |
| Accounts []akash.TestnetAccount `json:"accounts"` | ||
| Gov akash.TestnetGovConfig `json:"gov"` |
There was a problem hiding this comment.
Bech32 JSON parsing gap for Accounts/Delegations (sdk.AccAddress).
cfg.Accounts uses app.TestnetAccount, which has sdk.AccAddress. Without a custom UnmarshalJSON, JSON expects base64, not bech32. Same for Delegations’ Address field. This will confuse users and likely break config loading.
Two viable fixes (pick one):
- Implement UnmarshalJSON on app.TestnetAccount and app.TestnetDelegation to parse bech32 addresses, or
- Keep local testnetify JSON types that use your AccAddress/ValAddress wrappers, then convert to app types before building appConfig.
I can draft the UnmarshalJSON implementations in app/testnet.go if you prefer. Would you like that?
🤖 Prompt for AI Agents
In cmd/akash/cmd/testnetify/config.go around lines 63-64, cfg.Accounts and
related Delegations use app.TestnetAccount/TestnetDelegation which contain
sdk.AccAddress/sdk.ValAddress that JSON unmarshals as base64 by default; update
to support bech32 by either (A) adding UnmarshalJSON methods on
app.TestnetAccount and app.TestnetDelegation to accept bech32 strings and decode
them into AccAddress/ValAddress during JSON unmarshal, or (B) create local
testnetify JSON DTOs that use lightweight bech32 string wrappers (or
AccAddress/ValAddress wrapper types with custom UnmarshalJSON) to parse bech32,
then convert those DTOs into the app types before building appConfig — implement
one approach consistently for both Accounts and Delegations and add tests for
bech32 parsing and error cases.
1 participant
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md