v0.24.0: cw@v3 #292
v0.24.0: cw@v3 #292
Conversation
merge antstalepresh patch
…gradeHandler logic to in-place-testnet
V022 custom testnet
dev: access appKeepers from top level app
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/app.go (1)
151-161: Return type mismatch for GetWasmOpts.Function returns []wasm.Option but builds []wasmkeeper.Option. Fix the signature to avoid compile issues.
-func GetWasmOpts(appOpts servertypes.AppOptions) []wasm.Option { +func GetWasmOpts(appOpts servertypes.AppOptions) []wasmkeeper.Option { var wasmOpts []wasmkeeper.Option if cast.ToBool(appOpts.Get("telemetry.enabled")) { wasmOpts = append(wasmOpts, wasmkeeper.WithVMCacheMetrics(prometheus.DefaultRegisterer)) } // default wasm gas configuration. wasmOpts = append(wasmOpts, wasmkeeper.WithGasRegister(wasmtypes.NewWasmGasRegister(wasmtypes.DefaultGasRegisterConfig()))) return wasmOpts }
🧹 Nitpick comments (13)
app/upgrades/types.go (2)
20-21: Clarify parameter meaning in the docstring.“validators homepath” is ambiguous. Suggest “node home directory path (homePath)”.
Apply this diff to tighten the comment:
- // CreateUpgradeHandler defines the function that creates an upgrade handler. NOTE: string is a validators homepath + // CreateUpgradeHandler defines the function that creates an upgrade handler. + // NOTE: the string parameter is the node home directory path.
27-31: Fix typos in interface comment.“interrace” → “interface”, “fullfil” → “fulfil”.
Apply:
-// BaseAppParamManager defines an interrace that BaseApp is expected to fullfil +// BaseAppParamManager defines an interface that BaseApp is expected to fulfilapp/upgrades/v024/upgrades.go (3)
20-24: Avoid shadowing standard packages and use ctx consistently.Parameter named “context” shadows the import. Use “ctx”.
-return func(context context.Context, _ upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { - - sdkCtx := sdk.UnwrapSDKContext(context) +return func(ctx context.Context, _ upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { + + sdkCtx := sdk.UnwrapSDKContext(ctx)
59-61: Return nil error explicitly after best‑effort config writes.Avoid leaking a stale “err” from earlier steps; return nil on success.
- return versionMap, err + return versionMap, nil
42-47: Optional: keep TimeoutPropose < TimeoutCommit for faster proposal.Current code sets both to 2400ms. If you want ~2.4s blocks with a faster propose, consider 2000ms propose and 2400ms commit.
- consensus["timeout_commit"] = "2400ms" // 2.4s - consensus["timeout_propose"] = "2400ms" // 2.4s + consensus["timeout_commit"] = "2400ms" // 2.4s + consensus["timeout_propose"] = "2000ms" // faster proposecmd/bitsongd/cmd/root.go (3)
89-109: CWD “data/” cleanup can delete unexpected directories.You remove ./data if empty on every run. Scope to tempDir (or a known path) to avoid touching user repos.
- currentDir, err := os.Getwd() + currentDir, err := os.Getwd() ... - dataDir := filepath.Join(currentDir, "data") + dataDir := filepath.Join(tempDir, "data")
188-193: Comment mismatch: 2000ms ≠ 2.4s.TimeoutPropose is set to 2000ms but the comment says 2.4s.
- config.Consensus.TimeoutPropose = 2000 * time.Millisecond // 2.4s block time + config.Consensus.TimeoutPropose = 2000 * time.Millisecond // 2.0s propose
443-451: Be resilient to non‑Viper AppOptions.Type assertion to *viper.Viper can fail depending on the caller.
- viperAppOpts, ok := appOpts.(*viper.Viper) - if !ok { - return servertypes.ExportedApp{}, errors.New("appOpts is not viper.Viper") - } + viperAppOpts, ok := appOpts.(*viper.Viper) + if !ok { + // fallback: copy into a new Viper and continue + v := viper.New() + // best effort: no inv-check override in this path + viperAppOpts = v + }app/app.go (2)
401-403: Differentiate error messages for pinned code initialization.Helps operators debug whether wasm or ibc‑wasm failed.
- if err := app.IBCWasmClientKeeper.InitializePinnedCodes(ctx); err != nil { - tmos.Exit(fmt.Sprintf("failed initialize pinned codes %s", err)) - } + if err := app.IBCWasmClientKeeper.InitializePinnedCodes(ctx); err != nil { + tmos.Exit(fmt.Sprintf("failed initialize IBC-wasm pinned codes %s", err)) + }
328-331: Duplicate store decoder registration.app.sm.RegisterStoreDecoders() is called twice; keep one.
- app.sm.RegisterStoreDecoders() + // already registered above(Delete the later call at Line 414.)
Also applies to: 414-415
cmd/bitsongd/cmd/init.go (3)
38-41: Fix stale comment and document new flag.The comment mentions FlagSeed but the code defines FlagRecover and the new FlagSeedPeers. Update to avoid confusion.
- // FlagSeed defines a flag to initialize the private validator key from a specific seed. - FlagRecover = "recover" - FlagSeedPeers = "seed-peers" + // FlagRecover initializes the private validator key from a BIP39 mnemonic. + // FlagSeedPeers, when set, seeds the config with known bitsong-2b (mainnet) peers. + FlagRecover = "recover" + FlagSeedPeers = "seed-peers"
105-108: Fix inaccurate comments on consensus timeouts; confirm tuning.The comments label both as “2.4s block time”. Propose clarifying and confirming these values match mainnet ops guidance.
- config.Consensus.TimeoutCommit = 2400 * time.Millisecond // 2.4s block time - config.Consensus.TimeoutPropose = 2000 * time.Millisecond // 2.4s block time + config.Consensus.TimeoutCommit = 2400 * time.Millisecond // target ≈2.4s block time (with other timeouts) + config.Consensus.TimeoutPropose = 2000 * time.Millisecond // 2.0s propose stepPlease confirm these timeouts have been tested against current validator performance and CometBFT defaults on bitsong-2b.
198-198: Tighten help text to signal mainnet-only usage.Make it explicit this flag seeds bitsong-2b peers, avoiding misuse on testnets.
- cmd.Flags().Bool(FlagSeedPeers, false, "Seed the config file with known network peers for bitsong-2b") + cmd.Flags().Bool(FlagSeedPeers, false, "Seed config with known bitsong-2b (mainnet) peers")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/app.go(8 hunks)app/upgrades/types.go(1 hunks)app/upgrades/v024/upgrades.go(1 hunks)app/upgrades/v024/upgrades_test.go(1 hunks)cmd/bitsongd/cmd/init.go(4 hunks)cmd/bitsongd/cmd/root.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v024/upgrades_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
app/upgrades/types.go (1)
app/keepers/keepers.go (1)
AppKeepers(108-150)
cmd/bitsongd/cmd/root.go (3)
cmd/bitsongd/cmd/init.go (1)
InitCmd(74-205)app/modules.go (1)
AppModuleBasics(74-105)app/app.go (1)
DefaultNodeHome(108-108)
app/upgrades/v024/upgrades.go (3)
app/upgrades/types.go (1)
BaseAppParamManager(29-32)app/keepers/keepers.go (1)
AppKeepers(108-150)app/upgrades/v024/constants.go (1)
UpgradeName(9-9)
app/app.go (2)
app/upgrades/types.go (1)
Upgrade(16-25)app/upgrades/v024/constants.go (1)
Upgrade(12-16)
⏰ 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). (1)
- GitHub Check: build-docker
🔇 Additional comments (4)
app/app.go (2)
375-389: Snapshot extension wiring LGTM.Registration of wasm and IBC‑wasm snapshotters aligns with the v10 modules.
94-101: Upgrade list: confirm intentional removal of earlier handlers.
app/app.go (lines 94–101) only enables v024; repo shows only app/upgrades/v024/constants.go (UpgradeName = "v024"). Confirm earlier upgrades were intentionally removed or restore/add missing upgrade handlers to match the chain’s upgrade history.cmd/bitsongd/cmd/root.go (2)
185-198: Remaining WasmConfig references found — review and migrate as needed.rg output:
- app/ante_handler.go:30 — WasmConfig wasmTypes.NodeConfig
- app/ante_handler.go:151 — options.WasmConfig.SimulationGasLimit
- app/app.go:252 — ibcWasmConfig := wasmlctypes.WasmConfig{
- app/app.go:356 — WasmConfig: wasmConfig,
- app/keepers/keepers.go:162 — ibcWasmConfig ibcwlctypes.WasmConfig
Actions:
- If wasmlctypes.WasmConfig / ibcwlctypes.WasmConfig should be the same as wasmtypes.NodeConfig, migrate those declarations/initializers and update call sites.
- If they are intentionally distinct IBC/wasm-layer types, confirm compatibility with the new NodeConfig usage (fields/initializers) and document the distinction.
248-269: Potential duplicate "export" command registrationcmd/bitsongd/cmd/root.go registers export twice: server.AddCommands(..., appExport, ...) (line 256) and server.ExportCmd(appExport, ...) (line 268). Check the Cosmos SDK version in go.mod — if AddCommands already adds ExportCmd for your SDK, remove the explicit server.ExportCmd.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/ict/setup.go (2)
79-83: Fix Bech32 pubkey prefixes; current values are incorrect.Pubkey prefixes should end with 'pub' variants; otherwise key encoding/printing can be wrong.
Apply:
- sdk.GetConfig().SetBech32PrefixForAccount("bitsong", "bitsong") - sdk.GetConfig().SetBech32PrefixForValidator("bitsongvaloper", "bitsong") - sdk.GetConfig().SetBech32PrefixForConsensusNode("bitsongvalcons", "bitsong") + sdk.GetConfig().SetBech32PrefixForAccount("bitsong", "bitsongpub") + sdk.GetConfig().SetBech32PrefixForValidator("bitsongvaloper", "bitsongvaloperpub") + sdk.GetConfig().SetBech32PrefixForConsensusNode("bitsongvalcons", "bitsongvalconspub")Optionally seal after SetCoinType:
sdk.GetConfig().SetCoinType(118) + sdk.GetConfig().Seal()
32-33: Pin the relayer image version — avoid "main".Set IBCRelayerVersion in tests/ict/setup.go (lines 32–33) to a released tag or image digest (not "main"); it’s used by interchaintestrelayer.CustomDockerImage in tests/ict/polytone_test.go:235, tests/ict/module_pfm_test.go:125, and tests/ict/ibc_hooks_test.go:71.
cmd/bitsongd/cmd/root.go (1)
89-109: Don’t delete $CWD/data; scope cleanup to tempDir.This block can remove an unrelated ./data folder in the user’s working directory. Remove it or scope to tempDir.
- // Get current working directory - currentDir, err := os.Getwd() - if err != nil { - panic(err) - } - dataDir := filepath.Join(currentDir, "data") - - // Check if data directory exists - if _, err := os.Stat(dataDir); err == nil { - // Directory exists, check if it's empty - dirEntries, err := os.ReadDir(dataDir) - if err != nil { - panic(err) - } else if len(dirEntries) == 0 { - // Directory is empty, remove it - if err := os.RemoveAll(dataDir); err != nil { - panic(err) - } - } - } + // No-op: tempDir is removed above. Avoid touching $CWD/data.
♻️ Duplicate comments (2)
app/keepers/keepers.go (2)
87-89: Wrong CosmWasm ABI feature flag; dedupe markers.Use the ABI marker “interface_version_9” (CW 3.0) and dedupe any existing interface_version_* entries; keep a single ABI flag. Also retain “bitsong”.
-func ExtendedBuiltInCapabilities() []string { - return append(wasmkeeper.BuiltInCapabilities(), "bitsong", "cosmwasm_3_0") -} +func ExtendedBuiltInCapabilities() []string { + base := wasmkeeper.BuiltInCapabilities() + seen := make(map[string]struct{}, len(base)+2) + out := make([]string, 0, len(base)+2) + for _, c := range base { + if strings.HasPrefix(c, "interface_version_") { + continue + } + if _, ok := seen[c]; ok { + continue + } + seen[c] = struct{}{} + out = append(out, c) + } + for _, c := range []string{"bitsong", "interface_version_9"} { + if _, ok := seen[c]; ok { + continue + } + seen[c] = struct{}{} + out = append(out, c) + } + return out +}Add missing import:
+ "strings"
434-438: Pass ChannelKeeperV2 to wasm IBC handler.wasm.NewIBCHandler expects ChannelKeeperV2 as the 4th arg in recent wasmd; passing ChannelKeeper can break ordered/unordered channel flows.
- AddRoute(wasmtypes.ModuleName, wasm.NewIBCHandler(appKeepers.WasmKeeper, appKeepers.IBCKeeper.ChannelKeeper, appKeepers.TransferKeeper, appKeepers.IBCKeeper.ChannelKeeper)) + AddRoute(wasmtypes.ModuleName, wasm.NewIBCHandler(appKeepers.WasmKeeper, appKeepers.IBCKeeper.ChannelKeeper, appKeepers.TransferKeeper, appKeepers.IBCKeeper.ChannelKeeperV2))
🧹 Nitpick comments (7)
tests/ict/setup.go (1)
59-75: BaseCfg is sane; set faster consensus timeouts and non-zero gas for test stability.ConfigFileOverrides are currently nil in tests/ict/setup.go:72 and tests/ict/module_pfm_test.go:67.
- Add ConfigFileOverrides to shorten consensus timeouts (e.g., timeout_propose / timeout_commit ~1s) to speed tests.
- Use a minimal non-zero GasPrices (e.g., "0.001ubtsg") instead of "0ubtsg" to avoid zero-fee edge cases.
cmd/bitsongd/cmd/root.go (2)
60-66: Double address-verifier setup; avoid conflicting config.Config is also set in app/app.go (SetAddressPrefixes). Ensure only one verifier wins before sealing, or remove this duplication to avoid confusion.
444-451: Rigid viper cast in export path.appOpts may not always be *viper.Viper. Consider handling server.EmptyAppOptions or using server.GetViper() accessors to avoid panics.
app/app.go (2)
151-161: Option type alias: verify or align types.Function returns []wasm.Option but builds []wasmkeeper.Option. If Option isn’t a type alias in your wasmd version, this won’t compile. Either return []wasmkeeper.Option or declare wasmOpts as []wasm.Option.
-func GetWasmOpts(appOpts servertypes.AppOptions) []wasm.Option { - var wasmOpts []wasmkeeper.Option +func GetWasmOpts(appOpts servertypes.AppOptions) []wasmkeeper.Option { + var wasmOpts []wasmkeeper.Option
328-331: Duplicate RegisterStoreDecoders call.app.sm.RegisterStoreDecoders() is called twice. Remove the later duplicate.
- app.sm.RegisterStoreDecoders()Also applies to: 414-416
app/keepers/keepers.go (2)
387-389: Use ibcWasmConfig.MemoryCacheSize for LC VM.The 08‑wasm VM should use its own MemoryCacheSize, not the main wasmConfig’s.
- lcWasmer, err := wasmvm.NewVM(ibcWasmConfig.DataDir, wasmCapabilities, 32, ibcWasmConfig.ContractDebugMode, wasmConfig.MemoryCacheSize) + lcWasmer, err := wasmvm.NewVM(ibcWasmConfig.DataDir, wasmCapabilities, 32, ibcWasmConfig.ContractDebugMode, ibcWasmConfig.MemoryCacheSize)
324-331: Initialize WasmHooks after WasmKeeper creation.WasmHooks is constructed with a nil WasmKeeper then reassigned later. Safer to create hooks after appKeepers.WasmKeeper is set.
- // Configure the ibchooks keeper - hooksKeeper := ibchookskeeper.NewKeeper( - appKeepers.keys[ibchookstypes.StoreKey], - ) - appKeepers.IBCHooksKeeper = &hooksKeeper - // Setup the ICS4Wrapper used by the hooks middleware - wasmHooks := ibchooks.NewWasmHooks(appKeepers.IBCHooksKeeper, appKeepers.WasmKeeper, Bech32Prefix) - appKeepers.Ics20WasmHooks = &wasmHooks + // Configure the ibchooks keeper + hooksKeeper := ibchookskeeper.NewKeeper(appKeepers.keys[ibchookstypes.StoreKey]) + appKeepers.IBCHooksKeeper = &hooksKeeper + // Defer WasmHooks creation until after WasmKeeper is set (see below). @@ - appKeepers.ContractKeeper = wasmkeeper.NewDefaultPermissionKeeper(appKeepers.WasmKeeper) - appKeepers.Ics20WasmHooks.ContractKeeper = appKeepers.WasmKeeper + appKeepers.ContractKeeper = wasmkeeper.NewDefaultPermissionKeeper(appKeepers.WasmKeeper) + wasmHooks := ibchooks.NewWasmHooks(appKeepers.IBCHooksKeeper, appKeepers.WasmKeeper, Bech32Prefix) + appKeepers.Ics20WasmHooks = &wasmHooksAlso applies to: 424-427
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/app.go(8 hunks)app/keepers/keepers.go(13 hunks)app/upgrades/v024/upgrades.go(1 hunks)cmd/bitsongd/cmd/root.go(4 hunks)tests/ict/setup.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v024/upgrades.go
🧰 Additional context used
🧬 Code graph analysis (4)
tests/ict/setup.go (1)
x/fantoken/types/codec.go (1)
RegisterInterfaces(23-40)
app/app.go (3)
app/upgrades/types.go (1)
Upgrade(16-25)app/upgrades/v024/constants.go (1)
Upgrade(12-16)app/keepers/keepers.go (1)
ExtendedBuiltInCapabilities(87-89)
app/keepers/keepers.go (3)
x/fantoken/keeper/keeper.go (2)
Keeper(17-26)NewKeeper(28-54)x/fantoken/types/expected_keepers.go (3)
AccountKeeper(22-24)BankKeeper(27-37)DistrKeeper(10-12)app/keepers/acceptedQueries.go (1)
AcceptedQueries(19-63)
cmd/bitsongd/cmd/root.go (3)
cmd/bitsongd/cmd/init.go (1)
InitCmd(74-205)app/modules.go (1)
AppModuleBasics(74-105)app/app.go (1)
DefaultNodeHome(108-108)
⏰ 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). (1)
- GitHub Check: build-docker
🔇 Additional comments (6)
tests/ict/setup.go (5)
1-1: Package rename to ict is appropriate.Matches the new ICT test layout and isolates the suite cleanly.
101-104: CreateChain correctly reuses BaseCfg and swaps image.Simple and readable.
29-31: Confirmed: BitsongE2eRepo is used by GetDockerImageInfo.GetDockerImageInfo assigns repo = BitsongE2eRepo (tests/ict/ci.go:11–15) and callers use its return (tests/ict/setup.go:35, tests/ict/basic_upgrade_test.go:41), so BitsongE2eRepo is live — not dead code.
90-95: Localhost light-client registration commented out—confirm no residual 09-localhost usage.If any types/messages remain, blocksdb decoding or queries can fail.
8-11: Interchaintest v10 import upgrade looks good — confirm go.mod replaceAll imports use v10 (no pre-v10 matches). tests/ict/go.mod requires v10 and contains a replace to github.com/burnt-labs/interchaintest/v10 v10.0.0-wasmvm3 (tests/ict/go.mod:10,23). Confirm the replace to the burnt-labs fork is intentional; remove or align it to upstream v10 if not.
cmd/bitsongd/cmd/root.go (1)
182-203: NodeConfig template compatibility — resolved.
wasmd v0.61.x uses wasm/types.NodeConfig with DefaultNodeConfig(); there is no DefaultNodeConfigTemplate() helper, and the SDK's serverconfig.DefaultConfigTemplate is used for the app template — the current approach is correct.
Feat/turn my swag onnnnnn
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 (8)
scripts/test_node.sh (3)
103-105: Bug: uses undefined variable PROFF_LADDER.Should use PROFF (defined at Line 23). As-is, pprof binding becomes invalid.
Apply this diff:
-# replace pprof_laddr = "localhost:6060" binding -sed -i 's/pprof_laddr = "localhost:6060"/pprof_laddr = "localhost:'$PROFF_LADDER'"/g' $HOME_DIR/config/config.toml +# replace pprof_laddr = "localhost:6060" binding +sed -i 's/pprof_laddr = "localhost:6060"/pprof_laddr = "localhost:'$PROFF'"/g' $HOME_DIR/config/config.toml
30-36: Inconsistent alias vs var usage causes config in wrong HOME.You alias BINARY at Line 30, but Lines 35-36 invoke
$BINARY(the var), not the alias, so--home=$HOME_DIRis not used and client config lands in the default home.Apply this diff to use the alias consistently:
-$BINARY config keyring-backend $KEYRING -$BINARY config chain-id $CHAIN_ID +BINARY config keyring-backend $KEYRING +BINARY config chain-id $CHAIN_IDAlternatively, drop the alias and pass
--home="$HOME_DIR"explicitly everywhere.
100-102: Overbroad sed flips every “enable = false” in app.toml.This may unintentionally enable non-API features (grpc-web, gql, telemetry, etc.). Since you already pass
--api.enable=true, prefer a targeted edit or remove this sed.Option A (remove; rely on CLI flag):
-sed -i 's/enable = false/enable = true/g' $HOME_DIR/config/app.tomlOption B (target [api] section only; GNU sed):
-sed -i 's/enable = false/enable = true/g' $HOME_DIR/config/app.toml +sed -i -E '/^\[api\]/,/^\[/{s/^(\s*enable\s*=\s*)false/\1true/}' $HOME_DIR/config/app.tomlapp/modules.go (2)
71-105: Fix: AppModuleBasics should register Basic modules; protocolpool.AppModule{} is the full module.NewBasicManager expects AppModuleBasic; using AppModule will not compile.
- protocolpool.AppModule{}, + protocolpool.AppModuleBasic{},
169-198: Deduplicate init order; duplicates can cause double init or confused ordering.authtypes.ModuleName and ibcwasmtypes.ModuleName each appear twice. Remove duplicates and keep one authoritative position for each.
@@ - authtypes.ModuleName, + authtypes.ModuleName, @@ - authtypes.ModuleName, + # (removed duplicate authtypes) @@ - ibcwasmtypes.ModuleName, + ibcwasmtypes.ModuleName, @@ - ibcwasmtypes.ModuleName, + # (removed duplicate ibcwasmtypes)If helpful, I can propose a concrete, deduped order aligned with SDK defaults.
app/app.go (1)
151-161: Fix return type: []wasmkeeper.Option (compile-time mismatch).Function returns []wasm.Option but builds []wasmkeeper.Option; this won’t compile.
Apply:
-func GetWasmOpts(appOpts servertypes.AppOptions) []wasm.Option { +func GetWasmOpts(appOpts servertypes.AppOptions) []wasmkeeper.Option { var wasmOpts []wasmkeeper.Option if cast.ToBool(appOpts.Get("telemetry.enabled")) { wasmOpts = append(wasmOpts, wasmkeeper.WithVMCacheMetrics(prometheus.DefaultRegisterer)) } // default wasm gas configuration. wasmOpts = append(wasmOpts, wasmkeeper.WithGasRegister(wasmtypes.NewWasmGasRegister(wasmtypes.DefaultGasRegisterConfig()))) return wasmOpts }app/keepers/keepers.go (2)
189-196: Initialize BankKeeper before CrisisKeeper (prevents zero-value capture).CrisisKeeper is constructed with appKeepers.BankKeeper before BankKeeper is set, so it captures a zero-value keeper.
Move BankKeeper creation above CrisisKeeper:
+ appKeepers.BankKeeper = bankkeeper.NewBaseKeeper( + appCodec, runtime.NewKVStoreService(appKeepers.keys[banktypes.StoreKey]), appKeepers.AccountKeeper, BlockedAddrs(), + govModAddress, bApp.Logger(), + ) + appKeepers.CrisisKeeper = crisiskeeper.NewKeeper( appCodec, runtime.NewKVStoreService(appKeepers.keys[crisistypes.StoreKey]), invCheckPeriod, appKeepers.BankKeeper, authtypes.FeeCollectorName, govModAddress, addresscodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()), ) - -appKeepers.BankKeeper = bankkeeper.NewBaseKeeper( - appCodec, runtime.NewKVStoreService(appKeepers.keys[banktypes.StoreKey]), appKeepers.AccountKeeper, BlockedAddrs(), - govModAddress, bApp.Logger(), -)Also applies to: 222-226
323-331: Construct WasmKeeper before ibc-hooks WasmHooks (avoid nil keeper in middleware).WasmHooks is created with appKeepers.WasmKeeper before it’s initialized. Reorder: create wasm VMs and WasmKeeper first, then WasmHooks/ICS4 wrapper/PacketForwardKeeper.
Minimal outline:
- Move the block that creates wasmVm, lcWasmer, and appWasmKeeper (lines 380–412) above lines 323–331.
- After WasmKeeper is set, create wasmHooks and HooksICS4Wrapper, then PacketForwardKeeper.
If helpful, I can generate a precise patch once you confirm preferred insertion point.
Also applies to: 380-412
♻️ Duplicate comments (3)
go.mod (1)
3-3: Align GitHub Actions go-version with go.mod’s Go 1.24.3Workflows often lag the module toolchain. Please sync actions/setup-go versions to 1.24.3 across all workflows.
#!/bin/bash # Verify setup-go versions match go.mod's 'go 1.24.3' rg -n "uses: *actions/setup-go" .github/workflows --glob "**/*.{yml,yaml}" rg -n "go-version:" .github/workflows --glob "**/*.{yml,yaml}" rg -n "^go\s+1\.24\.3$" go.modapp/keepers/keepers.go (2)
87-89: Advertise the correct CW 3.0 ABI flag and dedupe version markers.Use interface_version_9 (CW 3.0) and remove any existing interface_version_* entries; also add a short doc comment.
@@ -import ( +import ( + "strings" @@ -func ExtendedBuiltInCapabilities() []string { - return append(wasmkeeper.BuiltInCapabilities(), "bitsong", "cosmwasm_3_0") -} +// ExtendedBuiltInCapabilities returns the node's advertised WASM features, +// extending wasmd's defaults with Bitsong and a single CW 3.0 ABI marker. +func ExtendedBuiltInCapabilities() []string { + base := wasmkeeper.BuiltInCapabilities() + caps := make([]string, 0, len(base)+2) + seen := make(map[string]struct{}, len(base)+2) + for _, c := range base { + if strings.HasPrefix(c, "interface_version_") { + continue + } + if _, ok := seen[c]; ok { + continue + } + seen[c] = struct{}{} + caps = append(caps, c) + } + for _, c := range []string{"bitsong", "interface_version_9"} { + if _, ok := seen[c]; ok { + continue + } + seen[c] = struct{}{} + caps = append(caps, c) + } + return caps +}#!/bin/bash echo "wasmvm dep in go.mod:"; rg -nP 'github.com/CosmWasm/wasmvm' go.mod -n echo "advertised ABI flags:"; rg -nP 'interface_version_|cosmwasm_[0-9]+' -n app | sed -n '1,200p'
433-437: Pass ChannelKeeperV2 to wasm IBC handler.wasm.NewIBCHandler’s 4th arg should be ChannelKeeperV2.
- .AddRoute(wasmtypes.ModuleName, wasm.NewIBCHandler(appKeepers.WasmKeeper, appKeepers.IBCKeeper.ChannelKeeper, appKeepers.TransferKeeper, appKeepers.IBCKeeper.ChannelKeeper)) + .AddRoute(wasmtypes.ModuleName, wasm.NewIBCHandler(appKeepers.WasmKeeper, appKeepers.IBCKeeper.ChannelKeeper, appKeepers.TransferKeeper, appKeepers.IBCKeeper.ChannelKeeperV2))#!/bin/bash rg -nP '(NewIBCHandler\(|ChannelKeeperV2)' -C2 -- app | sed -n '1,200p'
🧹 Nitpick comments (18)
scripts/test_node.sh (1)
119-119: Comment contradicts min gas price; not “0 gas fees”.You set
--minimum-gas-prices=0.0025ubtsg. Fix the comment or set zero.Apply one:
-# Start the node with 0 gas fees +# Start the node with 0.0025 ubtsg min gas priceor
- BINARY start --pruning=nothing --minimum-gas-prices=0.0025ubtsg ... + BINARY start --pruning=nothing --minimum-gas-prices=0ubtsg ...scripts/gen-pulsar.sh (4)
1-3: Harden script flags.Use nounset for early failure on typos and a safe IFS.
Apply:
-# this script is for generating protobuf files for the new google.golang.org/protobuf API -set -eo pipefail +# this script is for generating protobuf files for the new google.golang.org/protobuf API +set -euo pipefail +IFS=$'\n\t'
12-14: Clean step is aggressive; constrain scope.Deleting all *.pb.go under api is fine if api/ is 100% generated. If any hand-written Go exists there, it can be removed. Consider limiting to known module roots or guarding with git.
Example guard:
-(cd api; find ./ -type f \( -iname \*.pulsar.go -o -iname \*.pb.go -o -iname \*.cosmos_orm.go -o -iname \*.pb.gw.go \) -delete; find . -empty -type d -delete; cd ..) +(cd api; git ls-files -z | xargs -0 -I{} rm -f "{}"; find . -empty -type d -delete; cd ..)Or maintain a
.generatedsubtree exclusively for generated code.
15-16: Add preflight checks for buf and go.Fail early if required tools are missing.
Apply:
echo "Generating API module" -(cd proto; buf generate --template buf.gen.pulsar.yaml) +command -v go >/dev/null || { echo "go not found"; exit 1; } +command -v buf >/dev/null || { echo "buf not found"; exit 1; } +(cd proto; buf generate --template buf.gen.pulsar.yaml)
5-11: Pin go-pulsar and go-grpc; protoc-gen-go not required by current buf template.proto/buf.gen.pulsar.yaml lists plugins "go-pulsar" and "go-grpc" and buf is present (/usr/bin/buf), so pin the two installed tools and omit protoc-gen-go unless you add the plain "go" plugin.
Proposed change:
-protoc_install_gopulsar() { - go install github.com/cosmos/cosmos-proto/cmd/protoc-gen-go-pulsar@latest - go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest -} +protoc_install_gopulsar() { + local PULSAR_VER=${PULSAR_VER:-v0.4.2} + local GRPC_VER=${GRPC_VER:-v1.4.0} + go install "github.com/cosmos/cosmos-proto/cmd/protoc-gen-go-pulsar@${PULSAR_VER}" + go install "google.golang.org/grpc/cmd/protoc-gen-go-grpc@${GRPC_VER}" +}If you later add the "go" plugin to proto/buf.gen.pulsar.yaml, install and pin google.golang.org/protobuf/cmd/protoc-gen-go.
docs/static/index.html (2)
5-9: Minor UX: add viewport meta and more descriptive title.Improves mobile rendering and branding.
Apply:
- <meta charset="UTF-8"> - <title>Swagger UI</title> + <meta charset="UTF-8"> + <meta name="viewport" content="width=device-width, initial-scale=1" /> + <title>Bitsong API — Swagger UI</title>
41-54: Optional: allow switching specs and surface fetch errors.Makes the page more robust during local dev.
Example:
- const ui = SwaggerUIBundle({ - url: "./swagger.yaml", + const ui = SwaggerUIBundle({ + urls: [{url: "./swagger.yaml", name: "Local"}], dom_id: '#swagger-ui', deepLinking: true, queryConfigEnabled: false, presets: [ SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ], plugins: [ SwaggerUIBundle.plugins.DownloadUrl ], layout: "StandaloneLayout" }); + ui.getSystem().fn.fetchError && console.error('Failed to load swagger.yaml');api/bitsong/fantoken/v1beta1/genesis.pulsar.go (1)
245-248: Brittle type assertion in Set() for fan_tokens listSet() assumes the incoming List is *_GenesisState_2_list; different List implementations will panic. If feasible in the generator, accept any protoreflect.List and copy elements instead of asserting concrete type.
scripts/gen-swagger.sh (4)
13-13: Update Node image: node:14-alpine is EOL; switch to current LTS.For security and supply-chain hygiene, use node:20-alpine (or 22).
- docker run --rm -v "$project_root":/workspace -w /workspace node:14-alpine npm "$@" + docker run --rm -v "$project_root":/workspace -w /workspace node:20-alpine npm "$@" @@ - docker run --rm -v "$project_root":/workspace -w /workspace node:14-alpine npx swagger-combine "$@" + docker run --rm -v "$project_root":/workspace -w /workspace node:20-alpine npx swagger-combine "$@" @@ - docker run --rm -v "$project_root":/workspace -w /workspace node:14-alpine npx swagger-merger "$@" + docker run --rm -v "$project_root":/workspace -w /workspace node:20-alpine npx swagger-merger "$@"Also applies to: 28-28, 40-40
137-146: Make host configurable (don’t hardcode localhost:1317).Expose via SWAGGER_HOST env with a sensible default.
-base_json=$(jq -n --arg version "$version" '{ +base_json=$(jq -n --arg version "$version" --arg host "${SWAGGER_HOST:-localhost:1317}" '{ swagger: "2.0", info: { title: "Bitsong Network API", version: $version, description: "REST API for Bitsong blockchain" }, - host: "localhost:1317", + host: $host, schemes: ["http", "https"], consumes: ["application/json"], produces: ["application/json"], paths: {}, definitions: {} }')
169-179: Deterministic and safe operationId uniquifying.Random suffixes create noisy diffs and “keys[]” may hit non-method keys; guard by HTTP verbs and compute a stable suffix.
-# Loop through all paths and methods to update any "operationId" by appending a random 5-character suffix. -jq -r '.paths | to_entries[] | "\(.key) \(.value | keys[])"' "$temp_file" | while read -r path method; do - # Generate a simple random suffix using timestamp and process ID - suffix=$(printf "%05d" $((RANDOM % 100000))) +# Loop through all paths/methods; touch only HTTP verbs; append deterministic 6‑char checksum. +jq -r '.paths | to_entries[] as $e + | $e.value | to_entries[] + | select(.key | test("^(get|post|put|patch|delete|options|head)$")) + | "\($e.key) \(.key)"' "$temp_file" | while read -r path method; do + suffix=$(printf "%s %s" "$path" "$method" | cksum | awk '{print $1}' | cut -c1-6) temp_file2=$(mktemp) - jq --arg path "$path" --arg method "$method" --arg suffix "$suffix" ' - if (.paths[$path][$method] | has("operationId")) + jq --arg path "$path" --arg method "$method" --arg suffix "$suffix" ' + if ((.paths[$path][$method] | type) == "object" and (.paths[$path][$method] | has("operationId"))) then .paths[$path][$method].operationId |= (. + "_" + $suffix) else . end' "$temp_file" > "$temp_file2" mv "$temp_file2" "$temp_file" done
8-19: Unused helper.
run_npmis defined but never used; drop it or use it consistently.scripts/makefiles/proto.mk (2)
18-27: Help text and targets out of sync; remove stale entry and clarify semantics.Currently: help lists proto-gen-swagger and proto-docs, but only proto-gen (which runs Swagger) and proto-gen-pulsar exist.
Apply:
- @echo " proto-gen-swagger. Generate Protobuf files" - @echo " proto-gen-pulsar Generate Protobuf files" + @echo " proto-gen Generate Swagger docs (proto-first)" + @echo " proto-gen-swagger Generate Swagger docs" + @echo " proto-gen-pulsar Generate Pulsar protobufs" @@ - @echo " proto-docs Create Swagger API docs" + @echo ""And add an alias target so both names work:
+proto-gen-swagger: + @echo "Generating Swagger API docs" + @$(protoImage) sh ./scripts/gen-swagger.sh
8-10: Unused variables.SWAGGER_DIR and THIRD_PARTY_DIR are not used here; consider removing to avoid drift.
app/modules.go (1)
244-246: Minor: avoid reinitializing encoding; return the cached txConfig.
GetEncodingConfig()likely rebuilds. Use the field set in BitsongApp.-func (app *BitsongApp) GetTxConfig() client.TxConfig { - return GetEncodingConfig().TxConfig -} +func (app *BitsongApp) GetTxConfig() client.TxConfig { + return app.txConfig +}app/app.go (3)
563-565: Handle RegisterSwaggerAPI error.Current call ignores error; bubble up or log to fail fast if swagger assets aren’t found.
- RegisterSwaggerAPI(clientCtx, apiSvr) + if err := RegisterSwaggerAPI(clientCtx, apiSvr); err != nil { + panic(fmt.Errorf("register swagger: %w", err)) + }
328-331: Remove duplicate RegisterStoreDecoders() calls.It’s invoked 2–3 times; keep one.
- app.sm.RegisterStoreDecoders() - - reflectionSvc := getReflectionService() - reflectionv1.RegisterReflectionServiceServer(app.GRPCQueryRouter(), reflectionSvc) - -app.sm.RegisterStoreDecoders() + reflectionSvc := getReflectionService() + reflectionv1.RegisterReflectionServiceServer(app.GRPCQueryRouter(), reflectionSvc)- app.sm.RegisterStoreDecoders()Also applies to: 337-338, 414-416
102-105: Unify DefaultNodeHome initialization.Defined in var block and reset again in init(); keep a single source to avoid drift.
Also applies to: 195-203
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
api/bitsong/fantoken/v1beta1/query_grpc.pb.gois excluded by!**/*.pb.goapi/bitsong/fantoken/v1beta1/tx_grpc.pb.gois excluded by!**/*.pb.goapi/bitsong/smartaccount/v1beta1/query_grpc.pb.gois excluded by!**/*.pb.goapi/bitsong/smartaccount/v1beta1/tx_grpc.pb.gois excluded by!**/*.pb.godocs/static/favicon-16x16.pngis excluded by!**/*.pngdocs/static/favicon-32x32.pngis excluded by!**/*.pngswagger/swagger-ui/swagger-ui-bundle.js.mapis excluded by!**/*.mapswagger/swagger-ui/swagger-ui-es-bundle-core.js.mapis excluded by!**/*.mapswagger/swagger-ui/swagger-ui-es-bundle.js.mapis excluded by!**/*.mapswagger/swagger-ui/swagger-ui-standalone-preset.js.mapis excluded by!**/*.mapswagger/swagger-ui/swagger-ui.css.mapis excluded by!**/*.mapswagger/swagger-ui/swagger-ui.js.mapis excluded by!**/*.map
📒 Files selected for processing (19)
api/bitsong/fantoken/v1beta1/fantoken.pulsar.go(1 hunks)api/bitsong/fantoken/v1beta1/genesis.pulsar.go(1 hunks)api/bitsong/fantoken/v1beta1/gov.pulsar.go(1 hunks)api/bitsong/fantoken/v1beta1/params.pulsar.go(1 hunks)api/bitsong/smartaccount/v1beta1/genesis.pulsar.go(1 hunks)api/bitsong/smartaccount/v1beta1/models.pulsar.go(1 hunks)api/bitsong/smartaccount/v1beta1/params.pulsar.go(1 hunks)api/cosmos/crypto/bls12381/keys.pulsar.go(1 hunks)app/app.go(10 hunks)app/keepers/keepers.go(13 hunks)app/modules.go(4 hunks)docs/docs.go(1 hunks)docs/static/index.html(1 hunks)go.mod(8 hunks)scripts/gen-pulsar.sh(1 hunks)scripts/gen-swagger.sh(1 hunks)scripts/makefiles/proto.mk(2 hunks)scripts/test_node.sh(2 hunks)swagger/config.json(0 hunks)
💤 Files with no reviewable changes (1)
- swagger/config.json
✅ Files skipped from review due to trivial changes (1)
- api/cosmos/crypto/bls12381/keys.pulsar.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-04T15:11:55.240Z
Learnt from: hard-nett
PR: bitsongofficial/go-bitsong#265
File: x/fantoken/client/cli/tx.go:451-454
Timestamp: 2025-02-04T15:11:55.240Z
Learning: In the BitSong implementation of cosmos-sdk gov module's NewMsgSubmitProposal, ValidateBasic() is not part of the interface, so the previous validation check in GetCmdUpdateFantokenFees was correctly commented out.
Applied to files:
api/bitsong/fantoken/v1beta1/gov.pulsar.go
📚 Learning: 2025-02-04T15:11:55.240Z
Learnt from: hard-nett
PR: bitsongofficial/go-bitsong#265
File: x/fantoken/client/cli/tx.go:451-454
Timestamp: 2025-02-04T15:11:55.240Z
Learning: In cosmos-sdk v0.50.x (as used in BitSong), NewMsgSubmitProposal does not have ValidateBasic() as an interface function, reflecting the SDK's modern protobuf-based message system. The validation check in GetCmdUpdateFantokenFees should not be included.
Applied to files:
api/bitsong/fantoken/v1beta1/gov.pulsar.go
🧬 Code graph analysis (7)
api/bitsong/fantoken/v1beta1/params.pulsar.go (1)
api/bitsong/smartaccount/v1beta1/params.pulsar.go (3)
Params(612-628)Params(643-643)Params(646-648)
api/bitsong/fantoken/v1beta1/fantoken.pulsar.go (1)
tests/ict/setup.go (1)
Denom(27-27)
api/bitsong/fantoken/v1beta1/genesis.pulsar.go (2)
api/bitsong/fantoken/v1beta1/fantoken.pulsar.go (3)
FanToken(1334-1345)FanToken(1360-1360)FanToken(1363-1365)api/bitsong/fantoken/v1beta1/params.pulsar.go (3)
Params(624-632)Params(647-647)Params(650-652)
api/bitsong/smartaccount/v1beta1/genesis.pulsar.go (2)
api/bitsong/smartaccount/v1beta1/models.pulsar.go (3)
AccountAuthenticator(565-581)AccountAuthenticator(596-596)AccountAuthenticator(599-601)api/bitsong/smartaccount/v1beta1/params.pulsar.go (3)
Params(612-628)Params(643-643)Params(646-648)
app/keepers/keepers.go (5)
x/smart-account/keeper/keeper.go (2)
Keeper(27-37)NewKeeper(39-58)x/fantoken/keeper/keeper.go (2)
Keeper(17-26)NewKeeper(28-54)x/fantoken/types/expected_keepers.go (3)
AccountKeeper(22-24)BankKeeper(27-37)DistrKeeper(10-12)app/app.go (1)
Bech32Prefix(85-85)app/keepers/acceptedQueries.go (1)
AcceptedQueries(19-63)
app/modules.go (5)
x/fantoken/module.go (1)
NewAppModule(102-108)x/fantoken/types/expected_keepers.go (3)
AccountKeeper(22-24)BankKeeper(27-37)DistrKeeper(10-12)x/fantoken/types/keys.go (1)
ModuleName(9-9)app/app.go (1)
BitsongApp(171-192)x/fantoken/keeper/keeper.go (1)
Keeper(17-26)
app/app.go (4)
app/upgrades/types.go (1)
Upgrade(16-25)app/upgrades/v024/constants.go (1)
Upgrade(12-16)app/keepers/keepers.go (1)
ExtendedBuiltInCapabilities(87-89)docs/docs.go (1)
Docs(6-6)
⏰ 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). (1)
- GitHub Check: build-docker
🔇 Additional comments (10)
scripts/test_node.sh (1)
50-50: Init args change looks fine.Dropping
--default-denomaligns with newer SDK defaults; the subsequent genesis JQ sets denoms explicitly.Please run a quick sanity check to ensure
client.tomlandapp.tomllive under$HOME_DIRand RPC/API endpoints bind as expected.api/bitsong/fantoken/v1beta1/params.pulsar.go (1)
1-15: Generated Pulsar code — looks standard.File structure and methods match protoc-gen-go(-pulsar) output; avoid manual edits. Keep generator versions pinned for reproducibility.
Please regenerate once with the updated
scripts/gen-pulsar.shto ensure a clean, pinned build and rungo vet ./...to catch any generator drift.docs/docs.go (1)
5-6: LGTM — embedded docs FS looks correct; swagger.yaml present, server mount not verified.Found docs/static/swagger.yaml; repo search returned no matches for use of Docs or typical mounting calls (http.FileServer(http.FS(...)) / fs.Sub). Confirm the server registers the embedded FS.
api/bitsong/smartaccount/v1beta1/models.pulsar.go (1)
562-581: LGTM — generated model looks correctReflection paths, marshal/unmarshal, and field encodings are consistent. No issues spotted.
api/bitsong/smartaccount/v1beta1/params.pulsar.go (1)
1-14: Generated file: keep source of truth in .proto and regen only.Looks consistent with Pulsar fast-reflection patterns. No manual edits recommended.
Please pin and document the exact generator versions (protoc, protoc-gen-go, protoc-gen-go-pulsar) in your tooling to ensure reproducible builds.
api/bitsong/smartaccount/v1beta1/genesis.pulsar.go (1)
1-14: Generated file: LGTM.Structure matches Params/AuthenticatorData/GenesisState with fast-reflection and protoiface methods. Avoid manual edits.
Ensure AccountAuthenticator is generated before this file (build order) and that the module’s go_package paths resolve.
app/modules.go (2)
123-142: Pointer/value wiring looks correct for SDK v0.50+/wasmd v0.50+.Gov by value, mint/slashing/distr by value-deref, wasm by value, evidence by value-deref, transfer by value-deref, protocolpool by value-deref. Good.
Please confirm against the exact SDK/wasmd versions pulled in go.mod to avoid silent interface mismatches.
147-156: Validate Begin/End/Init blocker ordering for new modules (ibc-hooks, 08-wasm) and core modules (staking, gov).
Ordering drives invariants and handler side-effects. With the added ibc-hooks and 08-wasm modules, confirm Begin/End/Init sequences in app/modules.go — functions orderBeginBlockers / orderEndBlockers / orderInitBlockers (≈ lines 147–167) match SDK/IBC/wasmd guidance.
Automated verification failed in the sandbox; run locally to (1) list the three blocker arrays and check for duplicate entries, and (2) confirm the relative positions of stakingtypes.ModuleName, govtypes.ModuleName, ibctransfertypes.ModuleName, wasmtypes.ModuleName, ibcwasmtypes.ModuleName, and ibchookstypes.ModuleName against SDK/IBC/wasmd recommendations. Return the outputs or confirm no changes required.scripts/gen-swagger.sh (2)
4-6: Confirm plugin/tooling expectations — verification incompleteRan checks inside the proto-builder image: buf --version => 1.55.1; the image lacks
which(so checks fell back to "not found"); protoc-gen-swagger and protoc-gen-openapiv2 were not found; buf.gen.swagger.yaml is missing.File: scripts/gen-swagger.sh Lines: 4-6
prepare_swagger_gen() { go install github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger@v1.16.0 }
- Ensure buf.gen.swagger.yaml (exists in the repo) expects
protoc-gen-swagger(notprotoc-gen-openapiv2) and that the proto-builder image hasprotoc-gen-swaggeron PATH aftergo install(GOBIN/GOPATH/bin must be in PATH).- Verify inside the proto-builder container with:
command -v protoc-gen-swagger || echo missing; command -v protoc-gen-openapiv2 || echo missing; cat buf.gen.swagger.yaml
196-200: Double-check swagger-combine usage & CI generationVerification: running the provided checks showed npx swagger-combine returned "No config file in arguments" (v1.5.4) and tmp-swagger-gen/_all/FINAL.json was missing (FileNotFoundError), so I could not validate the current invocation.
Action: either pass a proper swagger-combine config (it expects a config, not a pre-merged spec) or replace this step with swagger-merger or swagger2openapi to do JSON→YAML + $ref expansion; add a CI assertion that ./docs/static/swagger.yaml is produced.Location: scripts/gen-swagger.sh lines 196-200.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/keepers/keys.go (1)
34-58: Restore capability store key registration.We still mount the capability module (see
orderBeginBlockers/EndBlockersinapp/modules.go) and the IBC keeper depends on the capability keeper being initialized with its store. Removingcapabilitytypes.StoreKeyfrom the KV store list leaves the capability keeper without a backing store, so any access will panic at runtime during app bootstrapping. Please add the capability store key back intostoretypes.NewKVStoreKeys(...)before this merges.crisistypes.StoreKey, paramstypes.StoreKey, consensusparamtypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, evidencetypes.StoreKey, + capabilitytypes.StoreKey, packetforwardtypes.StoreKey,app/keepers/keepers.go (1)
224-337: Make keeper members pointer-consistent.Several keepers (
FeeGrantKeeper,AuthzKeeper,DistrKeeper,SlashingKeeper,MintKeeper,TransferKeeper,FanTokenKeeper) are stored as pointers, yet we assign them values from their constructors. This copies the keeper structs (with mutexes and caches) and leaves the app with dangling references. Please store the pointer returned by the constructors (or take the address before assigning).- feegrantKeeper := feegrantkeeper.NewKeeper(...) - appKeepers.FeeGrantKeeper = &feegrantKeeper + appKeepers.FeeGrantKeeper = feegrantkeeper.NewKeeper(...)(Apply the same pattern for AuthzKeeper, DistrKeeper, SlashingKeeper, MintKeeper, TransferKeeper, FanTokenKeeper, and any other keeper stored as a pointer.)
🧹 Nitpick comments (4)
scripts/makefiles/bsh.mk (1)
1-34: Declare these targets as phony for reliability.All of these targets just run shell scripts; if a file named
bsh-*ever appears,makecould incorrectly treat the target as up to date. Declaring them.PHONYavoids that edge case.Add the phony declaration near the top:
+.PHONY: bsh-help bsh bsh-all bsh-aa bsh-ibchook bsh-upgrade bsh-staking-hooks bsh-polytone bsh-pfm bsh-nfts + bsh-help:app/app.go (1)
395-405: PropagateRegisterSwaggerAPIerrors.
RegisterSwaggerAPInow returns an error but we drop it silently inRegisterAPIRoutes. If the embed is missing, the API server will think Swagger is enabled even though route registration failed. Please log or surface the error so operators can see the failure.- if apiConfig.Swagger { - RegisterSwaggerAPI(clientCtx, apiSvr) - } + if apiConfig.Swagger { + if err := RegisterSwaggerAPI(clientCtx, apiSvr); err != nil { + clientCtx.PrintErr(fmt.Errorf("failed to register swagger API: %w", err)) + } + }app/keepers/keepers.go (2)
399-408: Keep the IBC Wasm keeper as a pointer.
ibcwlckeeper.NewKeeperWithVMreturns a struct value; assigning it by value (appKeepers.IBCWasmClientKeeper = &ibcWasmClientKeeper) works, but everywhere else we treat the keeper as a pointer. Make the constructor’s return pointer consistent to avoid accidental copies (and match the upstream pattern).
419-423: Seal the router after route registration.After adding the wasm route, remember to call
ibcRouter.Seal()before handing it toSetRouter; otherwise, downstream code can mutate the router at runtime. This is the upstream pattern in ibc-go v10.- appKeepers.IBCKeeper.SetRouter(ibcRouter) + appKeepers.IBCKeeper.SetRouter(ibcRouter.Seal())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)app/app.go(10 hunks)app/keepers/keepers.go(12 hunks)app/keepers/keys.go(2 hunks)app/modules.go(3 hunks)app/testing/test_suite.go(3 hunks)app/upgrades/v024/upgrades.go(1 hunks)app/upgrades/v024/upgrades_test.go(1 hunks)scripts/makefiles/bsh.mk(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- app/upgrades/v024/upgrades_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
app/upgrades/v024/upgrades.go (3)
app/upgrades/types.go (1)
BaseAppParamManager(29-32)app/keepers/keepers.go (1)
AppKeepers(102-139)app/upgrades/v024/constants.go (1)
UpgradeName(9-9)
app/keepers/keepers.go (4)
x/smart-account/keeper/keeper.go (2)
Keeper(27-37)NewKeeper(39-58)x/fantoken/keeper/keeper.go (2)
Keeper(17-26)NewKeeper(28-54)x/fantoken/types/expected_keepers.go (3)
AccountKeeper(22-24)BankKeeper(27-37)DistrKeeper(10-12)app/app.go (2)
Bech32Prefix(117-117)Bech32PrefixAccAddr(143-143)
app/app.go (4)
app/upgrades/types.go (1)
Upgrade(16-25)app/keepers/keepers.go (3)
ExtendedBuiltInCapabilities(83-85)NewAppKeepers(141-450)AppKeepers(102-139)x/fantoken/module.go (1)
NewAppModule(102-108)docs/docs.go (1)
Docs(6-6)
app/modules.go (5)
x/fantoken/module.go (1)
NewAppModule(102-108)x/fantoken/types/expected_keepers.go (3)
AccountKeeper(22-24)BankKeeper(27-37)DistrKeeper(10-12)app/app.go (1)
BitsongApp(203-224)x/smart-account/keeper/keeper.go (1)
Keeper(27-37)x/fantoken/keeper/keeper.go (1)
Keeper(17-26)
🔇 Additional comments (4)
app/testing/test_suite.go (2)
66-70: Good call ensuring deterministic test configs.Creating the minimal configs up front avoids
SetupWithCustomHomeflakiness from missing TOML files during upgrade tests.
155-175: Helper keeps the temp config lean and reproducible.The scoped
config.tomlwith shortened consensus timeouts keeps tests fast while staying readable. Nice touch.app/upgrades/v024/upgrades.go (1)
42-51: Use theBaseAppParamManagerinstead of pokingConsensusParamsKeeperdirectly.
CreateV024Upgradealready receives theBaseAppParamManagerso the handler can write consensus params via BaseApp, which takes care of in-memory caches. Callingk.ConsensusParamsKeeper.ParamsStore.Setbypasses BaseApp’s store loader and risks the new params not being observed until the next height. Please switch theSetcall tobpm.StoreConsensusParams. That matches the intent of the interface you’re already passing in.- err = k.ConsensusParamsKeeper.ParamsStore.Set(sdkCtx, consensusParams) - if err != nil { - return nil, err - } + if err := bpm.StoreConsensusParams(sdkCtx, consensusParams); err != nil { + return nil, err + }Likely an incorrect or invalid review comment.
app/keepers/keepers.go (1)
377-398: Pass ChannelKeeperV2 into the Wasm IBC handler.
wasm.NewIBCHandler’s fourth argument expects the ChannelKeeperV2 implementation to satisfy the async acknowledgement path. Passing the legacyChannelKeeperprevents wasm contracts from receiving async acks under ibc-go v10. UseappKeepers.IBCKeeper.ChannelKeeperV2instead.- AddRoute(wasmtypes.ModuleName, wasm.NewIBCHandler(appKeepers.WasmKeeper, appKeepers.IBCKeeper.ChannelKeeper, appKeepers.TransferKeeper, appKeepers.IBCKeeper.ChannelKeeper)) + AddRoute(wasmtypes.ModuleName, wasm.NewIBCHandler(appKeepers.WasmKeeper, appKeepers.IBCKeeper.ChannelKeeper, appKeepers.TransferKeeper, appKeepers.IBCKeeper.ChannelKeeperV2))
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/modules.go (1)
124-153: Duplicate modules in SetOrderInitGenesis list.authtypes.ModuleName and ibcwasmtypes.ModuleName appear twice. Duplicate entries can cause initialization/order issues.
Apply this diff to dedupe:
return []string{ - authtypes.ModuleName, + authtypes.ModuleName, banktypes.ModuleName, fantokentypes.ModuleName, distrtypes.ModuleName, stakingtypes.ModuleName, slashingtypes.ModuleName, govtypes.ModuleName, minttypes.ModuleName, crisistypes.ModuleName, packetforwardtypes.ModuleName, ibctransfertypes.ModuleName, ibcexported.ModuleName, evidencetypes.ModuleName, feegrant.ModuleName, - authz.ModuleName, - authtypes.ModuleName, - ibcwasmtypes.ModuleName, + authz.ModuleName, + ibcwasmtypes.ModuleName, smartaccounttypes.ModuleName, genutiltypes.ModuleName, paramstypes.ModuleName, upgradetypes.ModuleName, vestingtypes.ModuleName, wasmtypes.ModuleName, - ibcwasmtypes.ModuleName, ibchookstypes.ModuleName, protocolpooltypes.ModuleName, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/app.go(10 hunks)app/modules.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/app.go (5)
app/upgrades/types.go (1)
Upgrade(16-25)app/upgrades/v024/constants.go (1)
Upgrade(12-16)app/keepers/keepers.go (3)
ExtendedBuiltInCapabilities(83-85)NewAppKeepers(141-450)AppKeepers(102-139)x/fantoken/module.go (1)
NewAppModule(102-108)docs/docs.go (1)
Docs(6-6)
app/modules.go (5)
x/fantoken/module.go (1)
NewAppModule(102-108)x/fantoken/types/expected_keepers.go (3)
AccountKeeper(22-24)BankKeeper(27-37)DistrKeeper(10-12)app/app.go (1)
BitsongApp(204-225)x/smart-account/keeper/keeper.go (1)
Keeper(27-37)x/fantoken/keeper/keeper.go (1)
Keeper(17-26)
⏰ 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). (1)
- GitHub Check: build-docker
🔇 Additional comments (6)
app/app.go (3)
361-371: Confirm AppModule constructor expects a value keeper or pointer.ibcwasm.NewAppModule(*app.IBCWasmClientKeeper) passes a copied keeper. If NewAppModule expects a pointer (common for keepers with internal mutexes), this should be changed to pass the pointer. Please confirm the constructor signature for v10.
If the constructor accepts a pointer, update to:
- ibcwasm.NewAppModule(*app.IBCWasmClientKeeper), + ibcwasm.NewAppModule(app.IBCWasmClientKeeper),
697-714: Swagger route registration looks good.Static FS and redirects are wired cleanly; returning an error instead of panic is appropriate.
Ensure apiSvr.Router supports PathPrefix (Gorilla Mux). If the SDK version moved away from mux, adjust to the router’s equivalent mounting API.
304-309: Pass keeper pointer to NewLightClientModule (copying keeper breaks internals).You're dereferencing the keeper and passing a value. This copies internal state (mutexes, caches), leading to subtle bugs. Pass the pointer instead. Also keep the unified alias from the import fix.
Apply this diff:
- tmLightClientModule := ibctm.NewLightClientModule(appCodec, storeProvider) - ibcWasmLightClientModule := ibcwasm.NewLightClientModule(*appKeepers.IBCWasmClientKeeper, storeProvider) - clientKeeper.AddRoute(ibctm.ModuleName, &tmLightClientModule) - clientKeeper.AddRoute(ibcwasmtypes.ModuleName, ibcWasmLightClientModule) + tmLightClientModule := ibctm.NewLightClientModule(appCodec, storeProvider) + ibcWasmLightClientModule := ibcwasm.NewLightClientModule(appKeepers.IBCWasmClientKeeper, storeProvider) + clientKeeper.AddRoute(ibctm.ModuleName, &tmLightClientModule) + clientKeeper.AddRoute(ibcwasmtypes.ModuleName, ibcWasmLightClientModule)app/modules.go (3)
102-111: BeginBlockers order change acknowledged.Mint/auth first looks intentional for your migration. Ensure downstream invariants are satisfied.
Please confirm this matches your desired execution sequence for v10 (especially wrt distribution and staking hooks).
156-178: Simulation module wiring looks consistent with v10.Mint now uses its own subspace; other keeper/value usages align with constructors.
181-187: Accessor methods are fine for testing.Returning keeper values for ibctesting is acceptable here.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/makefiles/proto.mk (1)
26-26: Remove stale help text for non-existent target.The help text references
proto-docsbut this target was removed in the changes (as noted in the AI summary). This creates confusion for users.@echo " proto-image-build Build the protobuf Docker image" @echo " proto-image-push Push the protobuf Docker image" - @echo " proto-docs Create Swagger API docs"
♻️ Duplicate comments (4)
go.mod (2)
3-3: This concern has been previously raised regarding Go version alignment with CI workflows.
233-235: This concern has been previously raised regarding temporary replace directives to permissionlessweb forks.scripts/gen-swagger.sh (2)
68-72: This concern has been previously raised regarding the malformed find expression.
88-97: This concern has been previously raised regarding jq being used before the existence check.
🧹 Nitpick comments (3)
scripts/gen-swagger.sh (3)
116-116: Prefer arithmetic expansion overexprcommand.The
exprcommand is archaic and slower than bash's built-in arithmetic expansion.- counter=$(expr $counter + 1) + counter=$((counter + 1))
105-117: Add safeguard for empty file list.If no swagger files are found, the
for f in $filesloop could behave unexpectedly when$filesis empty, potentially iterating once with an empty string.files=$(find ./tmp-swagger-gen -name '*.swagger.json' -print0 | xargs -0) +if [ -z "$files" ]; then + echo "Warning: No swagger files found to process" +else counter=0 for f in $files; do echo "[+] $f" case "$f" in *router*) cp "$f" ./tmp-swagger-gen/_all/pfm-$counter.json ;; *cosmwasm*) cp "$f" ./tmp-swagger-gen/_all/cosmwasm-$counter.json ;; *bitsong*) cp "$f" ./tmp-swagger-gen/_all/bitsong-$counter.json ;; *cosmos*) cp "$f" ./tmp-swagger-gen/_all/cosmos-$counter.json ;; *) cp "$f" ./tmp-swagger-gen/_all/other-$counter.json ;; esac counter=$((counter + 1)) done +fi
169-178: Consider more robust random suffix generation.The current approach uses
$RANDOM % 100000which could produce collisions in large API surfaces, and the suffix is applied sequentially rather than uniquely per operation.For better uniqueness, consider using a hash-based approach:
jq -r '.paths | to_entries[] | "\(.key) \(.value | keys[])"' "$temp_file" | while read -r path method; do - # Generate a simple random suffix using timestamp and process ID - suffix=$(printf "%05d" $((RANDOM % 100000))) + # Generate a deterministic suffix from path and method + suffix=$(echo "${path}${method}" | md5sum | cut -c1-5) temp_file2=$(mktemp) jq --arg path "$path" --arg method "$method" --arg suffix "$suffix" ' if (.paths[$path][$method] | has("operationId")) then .paths[$path][$method].operationId |= (. + "_" + $suffix) else . end' "$temp_file" > "$temp_file2" mv "$temp_file2" "$temp_file" doneThis ensures the same path/method always gets the same suffix, improving reproducibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(8 hunks)scripts/gen-swagger.sh(1 hunks)scripts/makefiles/build.mk(1 hunks)scripts/makefiles/proto.mk(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker
🔇 Additional comments (3)
scripts/makefiles/build.mk (1)
48-48: Consider idempotency of the release preparation script.Adding release preparation as a side effect of
build-reproduciblemeans the script will run every time reproducible binaries are built. Ensure the script is idempotent and safe to run multiple times, especially in CI/CD environments where builds may be retriggered.scripts/makefiles/proto.mk (1)
30-38: LGTM: Target renames align with new generation workflow.The refactoring consolidates Swagger and Pulsar generation under distinct, clearly-named targets. The dependency update in
proto-all(Line 30) correctly reflects the newproto-gen-swaggertarget, and the script paths (Lines 34, 38) match the new file locations mentioned in the PR changes.go.mod (1)
18-29: Upgrade path and tests in place. The v024 upgrade handler (CreateV024Upgrade) and its unit tests exist underapp/upgrades/v024, and ICT suites (basic_upgrade_test.go,TestIbcHooks,TestPacketForwardMiddlewareRouter,TestPolytone) cover WASM and IBC flows.
| npm "$@" | ||
| else | ||
| if command -v docker &>/dev/null; then | ||
| docker run --rm -v "$project_root":/workspace -w /workspace node:14-alpine npm "$@" |
There was a problem hiding this comment.
Update Docker image to a supported Node.js version.
The script uses node:14-alpine which reached end-of-life in April 2023. This poses security risks and compatibility issues with modern npm packages.
Update to a supported Node.js LTS version:
- docker run --rm -v "$project_root":/workspace -w /workspace node:14-alpine npm "$@"
+ docker run --rm -v "$project_root":/workspace -w /workspace node:20-alpine npm "$@"Apply similar changes to Lines 28 and 40.
Also applies to: 28-28, 40-40
🤖 Prompt for AI Agents
scripts/gen-swagger.sh lines 13, 28, and 40: the Docker image reference uses an
EOL Node.js version; update the image tag to a supported LTS (e.g., replace
node:14-alpine with node:20-alpine) on lines 13, 28 and 40, preserving the
existing volume, workdir and npm "$@" invocation, then run the script to verify
commands succeed with the new Node image.
| # a lot of time due to QEMU virtualization but it's the only way (afaik) | ||
| # to get a statically linked binary with CosmWasm | ||
| build-reproducible: build-reproducible-amd64 build-reproducible-arm64 | ||
| sh scripts/release/prep-release.sh |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the script path for consistency.
This line calls scripts/release/prep-release.sh, but line 89 calls scripts/prep-release.sh (without the release/ subdirectory). Are these intended to be different scripts, or is one path incorrect?
Run the following script to check if both scripts exist and whether they differ:
🏁 Script executed:
#!/bin/bash
# Verify the existence and differences between the two prep-release scripts
echo "=== Checking script existence ==="
if [ -f "scripts/release/prep-release.sh" ]; then
echo "✓ scripts/release/prep-release.sh exists"
else
echo "✗ scripts/release/prep-release.sh NOT FOUND"
fi
if [ -f "scripts/prep-release.sh" ]; then
echo "✓ scripts/prep-release.sh exists"
else
echo "✗ scripts/prep-release.sh NOT FOUND"
fi
echo ""
echo "=== Comparing scripts (if both exist) ==="
if [ -f "scripts/release/prep-release.sh" ] && [ -f "scripts/prep-release.sh" ]; then
if diff -q scripts/release/prep-release.sh scripts/prep-release.sh > /dev/null; then
echo "Scripts are identical"
else
echo "Scripts differ:"
diff -u scripts/prep-release.sh scripts/release/prep-release.sh
fi
fiLength of output: 153
Fix incorrect script invocation path
Line 89 invokes scripts/prep-release.sh which doesn’t exist—change it to scripts/release/prep-release.sh.
🤖 Prompt for AI Agents
In scripts/makefiles/build.mk around line 48, the Makefile currently invokes a
non-existent script path; update the script invocation to use the correct path
"scripts/release/prep-release.sh" (replace any occurrence of
"scripts/prep-release.sh" with "scripts/release/prep-release.sh") so the release
prep script is called from the proper subdirectory.
Closes:
Ibc-Hooks #294 : Added Ibc-Hooks Interchaintest CI
tests: Wasm + IBC integration #287 : Re-Added Polytone Interchaintest CI
fix: PR & Upgrade CI #286: Includes upgrade ci using live network state (permission gated by workfspace maintainers)
fix: App Upgrade Local & CI Tests #288: ^
Interchain Query + Wasm #295
Introduces major upgrades to ibc-go, wasmvm, 08-wasm light-client, ibc-hooks, pfm, and removes unused modules
Extends the supported wasm capabilities to the latest cw version. Its important to ensure we don't have any contracts using the removed AllBalances query. The core bs-nfts do not, we must ensure DAODAO does not make use of this query. These are the two main frameworks that will be impacted.
Correctly wires in our swagger & pulsar client generations (this is why PR change count is so large)
These are breaking changes & require a coordinated upgrade
Summary by CodeRabbit