feat: sdk-50 #1998
feat: sdk-50 #1998
Conversation
WalkthroughRepository-wide migration: module path and dependencies updated; CometBFT/Cosmos SDK APIs adopted; major app refactor (keepers, baseapp, genesis, ante); CI unified with a composite setup action; SDL/events subsystems removed; many new e2e integration tests and supporting tooling added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Repo as GitHub
participant CI as GitHub Actions
participant Setup as setup-ubuntu Action
participant Make as Make/Scripts
participant App as AkashApp
participant SDK as Cosmos SDK / CometBFT
Note over Dev,Repo: PR introduces repo-wide migration and CI centralization
Dev->>Repo: push changes
Repo->>CI: workflows triggered
CI->>Setup: run setup-ubuntu (fetch tags, install deps, detect Go)
Setup-->>CI: export GOVERSION & tools to GITHUB_ENV
CI->>Make: run build/test targets using standardized environment
Make->>App: build & run (appCreator.newApp / Setup)
App->>SDK: InitSpecialKeepers() → InitNormalKeepers() (keeper wiring)
SDK-->>App: provide runtime types (txConfig, baseapp lifecycle)
CI->>App: run e2e suites (gRPC/CLI)
App-->>CI: upload artifacts (logs, validators)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (3)
.goreleaser-docker.yaml (1)
78-81:archives.formatsis not a valid GoReleaser keyGoReleaser expects a singular
formatfield, notformats. The current YAML will be ignored and the build will fall back to the defaulttar.gz, breaking the “single binary” expectation.archives: - - formats: - - binary + - format: binarycmd/akash/cmd/bech32.go (1)
33-36: Avoidpanicin CLI command pathA malformed address should surface as a regular error rather than crash the CLI.
- bech32Addr, err := bech32.ConvertAndEncode(bech32prefix, bz) - if err != nil { - panic(err) - } + bech32Addr, err := bech32.ConvertAndEncode(bech32prefix, bz) + if err != nil { + return err + }cmd/akash/cmd/auth.go (1)
103-103: Fix incorrect flag usageLine 103 uses
FlagJWTAccessinstead ofFlagJWTScopewhen retrieving the scope string, which appears to be a copy-paste error.- scopeString, err := cmd.Flags().GetString(FlagJWTAccess) + scopeString, err := cmd.Flags().GetString(FlagJWTScope)
♻️ Duplicate comments (4)
make/test-simulation.mk (3)
12-13: Same concern as above for determinism test targetSee previous comment.
If all good you can ignore.
17-18: Import/export simulation now also compiles without mainnet tag?Re-verify for this target as well.
22-23: Simulation-after-import target: tag removalSame verification applies.
tests/e2e/certs_cli_test.go (1)
92-146: Comprehensive client certificate testsGood validation of argument rejection and complete lifecycle testing. Note: Same ValidateTx return value issue as mentioned above applies here.
🧹 Nitpick comments (51)
README.md (1)
31-32: Link text still says “tendermint” after switching to CometBFTTo avoid confusion, update the visible link label to
CometBFTwhile keeping the new URL.-The Suite is composed of one binary, `akash`, which contains a ([tendermint](https://github.com/cometbft/cometbft)-powered) blockchain node that +The Suite is composed of one binary, `akash`, which contains a ([CometBFT](https://github.com/cometbft/cometbft)-powered) blockchain node thattests/upgrade/types/types.go (1)
1-1: Explain disabledrevivelinterAdding
//nolint: revivesilences all future lint findings for this file. Consider narrowing the directive (e.g.//nolint:revive,stylecheck) or adding a short comment explaining why it’s required.docgen/main.go (1)
17-18: Bubble up the error fromroot.NewRootCmd()instead of ignoring it.
doc.GenMarkdownTreewill panic on anil*cobra.Command; losing early context makes the failure harder to diagnose.- cmd, _ := root.NewRootCmd() + cmd, err := root.NewRootCmd() + if err != nil { + fmt.Fprintf(os.Stderr, "failed to build root command: %v\n", err) + os.Exit(1) + }.env (1)
14-15: Add the new path to$PATHwhere required tooling expects it.
AKASH_RUN_BINis exported but not yet appended toPATH. CI scripts that invoke binaries placed here will fail unless they reference the full path.CHANGELOG.md (1)
41-41: Convert bare URL to markdown link to satisfy MD034 and keep style consistent
markdownlintrightfully flags the raw URL. We’ve been linking other references, so let’s wrap this one too.-* (ibc-go) Use ibc v4.4.0 https://pkg.akt.dev/node/pull/1825 +* (ibc-go) Use ibc v4.4.0 ([#1825](https://pkg.akt.dev/node/pull/1825))make/codegen.mk (1)
8-9: Declare the new target as phony and prevent accidental file collisionAdd it beside the others to avoid GNU make treating a real
mocksdirectory/file
as up-to-date..PHONY: generate generate: $(MOCKERY) $(GO) generate ./... .PHONY: codegen codegen: generate + +.PHONY: mocks mocks: $(MOCKERY) $(MOCKERY)make/test-integration.mk (1)
11-11: Timeout increase is appropriate for integration tests.The timeout increase from 300s to 600s is reasonable for comprehensive test suites, though you may want to consider making this configurable via an environment variable for flexibility in different CI environments.
Consider making the timeout configurable:
- $(GO_TEST) -v -timeout 600s $(TEST_MODULES) + $(GO_TEST) -v -timeout $(TEST_TIMEOUT) $(TEST_MODULES)Then add
TEST_TIMEOUT ?= 600sat the top of the file.tests/e2e/cli_test.go (1)
14-14: Consider making the default deposit configurable.The hardcoded deposit value may need adjustment for different test environments or network conditions. Consider making it configurable via environment variables or test configuration.
-var DefaultDeposit = sdk.NewCoin("uakt", sdk.NewInt(5000000)) +var DefaultDeposit = getDefaultDeposit() + +func getDefaultDeposit() sdk.Coin { + if envDeposit := os.Getenv("TEST_DEFAULT_DEPOSIT"); envDeposit != "" { + if amount, err := sdk.ParseCoinNormalized(envDeposit); err == nil { + return amount + } + } + return sdk.NewCoin("uakt", sdk.NewInt(5000000)) +}tests/e2e/grpc_test.go (1)
13-30: Consider parallel test execution for better performance.The test suites are currently executed sequentially, which may increase overall test runtime. If the test suites are independent and don't share state, consider running them in parallel.
func TestIntegrationGRPC(t *testing.T) { - dg := &deploymentGRPCRestTestSuite{} - dg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, dg) - - cg := &certsGRPCRestTestSuite{} - cg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, cg) - - mg := &marketGRPCRestTestSuite{} - mg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, mg) - - pg := &providerGRPCRestTestSuite{} - pg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, pg) - - suite.Run(t, dg) - suite.Run(t, cg) - suite.Run(t, mg) - suite.Run(t, pg) + testSuites := []suite.TestingSuite{ + func() suite.TestingSuite { + dg := &deploymentGRPCRestTestSuite{} + dg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, dg) + return dg + }(), + func() suite.TestingSuite { + cg := &certsGRPCRestTestSuite{} + cg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, cg) + return cg + }(), + func() suite.TestingSuite { + mg := &marketGRPCRestTestSuite{} + mg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, mg) + return mg + }(), + func() suite.TestingSuite { + pg := &providerGRPCRestTestSuite{} + pg.NetworkTestSuite = testutil.NewNetworkTestSuite(nil, pg) + return pg + }(), + } + + for _, ts := range testSuites { + ts := ts // capture loop variable + t.Run("", func(t *testing.T) { + t.Parallel() + suite.Run(t, ts) + }) + } }_docs/adr/adr-001-network-upgrades.md (1)
43-48: Import path updates are correct for the CometBFT migration.The updates from Tendermint to CometBFT imports and the new
cosmossdk.iomodule paths align with the broader codebase modernization.However, address the formatting issue by replacing hard tabs with spaces:
import ( - "github.com/cometbft/cometbft/libs/log" + "github.com/cometbft/cometbft/libs/log" - storetypes "cosmossdk.io/store" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/types/module" - upgradetypes "cosmossdk.io/x/upgrade/types" + storetypes "cosmossdk.io/store" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/module" + upgradetypes "cosmossdk.io/x/upgrade/types"cmd/akash/cmd/testnetify/config.go (2)
11-16: Import aliases are fine—confirm unused imports after rebaseCosmos SDK 0.50+ split
sdkmathout oftypes. Good catch.
Just ensurego vet ./...passes after the switch; straysdkmathvssdk.Intmismatches are easy to miss.
61-66: Unexportedupgradefield is silent in JSON
upgrade akash.TestnetUpgradelacks a JSON tag and is private, so it will never marshal/unmarshal.
Add an explicit ignore tag for clarity.-upgrade akash.TestnetUpgrade +upgrade akash.TestnetUpgrade `json:"-"`tests/upgrade/test-cases.json (1)
21-26: Whitespace artefact hurts diff readability
"}, {is valid JSON but makes reviews painful.
Runjq -S . tests/upgrade/test-cases.json > tmp && mv tmp …to pretty-print and keep future diffs clean..github/workflows/release.yaml (1)
60-66: Hard-coded test config path assumes CWD
TEST_CONFIG: test-config-gha.jsonrelies ontests/upgradebeing the working directory later.
Safer:env: TEST_CONFIG: tests/upgrade/test-config-gha.json.github/actions/setup-ubuntu/action.yaml (2)
1-4: Missingdescriptionfield breaks Marketplace visibilityGitHub composite actions should include a short description.
name: setup-ubuntu +description: "Common environment bootstrap for Akash CI jobs"
10-13:sudo apt installwithoutapt-updatecan fail on new runnersPrepend
sudo apt-get update -yto avoid “package not found” errors.app/ante.go (2)
39-43: Remove "akash" references from error messages.Since the code is being refactored to remove Akash-specific logic, the error messages should be generic.
Apply this diff to make error messages consistent:
- return nil, sdkerrors.ErrLogic.Wrap("akash governance keeper is required for ante builder") + return nil, sdkerrors.ErrLogic.Wrap("governance keeper is required for ante builder") } if options.FeegrantKeeper == nil { - return nil, sdkerrors.ErrLogic.Wrap("akash feegrant keeper is required for ante builder") + return nil, sdkerrors.ErrLogic.Wrap("feegrant keeper is required for ante builder")
52-52: Consider documenting the nil TxFeeChecker parameter.The nil parameter represents the TxFeeChecker, which defaults to the standard fee checker when nil. Consider adding a comment to clarify this for future maintainers.
- ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, nil), + ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, nil), // nil uses default TxFeeCheckertests/e2e/certs_grpc_test.go (1)
75-114: Remove or implement the commented REST API test.Large blocks of commented code should be avoided. If this REST API test is planned, track it as a TODO or issue. Otherwise, remove it.
Would you like me to help implement this REST API test or create an issue to track this work?
app/upgrades.go (1)
23-27: Empty pre-upgrade handler implementation.The
customPreUpgradeHandleris called but has no implementation. If this is intentional placeholder code, consider adding a TODO comment. Otherwise, remove the call until needed.Would you like me to help implement the pre-upgrade handler logic or should we remove this placeholder code?
Also applies to: 56-57
.envrc (1)
62-62: Fix typo in error message.There's an extra space in the error message.
- echo "git-cliff is not installed" + echo "git-cliff is not installed"tests/e2e/provider_cli_test.go (1)
33-33: Fix incorrect comment.The comment is misleading as this creates a provider, not a deployment.
- // create deployment + // create providerapp/sim/sim_utils.go (1)
53-74: Consider improving error handling in SimulationOperations.The function uses
panicfor file reading and JSON unmarshaling errors, which could be harsh for simulation testing scenarios. Consider returning errors instead to allow for more graceful error handling by the caller.func SimulationOperations(app *akash.AkashApp, cdc codec.JSONCodec, config simtypes.Config) ([]simtypes.WeightedOperation, error) { simState := module.SimulationState{ AppParams: make(simtypes.AppParams), Cdc: cdc, } if config.ParamsFile != "" { bz, err := os.ReadFile(config.ParamsFile) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to read params file: %w", err) } err = json.Unmarshal(bz, &simState.AppParams) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to unmarshal params: %w", err) } } simState.LegacyProposalContents = app.SimulationManager().GetProposalContents(simState) //nolint:staticcheck simState.ProposalMsgs = app.SimulationManager().GetProposalMsgs(simState) - return app.SimulationManager().WeightedOperations(simState) + return app.SimulationManager().WeightedOperations(simState), nil }app/option.go (1)
41-51: Add missing function documentation.For consistency and clarity, consider adding documentation comments to these functions.
+// WithGenesis sets the genesis setup function for app func WithGenesis(val SetupGenesisFn) SetupAppOption { return func(t *setupAppOptions) { t.genesisFn = val } } +// WithEncConfig sets the encoding configuration for app func WithEncConfig(val sdkutil.EncodingConfig) SetupAppOption { return func(t *setupAppOptions) { t.encCfg = val } }tests/README.md (4)
3-10: Improve sentence variety to enhance readability.Consider rephrasing to avoid three consecutive sentences starting with "LocalOsmosis".
-LocalOsmosis is a complete Osmosis testnet containerized with Docker and orchestrated with a simple docker-compose file. LocalOsmosis comes preconfigured with opinionated, sensible defaults for a standard testing environment. - -LocalOsmosis comes in two flavors: +LocalOsmosis is a complete Osmosis testnet containerized with Docker and orchestrated with a simple docker-compose file. It comes preconfigured with opinionated, sensible defaults for a standard testing environment. + +The testnet is available in two flavors:
124-129: Add language identifier to code block.The fenced code block should specify a language for proper syntax highlighting.
-``` +```text {"level": "info", "module": "server", "module": "consensus", "err": "cannot find validator 122 in valSet of size 1: invalid valid ator index", "time": "2024-10-21T21:34:30Z" ,"message": "failed attempting to add vote"} {"level": "error" , "module": "server", "module":" consensus" ,"height" :22731539, "round" :0, "peer": "27e14df66c9e4cd6b176b@dcabadfa 96750f911", "msg_type": "*consensus. VoteMessage", "err": "error adding vote", "time": "2024-10-21T21:34:307","message":"failed to process message"} {"level": "info" ,"module": "server" ', "module": "consensus", "err": "cannot find validator 107 in valSet of size 1: invalid valid ator index", "time": "2024-10-21T21:34:30Z", "message": "failed attempting to add vote"} -``` +```
134-134: Format bare URLs as proper markdown links.Convert bare URLs to markdown link format for better readability.
-The settings for in place testnet are done in https://github.com/osmosis-labs/osmosis/blob/bb7a94e2561cc63b60ee76ec71a3e04e9688b22c/app/app.go#L773. Modify the parameters in `InitOsmosisAppForTestnet` to modify in place testnet parameters. For example, if you were to modify epoch hours, you would be modifying https://github.com/osmosis-labs/osmosis/blob/bb7a94e2561cc63b60ee76ec71a3e04e9688b22c/app/app.go#L942-L967 . +The settings for in place testnet are done in [app/app.go#L773](https://github.com/osmosis-labs/osmosis/blob/bb7a94e2561cc63b60ee76ec71a3e04e9688b22c/app/app.go#L773). Modify the parameters in `InitOsmosisAppForTestnet` to modify in place testnet parameters. For example, if you were to modify epoch hours, you would be modifying [app/app.go#L942-L967](https://github.com/osmosis-labs/osmosis/blob/bb7a94e2561cc63b60ee76ec71a3e04e9688b22c/app/app.go#L942-L967).
139-140: Maintain consistent terminology throughout the document.Use consistent spelling for "preconfigured" as used earlier in the document.
-LocalOsmosis is pre-configured with one validator and 9 accounts with ION and OSMO balances. +LocalOsmosis is preconfigured with one validator and 9 accounts with ION and OSMO balances.app/sim/sim_config.go (3)
23-23: Address or track the TODO comment.The TODO indicates that
FlagOnOperationValueshould be removed in favor of binary search for invariant violation.Would you like me to open an issue to track this TODO for future implementation of binary search for invariant violations?
35-54: Consider using conventional lowercase flag names.The current flag names are capitalized (e.g., "Genesis", "Params"), which is unconventional for CLI flags. Consider using lowercase with hyphens for better CLI usability.
Example changes:
- flag.StringVar(&FlagGenesisFileValue, "Genesis", "", "custom simulation genesis file; cannot be used with params file") - flag.StringVar(&FlagParamsFileValue, "Params", "", "custom simulation params file which overrides any random params; cannot be used with genesis") + flag.StringVar(&FlagGenesisFileValue, "genesis", "", "custom simulation genesis file; cannot be used with params file") + flag.StringVar(&FlagParamsFileValue, "params", "", "custom simulation params file which overrides any random params; cannot be used with genesis")
74-74: Consider making DBBackend configurable.The database backend is hardcoded to "goleveldb". Consider adding a flag to make this configurable for different simulation scenarios.
Add a new flag variable and flag registration:
// In the flag variables section var FlagDBBackendValue string // In GetSimulatorFlags() flag.StringVar(&FlagDBBackendValue, "DBBackend", "goleveldb", "database backend to use for simulation") // In NewConfigFromFlags() DBBackend: FlagDBBackendValue,app/genesis.go (1)
105-109: Consider migrating from legacy math functions.The code uses deprecated
sdkmath.LegacyOneDec()andsdkmath.LegacyZeroDec(). While these work, consider using the newer math functions for future compatibility.Replace legacy functions with their modern equivalents:
- DelegatorShares: sdkmath.LegacyOneDec(), + DelegatorShares: sdkmath.NewDec(1),- Commission: stakingtypes.NewCommission(sdkmath.LegacyZeroDec(), sdkmath.LegacyZeroDec(), sdkmath.LegacyZeroDec()), + Commission: stakingtypes.NewCommission(sdkmath.NewDec(0), sdkmath.NewDec(0), sdkmath.NewDec(0)),app/app_configure.go (1)
55-55: Consider removing the unused parameter or documenting why it's ignored.The function accepts a
[]stringparameter but doesn't use it. If this parameter is required by an interface, consider adding a comment explaining why it's ignored. Otherwise, remove it to avoid confusion.If the parameter is not required by an interface:
-func OrderInitGenesis(_ []string) []string { +func OrderInitGenesis() []string {If it's required by an interface, add documentation:
+// OrderInitGenesis returns module names in order for init genesis calls. +// The parameter is ignored as the order is predetermined for the Akash application. func OrderInitGenesis(_ []string) []string {tests/e2e/provider_grpc_test.go (1)
28-29: Consider making the test data path more robust.Using a relative path with
filepath.Abscan be fragile if tests are run from different directories. Consider using a more robust approach to locate test data files.-providerPath, err := filepath.Abs("../../x/provider/testdata/provider.yaml") +// Use a helper function or environment variable to locate test data +providerPath := filepath.Join(testutil.GetTestDataDir(), "provider", "testdata", "provider.yaml")tests/e2e/market_grpc_test.go (1)
72-76: Consider making test data paths more robust.Similar to the provider test, using relative paths with
filepath.Abscan be fragile. Consider using a consistent approach across all e2e tests.-deploymentPath, err := filepath.Abs("../../x/deployment/testdata/deployment.yaml") -s.Require().NoError(err) - -providerPath, err := filepath.Abs("../../x/provider/testdata/provider.yaml") -s.Require().NoError(err) +deploymentPath := filepath.Join(testutil.GetTestDataDir(), "deployment", "testdata", "deployment.yaml") +providerPath := filepath.Join(testutil.GetTestDataDir(), "provider", "testdata", "provider.yaml")cmd/akash/cmd/app_creator.go (1)
50-77: Consider propagating errors instead of panicking where feasible.While panicking during app initialization is common in Cosmos SDK applications, consider whether some of these errors could be propagated to allow for better error handling at higher levels.
For example, the method signature could be updated to return an error:
-func (a appCreator) newApp( +func (a appCreator) newApp( logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts servertypes.AppOptions, -) servertypes.Application { +) (servertypes.Application, error) {app/modules.go (3)
90-90: Address the TODO commentThe TODO comment references issue akash-network/support#4. Please ensure this is tracked and addressed.
Would you like me to open a new issue to track this TODO if the referenced issue is closed?
144-146: Consider documenting the nolint directiveThe
//nolint: staticcheckdirective suggests the params module might be deprecated. Consider adding a comment explaining why it's still needed.+ // params module is deprecated but still required for migration compatibility params.NewAppModule( //nolint: staticcheck app.Keepers.Cosmos.Params, ),
290-329: Inconsistent spacing between Akash modulesThe empty lines between Akash simulation modules (lines 295, 305, 316, 324) are inconsistent with the formatting in the
appModulesfunction where modules are listed without extra spacing.// akash sim modules take.NewAppModule( app.cdc, app.Keepers.Akash.Take, ), - deployment.NewAppModule( app.cdc, app.Keepers.Akash.Deployment, app.Keepers.Akash.Market, app.Keepers.Akash.Escrow, app.Keepers.Cosmos.Acct, app.Keepers.Cosmos.Bank, app.Keepers.Cosmos.Authz, ), - market.NewAppModule( app.cdc, app.Keepers.Akash.Market, app.Keepers.Akash.Escrow, app.Keepers.Akash.Audit, app.Keepers.Akash.Deployment, app.Keepers.Akash.Provider, app.Keepers.Cosmos.Acct, app.Keepers.Cosmos.Bank, ), - provider.NewAppModule( app.cdc, app.Keepers.Akash.Provider, app.Keepers.Cosmos.Acct, app.Keepers.Cosmos.Bank, app.Keepers.Akash.Market, ), - cert.NewAppModule( app.cdc, app.Keepers.Akash.Cert, ),tests/e2e/deployment_grpc_test.go (1)
34-35: Consider making deployment path more flexibleThe hardcoded relative path might break if tests are moved or run from different directories.
Consider using testdata embedded in the test package or making the path configurable through environment variables.
tests/upgrade/upgrade_test.go (1)
401-403: Track the temporary gas workaroundThe fixed gas value is a workaround for an RPC error. This should be tracked and fixed when the underlying issue is resolved.
Would you like me to create an issue to track fixing the "auto" gas calculation RPC error?
app/sim_test.go (1)
294-299: Consider documenting why IBC host store key is commented out.The IBC host store key comparison is commented out. If this is intentional (e.g., due to non-deterministic state), please add a comment explaining why.
- // { - // ibchost.StoreKey, - // appA, - // appB, - // [][]byte{}, - // }, + // IBC host store key comparison is skipped because [reason] + // { + // ibchost.StoreKey, + // appA, + // appB, + // [][]byte{}, + // },cmd/akash/cmd/genesis.go (1)
140-141: Address TODO comments for community pool initialization.There are TODO comments for setting initial community pool in both distribution and crisis modules. These should be addressed or tracked in an issue.
Would you like me to create an issue to track the community pool initialization or help implement it?
Also applies to: 162-163
tests/e2e/market_cli_test.go (1)
482-502: Consider adding a comment explaining the expected state.The test correctly verifies no open orders remain, but it would be clearer with a comment explaining that orders are automatically closed when leases are created.
// Naming as Test{number} just to run all tests sequentially func (s *marketIntegrationTestSuite) Test4CloseOrder() { + // Orders should be closed automatically when leases are created in previous tests ctx := context.Background()tests/e2e/deployment_cli_test.go (1)
314-315: Add safety check for group existenceConsider adding a check to ensure the deployment has groups before accessing them to prevent potential panics.
+s.Require().Greater(len(createdDep.Groups), 0, "Deployment should have at least one group") s.Require().NotEqual(0, len(createdDep.Groups))cmd/akash/cmd/testnetify/testnetify.go (1)
54-76: Document the required argument in command helpThe command now requires exactly one argument, but this isn't mentioned in the help text.
Update the example to show the required argument:
- Example: "testnetify", + Example: "testnetify <chain-id>",app/testnet.go (2)
56-60: Function signature properly updated for new architecture.The addition of the
dbparameter aligns with the new store management approach. Consider passingTestnetConfigby pointer for consistency with Go conventions for struct parameters.
88-88: Consider handling iterator.Close() errors.The pattern
_ = iterator.Close()ignores potential errors. While errors from Close() are rare, it's better to handle them consistently.- _ = iterator.Close() + if err := iterator.Close(); err != nil { + return nil + }app/types/app.go (1)
483-510: Params keeper initialization handles all required subspaces.Note that this uses the deprecated params module (as indicated by nolint comments). Consider planning migration to the new parameter handling approach in future updates.
app/app.go (2)
390-401: Consider error handling instead of panic in lifecycle methods.While panicking ensures the chain halts on critical errors, consider if graceful error handling might be more appropriate for some cases.
510-514: Reflection usage for chainID is a temporary workaround.As noted in the comment, this reflection-based approach should be replaced once SDK 0.50.x provides proper access methods. Consider adding error handling for the reflection operation.
func (app *AkashApp) ChainID() string { field := reflect.ValueOf(app.BaseApp).Elem().FieldByName("chainID") + if !field.IsValid() { + return "" + } return field.String() }
| runs-on: gh-runner-test | ||
| needs: |
There was a problem hiding this comment.
Unknown runner label gh-runner-test
actionlint flags this label as not belonging to any GitHub-hosted runner set.
If it refers to a self-hosted runner, add it to an actionlint.yaml allow-list; otherwise switch to ubuntu-latest.
🧰 Tools
🪛 actionlint (1.7.7)
41-41: label "gh-runner-test" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
In .github/workflows/release.yaml at lines 41 to 42, the runner label
'gh-runner-test' is not recognized as a valid GitHub-hosted runner. If this is a
self-hosted runner, add 'gh-runner-test' to the allow-list in the
actionlint.yaml configuration file to prevent linting errors. Otherwise, replace
'gh-runner-test' with a valid GitHub-hosted runner label such as 'ubuntu-latest'
to ensure compatibility.
| ) | ||
| require.NoError(s.T(), err) | ||
| require.NoError(s.T(), s.Network().WaitForNextBlock()) | ||
| _ = s.ValidateTx(result.Bytes()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check ValidateTx return values
The return values from ValidateTx are ignored. Consider checking them to ensure transaction validation succeeded.
- _ = s.ValidateTx(result.Bytes())
+ txResp := s.ValidateTx(result.Bytes())
+ require.NotNil(s.T(), txResp)
+ require.Equal(s.T(), uint32(0), txResp.Code, "transaction validation failed")Also applies to: 60-60
🤖 Prompt for AI Agents
In tests/e2e/certs_cli_test.go at lines 46 and 60, the return values from the
ValidateTx call are ignored. Update the code to capture the returned values,
typically an error or a boolean indicating success, and add appropriate checks
such as error handling or assertions to confirm that the transaction validation
succeeded. This ensures that any validation failure is detected and handled
properly in the test.
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
go.mod (6)
3-5: Pin CI, Docker and dev tooling to Go 1.24.x to satisfygo/toolchain.Your module requires go 1.24.3 and toolchain go1.24.4. Ensure all builders use 1.24.x or
toolchainwill error.Run to audit configs:
#!/usr/bin/env bash fd -t f .github/workflows | xargs -r rg -n "actions/setup-go|go-version|go-version-file" -A2 fd -t f 'Dockerfile*|devcontainer.json|Makefile|Taskfile*' | xargs -r rg -n "FROM golang|GO_VERSION|toolchain|1\.24"
1-1: Module path migrated topkg.akt.dev/node— scrub old imports and URLs repo-wide.Ensure no lingering
github.com/akash-network/nodeimports/links remain in code, docs, CI, or Dockerfiles.#!/usr/bin/env bash rg -n --hidden --glob '!.git' $'github.com/akash-network/node|module github.com/akash-network/node'Also applies to: 50-52
73-76: Drop stale replace pins for x/exp and rapid; they conflict with the graph (build risk).You pin
golang.org/x/expto a 2023 commit andrapidto v0.5.5 while the graph pulls newer versions (x/exp 2025-03…, rapid v1.2.0). These replaces can force API mismatches under SDK 0.53.x.Apply:
@@ - // stick with compatible version or x/exp in v0.47.x line - golang.org/x/exp => golang.org/x/exp v0.0.0-20230711153332-06a737ee72cb - // stick with compatible version of rapid in v0.47.x line - pgregory.net/rapid => pgregory.net/rapid v0.5.5Then:
go mod tidyAlso applies to: 261-261, 279-279
244-246: Replace Zondax HID/Ledger with intended forks to avoid darwin/arm64 and transport issues.
github.com/zondax/hid v0.9.2andgithub.com/zondax/ledger-go v0.14.3still resolve indirectly. If the plan is to usetroian/hidandakash-network/ledger-go, add replaces so the graph stops pulling Zondax.@@ replace ( @@ github.com/cosmos/gogoproto => github.com/akash-network/gogoproto v1.7.0-akash.2 + // use akash fork of ledger-go + github.com/zondax/ledger-go => github.com/akash-network/ledger-go v0.14.3 + // use maintained HID (darwin/arm64 fixes) + github.com/zondax/hid => github.com/troian/hid v0.13.2 @@ )Then:
go mod tidyAlso applies to: 59-66
177-177: Vulnerability: hashicorp/go-getter v1.7.8 — bump to v1.7.9 (symlink attack fix).Known GHSA-wjrx-6529-hcj3 affects 1.7.8; fixed in 1.7.9.
- github.com/hashicorp/go-getter v1.7.8 // indirect + github.com/hashicorp/go-getter v1.7.9 // indirectFollow with:
go get github.com/hashicorp/[email protected] go mod tidy
258-258: Invalid module path:go.yaml.in/yaml/v2— remove (duplicatesgopkg.in/yaml.v2).
go.yaml.in/yaml/v2is not a valid module path and will breakgo mod tidy. You already havegopkg.in/yaml.v2 v2.4.0.- go.yaml.in/yaml/v2 v2.4.2 // indirectThen:
go mod tidyAlso applies to: 273-273
🧹 Nitpick comments (5)
cmd/akash/cmd/root.go (5)
62-67: Typos and updated terminology in comment.Fix minor typos and update Tendermint → CometBFT for consistency with imports.
Apply this diff:
- // seed child command execution with values such as AccountRetriver, Keyring, - // and a Tendermint RPC. This requires the use of a pointer reference when + // seed child command execution with values such as AccountRetriever, Keyring, + // and a CometBFT RPC. This requires the use of a pointer reference when
73-77: Avoid flag redefinition risk: don’t add flags inside Execute path.Defining persistent flags inside ExecuteWithCtx can cause “flag redefined” errors if the function is called more than once with the same rootCmd (tests, embedding, or multiple runs). Define these flags during command construction (initRootCmd) instead.
Apply this diff to remove here:
- rootCmd.PersistentFlags().String(cflags.FlagLogLevel, zerolog.InfoLevel.String(), "The logging level (trace|debug|info|warn|error|fatal|panic)") - rootCmd.PersistentFlags().String(cflags.FlagLogFormat, cmtcfg.LogFormatPlain, "The logging format (json|plain)") - rootCmd.PersistentFlags().Bool(cflags.FlagLogColor, false, "Pretty logging output. Applied only when log_format=plain") - rootCmd.PersistentFlags().String(cflags.FlagLogTimestamp, "", "Add timestamp prefix to the logs (rfc3339|rfc3339nano|kitchen)")Then add them in initRootCmd (see next comment).
82-109: Define logging flags at construction time (initRootCmd).Move the logging flags here to avoid late mutations and duplicate registration risk. This keeps all flag wiring in one place.
Apply this diff:
func initRootCmd(rootCmd *cobra.Command, encodingConfig sdkutil.EncodingConfig) { ac := appCreator{encodingConfig} home := app.DefaultHome + // Logging flags (define during construction to avoid redefinition on repeated executes) + pf := rootCmd.PersistentFlags() + if pf.Lookup(cflags.FlagLogLevel) == nil { + pf.String(cflags.FlagLogLevel, zerolog.InfoLevel.String(), "The logging level (trace|debug|info|warn|error|fatal|panic)") + } + if pf.Lookup(cflags.FlagLogFormat) == nil { + pf.String(cflags.FlagLogFormat, cmtcfg.LogFormatPlain, "The logging format (json|plain)") + } + if pf.Lookup(cflags.FlagLogColor) == nil { + pf.Bool(cflags.FlagLogColor, false, "Pretty logging output. Applied only when log_format=plain") + } + if pf.Lookup(cflags.FlagLogTimestamp) == nil { + pf.String(cflags.FlagLogTimestamp, "", "Add timestamp prefix to the logs (rfc3339|rfc3339nano|kitchen)") + } + debugCmd := debug.Cmd()
100-106: Use the local ‘home’ var consistently.You define home := app.DefaultHome above; use it here to avoid drift if DefaultHome ever changes.
Apply this diff:
- PrepareGenesisCmd(app.DefaultHome, app.ModuleBasics()), + PrepareGenesisCmd(home, app.ModuleBasics()),
118-131: Minor consistency: reuse local ‘home’ in genesisCommand.You compute home := app.DefaultHome and still pass app.DefaultHome directly. Use the local variable consistently.
Apply this diff:
- cmd := cli.GetGenesisCmd(app.ModuleBasics(), encodingConfig.TxConfig, app.DefaultHome, encodingConfig.SigningOptions.ValidatorAddressCodec) + cmd := cli.GetGenesisCmd(app.ModuleBasics(), encodingConfig.TxConfig, home, encodingConfig.SigningOptions.ValidatorAddressCodec)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
cmd/akash/cmd/root.go(3 hunks)go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/akash/cmd/root.go (5)
app/config.go (1)
ModuleBasics(70-72)app/app.go (1)
DefaultHome(82-82)cmd/akash/cmd/bech32.go (1)
ConvertBech32Cmd(12-47)cmd/akash/cmd/genesis.go (1)
PrepareGenesisCmd(36-106)cmd/akash/cmd/testnetify/testnetify.go (1)
GetCmd(48-263)
🪛 OSV Scanner (2.1.0)
go.mod
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks
⏰ 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: network-upgrade
- GitHub Check: sims
- GitHub Check: release-dry-run
- GitHub Check: lint-go
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: build-macos
- GitHub Check: release-dry-run
- GitHub Check: tests
- GitHub Check: sims
- GitHub Check: lint-shell
- GitHub Check: lint-go
- GitHub Check: coverage
- GitHub Check: build-bins
- GitHub Check: build-macos
🔇 Additional comments (4)
go.mod (1)
22-25: IBC-Go v10 with SDK v0.53.x: pairing looks correct.
github.com/cosmos/ibc-go/v10 v10.3.0withcosmos-sdk v0.53.xis the right compatibility set. Good catch upgrading from v8.7.x.cmd/akash/cmd/root.go (3)
31-46: Root wiring LGTM for SDK v0.50 (encoding + interfaces).Using sdkutil.MakeEncodingConfig and registering interfaces via ModuleBasics is correct and future-proof for CometBFT/SDK v0.50+.
90-106: Command set looks solid.Good modernization: sdkserver.StatusCommand plus pkg.akt.dev/go/cli Query/Tx/Keys, pruning/snapshot wiring, and Rosetta with the new codec/registry.
40-41: Unify DefaultHome usage in cmd/akashI’ve confirmed that the only place
cli.DefaultHomeis used incmd/akashis in thePersistentPreRunEcall (line 40), while every other reference in this package usesapp.DefaultHome. The environment prefix"AKASH"is also consistently passed to bothcli.GetPersistentPreRunEandcmd.Execute. Switching this one line will ensure a single source of truth for the home directory across all code paths.Apply this patch in
cmd/akash/cmd/root.go:- PersistentPreRunE: cli.GetPersistentPreRunE(encodingConfig, []string{"AKASH"}, cli.DefaultHome), + PersistentPreRunE: cli.GetPersistentPreRunE(encodingConfig, []string{"AKASH"}, app.DefaultHome),– No other occurrences of
cli.DefaultHomeremain incmd/akash
– TheExecute(rootCmd, "AKASH")call incmd/akash/main.goalready matches the same"AKASH"prefix
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cmd/akash/cmd/app_creator.go (4)
3-26: Add missing imports to support filesystem checks.Follow-up diffs below introduce os.Stat and os.MkdirAll; please add the stdlib os import.
import ( - "errors" + "errors" "io" "path/filepath" + "os"
78-96: Optionally avoid wiring snapshot store when interval is zero.If FlagStateSyncSnapshotInterval resolves to 0 (disabled), you can skip SetSnapshot to save some overhead. Not a blocker—BaseApp tolerates it—but it keeps runtime intent explicit.
Example sketch (no need to merge as-is):
- Read interval into a variable.
- Only build snapshot store and include baseapp.SetSnapshot(...) when interval > 0.
58-60: Prefer a named constant for "genesis_file" app option (if available).Using a raw string key risks drift; if there is a cflags constant for this key, prefer it for consistency.
118-129: Streamline export home flag handling & confirm viper mutation scope
- The
homePathcheck in appExport (lines 118–120) never actually influences the export logic. The CLI’s root Viper already readsFlagHomeand applies it to the Tendermint config viaconfig.SetRoot(homeDir)before this point (see util/server/server.go:69–71). Either remove the redundant precondition here or, if you intended to override the export path, explicitly applyhomePathto the export logic (for example, re-setting the Tendermint config root).- The in-place mutation of the
viper.Viperinstance (Set(cflags.FlagInvCheckPeriod, 1)) is confined to the localappOptspassed intoappExport. We found no evidence that this same Viper object is shared concurrently elsewhere in the process, so this mutation poses no race or side-effect risk.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmd/akash/cmd/app_creator.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/akash/cmd/app_creator.go (4)
util/server/server.go (1)
GetAppDBBackend(221-231)app/app.go (2)
NewApp(105-262)AkashApp(92-102)app/testnet.go (2)
TestnetConfig(46-51)InitAkashAppForTestnet(56-283)cmd/akash/cmd/testnetify/config.go (1)
TestnetConfig(60-66)
⏰ 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). (16)
- GitHub Check: network-upgrade
- GitHub Check: lint-shell
- GitHub Check: sims
- GitHub Check: release-dry-run
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: build-bins
- GitHub Check: build-macos
- GitHub Check: network-upgrade-names
- GitHub Check: sims
- GitHub Check: release-dry-run
- GitHub Check: lint-go
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: build-bins
🔇 Additional comments (5)
cmd/akash/cmd/app_creator.go (5)
40-42: Inter-block cache wiring LGTM.
44-47: Skip-upgrade heights mapping LGTM.Minor nit: consider documenting expected units and source of heights for future maintainers.
98-104: App construction aligns with app.NewApp signature and options.Good use of EncodingConfig and BaseApp options fan-out.
130-141: Height-gated export logic LGTM.Creating the app with loadLatest=false and calling LoadHeight(height) is the correct pattern.
147-151: *Type assertion to AkashApp is appropriate here.Given newApp constructs akash.NewApp, this assertion is a safe contract for testnet wiring.
| homeDir := cast.ToString(appOpts.Get(cflags.FlagHome)) | ||
| chainID := cast.ToString(appOpts.Get(cflags.FlagChainID)) | ||
| if chainID == "" { | ||
| // fallback to genesis chain-id | ||
| genDocFile := filepath.Join(homeDir, cast.ToString(appOpts.Get("genesis_file"))) | ||
| appGenesis, err := genutiltypes.AppGenesisFromFile(genDocFile) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| chainID = appGenesis.ChainID | ||
| } | ||
|
|
There was a problem hiding this comment.
ChainID fallback may panic on default setups when genesis_file is unset; add robust resolution and existence check.
Today, if --chain-id is not provided and appOpts lacks "genesis_file", filepath.Join(homeDir, "") results in homeDir being passed to AppGenesisFromFile, which will fail and panic. Resolve the genesis path robustly (absolute vs. relative) and default to /config/genesis.json when unspecified. Also check that the file exists before parsing to provide a clearer error.
homeDir := cast.ToString(appOpts.Get(cflags.FlagHome))
chainID := cast.ToString(appOpts.Get(cflags.FlagChainID))
if chainID == "" {
- // fallback to genesis chain-id
- genDocFile := filepath.Join(homeDir, cast.ToString(appOpts.Get("genesis_file")))
- appGenesis, err := genutiltypes.AppGenesisFromFile(genDocFile)
- if err != nil {
- panic(err)
- }
-
- chainID = appGenesis.ChainID
+ // fallback to genesis chain-id
+ gf := cast.ToString(appOpts.Get("genesis_file"))
+ var genDocFile string
+ switch {
+ case gf == "":
+ genDocFile = filepath.Join(homeDir, "config", "genesis.json")
+ case filepath.IsAbs(gf):
+ genDocFile = gf
+ default:
+ genDocFile = filepath.Join(homeDir, gf)
+ }
+ if _, statErr := os.Stat(genDocFile); statErr != nil {
+ panic(errors.New("cannot resolve chain-id: genesis file not found at: " + genDocFile))
+ }
+ appGenesis, err := genutiltypes.AppGenesisFromFile(genDocFile)
+ if err != nil {
+ panic(err)
+ }
+ chainID = appGenesis.ChainID
}📝 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.
| homeDir := cast.ToString(appOpts.Get(cflags.FlagHome)) | |
| chainID := cast.ToString(appOpts.Get(cflags.FlagChainID)) | |
| if chainID == "" { | |
| // fallback to genesis chain-id | |
| genDocFile := filepath.Join(homeDir, cast.ToString(appOpts.Get("genesis_file"))) | |
| appGenesis, err := genutiltypes.AppGenesisFromFile(genDocFile) | |
| if err != nil { | |
| panic(err) | |
| } | |
| chainID = appGenesis.ChainID | |
| } | |
| homeDir := cast.ToString(appOpts.Get(cflags.FlagHome)) | |
| chainID := cast.ToString(appOpts.Get(cflags.FlagChainID)) | |
| if chainID == "" { | |
| // fallback to genesis chain-id | |
| gf := cast.ToString(appOpts.Get("genesis_file")) | |
| var genDocFile string | |
| switch { | |
| case gf == "": | |
| genDocFile = filepath.Join(homeDir, "config", "genesis.json") | |
| case filepath.IsAbs(gf): | |
| genDocFile = gf | |
| default: | |
| genDocFile = filepath.Join(homeDir, gf) | |
| } | |
| if _, statErr := os.Stat(genDocFile); statErr != nil { | |
| panic(errors.New("cannot resolve chain-id: genesis file not found at: " + genDocFile)) | |
| } | |
| appGenesis, err := genutiltypes.AppGenesisFromFile(genDocFile) | |
| if err != nil { | |
| panic(err) | |
| } | |
| chainID = appGenesis.ChainID | |
| } |
🤖 Prompt for AI Agents
In cmd/akash/cmd/app_creator.go around lines 54-66, the current ChainID fallback
blindly joins homeDir with the possibly-empty "genesis_file" and calls
AppGenesisFromFile which can panic; change this to: if the appOpts
"genesis_file" is empty, set genDocFile to filepath.Join(homeDir, "config",
"genesis.json"), otherwise resolve the provided path to an absolute path (use
filepath.Abs or filepath.Join if relative), then check os.Stat/genDocFile for
existence and return/handle a clear error if missing or unreadable instead of
panicking, and only then call genutiltypes.AppGenesisFromFile and propagate any
parsing error with context so the process fails gracefully with a helpful
message.
| snapshotDir := filepath.Join(homeDir, "data", "snapshots") | ||
| snapshotDB, err := dbm.NewDB("metadata", sdkserver.GetAppDBBackend(appOpts), snapshotDir) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| snapshotStore, err := snapshots.NewStore(snapshotDB, snapshotDir) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure snapshot directory exists before opening the metadata DB.
On a fresh home, dbm.NewDB(...) with snapshotDir may fail if the directory tree is missing. Create it explicitly to avoid first-run errors.
snapshotDir := filepath.Join(homeDir, "data", "snapshots")
+ if err := os.MkdirAll(snapshotDir, 0o755); err != nil {
+ panic(err)
+ }
snapshotDB, err := dbm.NewDB("metadata", sdkserver.GetAppDBBackend(appOpts), snapshotDir)
if err != nil {
panic(err)
}📝 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.
| snapshotDir := filepath.Join(homeDir, "data", "snapshots") | |
| snapshotDB, err := dbm.NewDB("metadata", sdkserver.GetAppDBBackend(appOpts), snapshotDir) | |
| if err != nil { | |
| panic(err) | |
| } | |
| snapshotStore, err := snapshots.NewStore(snapshotDB, snapshotDir) | |
| if err != nil { | |
| panic(err) | |
| } | |
| snapshotDir := filepath.Join(homeDir, "data", "snapshots") | |
| if err := os.MkdirAll(snapshotDir, 0o755); err != nil { | |
| panic(err) | |
| } | |
| snapshotDB, err := dbm.NewDB("metadata", sdkserver.GetAppDBBackend(appOpts), snapshotDir) | |
| if err != nil { | |
| panic(err) | |
| } | |
| snapshotStore, err := snapshots.NewStore(snapshotDB, snapshotDir) | |
| if err != nil { | |
| panic(err) | |
| } |
🤖 Prompt for AI Agents
In cmd/akash/cmd/app_creator.go around lines 67 to 77, the code opens the
metadata DB using snapshotDir without ensuring the directory exists; on first
run NewDB can fail if the directory tree is missing. Before calling dbm.NewDB,
create the snapshotDir (and parents) using os.MkdirAll with appropriate
permissions (e.g. 0o700 or 0700) and handle any returned error, then proceed to
open the DB and store.
| tcfg, valid := appOpts.Get(cflags.KeyTestnetConfig).(akash.TestnetConfig) | ||
| if !valid { | ||
| panic("cflags.KeyTestnetConfig is not of type akash.TestnetConfig") | ||
| } | ||
|
|
||
| // Make modifications to the normal AkashApp required to run the network locally | ||
| return akash.InitAkashAppForTestnet(akashApp, tcfg) | ||
| return akash.InitAkashAppForTestnet(akashApp, db, tcfg) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that cflags.KeyTestnetConfig is programmatically injected as akash.TestnetConfig.
Viper typically stores primitives/JSON; you’re asserting a concrete struct. Ensure the command path sets this value as a typed object before invoking newTestnetApp, otherwise this will panic at runtime.
🏁 Script executed:
#!/bin/bash
# Confirm that KeyTestnetConfig is set as akash.TestnetConfig (typed) somewhere before app start.
rg -nC3 -S 'KeyTestnetConfig|TestnetConfig' --type=go
# Also check if testnetify config unmarshalling populates akash.TestnetConfig and is then injected into viper.
rg -nC3 -S 'Set\(\s*cflags\.KeyTestnetConfig' --type=goLength of output: 5872
This will confirm how appOpts.Get is implemented and whether the concrete struct or a pointer is retrieved.
🏁 Script executed:
#!/bin/bash
# Locate app_creator.go and show file path and context around the testnet config usage
file=$(find . -type f -path "*/cmd/akash/cmd/app_creator.go")
echo "Found file: $file"
# Show lines around the Get and type assertion for TestnetConfig
sed -n '140,180p' "$file"Length of output: 1167
🏁 Script executed:
#!/bin/bash
# Show context around setting Viper.Set and invoking testnetAppCreator
sed -n '330,420p' cmd/akash/cmd/testnetify/testnetify.goLength of output: 3389
Fix pointer/value mismatch in TestnetConfig assertion
The current implementation will always panic because in cmd/akash/cmd/app_creator.go you do:
tcfg, valid := appOpts.Get(cflags.KeyTestnetConfig).(akash.TestnetConfig)
if !valid {
panic("cflags.KeyTestnetConfig is not of type akash.TestnetConfig")
}However, in cmd/akash/cmd/testnetify/testnetify.go you call:
appConfig := &akash.TestnetConfig{ … }
sctx.Viper.Set(cflags.KeyTestnetConfig, appConfig)so Viper stores a *akash.TestnetConfig, not the value type. The type assertion therefore always fails at runtime.
Please update one of the following:
-
Change the assertion to accept a pointer, then dereference:
- tcfg, valid := appOpts.Get(cflags.KeyTestnetConfig).(akash.TestnetConfig) + tcfgPtr, valid := appOpts.Get(cflags.KeyTestnetConfig).(*akash.TestnetConfig) + if !valid || tcfgPtr == nil { + panic("cflags.KeyTestnetConfig is not a *akash.TestnetConfig") + } + tcfg := *tcfgPtr
-
Or store the value instead of the pointer when setting the config:
- sctx.Viper.Set(cflags.KeyTestnetConfig, appConfig) + sctx.Viper.Set(cflags.KeyTestnetConfig, *appConfig)
Either approach will ensure the type assertion in newTestnetApp succeeds and prevent a panic.
🤖 Prompt for AI Agents
In cmd/akash/cmd/app_creator.go around lines 153 to 160, the type assertion
expects akash.TestnetConfig (value) but Viper stores *akash.TestnetConfig
(pointer), causing the assertion to fail and panic; change the logic to assert a
*akash.TestnetConfig, check for validity, dereference it into a value (or handle
both pointer and value via a type switch) and then pass the dereferenced value
into InitAkashAppForTestnet so the call succeeds without panicking.
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/app.go (2)
422-426: BlockedAddrs double-hashes module names; use address strings as-is.ModuleAccountAddrs already returns address strings; calling NewModuleAddress again yields wrong keys.
- for acc := range perms { - blockedAddrs[authtypes.NewModuleAddress(acc).String()] = !allowedReceivingModAcc[acc] - } + for addr := range perms { + blockedAddrs[addr] = !allowedReceivingModAcc[addr] + }
322-339: Remove or integrate unused getGenesisTime
The function getGenesisTime in app/app.go (lines 322–339) is never called; either wire it into NewApp/InitChainer so tests’ GenesisTime setting takes effect or delete it to eliminate dead code.
♻️ Duplicate comments (9)
go.mod (5)
3-5: Pin CI/tooling to Go 1.24.x to satisfy go/toolchain directives.Workflows/devcontainers/Dockerfiles must use 1.24.x or builds will fail on the toolchain directive.
73-76: Drop stale replaces for x/exp and rapid; they conflict with newer indirects.These pins force 2023-era x/exp and rapid v0.5.5 while the graph already pulls newer versions (see lines 261 and 279). Remove the replaces.
- // stick with compatible version or x/exp in v0.47.x line - golang.org/x/exp => golang.org/x/exp v0.0.0-20230711153332-06a737ee72cb - // stick with compatible version of rapid in v0.47.x line - pgregory.net/rapid => pgregory.net/rapid v0.5.5After removal: run
go mod tidy.
59-66: Add replaces to switch Zondax HID/Ledger to intended forks.go.mod still resolves zondax/hid and zondax/ledger-go indirectly. If the intent is troian/hid and akash-network/ledger-go, add replaces.
replace ( @@ github.com/cosmos/gogoproto => github.com/akash-network/gogoproto v1.7.0-akash.2 + // use akash fork of ledger-go + github.com/zondax/ledger-go => github.com/akash-network/ledger-go v0.14.3 + // use maintained HID (darwin/arm64 fixes) + github.com/zondax/hid => github.com/troian/hid v0.13.2 @@ )Then
go mod tidyto drop the indirect zondax entries.Also applies to: 244-245
258-258: Invalid module path “go.yaml.in/yaml/v2” — remove duplicate; keep gopkg.in/yaml.v2.This path is invalid and duplicates the correct entry at Line 273; it will break
go mod tidy.- go.yaml.in/yaml/v2 v2.4.2 // indirect
177-177: High severity: hashicorp/go-getter v1.7.8 vulnerable (GHSA-wjrx-6529-hcj3). Upgrade to v1.7.9.Patch is available in 1.7.9. Bump and tidy.
- github.com/hashicorp/go-getter v1.7.8 // indirect + github.com/hashicorp/go-getter v1.7.9 // indirectapp/sim_test.go (3)
116-124: “GenesisTime” set in tests isn’t applied by the app.getGenesisTime exists but isn’t used in NewApp; tests will not get deterministic genesis time.
Would you like a patch to plumb GenesisTime into NewApp or InitChainer?
37-39: Fix IBC host import: use core/24-host (not core/exported).This will otherwise fail to compile for StoreKey/ModuleName.
Apply:
- ibchost "github.com/cosmos/ibc-go/v10/modules/core/exported" + ibchost "github.com/cosmos/ibc-go/v10/modules/core/24-host"
477-481: Pass the current test’s T to NewTestLogger.Avoid constructing &testing.T{}.
- if sim.FlagVerboseValue { - logger = log.NewTestLogger(&testing.T{}) - } else { + if sim.FlagVerboseValue { + logger = log.NewTestLogger(t) + } else {app/app.go (1)
162-168: Initialization order bug with gov routes vs hooks (tracked).Please resolve by setting hooks before any gov proposal handler can call into hooked keepers.
Would you like me to propose a concrete reordering patch inside InitNormalKeepers?
🧹 Nitpick comments (1)
app/app.go (1)
505-511: Reflect-based ChainID getter is a stopgap.Acceptable short-term; ensure removal once SDK exposes ChainID in 0.50.x.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
app/app.go(7 hunks)app/mac.go(2 hunks)app/modules.go(1 hunks)app/sim_test.go(5 hunks)app/types/app.go(3 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/modules.go
🧰 Additional context used
🧬 Code graph analysis (4)
app/mac.go (1)
x/escrow/alias.go (1)
ModuleName(11-11)
app/types/app.go (4)
x/market/hooks/hooks.go (2)
New(23-28)Hooks(13-16)x/escrow/keeper/keeper.go (2)
Keeper(28-48)NewKeeper(50-66)x/market/keeper/keeper.go (2)
Keeper(47-54)NewKeeper(57-64)testutil/state/suite.go (1)
Keepers(49-58)
app/sim_test.go (6)
app/app.go (3)
AkashApp(92-102)NewApp(105-262)AppName(78-78)app/genesis.go (2)
NewDefaultGenesisState(61-64)GenesisState(30-30)testutil/sims/simulation_helpers.go (5)
BuildSimulationOperations(62-85)SetupSimulation(28-51)CheckExportSimulation(89-114)PrintStats(117-121)DiffKVStores(145-179)app/mac.go (1)
ModuleAccountAddrs(26-33)app/sim/sim_utils.go (3)
SetupSimulation(23-49)CheckExportSimulation(78-103)PrintStats(106-110)app/config.go (1)
ModuleBasics(70-72)
app/app.go (6)
app/types/app.go (1)
App(123-134)app/mac.go (2)
ModuleAccountPerms(13-24)ModuleAccountAddrs(26-33)app/app_configure.go (1)
OrderInitGenesis(54-82)app/ante.go (1)
NewAnteHandler(21-61)app/genesis.go (1)
GenesisState(30-30)upgrades/types/types.go (1)
GetHeightPatchesList(99-101)
🪛 OSV Scanner (2.1.0)
go.mod
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks
⏰ 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: sims
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: build-bins
- GitHub Check: coverage
- GitHub Check: build-macos
- GitHub Check: sims
- GitHub Check: tests
- GitHub Check: lint-shell
- GitHub Check: build-macos
- GitHub Check: build-bins
- GitHub Check: lint-go
- GitHub Check: release-dry-run
- GitHub Check: coverage
🔇 Additional comments (5)
go.mod (2)
22-25: IBC-Go v10 with Cosmos SDK v0.53.x — compatible pair.Good alignment: ibc-go v10.3.0 matches SDK v0.53.3. No action needed here.
20-20: CometBFT require + replace to Akash fork — alignment looks correct.The require (v0.38.17) and replace (v0.38.17-akash.2) are consistent.
Also applies to: 59-66
app/mac.go (3)
9-11: Imports look correct for v10 and escrow module alias.Good move to ibc-go v10 transfer types and to emodule alias.
26-33: LGTM: address derivation wired to ModuleAccountPerms.This keeps addrs in sync with perms.
13-23: ModuleAccountPerms covers all module accounts — verified against everyauthtypes.NewModuleAddress(…)usage inapp/; no Akash-specific module accounts are missing or require extra Burner/Minter permissions.
| transfertypes "github.com/cosmos/ibc-go/v10/modules/apps/transfer/types" | ||
| ibchost "github.com/cosmos/ibc-go/v10/modules/core/exported" | ||
| "github.com/cosmos/ibc-go/v10/testing/simapp" | ||
|
|
There was a problem hiding this comment.
Fix IBC host import: use core/24-host.
Required for ModuleName/StoreKey references elsewhere.
- ibchost "github.com/cosmos/ibc-go/v10/modules/core/exported"
+ ibchost "github.com/cosmos/ibc-go/v10/modules/core/24-host"📝 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.
| transfertypes "github.com/cosmos/ibc-go/v10/modules/apps/transfer/types" | |
| ibchost "github.com/cosmos/ibc-go/v10/modules/core/exported" | |
| "github.com/cosmos/ibc-go/v10/testing/simapp" | |
| transfertypes "github.com/cosmos/ibc-go/v10/modules/apps/transfer/types" | |
| ibchost "github.com/cosmos/ibc-go/v10/modules/core/24-host" | |
| "github.com/cosmos/ibc-go/v10/testing/simapp" |
🤖 Prompt for AI Agents
In app/app.go around lines 59 to 62, the IBC host import is wrong: replace the
import of ibchost from "github.com/cosmos/ibc-go/v10/modules/core/exported" with
the correct host package path
"github.com/cosmos/ibc-go/v10/modules/core/24-host" so ModuleName and StoreKey
references resolve correctly; update the import line and keep the ibchost alias
(or adjust references) accordingly.
| ibchost.StoreKey, | ||
| appA, | ||
| appB, | ||
| [][]byte{}, | ||
| }, | ||
| { | ||
| ibctransfertypes.StoreKey, | ||
| appA, | ||
| appB, | ||
| [][]byte{}, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Store comparison depends on ibchost import fix.
Once ibchost points to core/24-host, ibchost.StoreKey will resolve.
Run to ensure no remaining “core/exported” imports:
🏁 Script executed:
#!/bin/bash
rg -n 'ibc-go/.*/core/exported' -C2Length of output: 1512
Replace all remaining core/exported imports with core/24-host
In the following files, update
github.com/cosmos/ibc-go/v10/modules/core/exported → github.com/cosmos/ibc-go/v10/modules/core/24-host
so that ibchost.StoreKey resolves correctly:
- app/app_configure.go
- app/app.go
- app/sim_test.go
- app/types/app.go
🤖 Prompt for AI Agents
In app/sim_test.go around lines 299 to 309 (and also update
app/app_configure.go, app/app.go, and app/types/app.go), replace any import of
"github.com/cosmos/ibc-go/v10/modules/core/exported" with
"github.com/cosmos/ibc-go/v10/modules/core/24-host" so that ibchost.StoreKey
resolves correctly; update the import blocks in those files, remove the old
import, add the new one, adjust any symbol references if needed, and run
goimports/gofmt to tidy imports.
| ibcexported "github.com/cosmos/ibc-go/v10/modules/core/exported" | ||
| ibckeeper "github.com/cosmos/ibc-go/v10/modules/core/keeper" | ||
| ibctm "github.com/cosmos/ibc-go/v10/modules/light-clients/07-tendermint" | ||
| emodule "pkg.akt.dev/go/node/escrow/module" |
There was a problem hiding this comment.
Use ibc host constants from core/24-host; replace ibcexported.
StoreKey/ModuleName live under core/24-host in ibc-go v10.
- ibcexported "github.com/cosmos/ibc-go/v10/modules/core/exported"
+ ibchost "github.com/cosmos/ibc-go/v10/modules/core/24-host"And replace usages in this file accordingly:
- runtime.NewKVStoreService(app.keys[ibcexported.StoreKey]),
- app.GetSubspace(ibcexported.ModuleName),
+ runtime.NewKVStoreService(app.keys[ibchost.StoreKey]),
+ app.GetSubspace(ibchost.ModuleName),- paramsKeeper.Subspace(ibcexported.ModuleName).WithKeyTable(ibctable)
+ paramsKeeper.Subspace(ibchost.ModuleName).WithKeyTable(ibctable)- ibcexported.StoreKey,
+ ibchost.StoreKey,Also applies to: 72-80
🤖 Prompt for AI Agents
In app/types/app.go around lines 56-59 (and similarly 72-80), the code imports
ibcexported from ibc-go v10 but StoreKey/ModuleName constants now live in
core/24-host; replace the ibcexported import with an import alias for the
24-host package (e.g., ibchost
"github.com/cosmos/ibc-go/v10/modules/core/24-host") and update all usages that
referenced ibcexported.StoreKey, ibcexported.ModuleName, etc., to use the
ibchost alias (e.g., ibchost.StoreKey, ibchost.ModuleName) throughout the file.
| app.Keepers.Cosmos.Crisis = crisiskeeper.NewKeeper( //nolint: staticcheck | ||
| cdc, runtime.NewKVStoreService(app.keys[crisistypes.StoreKey]), | ||
| invCheckPeriod, | ||
| app.Keepers.Cosmos.Bank, | ||
| authtypes.FeeCollectorName, | ||
| authtypes.NewModuleAddress(govtypes.ModuleName).String(), | ||
| addresscodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Crisis keeper constructed before Bank keeper — likely invalid dependency.
Crisis requires a valid BankKeeper; here it receives a zero-value keeper.
Move Crisis keeper creation into InitNormalKeepers after Bank is initialized:
- app.Keepers.Cosmos.Crisis = crisiskeeper.NewKeeper( //nolint: staticcheck
- cdc, runtime.NewKVStoreService(app.keys[crisistypes.StoreKey]),
- invCheckPeriod,
- app.Keepers.Cosmos.Bank,
- authtypes.FeeCollectorName,
- authtypes.NewModuleAddress(govtypes.ModuleName).String(),
- addresscodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()),
- )Insert in InitNormalKeepers immediately after Bank:
+ app.Keepers.Cosmos.Crisis = crisiskeeper.NewKeeper( //nolint: staticcheck
+ cdc, runtime.NewKVStoreService(app.keys[crisistypes.StoreKey]),
+ invCheckPeriod,
+ app.Keepers.Cosmos.Bank,
+ authtypes.FeeCollectorName,
+ authtypes.NewModuleAddress(govtypes.ModuleName).String(),
+ addresscodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()),
+ )📝 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.
| app.Keepers.Cosmos.Crisis = crisiskeeper.NewKeeper( //nolint: staticcheck | |
| cdc, runtime.NewKVStoreService(app.keys[crisistypes.StoreKey]), | |
| invCheckPeriod, | |
| app.Keepers.Cosmos.Bank, | |
| authtypes.FeeCollectorName, | |
| authtypes.NewModuleAddress(govtypes.ModuleName).String(), | |
| addresscodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()), | |
| ) |
🤖 Prompt for AI Agents
In app/types/app.go around lines 227 to 235 the Crisis keeper is being
constructed before the Bank keeper is initialized, so it receives a zero-value
BankKeeper; remove the crisiskeeper.NewKeeper call from this location and
instead instantiate app.Keepers.Cosmos.Crisis inside InitNormalKeepers
immediately after the Bank keeper is created and assigned (pass the
now-initialized app.Keepers.Cosmos.Bank along with the same decoder,
KVStoreService, invCheckPeriod, fee collector and governance module address
arguments), ensuring the original parameters are preserved and no other code
depends on the old location.
| // register Transfer Keepers | ||
| app.Keepers.Cosmos.Transfer = ibctransferkeeper.NewKeeper( | ||
| cdc, | ||
| runtime.NewKVStoreService(app.keys[ibctransfertypes.StoreKey]), | ||
| app.GetSubspace(ibctransfertypes.ModuleName), | ||
| app.Keepers.Cosmos.IBC.ChannelKeeper, | ||
| app.Keepers.Cosmos.IBC.ChannelKeeper, | ||
| bApp.MsgServiceRouter(), | ||
| app.Keepers.Cosmos.Acct, | ||
| app.Keepers.Cosmos.Bank, | ||
| authtypes.NewModuleAddress(govtypes.ModuleName).String(), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Transfer keeper: pass PortKeeper, not ChannelKeeper twice.
Signature is (ChannelKeeper, PortKeeper, ...).
- app.Keepers.Cosmos.IBC.ChannelKeeper,
- app.Keepers.Cosmos.IBC.ChannelKeeper,
+ app.Keepers.Cosmos.IBC.ChannelKeeper,
+ app.Keepers.Cosmos.IBC.PortKeeper,🤖 Prompt for AI Agents
In app/types/app.go around lines 375 to 387, the Transfer keeper is being
constructed with ChannelKeeper passed twice; the NewKeeper signature expects
(ChannelKeeper, PortKeeper, ...). Replace the second
app.Keepers.Cosmos.IBC.ChannelKeeper argument with the PortKeeper (e.g.
app.Keepers.Cosmos.IBC.PortKeeper) so the call becomes NewKeeper(cdc,
runtime.NewKVStoreService(...), app.GetSubspace(...),
app.Keepers.Cosmos.IBC.ChannelKeeper, app.Keepers.Cosmos.IBC.PortKeeper,
bApp.MsgServiceRouter(), ...).
| // initParamsKeeper init params keeper and its subspaces | ||
| func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino, key, tkey storetypes.StoreKey) paramskeeper.Keeper { // nolint: staticcheck | ||
| paramsKeeper := paramskeeper.NewKeeper(appCodec, legacyAmino, key, tkey) // nolint: staticcheck | ||
|
|
||
| ibctable := ibcclienttypes.ParamKeyTable() | ||
| ibctable.RegisterParamSet(&ibcconnectiontypes.Params{}) | ||
|
|
||
| paramsKeeper.Subspace(authtypes.ModuleName).WithKeyTable(authtypes.ParamKeyTable()) // nolint: staticcheck | ||
| paramsKeeper.Subspace(banktypes.ModuleName).WithKeyTable(banktypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(stakingtypes.ModuleName).WithKeyTable(stakingtypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(minttypes.ModuleName).WithKeyTable(minttypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(distrtypes.ModuleName).WithKeyTable(distrtypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(slashingtypes.ModuleName).WithKeyTable(slashingtypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(govtypes.ModuleName).WithKeyTable(govv1.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(crisistypes.ModuleName).WithKeyTable(crisistypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(ibctransfertypes.ModuleName).WithKeyTable(ibctransfertypes.ParamKeyTable()) | ||
| paramsKeeper.Subspace(ibcexported.ModuleName).WithKeyTable(ibctable) | ||
| paramsKeeper.Subspace(icacontrollertypes.SubModuleName) | ||
| paramsKeeper.Subspace(icahosttypes.SubModuleName) | ||
|
|
||
| // akash params subspaces | ||
| paramsKeeper.Subspace(dtypes.ModuleName).WithKeyTable(dv1beta.ParamKeyTable()) | ||
| paramsKeeper.Subspace(mtypes.ModuleName).WithKeyTable(mtypes.ParamKeyTable()) | ||
| paramsKeeper.Subspace(astakingtypes.ModuleName).WithKeyTable(astakingtypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(agovtypes.ModuleName).WithKeyTable(agovtypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
| paramsKeeper.Subspace(ttypes.ModuleName).WithKeyTable(ttypes.ParamKeyTable()) // nolint: staticcheck // SA1019 | ||
|
|
||
| return paramsKeeper | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update params subspaces to use ibchost.ModuleName (not ibcexported).
Matches the import fix above.
- paramsKeeper.Subspace(ibcexported.ModuleName).WithKeyTable(ibctable)
+ paramsKeeper.Subspace(ibchost.ModuleName).WithKeyTable(ibctable)🤖 Prompt for AI Agents
In app/types/app.go around lines 472 to 500, the params subspace is registered
using ibcexported.ModuleName but should use ibchost.ModuleName to match the
import change; update the call
paramsKeeper.Subspace(ibcexported.ModuleName).WithKeyTable(ibctable) to
paramsKeeper.Subspace(ibchost.ModuleName).WithKeyTable(ibctable) (and ensure the
ibchost package is imported) so the IBC client param key table is registered
under the correct module name.
| func kvStoreKeys() []string { | ||
| keys := []string{ | ||
| consensusparamtypes.StoreKey, | ||
| authtypes.StoreKey, | ||
| feegrant.StoreKey, | ||
| authzkeeper.StoreKey, | ||
| banktypes.StoreKey, | ||
| stakingtypes.StoreKey, | ||
| minttypes.StoreKey, | ||
| distrtypes.StoreKey, | ||
| slashingtypes.StoreKey, | ||
| govtypes.StoreKey, | ||
| paramstypes.StoreKey, | ||
| ibcexported.StoreKey, | ||
| upgradetypes.StoreKey, | ||
| evidencetypes.StoreKey, | ||
| ibctransfertypes.StoreKey, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
kvStoreKeys should reference ibchost.StoreKey.
Keep key list consistent with ibc-go v10.
- ibcexported.StoreKey,
+ ibchost.StoreKey,📝 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.
| func kvStoreKeys() []string { | |
| keys := []string{ | |
| consensusparamtypes.StoreKey, | |
| authtypes.StoreKey, | |
| feegrant.StoreKey, | |
| authzkeeper.StoreKey, | |
| banktypes.StoreKey, | |
| stakingtypes.StoreKey, | |
| minttypes.StoreKey, | |
| distrtypes.StoreKey, | |
| slashingtypes.StoreKey, | |
| govtypes.StoreKey, | |
| paramstypes.StoreKey, | |
| ibcexported.StoreKey, | |
| upgradetypes.StoreKey, | |
| evidencetypes.StoreKey, | |
| ibctransfertypes.StoreKey, | |
| } | |
| func kvStoreKeys() []string { | |
| keys := []string{ | |
| consensusparamtypes.StoreKey, | |
| authtypes.StoreKey, | |
| feegrant.StoreKey, | |
| authzkeeper.StoreKey, | |
| banktypes.StoreKey, | |
| stakingtypes.StoreKey, | |
| minttypes.StoreKey, | |
| distrtypes.StoreKey, | |
| slashingtypes.StoreKey, | |
| govtypes.StoreKey, | |
| paramstypes.StoreKey, | |
| ibchost.StoreKey, | |
| upgradetypes.StoreKey, | |
| evidencetypes.StoreKey, | |
| ibctransfertypes.StoreKey, | |
| } | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In app/types/app.go around lines 502 to 520, the kvStoreKeys list references
ibcexported.StoreKey but should use ibchost.StoreKey to match ibc-go v10; update
the slice to replace ibcexported.StoreKey with ibchost.StoreKey and add the
proper ibchost import from the ibc-go v10 module (adjust import alias if
necessary) so the code compiles and stays consistent with ibc-go v10.
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
go.mod (6)
1-1: Module path looks correct; verify all imports/URLs migrated to pkg.akt.dev/nodeScan and update any lingering github.com/akash-network/node references in imports, docs, scripts, and CI.
3-5: Align CI/Docker/dev with Go 1.24.x to satisfy go/toolchain directivesPin Go 1.24.x in workflows (actions/setup-go), Dockerfiles (golang:1.24-*), and devcontainer/Makefiles to avoid toolchain errors.
258-258: Remove invalid module path go.yaml.in/yaml/v2 (duplicate of gopkg.in/yaml.v2)This breaks tidy; keep only gopkg.in/yaml.v2 (line 273).
Apply:
- go.yaml.in/yaml/v2 v2.4.2 // indirectThen: go mod tidy
73-76: Remove stale replace pins for x/exp and rapid (conflict with indirect graph)These force 2023-era versions while the graph needs newer (see lines 261, 279). Drop them.
Apply:
@@ - // stick with compatible version or x/exp in v0.47.x line - golang.org/x/exp => golang.org/x/exp v0.0.0-20230711153332-06a737ee72cb - // stick with compatible version of rapid in v0.47.x line - pgregory.net/rapid => pgregory.net/rapid v0.5.5Then run: go mod tidy
176-178: Upgrade hashicorp/go-getter to patch known symlink vulnerabilityv1.7.8 is flagged (GHSA-wjrx-6529-hcj3). Bump to 1.7.9.
Apply:
- github.com/hashicorp/go-getter v1.7.8 // indirect + github.com/hashicorp/go-getter v1.7.9 // indirectThen: go get github.com/hashicorp/[email protected] && go mod tidy
59-65: Add replaces for HID/Ledger forks to remove indirect zondax pullsYou still pull zondax/hid and zondax/ledger-go indirectly (lines 244–245). Add replaces to intended forks.
Apply:
@@ github.com/cosmos/cosmos-sdk => github.com/akash-network/cosmos-sdk v0.53.4-akash.1 @@ github.com/cosmos/gogoproto => github.com/akash-network/gogoproto v1.7.0-akash.2 + // use akash fork of ledger-go + github.com/zondax/ledger-go => github.com/akash-network/ledger-go v0.14.3 + // use maintained HID (darwin/arm64 fixes) + github.com/zondax/hid => github.com/troian/hid v0.13.2Then: go mod tidy
app/sim_test.go (3)
116-123: “GenesisTime” is set here, but may be ignored by NewApp—verify integration.If NewApp doesn’t read GenesisTime from appOpts, these settings have no effect. Confirm that getGenesisTime (or equivalent) is used inside NewApp.
#!/bin/bash # Verify NewApp consumes GenesisTime from appOpts. rg -n -C2 -P '\bgetGenesisTime\b|\bGenesisTime\b' app | sed -n '1,200p'Also applies to: 158-166, 400-408, 487-494
37-39: Fix IBC host import: use core/24-host (current path won’t compile for StoreKey).Update the import to the host package that defines StoreKey/ModuleName.
- ibchost "github.com/cosmos/ibc-go/v10/modules/core/exported" + ibchost "github.com/cosmos/ibc-go/v10/modules/core/24-host"
479-481: Pass the current test’s t to NewTestLogger (avoid &testing.T{}).Constructing a fresh testing.T is incorrect and can cause odd behavior.
- if sim.FlagVerboseValue { - logger = log.NewTestLogger(&testing.T{}) - } else { + if sim.FlagVerboseValue { + logger = log.NewTestLogger(t) + } else {
🧹 Nitpick comments (3)
app/sim_test.go (3)
206-211: Use appB’s codec for InitGenesis.Passing appA.AppCodec() couples the test to appA; use appB.AppCodec() for correctness and clarity.
- _, err = appB.MM.InitGenesis(ctxB, appA.AppCodec(), genesisState) + _, err = appB.MM.InitGenesis(ctxB, appB.AppCodec(), genesisState)
128-129: Prefer t.Logf over fmt.Printf in tests.Keeps test output scoped to the test and hidden unless it fails or -v is used.
- fmt.Printf("config--------\n%v", config) + t.Logf("config: %+v", config)
436-438: Also honor “skip” from the second SetupSimulation call.If the simulator is disabled, skip this phase too for consistency.
- _, newDB, newDir, _, val, err := sim.SetupSimulation("leveldb-app-sim-2", "Simulation-2") + _, newDB, newDir, _, val, err := sim.SetupSimulation("leveldb-app-sim-2", "Simulation-2") require.NoError(t, err, "simulation setup failed", val) + if val { + t.Skip("skipping secondary simulation setup") + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
app/sim_test.go(5 hunks)go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/sim_test.go (4)
app/app.go (4)
AkashApp(92-102)DefaultHome(82-82)NewApp(105-262)AppName(78-78)app/genesis.go (2)
NewDefaultGenesisState(61-64)GenesisState(30-30)testutil/sims/simulation_helpers.go (6)
BuildSimulationOperations(62-85)SetupSimulation(28-51)CheckExportSimulation(89-114)PrintStats(117-121)DiffKVStores(145-179)GetSimulationLog(125-141)app/sim/sim_utils.go (3)
SetupSimulation(23-49)CheckExportSimulation(78-103)PrintStats(106-110)
🪛 OSV Scanner (2.1.0)
go.mod
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks in github.com/hashicorp/go-getter
(GO-2025-3892)
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks
⏰ 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). (17)
- GitHub Check: network-upgrade
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: coverage
- GitHub Check: build-bins
- GitHub Check: lint-shell
- GitHub Check: lint-go
- GitHub Check: build-macos
- GitHub Check: tests
- GitHub Check: lint-shell
- GitHub Check: release-dry-run
- GitHub Check: build-bins
- GitHub Check: sims
- GitHub Check: lint-go
- GitHub Check: coverage
- GitHub Check: build-macos
- GitHub Check: tests
🔇 Additional comments (5)
go.mod (3)
24-24: LGTM: ibc-go v10.x matches Cosmos SDK v0.53.xCompatibility is correct for the 0.53 line.
70-72: Confirm single goleveldb version after tidyYou pin goleveldb via replace, but an indirect entry (line 234) references a different version. After tidy, ensure only the pinned one remains.
244-246: Follow-up: expect zondax/ to disappear after adding replaces*After adding the HID/Ledger replaces, these indirect zondax lines should drop from go.mod/go.sum.
app/sim_test.go (2)
10-11: Use x/authz/types.StoreKey (not keeper).StoreKey is defined in x/authz/types. Switch the import and usage to avoid a compile-time error and unused import.
- authzkeeper "github.com/cosmos/cosmos-sdk/x/authz/keeper" + authztypes "github.com/cosmos/cosmos-sdk/x/authz/types"- authzkeeper.StoreKey, + authztypes.StoreKey,Also applies to: 236-244
203-205: Use deliverTx contexts for genesis application (NewContext(false)).Reading/initializing genesis with a checkTx context is atypical; prefer false for clarity.
- ctxA := appA.NewContext(true) - ctxB := appB.NewContext(true) + ctxA := appA.NewContext(false) + ctxB := appB.NewContext(false)
Signed-off-by: Artur Troian <[email protected]>
during deposit authorization use only funds that remaining unset after deducting from owner balance Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
go.mod (4)
3-5: Align CI/Docker to Go 1.24.x toolchaingo 1.24.3 / toolchain 1.24.4 require builders pinned to 1.24.x. Ensure workflows, devcontainers, and Dockerfiles use 1.24.x to avoid toolchain errors.
Run to verify:
#!/usr/bin/env bash fd -t f .github/workflows | xargs -r rg -n "actions/setup-go|go-version" -A1 fd -t f Dockerfile | xargs -r rg -n "^FROM golang" fd -t f 'devcontainer.json|Makefile|Taskfile*' | xargs -r rg -n "GO_VERSION|toolchain|1\.24"
73-76: Remove stale replace pins for x/exp and rapid (conflict with graph)You pin very old versions while the graph already requires newer ones (x/exp at 2025-03…, rapid v1.2.0). This can cause API mismatches.
Apply:
- // stick with compatible version or x/exp in v0.47.x line - golang.org/x/exp => golang.org/x/exp v0.0.0-20230711153332-06a737ee72cb - // stick with compatible version of rapid in v0.47.x line - pgregory.net/rapid => pgregory.net/rapid v0.5.5Then:
go mod tidyAlso applies to: 261-261, 279-279
244-246: Add replaces to use troian/hid and akash-network/ledger-go forksgo.mod still pulls zondax/hid and zondax/ledger-go indirectly. If the intent is to use the maintained forks, add replaces to avoid mixing implementations.
Apply inside the existing replace block:
github.com/cosmos/gogoproto => github.com/akash-network/gogoproto v1.7.0-akash.2 + // use akash fork of ledger-go + github.com/zondax/ledger-go => github.com/akash-network/ledger-go v0.14.3 + // use maintained HID (darwin/arm64 fixes) + github.com/zondax/hid => github.com/troian/hid v0.13.2Then:
go mod tidy rg -n "zondax/(hid|ledger-go)" go.modAlso applies to: 59-76
258-258: Invalid module path “go.yaml.in/yaml/v2” — remove duplicate/bad entryThis path is invalid and duplicates gopkg.in/yaml.v2. It will break tidy.
Apply:
- go.yaml.in/yaml/v2 v2.4.2 // indirectThen:
go mod tidyAlso applies to: 273-273
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(1 hunks)
🧰 Additional context used
🪛 OSV Scanner (2.1.0)
go.mod
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks in github.com/hashicorp/go-getter
(GO-2025-3892)
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks
⏰ 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). (16)
- GitHub Check: sims
- GitHub Check: network-upgrade-names
- GitHub Check: release-dry-run
- GitHub Check: lint-shell
- GitHub Check: coverage
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: build-bins
- GitHub Check: build-macos
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: build-macos
- GitHub Check: sims
- GitHub Check: build-bins
- GitHub Check: lint-go
- GitHub Check: release-dry-run
🔇 Additional comments (2)
go.mod (2)
21-26: SDK 0.53.3 + IBC-Go v10.x combo looks correctThe pairing (Cosmos SDK v0.53.x, IBC-Go v10.x) is the supported matrix. Good upgrade.
59-65: Akash forks for CometBFT/SDK/gogoproto and goleveldb pin look goodReplaces to akash forks and the required goleveldb pin for 0.53.x are appropriate.
Also applies to: 70-72
Signed-off-by: Artur Troian <[email protected]>
|
Marked as stale; will be closed in five days. |
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
go.mod (3)
1-1: Module path migration: verify all imports/URLs updated to pkg.akt.dev/nodeScan and replace lingering github.com/akash-network/node references across code, docs, scripts, CI, and Dockerfiles.
#!/usr/bin/env bash rg -n --hidden --glob '!.git' $'github.com/akash-network/node' -C2
3-5: Go 1.24 toolchain: align CI/Docker/devcontainerPin actions/setup-go to 1.24.x and base images to golang:1.24-*. Prevent toolchain mismatch failures.
#!/usr/bin/env bash fd -t f .github/workflows | xargs -r rg -n "actions/setup-go|go-version" -A2 fd -t f Dockerfile . | xargs -r rg -n "^FROM golang" fd -t f 'devcontainer.json|Makefile|Taskfile*' | xargs -r rg -n "GO_VERSION|toolchain|1\.24"
55-77: HID/Ledger forks: add replaces if you intend troian/hid and akash-network/ledger-goIndirect zondax deps still pulled. Add replaces to target intended forks (darwin/arm64 fixes, Akash Ledger).
replace ( @@ github.com/cosmos/gogoproto => github.com/akash-network/gogoproto v1.7.0-akash.2 + // use akash fork of ledger-go + github.com/zondax/ledger-go => github.com/akash-network/ledger-go v0.14.3 + // use maintained HID (darwin/arm64 fixes) + github.com/zondax/hid => github.com/troian/hid v0.13.2 @@ )Then:
go mod tidy && rg -n "zondax/(hid|ledger-go)" go.modAlso applies to: 244-246
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(1 hunks)
🧰 Additional context used
🪛 OSV Scanner (2.1.0)
go.mod
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks in github.com/hashicorp/go-getter
(GO-2025-3892)
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks
⏰ 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). (9)
- GitHub Check: release-dry-run
- GitHub Check: network-upgrade-names
- GitHub Check: coverage
- GitHub Check: build-bins
- GitHub Check: sims
- GitHub Check: lint-go
- GitHub Check: lint-shell
- GitHub Check: build-macos
- GitHub Check: tests
🔇 Additional comments (1)
go.mod (1)
20-26: IBC-Go v10 with SDK v0.53.x is correctPairing ibc-go/v10.3.0 with cosmos-sdk v0.53.x/cometbft v0.38.x looks compatible.
| // stick with compatible version or x/exp in v0.47.x line | ||
| golang.org/x/exp => golang.org/x/exp v0.0.0-20230711153332-06a737ee72cb | ||
| // stick with compatible version of rapid in v0.47.x line | ||
| pgregory.net/rapid => pgregory.net/rapid v0.5.5 |
There was a problem hiding this comment.
Stale replace pins (x/exp, rapid) conflict with newer transitive versions
These forced downgrades will likely break builds under SDK 0.53.x. Remove them (preferred) or bump to the actually used versions.
@@
- // stick with compatible version or x/exp in v0.47.x line
- golang.org/x/exp => golang.org/x/exp v0.0.0-20230711153332-06a737ee72cb
- // stick with compatible version of rapid in v0.47.x line
- pgregory.net/rapid => pgregory.net/rapid v0.5.5Then:
go mod tidyAlso applies to: 261-261, 279-279
🤖 Prompt for AI Agents
In go.mod around lines 73-76, the replace directives pinning golang.org/x/exp
and pgregory.net/rapid to older commits/versions are stale and conflict with
newer transitive requirements; remove those replace lines (or update them to the
correct, current versions used by the build) and then run `go mod tidy` to
refresh module graph; repeat the same removal/update for the other occurrences
mentioned at lines 261 and 279 so the SDK 0.53.x transitive versions are allowed
to resolve correctly.
| github.com/gtank/merlin v0.1.1 // indirect | ||
| github.com/gtank/ristretto255 v0.1.2 // indirect | ||
| github.com/hashicorp/go-cleanhttp v0.5.2 // indirect | ||
| github.com/hashicorp/go-getter v1.7.8 // indirect |
There was a problem hiding this comment.
Security: hashicorp/go-getter v1.7.8 is vulnerable (symlink attack) — bump to v1.7.9
Patch to 1.7.9.
- github.com/hashicorp/go-getter v1.7.8 // indirect
+ github.com/hashicorp/go-getter v1.7.9 // indirectThen run:
go get github.com/hashicorp/[email protected] && go mod tidy🧰 Tools
🪛 OSV Scanner (2.1.0)
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks in github.com/hashicorp/go-getter
(GO-2025-3892)
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks
🤖 Prompt for AI Agents
In go.mod around line 177, the dependency github.com/hashicorp/go-getter is
pinned at v1.7.8 which is vulnerable; update the requirement to v1.7.9 and
refresh modules. Change the version entry to v1.7.9, then run: go get
github.com/hashicorp/[email protected] && go mod tidy to update go.sum and ensure
the lockfile is consistent; run your test suite or build to verify nothing
breaks.
| google.golang.org/genproto/googleapis/api v0.0.0-20240311132316-a219d84964c2 // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 // indirect | ||
| google.golang.org/protobuf v1.35.1 // indirect | ||
| go.yaml.in/yaml/v2 v2.4.2 // indirect |
There was a problem hiding this comment.
Invalid module path: go.yaml.in/yaml/v2 — remove; keep gopkg.in/yaml.v2
Bad path at Line 258 breaks go mod tidy. Delete it.
- go.yaml.in/yaml/v2 v2.4.2 // indirectAfterward: run go mod tidy.
Also applies to: 273-273
🤖 Prompt for AI Agents
In go.mod around lines 258 and 273, there are invalid module paths
"go.yaml.in/yaml/v2" which break go mod tidy; remove those incorrect entries and
ensure the correct module "gopkg.in/yaml.v2" remains, then run "go mod tidy" to
clean and verify module dependencies.
Signed-off-by: Artur Troian <[email protected]>
Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
go.mod (6)
1-5: Previously flagged: Module path and Go version alignmentThese issues have been identified in earlier reviews:
Module path migration (line 1): The change to
pkg.akt.dev/noderequires updating all references togithub.com/akash-network/nodeacross the codebase (docs, scripts, imports, CI configs, Dockerfiles).Go 1.24.x toolchain (lines 3-5): CI workflows, Dockerfiles (especially
_build/Dockerfile.test), devcontainer.json, and Makefiles need to be updated to use Go 1.24.x to match these requirements.Please refer to the detailed remediation steps in the previous review comments.
73-76: Previously flagged: Remove or update stale replace pinsThese replace directives pin
golang.org/x/expto a 2023 commit andpgregory.net/rapidto v0.5.5, but the dependency graph requires newer versions (a 2025 x/exp and rapid v1.2.0). This conflicts with SDK v0.53.x requirements and will likely cause build failures.The recommended fix (remove these stale replaces) was provided in an earlier review comment.
55-77: Previously flagged: Missing HID and Ledger fork replacesThe replace block is missing redirects for the Zondax HID and Ledger modules to the intended forks (
troian/hidandakash-network/ledger-go). This was identified in previous reviews with detailed remediation steps.Without these replaces, the original zondax modules (visible at lines 244-245) will be pulled instead of the forks.
244-245: Zondax modules present due to missing replace directivesThese indirect dependencies on the original Zondax modules are a symptom of the missing replace directives flagged earlier. Once you add the replaces pointing
github.com/zondax/hid→github.com/troian/hidandgithub.com/zondax/ledger-go→github.com/akash-network/ledger-go, these entries should disappear after runninggo mod tidy.
177-177: Previously flagged: Security vulnerability in go-getter v1.7.8This version of
github.com/hashicorp/go-getteris vulnerable to symlink attacks (GO-2025-3892, GHSA-wjrx-6529-hcj3). The fix is to upgrade to v1.7.9.Detailed remediation steps were provided in an earlier review comment.
258-258: Previously flagged: Invalid module path go.yaml.in/yaml/v2This is not a valid Go module path and will break
go mod tidy. The correct module path isgopkg.in/yaml.v2(present at line 273).The fix is to remove line 258, as detailed in a previous review comment.
script/upgrades.sh (1)
438-438: Remove error suppression for testnetify command.The
|| truesuppresses errors from the testnetify command, which could hide critical failures during state preparation.Apply this diff:
- $AKASH testnetify --home="$valdir" --testnet-rootdir="$validators_dir" --testnet-config="${STATE_CONFIG}" --yes || true + $AKASH testnetify --home="$valdir" --testnet-rootdir="$validators_dir" --testnet-config="${STATE_CONFIG}" --yesmake/init.mk (1)
70-74: Major version variables still missing for golangci-lint v2 and mockery v3.As noted in the previous review, the major version variables
GOLANGCI_LINT_MAJORandMOCKERY_MAJORare not defined. Without these, Go module install paths will be invalid (e.g.,github.com/golangci/golangci-lint/v2/cmd/golangci-lintandgithub.com/vektra/mockery/v3).Please add these variable definitions as suggested in the earlier review comment.
🧹 Nitpick comments (5)
tests/upgrade/upgrade_test.go (1)
55-55: Consider fixing typo: "Commited" → "Committed".The constant name
nodeEventBlockCommitedcontains a typo. While functional, fixing it would improve code consistency.Apply this diff to fix the typo:
- nodeEventBlockCommited + nodeEventBlockCommittedNote: This would require updating all references to this constant throughout the file (lines 1030, 1296).
script/upgrades.sh (3)
127-128: Add error handling for metadata fetching.The
curlcommand could fail silently if the URL is unreachable or returns invalid JSON. Consider adding error checks to fail fast with a clear message.Apply this diff to add error handling:
-CHAIN_METADATA=$(curl -s "${CHAIN_METADATA_URL}") -GENESIS_URL="$(echo "$CHAIN_METADATA" | jq -r '.codebase.genesis.genesis_url? // .genesis?')" +CHAIN_METADATA=$(curl -s "${CHAIN_METADATA_URL}") +if [[ -z "$CHAIN_METADATA" ]]; then + echoerr "Failed to fetch chain metadata from ${CHAIN_METADATA_URL}" + exit 1 +fi +GENESIS_URL="$(echo "$CHAIN_METADATA" | jq -r '.codebase.genesis.genesis_url? // .genesis?')" +if [[ -z "$GENESIS_URL" || "$GENESIS_URL" == "null" ]]; then + echoerr "Failed to extract genesis URL from chain metadata" + exit 1 +fi
160-177: Improve variable naming and add default case.The variable name
tar_cmdis misleading since the function now returns a file extension string, not a command. Additionally, there's no explicit handling when the file type doesn't match any case.Apply this diff to improve clarity:
function content_type() { case "$1" in *.tar.cz*) - tar_cmd="tar.cz" + file_ext="tar.cz" ;; *.tar.gz*) - tar_cmd="tar.gz" + file_ext="tar.gz" ;; *.tar.lz4*) - tar_cmd="tar.lz4" + file_ext="tar.lz4" ;; *.tar.zst*) - tar_cmd="tar.zst" + file_ext="tar.zst" ;; + *) + file_ext="" + ;; esac - echo "$tar_cmd" + echo "$file_ext" }
308-310: Remove redundant genesis file deletion.The
genesis_fileis declared and immediately deleted before any genesis file exists. This removal is unnecessary in theinitfunction sinceprepare_statenow handles genesis setup.Apply this diff:
AKASH=$genesis_bin/akash - genesis_file=${valdir}/config/genesis.json - rm -f "$genesis_file" - $AKASH init --home "$valdir" "$(jq -rc '.moniker' <<<"$val")" >/dev/null 2>&1make/test-upgrade.mk (1)
38-38: Verify removal of init dependency from test target.The
initprerequisite has been commented out, which means runningmake testwill no longer automatically initialize the test environment. This could break existing workflows if developers expect automatic initialization.Please confirm this is intentional. If so, consider updating documentation to clarify that users must run
make initormake test-resetbeforemake test.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(1 hunks)make/init.mk(4 hunks)make/test-upgrade.mk(4 hunks)script/upgrades.sh(12 hunks)tests/upgrade/test-cases.json(1 hunks)tests/upgrade/upgrade_test.go(14 hunks)
🧰 Additional context used
🪛 OSV Scanner (2.2.2)
go.mod
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks in github.com/hashicorp/go-getter
(GO-2025-3892)
[HIGH] 177-177: github.com/hashicorp/go-getter 1.7.8: HashiCorp go-getter Vulnerable to Symlink Attacks
🔇 Additional comments (25)
go.mod (1)
7-53: LGTM: Require block properly aligned with Cosmos SDK v0.53.3The dependencies are correctly updated:
- Cosmos SDK v0.53.3 with corresponding cosmossdk.io/* modules
- IBC-Go v10.3.0 (compatible with SDK v0.53.x)
- CometBFT v0.38.17
- Akash-specific packages using the new pkg.akt.dev module path
tests/upgrade/test-cases.json (1)
2-191: LGTM! Comprehensive upgrade test configuration.The test case structure is well-organized with:
- Clear version bump to v1.0.0
- Explicit module additions (07-tendermint, consensus) and removals (crisis, agov, astaking)
- Thorough multi-step migration paths for all affected modules
The incremental migration paths (e.g., auth: 2→3→4→5, staking: 2→3→4→5) properly reflect the expected upgrade sequence for Cosmos SDK v0.50+.
tests/upgrade/upgrade_test.go (5)
18-18: LGTM! Import updates align with SDK v0.50 migration.The import changes properly reflect the repository migration:
- Added
runtime/debugfor enhanced error reporting- Updated to
cosmossdk.io/math(standard SDK v0.50+ path)- Migrated to
pkg.akt.devmodule pathsAlso applies to: 25-25, 34-34, 36-38
401-403: Verify if gas estimation workaround needs follow-up.The hardcoded gas value (500000) bypasses the
autoestimation due to an RPC error. While this unblocks the upgrade test, consider whether this workaround should be tracked for future resolution.If the underlying gas estimation issue needs fixing, consider opening a follow-up issue to investigate the "unknown query path" RPC error.
875-888: LGTM! Enhanced error handling improves debuggability.The addition of explicit error handling with stack traces and error logging will significantly help diagnose test failures. The defer pattern correctly ensures logging happens even on panic.
1209-1245: LGTM! Adaptive watchdog timing improves test reliability.The generous initial timeout (180 minutes) for the first few blocks accommodates the overhead of chain startup and state migrations during upgrades. The automatic reduction to 20 minutes after 3 blocks ensures timely detection of stuck chains while avoiding false positives during initialization.
1266-1266: LGTM! Added alternative log pattern for block commit detection.The additional
executedBlock2pattern improves robustness by catching both indexed block logs and committed state logs, ensuring reliable block detection across different logging configurations.script/upgrades.sh (10)
36-38: LGTM! Clear default configuration.The new global variables provide sensible defaults for chain metadata and snapshot URLs while allowing STATE_CONFIG to be specified via CLI.
41-41: LGTM! Proper long option declaration.The new CLI options (chain-meta, snapshot-url, state-config) are correctly declared with the trailing colon to indicate they require arguments.
114-122: LGTM! Clean option parsing.The new case branches properly handle the chain-meta, snapshot-url, and state-config options by assigning values to the corresponding global variables.
138-158: LGTM! Clear extraction command mapping.The refactored
tar_by_content_typefunction cleanly maps file extensions to their corresponding extraction commands.
194-202: LGTM! Robust filename extraction.The case-insensitive grep and fallback to the original URL make this function more resilient.
204-212: LGTM! Useful redirect handling.The
content_locationfunction properly extracts HTTP redirect locations, which is needed for the snapshot download logic.
336-343: LGTM! Clean separation of concerns.The refactored
initfunction properly delegates key import and state preparation to dedicated functions, improving maintainability.
408-436: LGTM! Comprehensive snapshot handling.The snapshot download and extraction logic properly handles multiple compression formats and the nested directory structure. The check for an existing snapshot file avoids redundant downloads.
440-454: LGTM! Efficient multi-validator setup.The logic correctly replicates the first validator's state to subsequent validators and uses symlinks for the shared genesis.json file.
580-583: LGTM! Proper command dispatch.The new
prepare-statecommand follows the established pattern and correctly invokes theprepare_statefunction.make/test-upgrade.mk (4)
28-28: LGTM! Proper state config integration.The
inittarget correctly passes theSTATE_CONFIGvariable to the upgrades script using the new--state-configoption.
52-52: LGTM! Integrated state preparation in test-reset.The
test-resettarget correctly invokes the newprepare-statecommand with the appropriate configuration.
54-56: LGTM! Useful standalone target.The new
prepare-statetarget allows developers to explicitly prepare the test state without running a full reset, improving flexibility.
15-15: Ensure the intended default state configuration file is usedThe
tests/upgrade/directory contains bothtestnet.jsonandtest-config-gha.json, butSTATE_CONFIGcurrently defaults to$(ROOT_DIR)/tests/upgrade/testnet.json. If you intend to run againsttest-config-gha.json(e.g., for GitHub Actions), please update this default path accordingly.make/init.mk (4)
50-54: LGTM! Vendor mode validation is correct.The logic properly checks that when
GOMOD=vendor, thevendordirectory must exist, and provides a helpful error message.
84-84: LGTM! Debug version tracking added for cosmovisor.The separate version file for the debug build allows independent tracking of debug and release cosmovisor binaries.
93-93: No action required: AKASH_RUN_BIN is defined in.env(line 14) and the directory is created inmake/setup-cache.mkbefore usage inmake/init.mk, so theCOSMOVISOR_DEBUGpath is valid.
56-59: GOMOD is always defaulted and validated—no further tests needed. TheGOMOD ?= readonlydefault in Makefile combined with the early workspace-mode guard ensures-mod=$(GOMOD)never sees an empty or unsupported value; vendor mode likewise errors if./vendoris missing, so-modalways receives a valid setting.
| ifeq ($(GO111MODULE),off) | ||
| else | ||
| GOMOD=readonly | ||
| endif | ||
|
|
||
| ifneq ($(GOWORK),off) | ||
| # ifeq ($(shell test -e $(AKASH_ROOT)/go.work && echo -n yes),yes) | ||
| # GOWORK=${AKASH_ROOT}/go.work | ||
| # else | ||
| # GOWORK=off | ||
| # endif | ||
|
|
||
| ifeq ($(GOMOD),$(filter $(GOMOD),mod "")) | ||
| $(error '-mod may only be set to readonly or vendor when in workspace mode, but it is set to ""') | ||
| endif | ||
| endif |
There was a problem hiding this comment.
Handle undefined GOMOD when GO111MODULE=off.
When GO111MODULE=off (line 33), GOMOD remains undefined. This means lines 57-59 will pass -mod= (with an empty value) to Go commands, which may cause build failures. Consider setting a default:
ifeq ($(GO111MODULE),off)
+ GOMOD=
else
GOMOD=readonly
endifAdditionally, the error message on line 46 hardcodes "" instead of showing the actual value. Consider:
-$(error '-mod may only be set to readonly or vendor when in workspace mode, but it is set to ""')
+$(error -mod may only be set to readonly or vendor when in workspace mode, but it is set to "$(GOMOD)")📝 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.
| ifeq ($(GO111MODULE),off) | |
| else | |
| GOMOD=readonly | |
| endif | |
| ifneq ($(GOWORK),off) | |
| # ifeq ($(shell test -e $(AKASH_ROOT)/go.work && echo -n yes),yes) | |
| # GOWORK=${AKASH_ROOT}/go.work | |
| # else | |
| # GOWORK=off | |
| # endif | |
| ifeq ($(GOMOD),$(filter $(GOMOD),mod "")) | |
| $(error '-mod may only be set to readonly or vendor when in workspace mode, but it is set to ""') | |
| endif | |
| endif | |
| ifeq ($(GO111MODULE),off) | |
| GOMOD= | |
| else | |
| GOMOD=readonly | |
| endif | |
| ifneq ($(GOWORK),off) | |
| # ifeq ($(shell test -e $(AKASH_ROOT)/go.work && echo -n yes),yes) | |
| # GOWORK=${AKASH_ROOT}/go.work | |
| # else | |
| # GOWORK=off | |
| # endif | |
| ifeq ($(GOMOD),$(filter $(GOMOD),mod "")) | |
| $(error -mod may only be set to readonly or vendor when in workspace mode, but it is set to "$(GOMOD)") | |
| endif | |
| endif |
🤖 Prompt for AI Agents
In make/init.mk around lines 33 to 48, GOMOD is left undefined when
GO111MODULE=off which leads to passing an empty -mod= to Go commands and the
error message on line 46 hardcodes "" instead of showing the actual value; to
fix, set a sensible default for GOMOD in the GO111MODULE=off branch (e.g.
GOMOD=off or GOMOD=vendor depending on your intended behavior) so -mod is never
empty, and update the error message to interpolate the real value (use $(GOMOD)
in the message) so it reports the actual setting rather than a hardcoded empty
string.
| if len(res) != 1 && len(res) != 3 { | ||
| return fmt.Errorf("line \"%s\" does not match regex \"%s\"", line, rNewModule.String()) | ||
| } |
There was a problem hiding this comment.
Fix regex match validation logic.
The regex validation conditions appear incorrect. FindAllStringSubmatch returns a slice of match arrays, so checking len(res) != 1 && len(res) != 3 doesn't properly validate the match structure.
For line 1300-1302, apply this fix:
- if len(res) != 1 && len(res) != 3 {
+ if len(res) != 1 || len(res[0]) != 3 {
return fmt.Errorf("line \"%s\" does not match regex \"%s\"", line, rNewModule.String())
}For line 1309-1311, apply this fix:
- if len(res) != 1 && len(res) != 4 {
+ if len(res) != 1 || len(res[0]) != 4 {
return fmt.Errorf("line \"%s\" does not match regex \"%s\"", line, rModuleMigration.String())
}Explanation:
len(res)checks the number of matches found (should be 1)len(res[0])checks the number of capture groups in the first match (should be 3 or 4 including the full match)- Using
&&(AND) instead of||(OR) makes the validation ineffective
Also applies to: 1309-1311
🤖 Prompt for AI Agents
In tests/upgrade/upgrade_test.go around lines 1300-1302 and 1309-1311 the regex
match validation is wrong: use len(res) to ensure exactly one overall match and
check len(res[0]) for the number of capture groups; replace the current
condition with a check that fails if either there is not exactly one match or
the first match does not have the expected number of groups (e.g. if len(res) !=
1 || (len(res[0]) != 3 && len(res[0]) != 4) ) then return the fmt.Errorf with
the same message. Ensure the same fix is applied to both line ranges.
Signed-off-by: Artur Troian <[email protected]>
|
superseded by #2006 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/tests.yaml (1)
92-119: Pin third-party actions to immutable SHAs.These steps still pull external actions (
rokroskar/workflow-run-cleanup-action@master,docker/setup-qemu-action@v3,docker/setup-buildx-action@v3,fregante/setup-git-user@v2, etc.) by branch/tag. That’s a supply-chain risk and violates our workflow hardening guidelines. Please replace each with the vetted commit SHA for the intended release (e.g.,owner/repo@<full_sha>). Don’t forget later occurrences likeHatsuneMiku3939/direnv-action@v1,codecov/codecov-action@v4,actions/upload-artifact@v4, andbenc-uk/workflow-dispatch@v1. Based on static analysis hints.script/upgrades.sh (1)
138-158: Fix archive type matcher (.tar.xz) and ensure robust command mapping.Patterns use .tar.cz (typo) leading to wrong handling; keep defaults intact.
Apply:
function tar_by_content_type() { case "$1" in - *.tar.cz*) + *.tar.xz*) tar_cmd="tar -xJ -" ;; *.tar.gz*) tar_cmd="tar xzf -" ;; *.tar.lz4*) tar_cmd="lz4 -d | tar xf -" ;; *.tar.zst*) tar_cmd="zstd -cd | tar xf -" ;; *) tar_cmd="tar xf -" ;; esac echo "$tar_cmd" }
♻️ Duplicate comments (3)
.github/workflows/tests.yaml (1)
48-49: Restore required metadata on setup-ubuntu.
setup-ubuntuis still missing the mandatorydescription:in its action metadata, and actionlint will keep failing until it’s added. Please update./.github/actions/setup-ubuntu/action.yamlto include a meaningful description under thename:field. Based on static analysis hints.make/test-upgrade.mk (1)
15-15: Align default STATE_CONFIG with CI config.Default still points to testnet.json. Prior review requested switching to test-config-gha.json used by CI.
Apply:
-STATE_CONFIG ?= $(ROOT_DIR)/tests/upgrade/testnet.json +STATE_CONFIG ?= $(ROOT_DIR)/tests/upgrade/test-config-gha.jsonscript/upgrades.sh (1)
438-438: Do not suppress testnetify failures.
|| truehides critical errors; let the command fail to surface issues.Apply:
- $AKASH testnetify --home="$valdir" --testnet-rootdir="$validators_dir" --testnet-config="${STATE_CONFIG}" --yes || true + $AKASH testnetify --home="$valdir" --testnet-rootdir="$validators_dir" --testnet-config="${STATE_CONFIG}" --yes
🧹 Nitpick comments (5)
make/test-upgrade.mk (1)
24-34: Auto-derive SNAPSHOT_SOURCE from upgrade ref (keep sandbox/mainnet validation).Derive the default via upgrades.sh snapshot-source to avoid manual drift; retain your validation/error path.
Apply:
-SNAPSHOT_SOURCE ?= mainnet +SNAPSHOT_SOURCE ?= $(shell $(ROOT_DIR)/script/upgrades.sh snapshot-source $(UPGRADE_TO) || echo mainnet)script/upgrades.sh (4)
194-202: Harden content_name parsing (strip CR/LF and quotes).Current parsing can emit quoted names with newlines; downstream pattern matches may fail.
Apply:
-name=$(wget "$1" --spider --server-response -O - 2>&1 | grep -i "content-disposition" | awk -F"filename=" '{print $2}') +name=$(wget "$1" --spider --server-response -O - 2>&1 \ + | grep -i "content-disposition" \ + | sed -En 's/.*filename\*?="?([^";]+).*/\1/p' \ + | tr -d '\r\n"' )
204-212: Follow redirects case-insensitively and pick last Location.Some servers capitalize Location; pick the last hop.
Apply:
-name=$(wget "$1" --spider --server-response -O - 2>&1 | grep "location:" | awk '{print $2}' | tr -d '\n') +name=$(wget "$1" --spider --server-response -O - 2>&1 \ + | grep -i "location:" | tail -1 | awk '{print $2}' | tr -d '\r\n')
12-26: Update USAGE/help to include new commands/options.Help text doesn’t mention prepare-state, snapshot-source, upgrade-from-release, or new flags.
Happy to provide a patch if desired.
499-506: Minor: duplicate local declaration.
local validators_dirdeclared twice; remove the duplicate for clarity.Apply:
-local validators_dir local cosmovisor_dir local genesis_bin -local validators_dir
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/tests.yaml(8 hunks)make/test-upgrade.mk(4 hunks)script/upgrades.sh(13 hunks)tests/upgrade/test-cases.json(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/tests.yaml
66-66: description is required in metadata of "setup-ubuntu" action at "/home/jailuser/git/.github/actions/setup-ubuntu/action.yaml"
(action)
177-177: shellcheck reported issue in this script: SC2086:info:3:40: Double quote to prevent globbing and word splitting
(shellcheck)
177-177: shellcheck reported issue in this script: SC2086:info:4:44: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
tests/upgrade/test-cases.json (1)
2-178: Upgrade matrix additions look solidThe new v1.0.0 entry covers the added modules and all expected migration hops; nothing problematic spotted.
| run: | | ||
| test_required=$(./script/upgrades.sh test-required ${{ github.ref }}) | ||
| snapshot_source=$(./script/upgrades.sh snapshot-source ${{ github.ref }}) | ||
| echo "TEST_REQUIRED=$test_required" >> $GITHUB_ENV | ||
| echo "SNAPSHOT_SOURCE=$snapshot_source" >> $GITHUB_ENV |
There was a problem hiding this comment.
Quote GitHub ref in shell assignments.
./script/upgrades.sh … ${{ github.ref }} and the corresponding snapshot_source assignment need quoting; otherwise shellcheck SC2086 flags potential word splitting/globbing (and dispatches from refs with spaces would misbehave). Please wrap both expressions:
- test_required=$(./script/upgrades.sh test-required ${{ github.ref }})
- snapshot_source=$(./script/upgrades.sh snapshot-source ${{ github.ref }})
+ test_required="$(./script/upgrades.sh test-required "${{ github.ref }}")"
+ snapshot_source="$(./script/upgrades.sh snapshot-source "${{ github.ref }}")"Based on static analysis hints.
📝 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.
| run: | | |
| test_required=$(./script/upgrades.sh test-required ${{ github.ref }}) | |
| snapshot_source=$(./script/upgrades.sh snapshot-source ${{ github.ref }}) | |
| echo "TEST_REQUIRED=$test_required" >> $GITHUB_ENV | |
| echo "SNAPSHOT_SOURCE=$snapshot_source" >> $GITHUB_ENV | |
| run: | | |
| test_required="$(./script/upgrades.sh test-required "${{ github.ref }}")" | |
| snapshot_source="$(./script/upgrades.sh snapshot-source "${{ github.ref }}")" | |
| echo "TEST_REQUIRED=$test_required" >> $GITHUB_ENV | |
| echo "SNAPSHOT_SOURCE=$snapshot_source" >> $GITHUB_ENV |
🧰 Tools
🪛 actionlint (1.7.7)
177-177: shellcheck reported issue in this script: SC2086:info:3:40: Double quote to prevent globbing and word splitting
(shellcheck)
177-177: shellcheck reported issue in this script: SC2086:info:4:44: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
.github/workflows/tests.yaml around lines 177 to 181: the shell invocations pass
the unquoted GitHub ref into the script which can trigger shellcheck SC2086 and
may break on refs with spaces or glob characters; update both assignments to
wrap the GitHub ref in double quotes when calling the script (i.e. use
"./script/upgrades.sh test-required \"${{ github.ref }}\"" and
"./script/upgrades.sh snapshot-source \"${{ github.ref }}\"") so the ref is
treated as a single argument and prevents word-splitting/globbing, then keep
exporting TEST_REQUIRED and SNAPSHOT_SOURCE as before.
|
|
||
| .PHONY: test | ||
| test: $(COSMOVISOR) init | ||
| test: #$(COSMOVISOR) init |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify test flow without implicit init.
init dependency is commented out. Ensure bins/keys/state are prepared beforehand or tests will fail on missing artifacts/dirs.
Run:
🏁 Script executed:
#!/bin/bash
# Validate that validators workdir exists before test
test -d "$(pwd)/$(AP_RUN_DIR)/validators" && echo "validators dir present" || echo "missing validators dir"Length of output: 71
🏁 Script executed:
#!/bin/bash
# Locate definition or usage of AP_RUN_DIR in the repository
rg -n --color=never 'AP_RUN_DIR' -C3 .Length of output: 2762
Add an initialization prerequisite for the test target
The test rule (make/test-upgrade.mk:51) no longer runs init, so $(AP_RUN_DIR)/validators isn’t created and tests will fail. Either add init as a dependency of test or chain prepare-state, bins, and keys before test.
🤖 Prompt for AI Agents
In make/test-upgrade.mk around line 51, the test target no longer ensures
initialization so $(AP_RUN_DIR)/validators is not created; update the test rule
to depend on the init step (or chain prepare-state, bins, and keys) by adding
init as a prerequisite (or adding prepare-state bins keys before test) so the
validators directory and required state are created prior to running tests.
| .PHONY: prepare-state | ||
| prepare-state: | ||
| $(ROOT_DIR)/script/upgrades.sh --workdir=$(AP_RUN_DIR) --config="$(PWD)/config.json" --state-config=$(STATE_CONFIG) prepare-state |
There was a problem hiding this comment.
Pass SNAPSHOT_URL to prepare-state target for consistency.
test-reset passes --snapshot-url but prepare-state does not. This creates divergent behavior.
Apply:
prepare-state:
- $(ROOT_DIR)/script/upgrades.sh --workdir=$(AP_RUN_DIR) --config="$(PWD)/config.json" --state-config=$(STATE_CONFIG) prepare-state
+ $(ROOT_DIR)/script/upgrades.sh --workdir=$(AP_RUN_DIR) --config="$(PWD)/config.json" --state-config=$(STATE_CONFIG) --snapshot-url=$(SNAPSHOT_URL) prepare-state📝 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.
| .PHONY: prepare-state | |
| prepare-state: | |
| $(ROOT_DIR)/script/upgrades.sh --workdir=$(AP_RUN_DIR) --config="$(PWD)/config.json" --state-config=$(STATE_CONFIG) prepare-state | |
| .PHONY: prepare-state | |
| prepare-state: | |
| $(ROOT_DIR)/script/upgrades.sh --workdir=$(AP_RUN_DIR) --config="$(PWD)/config.json" --state-config=$(STATE_CONFIG) --snapshot-url=$(SNAPSHOT_URL) prepare-state |
🤖 Prompt for AI Agents
In make/test-upgrade.mk around lines 67 to 69, the prepare-state target invokes
upgrades.sh without passing the SNAPSHOT_URL variable while test-reset does,
causing inconsistent behavior; update the command for prepare-state to include
the --snapshot-url=$(SNAPSHOT_URL) argument (keeping existing flags) so both
targets receive the same snapshot URL input.
| CHAIN_METADATA=$(curl -s "${CHAIN_METADATA_URL}") | ||
| GENESIS_URL="$(echo "$CHAIN_METADATA" | jq -r '.codebase.genesis.genesis_url? // .genesis?')" | ||
|
|
There was a problem hiding this comment.
Don’t unconditionally fetch CHAIN_METADATA (set -e causes premature exits).
This curl runs for every command; network failure aborts even for bins/keys/clean. Fetch only when needed (init/prepare-state).
Apply:
-CHAIN_METADATA=$(curl -s "${CHAIN_METADATA_URL}")
-GENESIS_URL="$(echo "$CHAIN_METADATA" | jq -r '.codebase.genesis.genesis_url? // .genesis?')"
+if [[ "$1" == "init" || "$1" == "prepare-state" ]]; then
+ CHAIN_METADATA=$(curl -sf "${CHAIN_METADATA_URL}")
+ GENESIS_URL="$(echo "$CHAIN_METADATA" | jq -r '.codebase.genesis.genesis_url? // .genesis?')"
+fi📝 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.
| CHAIN_METADATA=$(curl -s "${CHAIN_METADATA_URL}") | |
| GENESIS_URL="$(echo "$CHAIN_METADATA" | jq -r '.codebase.genesis.genesis_url? // .genesis?')" | |
| # Only fetch chain metadata when initializing or preparing state | |
| if [[ "$1" == "init" || "$1" == "prepare-state" ]]; then | |
| CHAIN_METADATA=$(curl -sf "${CHAIN_METADATA_URL}") | |
| GENESIS_URL="$(echo "$CHAIN_METADATA" | jq -r '.codebase.genesis.genesis_url? // .genesis?')" | |
| fi |
🤖 Prompt for AI Agents
In script/upgrades.sh around lines 127-129, CHAIN_METADATA is fetched
unconditionally which causes the script to fail early under set -e when
network/curl errors occur; change the script so CHAIN_METADATA (and subsequent
GENESIS_URL extraction) is only fetched when the invoked command requires it
(e.g., init or prepare-state) by moving the curl/jq logic into the branches or a
conditional that checks the command name (case or if [[ "$CMD" == "init" ||
"$CMD" == "prepare-state" ]]) before running curl; ensure any curl failures are
handled gracefully (return non-zero only when actually needed) rather than
unconditionally aborting other commands.
| function content_type() { | ||
| case "$1" in | ||
| *.tar.cz*) | ||
| tar_cmd="tar.cz" | ||
| ;; | ||
| *.tar.gz*) | ||
| tar_cmd="tar.gz" | ||
| ;; | ||
| *.tar.lz4*) | ||
| tar_cmd="tar.lz4" | ||
| ;; | ||
| *.tar.zst*) | ||
| tar_cmd="tar.zst" | ||
| ;; | ||
| esac | ||
|
|
||
| echo "$tar_cmd" | ||
| } |
There was a problem hiding this comment.
content_type returns a token; init it and fix .tar.xz.
Uninitialized var risks stale value; also fix extension typo.
Apply:
function content_type() {
- case "$1" in
- *.tar.cz*)
- tar_cmd="tar.cz"
+ local tar_cmd=""
+ case "$1" in
+ *.tar.xz*)
+ tar_cmd="tar.xz"
;;
*.tar.gz*)
tar_cmd="tar.gz"
;;
*.tar.lz4*)
tar_cmd="tar.lz4"
;;
*.tar.zst*)
tar_cmd="tar.zst"
;;
esac
echo "$tar_cmd"
}📝 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.
| function content_type() { | |
| case "$1" in | |
| *.tar.cz*) | |
| tar_cmd="tar.cz" | |
| ;; | |
| *.tar.gz*) | |
| tar_cmd="tar.gz" | |
| ;; | |
| *.tar.lz4*) | |
| tar_cmd="tar.lz4" | |
| ;; | |
| *.tar.zst*) | |
| tar_cmd="tar.zst" | |
| ;; | |
| esac | |
| echo "$tar_cmd" | |
| } | |
| function content_type() { | |
| local tar_cmd="" | |
| case "$1" in | |
| *.tar.xz*) | |
| tar_cmd="tar.xz" | |
| ;; | |
| *.tar.gz*) | |
| tar_cmd="tar.gz" | |
| ;; | |
| *.tar.lz4*) | |
| tar_cmd="tar.lz4" | |
| ;; | |
| *.tar.zst*) | |
| tar_cmd="tar.zst" | |
| ;; | |
| esac | |
| echo "$tar_cmd" | |
| } |
🤖 Prompt for AI Agents
In script/upgrades.sh around lines 160 to 177, the content_type function uses
tar_cmd without initializing it and misses the .tar.xz pattern; to fix,
initialize tar_cmd to an empty string (e.g., tar_cmd="") before the case block,
add a case arm for *.tar.xz* that sets tar_cmd="tar.xz", and keep the existing
patterns; finally ensure the function echoes tar_cmd (which will now be empty
when no match) so callers don't receive a stale value.
| AKASH=$genesis_bin/akash | ||
|
|
||
| genesis_file=${valdir}/config/genesis.json | ||
| rm -f "$genesis_file" | ||
|
|
There was a problem hiding this comment.
Guard: ensure akash binary exists before prepare_state.
prepare-state can be invoked standalone; fail fast with actionable hint if bins weren’t built.
Apply:
AKASH=$genesis_bin/akash
+ if [[ ! -x "$AKASH" ]]; then
+ echo "akash binary not found at $AKASH. Run '$0 bins' (or 'make bins') first." >&2
+ exit 1
+ fi📝 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.
| AKASH=$genesis_bin/akash | |
| genesis_file=${valdir}/config/genesis.json | |
| rm -f "$genesis_file" | |
| AKASH=$genesis_bin/akash | |
| if [[ ! -x "$AKASH" ]]; then | |
| echo "akash binary not found at $AKASH. Run '$0 bins' (or 'make bins') first." >&2 | |
| exit 1 | |
| fi | |
| genesis_file=${valdir}/config/genesis.json | |
| rm -f "$genesis_file" |
🤖 Prompt for AI Agents
In script/upgrades.sh around lines 373 to 377, add a guard before using the
AKASH variable to ensure the akash binary exists and is executable; if not,
print a clear actionable error (e.g. telling the developer to build the binaries
or run the build script and showing expected path) and exit with a non-zero
status so prepare-state cannot run standalone without built bins.
| if [[ $cnt -eq 0 ]]; then | ||
| if [[ "${GENESIS_URL}" =~ ^https?:\/\/.* ]]; then | ||
| echo "Downloading genesis from ${GENESIS_URL}" | ||
|
|
||
| pv_args="-petrafb -i 5" | ||
| sz=$(content_size "${GENESIS_URL}") | ||
| # shellcheck disable=SC2181 | ||
| if [ $? -eq 0 ]; then | ||
| if [[ -n $sz ]]; then | ||
| pv_args+=" -s $sz" | ||
| fi | ||
|
|
||
| tar_cmd=$(content_type "$(content_name "${GENESIS_URL}")") | ||
|
|
||
| if [ "$tar_cmd" != "" ]; then | ||
| # shellcheck disable=SC2086 | ||
| wget -nq -O - "${GENESIS_URL}" | pv $pv_args | eval "$tar_cmd" | ||
| else | ||
| wget -q --show-progress -O "$genesis_file" "${GENESIS_URL}" | ||
| fi | ||
| else | ||
| echo "unable to download genesis" | ||
| fi | ||
| else | ||
| echo "Unpacking genesis from ${GENESIS_URL}" | ||
| tar_cmd=$(content_type "${GENESIS_URL}") | ||
| # shellcheck disable=SC2086 | ||
| (pv -petrafb -i 5 "${GENESIS_URL}" | eval "$tar_cmd") 2>&1 | stdbuf -o0 tr '\r' '\n' | ||
| fi | ||
|
|
There was a problem hiding this comment.
Bug: executing content_type token as a command. Use tar_by_content_type.
content_type returns a label (e.g., tar.gz), not an extraction command; eval fails.
Apply:
- tar_cmd=$(content_type "$(content_name "${GENESIS_URL}")")
+ tar_cmd=$(tar_by_content_type "$(content_name "${GENESIS_URL}")")
- if [ "$tar_cmd" != "" ]; then
+ if [ -n "$tar_cmd" ]; then
# shellcheck disable=SC2086
wget -nq -O - "${GENESIS_URL}" | pv $pv_args | eval "$tar_cmd"
else
wget -q --show-progress -O "$genesis_file" "${GENESIS_URL}"
fi📝 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 [[ $cnt -eq 0 ]]; then | |
| if [[ "${GENESIS_URL}" =~ ^https?:\/\/.* ]]; then | |
| echo "Downloading genesis from ${GENESIS_URL}" | |
| pv_args="-petrafb -i 5" | |
| sz=$(content_size "${GENESIS_URL}") | |
| # shellcheck disable=SC2181 | |
| if [ $? -eq 0 ]; then | |
| if [[ -n $sz ]]; then | |
| pv_args+=" -s $sz" | |
| fi | |
| tar_cmd=$(content_type "$(content_name "${GENESIS_URL}")") | |
| if [ "$tar_cmd" != "" ]; then | |
| # shellcheck disable=SC2086 | |
| wget -nq -O - "${GENESIS_URL}" | pv $pv_args | eval "$tar_cmd" | |
| else | |
| wget -q --show-progress -O "$genesis_file" "${GENESIS_URL}" | |
| fi | |
| else | |
| echo "unable to download genesis" | |
| fi | |
| else | |
| echo "Unpacking genesis from ${GENESIS_URL}" | |
| tar_cmd=$(content_type "${GENESIS_URL}") | |
| # shellcheck disable=SC2086 | |
| (pv -petrafb -i 5 "${GENESIS_URL}" | eval "$tar_cmd") 2>&1 | stdbuf -o0 tr '\r' '\n' | |
| fi | |
| if [[ $cnt -eq 0 ]]; then | |
| if [[ "${GENESIS_URL}" =~ ^https?:\/\/.* ]]; then | |
| echo "Downloading genesis from ${GENESIS_URL}" | |
| pv_args="-petrafb -i 5" | |
| sz=$(content_size "${GENESIS_URL}") | |
| # shellcheck disable=SC2181 | |
| if [ $? -eq 0 ]; then | |
| if [[ -n $sz ]]; then | |
| pv_args+=" -s $sz" | |
| fi | |
| tar_cmd=$(tar_by_content_type "$(content_name "${GENESIS_URL}")") | |
| if [ -n "$tar_cmd" ]; then | |
| # shellcheck disable=SC2086 | |
| wget -nq -O - "${GENESIS_URL}" | pv $pv_args | eval "$tar_cmd" | |
| else | |
| wget -q --show-progress -O "$genesis_file" "${GENESIS_URL}" | |
| fi | |
| else | |
| echo "unable to download genesis" | |
| fi | |
| else | |
| echo "Unpacking genesis from ${GENESIS_URL}" | |
| tar_cmd=$(content_type "${GENESIS_URL}") | |
| # shellcheck disable=SC2086 | |
| (pv -petrafb -i 5 "${GENESIS_URL}" | eval "$tar_cmd") 2>&1 | stdbuf -o0 tr '\r' '\n' | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In script/upgrades.sh around lines 378 to 407, the code calls content_type which
returns a label (e.g., "tar.gz") and then tries to eval that label as a command
— replace those calls with tar_by_content_type which returns an actual
extraction command string, i.e., call tar_by_content_type with the same argument
you previously passed to content_type (use content_name "${GENESIS_URL}" for the
first occurrence and "${GENESIS_URL}" for the second), assign its output to
tar_cmd, and then execute tar_cmd as before (keeping the pv/wget piping and any
shellcheck disables) so you no longer eval a non-command token.
| tar_cmd=$(content_type "${GENESIS_URL}") | ||
| # shellcheck disable=SC2086 | ||
| (pv -petrafb -i 5 "${GENESIS_URL}" | eval "$tar_cmd") 2>&1 | stdbuf -o0 tr '\r' '\n' | ||
| fi |
There was a problem hiding this comment.
Same issue for local genesis path.
Use tar_by_content_type for file path extraction.
Apply:
- tar_cmd=$(content_type "${GENESIS_URL}")
+ tar_cmd=$(tar_by_content_type "${GENESIS_URL}")
# shellcheck disable=SC2086
(pv -petrafb -i 5 "${GENESIS_URL}" | eval "$tar_cmd") 2>&1 | stdbuf -o0 tr '\r' '\n'📝 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.
| tar_cmd=$(content_type "${GENESIS_URL}") | |
| # shellcheck disable=SC2086 | |
| (pv -petrafb -i 5 "${GENESIS_URL}" | eval "$tar_cmd") 2>&1 | stdbuf -o0 tr '\r' '\n' | |
| fi | |
| tar_cmd=$(tar_by_content_type "${GENESIS_URL}") | |
| # shellcheck disable=SC2086 | |
| (pv -petrafb -i 5 "${GENESIS_URL}" | eval "$tar_cmd") 2>&1 | stdbuf -o0 tr '\r' '\n' | |
| fi |
🤖 Prompt for AI Agents
In script/upgrades.sh around lines 403 to 406, the code uses content_type for
local genesis path extraction which fails for file paths; replace the call to
content_type with tar_by_content_type for GENESIS_URL so local files are handled
correctly (e.g., tar_cmd=$(tar_by_content_type "${GENESIS_URL}")), keep the
existing pv | eval pipeline and the shellcheck disable comment, and ensure the
variable is quoted where appropriate to avoid word-splitting.
refs akash-network/support#212