v0.22.0: - resolve v018-v021 issues #278
Conversation
📝 Walkthrough""" WalkthroughThis update implements a comprehensive refactor and feature extension across the Bitsong blockchain application. Key changes include correcting the "cadance" module name to "cadence" throughout the codebase, embedding Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant App
participant UpgradeHandler
participant Keeper
participant ProtocolPool
participant Exporter
CLI->>App: Start with new keeper structure & protocolpool
App->>Keeper: Direct keeper field access (no AppKeepers indirection)
App->>UpgradeHandler: Run v022 upgrade
UpgradeHandler->>Keeper: Iterate validators & delegations
UpgradeHandler->>Keeper: Patch delegation rewards as needed
UpgradeHandler->>ProtocolPool: Fund community pool via protocolpool
CLI->>Exporter: Run custom-export command
Exporter->>App: Export state with custom patch logic
Exporter->>CLI: Output JSON state
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
merge antstalepresh patch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (27)
app/upgrades/v021/upgrades.go (2)
181-265: Consider maintaining consistent logging patterns.
Some logging statements use structured logs (sdkCtx.Logger().Info("message", "key", value)), while others rely on string construction withfmt.Sprintf. Adopting a unified mechanism (preferably the structured approach) would enhance clarity and consistency in logs.
368-371: Fix potential function name typo.
CustommTokensFromSharesTruncatedhas an extra "m" in its identifier. Consider renaming for readability and consistency.-func CustommTokensFromSharesTruncated(t math.Int, ds math.LegacyDec, shares math.LegacyDec) math.LegacyDec { +func CustomTokensFromSharesTruncated(t math.Int, ds math.LegacyDec, shares math.LegacyDec) math.LegacyDec {go.mod (1)
235-238: Consider removing or gating debug replacement lines.If these replacements are for debugging only, keep them out of production or wrap them in a condition to avoid confusion.
app/upgrades/v022/upgrades.go (5)
41-50: Streamline error handling in patch logic.The patch logic calls
panic(err). Evaluate if it’s safer to bubble up errors. Panics might disrupt the entire chain upgrade in production, so returning an error might be more graceful.
197-264: Extract magic number for marginOfErr into a constant.The code uses a 0.095 margin of error. Use a descriptive constant or configuration to avoid confusion and facilitate future changes.
266-383: Review the unconditional panic in patch check.If
patchedis false, the logic triggers a panic. Catching the error might be preferable to prevent a potential chain halt in production scenarios.
385-425: Replace debug printing with structured logging.These print statements can clutter production logs. Switch to a logger or remove them to keep logs consistent and maintainable.
442-454: Revisit the zero reference count panic.Panicking if
historical.ReferenceCountis already zero might be too severe. Evaluate whether returning an error can handle this scenario more gracefully.server/util.go (2)
18-27: Consider using stricter file permissions for trace logs.
Currently, theopenTraceWriterfunction uses0o666, allowing read/write for all. For production environments, consider restricting permissions (e.g.,0o644) to reduce security risk.- 0o666, + 0o644,
29-34: Add usage notes or help text.
AddTestnetCreatorCommandexposes a new CLI command but lacks any descriptive usage messages. Consider clarifying its functionality (e.g., flags and expected arguments) to improve the developer experience.app/upgrades/v022/upgrades_test.go (2)
68-72: Remove or utilize commented-out code.
Lines 68-72 are commented out without explanation. Consider removing them if they are no longer needed, or add a comment explaining why they are temporarily disabled.
88-91: Switch to test logs instead of println for more structured output.
Printing directly to standard output can bury information. Using test logs such ass.T().Logf(ort.Logf) keeps logs organized under the relevant test.- fmt.Println("~~~~~~~~~ POST UPGRADE DEBUG ~~~~~~~~~~~~") - fmt.Printf("delAddr: %v\n", delAddr) - fmt.Printf("valAddr: %v\n", valAddr) + s.T().Logf("~~~~~~~~~ POST UPGRADE DEBUG ~~~~~~~~~~~~") + s.T().Logf("delAddr: %v", delAddr) + s.T().Logf("valAddr: %v", valAddr)cmd/bitsongd/cmd/custom_export.go (5)
1-2: Establish package-level documentation.Consider adding package-level documentation to clearly convey the purpose of this new command package and how it integrates into the overall codebase.
3-20: Consider grouping related imports.Grouping imports by standard library, external libraries, and local packages can improve readability. However, it's a minor style preference.
22-27: Clarify flag naming and usage.The flags such as
FlagForZeroHeight,FlagJailAllowedAddrs, andFlagNewOperatorAddrare meaningful. Brief inline comments or usage clarifications can help maintainers quickly understand these parameters.
136-139: Ensure DB cleanup is handled.Opening the DB is straightforward. Evaluate whether the caller handles closing it in all code paths to prevent resource leakage.
141-150: Add trace file existence checks.If the trace writer file can fail to open or be invalid, consider more robust error and fallback handling to ensure minimal friction during debugging.
cmd/bitsongd/cmd/root.go (2)
270-270: Highlight new testnet command.Registering
AddTestnetCreatorCommandensures better scoping for testnet creation. Confirm that any previously invoked testnet commands or scripts are updated accordingly.
281-281: Add usage docs forcustom-export.Adding
CustomExportCmd(customAppExport, bitsong.DefaultNodeHome)is crucial for debugging. A short mention in the project’s README or command help describing the debugging process will be helpful.app/upgrades/v022/constants.go (5)
9-13: Consider clarifying the usage of patch validator addresses.
Currently,PatchVal1andPatchVal2are defined, but their usage is not immediately clear here. Adding a short comment about when and where they are applied will help future maintainers.
15-19: Add a note explaining emptyStoreUpgrades.
TheUpgradevariable has emptyAddedandDeletedslices forStoreUpgrades. If this is intentional (e.g., no store migrations are needed), it may be helpful to add a comment clarifying.
21-28: Enhance documentation forConditionalJSON.
This struct aggregates multiple upgrade-related data structures. Documenting each field's purpose and how they are used in the upgrade process will greatly improve maintainability.
30-42: Verify JSON tags for distribution slash events.
DistrSlashObject,DistrSlashEvent, andSlashdo not consistently define JSON tags except for a few fields. If these types are serialized and consumed externally, consider adding tags to all fields (or ensuring consistent usage) for clarity.
48-52: Confirm thePatchedDelegationfield naming convention.
The fieldPatchedDelegation string \json:"patch"`` differs from the type name “PatchedDelegation”. Consider renaming the JSON tag to match the field or adding a clarifying comment for consistency.server/start.go (3)
111-207: Evaluate backup mechanism before modifying state inInPlaceTestnetCreator.
This command irreversibly modifies the existing local state after user confirmation. Consider offering an auto-backup functionality (e.g., copying data folder contents before modification) to mitigate accidental data loss.
209-261: Maintain consistency in flag naming and documentation.
A large set of flags is introduced here. While they've been grouped logically, adding short doc comments for potentially confusing flags (e.g.,FlagMempoolMaxTxs,FlagDisableIAVLFastNode) within the code—or referencing relevant docs—would improve dev experience.
872-911: Check for encryption or secure storage of nodeKey.
p2p.LoadOrGenNodeKey(cfg.NodeKeyFile())loads a node key in plaintext form. If operator security requires protecting node identity keys, consider documenting encryption or secure storage at rest.
📜 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 (29)
app/app.go(6 hunks)app/export_custom.go(1 hunks)app/upgrades/v010/constants.go(0 hunks)app/upgrades/v010/upgrades.go(0 hunks)app/upgrades/v011/constants.go(0 hunks)app/upgrades/v011/upgrades.go(0 hunks)app/upgrades/v013/constants.go(0 hunks)app/upgrades/v013/upgrades.go(0 hunks)app/upgrades/v014/constants.go(0 hunks)app/upgrades/v014/upgrades.go(0 hunks)app/upgrades/v015/constants.go(0 hunks)app/upgrades/v015/upgrades.go(0 hunks)app/upgrades/v016/constants.go(0 hunks)app/upgrades/v016/upgrades.go(0 hunks)app/upgrades/v018/constants.go(0 hunks)app/upgrades/v018/upgrades.go(0 hunks)app/upgrades/v020/constants.go(0 hunks)app/upgrades/v020/upgrade_test.go(0 hunks)app/upgrades/v020/upgrades.go(0 hunks)app/upgrades/v021/upgrades.go(3 hunks)app/upgrades/v022/constants.go(1 hunks)app/upgrades/v022/upgrades.go(1 hunks)app/upgrades/v022/upgrades_test.go(1 hunks)cmd/bitsongd/cmd/custom_export.go(1 hunks)cmd/bitsongd/cmd/root.go(4 hunks)go.mod(4 hunks)server/start.go(1 hunks)server/util.go(1 hunks)x/smart-account/ante/ante.go(1 hunks)
💤 Files with no reviewable changes (17)
- app/upgrades/v013/constants.go
- app/upgrades/v016/upgrades.go
- app/upgrades/v011/constants.go
- app/upgrades/v018/constants.go
- app/upgrades/v015/upgrades.go
- app/upgrades/v020/constants.go
- app/upgrades/v014/constants.go
- app/upgrades/v015/constants.go
- app/upgrades/v020/upgrade_test.go
- app/upgrades/v016/constants.go
- app/upgrades/v011/upgrades.go
- app/upgrades/v014/upgrades.go
- app/upgrades/v010/constants.go
- app/upgrades/v018/upgrades.go
- app/upgrades/v020/upgrades.go
- app/upgrades/v010/upgrades.go
- app/upgrades/v013/upgrades.go
🔇 Additional comments (27)
app/upgrades/v021/upgrades.go (6)
17-18: No issues found.
These new imports from the distribution and staking modules appear correct and properly used.
75-75: Confirm usage.
The call toCustomCalculateDelegationRewardsis a new injection point for reward calculus. Please ensure all relevant invocation paths have been updated or verified to leverage this new function correctly.
83-83: Reapplication of reward-patch logic approved.
ReusingV018ManualDelegationRewardsPatchhere appears consistent with the approach of re-patching delegations as needed.
267-316: Verify the 5% margin assumption.
At line 305, a 5% margin of error is hardcoded for stake reconciliation. Please confirm that this domain-specific threshold is correct and consider exposing it as a configurable parameter if it may vary in different deployments.
318-341: Reward calculation logic looks solid.
Truncating the ratio difference prevents accidental overpayment or negative rewards. The approach seems consistent with Cosmos SDK standards.
343-356: Reference count logic validated.
The approach of decrementing reference counts and pruning historical rewards on zero references aligns with Cosmos distribution design. No issues found here.x/smart-account/ante/ante.go (1)
288-293: Remove or reintroduce the commented feegrant logic.Currently, the error message references an "active feegrant allowance," but the logic for retrieving and checking that allowance is commented out, effectively disallowing fee grants. This may break expected fee grant functionality. If you truly intend to remove fee grant support, consider removing the commented lines to avoid confusion. Otherwise, reintroduce the feegrant checks for correctness.
go.mod (6)
19-19: Bump CometBFT from v0.38.16 to v0.38.17 is consistent with the PR objective.No issues found. This aligns with addressing security vulnerabilities present in earlier CometBFT versions.
35-35: Confirm necessity of new dependency.Before finalizing, ensure that "github.com/hashicorp/go-metrics v0.5.4" is indeed required, as adding new dependencies can bloat the go.mod if it's not used.
37-37: Prometheus client update from v1.20.5 to v1.21.0.Looks fine. Minor version bump typically includes bug fixes and performance improvements.
40-40: Cobra upgrade to v1.9.1.This is also a minor version bump, usually containing new features & bug fixes.
45-45: Added 'golang.org/x/sync v0.11.0' dependency.It’s generally beneficial. Confirm that it is actually in use.
98-98: Secp256k1 upgrade from v4.3.0 to v4.4.0.This ensures you have the latest improvements in cryptographic libraries.
app/upgrades/v022/upgrades.go (2)
427-440: Validate reference count increment logic.The code panics if
ReferenceCountexceeds 2. Confirm this limit aligns with your design constraints, or consider a safe error return if it must remain below 2.
456-487: Initialization logic for new delegations.Ensure that the reference count increments do not create orphan states if the process is interrupted. Additional test coverage is recommended to verify atomicity.
app/app.go (6)
64-64: Check alias consistency forcmtproto.Renaming the Tendermint proto alias to
cmtprotoreflects the shift to CometBFT. Ensure all references totmprotothroughout the code are replaced to maintain consistency and avoid potential import conflicts.
73-75: Confirm older upgrades are no longer needed.Ensure you're not skipping critical upgrade logic from older versions. If the chain never ran versions v010-v020 or if these are fully deprecated, removing them is fine; otherwise, consider retaining them for node operators upgrading from earlier versions.
95-98: Validate removal of older upgrade references.Commenting out older upgrades may cause issues for node operators on versions preceding v021. Confirm that the chain will not require the older upgrade pathways.
394-394: ReviewNewUncachedContextusage.Using
NewUncachedContext(true, cmtproto.Header{})indicates a context with minimal overhead. Ensure that any essential block header fields (e.g., ChainID, Time) do not need setting in this scenario.
657-657: Establish block header details for testnet context.Similarly, when initializing the testnet context with
cmtproto.Header{}, ensure that necessary header info (e.g., block time) is properly handled or is intentionally omitted in a testing environment.
50-50:Details
✅ Verification successful
Confirm usage of new crypto package.
Replacing
"cosmossdk.io/api/cosmos/crypto/ed25519"with"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"can help ensure consistency across modules. Verify that any downstream references to ed25519 are updated accordingly.Run the following script to confirm there are no stale references to the old
cosmossdk.io/api/cosmos/crypto/ed25519import:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'import "cosmossdk.io/api/cosmos/crypto/ed25519"'Length of output: 70
Crypto Package Update Verification Needed
- The file
app/app.gonow imports"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519".- Please verify that no lingering references to the old package (
"cosmossdk.io/api/cosmos/crypto/ed25519") exist anywhere in the codebase.- You can run the above command to search the repository. If it returns no matches, then the new crypto package is being used consistently and any downstream references to ed25519 have been updated accordingly.
🏁 Script executed:
#!/bin/bash rg 'cosmossdk.io/api/cosmos/crypto/ed25519' -n .Length of output: 170
Crypto Package Update Confirmed
- The updated import in
app/app.gonow correctly uses"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519".- A repository-wide search found no references to
"cosmossdk.io/api/cosmos/crypto/ed25519", confirming that all downstream references have been updated accordingly.cmd/bitsongd/cmd/custom_export.go (1)
29-135: Validate the custom export logic.The
CustomExportCmdfunction is well-structured for debugging local upgrades. Double-check that:
- All flags are validated before operating on them.
- No CLI usage collisions exist with existing commands.
cmd/bitsongd/cmd/root.go (2)
37-37: Confirm separate testnet server usage.Introducing
testnetserver.AddTestnetCreatorCommandclarifies the testnet creation path. Ensure that original server creation logic is unaffected by this addition.
461-507: Review extended export mechanism.The
customAppExportfunction introduces custom logic for exporting state. Verify that only pinned differences from the standardappExportlogic are introduced and no extraneous changes have been made.app/upgrades/v022/constants.go (1)
21-56:Details
✅ Verification successful
Validate usage of newly defined structs.
These new types (ConditionalJSON,DistrSlashObject,DistrSlashEvent,Slash,ZeroSharesDelegation,PatchedDelegation, andNilDelegationCalculation) appear foundational for custom logic in the v022 upgrade, but no direct usage is shown here.Please run the following script to locate all usage references and confirm they are properly tested:
🏁 Script executed:
#!/bin/bash # Description: Search the codebase for references to newly introduced types in `v022/constants.go`. rg -A 4 "ConditionalJSON" rg -A 4 "DistrSlashObject" rg -A 4 "DistrSlashEvent" rg -A 4 "Slash" rg -A 4 "ZeroSharesDelegation" rg -A 4 "PatchedDelegation" rg -A 4 "NilDelegationCalculation"Length of output: 92292
Usage Verified – No Changes Required
The newly defined types inapp/upgrades/v022/constants.go(namely,ConditionalJSON,DistrSlashObject,DistrSlashEvent,Slash,ZeroSharesDelegation,PatchedDelegation, andNilDelegationCalculation) are actively utilized in the codebase. In particular:
ConditionalJSONis instantiated in bothapp/export_custom.goandapp/upgrades/v022/upgrades.go.DistrSlashObjectandDistrSlashEventare referenced in the same files, confirming their role in the custom logic.- The other types (
ZeroSharesDelegation,PatchedDelegation, andNilDelegationCalculation) are also employed as part of the data structures used byConditionalJSON.Given these references, the intended usage for the v022 upgrade has been validated. No further modifications to the new structs appear necessary, though please ensure that relevant tests are kept up-to-date.
server/start.go (2)
275-298: Validate fallback when CometBFT is disabled.
The code defaults tostartStandAloneifwithCmtis false. Ensure all downstream components (telemetry, in-process calls, etc.) gracefully handle this mode and that they are tested to avoid unexpected corner cases.
817-870: Confirm robust error handling when running in “gRPC only” mode.
InstartInProcess, ifgRPCOnlyis true, CometBFT is skipped, and an in-process node is never started. Verify that subsequent calls (like the ones that register node services) do not cause errors or no-ops in this scenario.
…gradeHandler logic to in-place-testnet
V022 custom testnet
dev: access appKeepers from top level app
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (4)
x/fantoken/module.go (1)
181-187: 🛠️ Refactor suggestionDuplicate simulation initialization methods
The file contains both the new
GenerateGenesisState(lines 125-128) and the existingSimulatorGenesisState(lines 181-187) methods, which have overlapping functionality. Additionally, there are commented-out function definitions at the end (lines 189-193) that should be removed.-// GenerateGenesisState creates a randomized GenState of the valset module. -func (am AppModule) SimulatorGenesisState(simState *module.SimulationState) { - ftDefaultGen := types.DefaultGenesisState() - ftDefaultGen.Params.IssueFee = sdk.NewCoin("stake", math.NewInt(10000000)) - ftDefaultGenJson := simState.Cdc.MustMarshalJSON(ftDefaultGen) - simState.GenState[types.ModuleName] = ftDefaultGenJson -} - -// // GenerateGenesisState creates a randomized GenState of the module. -// func (a AppModule) GenerateGenesisState(_ *module.SimulationState) {} - -// // RegisterStoreDecoder registers a decoder for module simulation testing -// func (am AppModule) RegisterStoreDecoder(sdr simtypes.StoreDecoderRegistry) {}Instead, update the new GenerateGenesisState implementation to incorporate the stake fee setup:
// GenerateGenesisState creates a randomized GenState of the fantoken module. func (AppModule) GenerateGenesisState(simState *module.SimulationState) { ftDefaultGen := types.DefaultGenesisState() ftDefaultGen.Params.IssueFee = sdk.NewCoin("stake", math.NewInt(10000000)) ftDefaultGenJson := simState.Cdc.MustMarshalJSON(ftDefaultGen) simState.GenState[types.ModuleName] = ftDefaultGenJson }Also applies to: 189-193
x/cadence/keeper/querier.go (1)
24-33: 🛠️ Refactor suggestionAdd defensive
nilcheck for the request object
CadenceContractswill panic ifreqisnil(e.g. a malformed gRPC caller).
Most Cosmos-SDK query handlers guard against this with:if req == nil { return nil, status.Error(codes.InvalidArgument, "request cannot be nil") }Adding the check keeps the node process from crashing on malformed traffic.
Same applies to empty‐pagination defaults ifreq.Pagination == nil.x/cadence/keeper/cadance.go (1)
1-233: 🛠️ Refactor suggestionRename file to match module name.
The file is still named "cadance.go" while all references inside have been correctly updated to "cadence". The filename should also be updated to "cadence.go" for consistency.
app/app.go (1)
260-273:⚠️ Potential issue
GetKVStoreKeydoes not exist – build will fail
app.keys = app.GetKVStoreKey()attempts to call an undefined method and will not compile.Suggested quick fix – reuse the map already created inside
keepers.NewAppKeepers(it is returned byapp.GetKVStoreKeyMap()in upstream sdk) or expose a helper:- app.keys = app.GetKVStoreKey() + app.keys = app.AppKeepers.GetKVStoreKeyMap() // or simply keep a reference returned by NewAppKeepersIf
NewAppKeepersalready returns the kv-store key map, store it there and drop this call entirely.🧰 Tools
🪛 golangci-lint (1.64.8)
272-272: app.GetKVStoreKey undefined (type *BitsongApp has no field or method GetKVStoreKey)
(typecheck)
♻️ Duplicate comments (4)
app/upgrades/v022/upgrades_test.go (2)
108-111: Use test assertions instead of panics.In testing contexts, panics terminate the entire test suite. It is often more informative to use
s.Require().NoError(err)or similar test assertions to allow the test framework to handle errors gracefully.
134-137: Use test assertions instead of panics.In testing contexts, panics terminate the entire test suite. It is often more informative to use
s.Require().NoError(err)or similar test assertions to allow the test framework to handle errors gracefully.app/upgrades/v021/upgrades.go (1)
360-365: Panic on reference-count overflow still presentA previous review (see past comments) already highlighted that panicking here will crash the whole node.
Returning an error or at least logging a critical message would lead to safer recovery in production.app/upgrades/v022/upgrades.go (1)
454-466: Repeated panic on reference-count overflowSame concern as raised for v021: panicking inside
customIncrementReferenceCountwill crash the node if the invariant is violated.
Consider returning the error upwards or at least logging + continuing to allow the upgrade to complete.
🧹 Nitpick comments (28)
x/cadence/spec/02_state.md (1)
20-20: Grammar improvement suggestionConsider adding a comma in the following sentence for better readability:
-The `x/cadence` module's `GenesisState` defines the state necessary for initializing the chain from a previously exported height. It simply contains the gas limit parameter which is used to determine the maximum amount of gas that can be used by a contract. This value can be modified with a governance proposal. +The `x/cadence` module's `GenesisState` defines the state necessary for initializing the chain from a previously exported height. It simply contains the gas limit parameter, which is used to determine the maximum amount of gas that can be used by a contract. This value can be modified with a governance proposal.🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...eight. It simply contains the gas limit parameter which is used to determine the maximum ...(AI_HYDRA_LEO_MISSING_COMMA)
x/cadence/spec/03_integration.md (1)
32-32: Minor language improvement opportunity.Consider rewording "where all of the contract's custom end block logic" to "where all the contract's custom end block logic" to be more concise.
-At the end of every block, registered contracts will execute the `CadenceEndBlock` Sudo message. This is where all of the contract's custom end block logic can be performed. Please keep in mind that contracts which exceed the gas limit specified in the params will be jailed. +At the end of every block, registered contracts will execute the `CadenceEndBlock` Sudo message. This is where all the contract's custom end block logic can be performed. Please keep in mind that contracts which exceed the gas limit specified in the params will be jailed.🧰 Tools
🪛 LanguageTool
[style] ~32-~32: Consider removing “of” to be more concise
Context: ...ceEndBlock` Sudo message. This is where all of the contract's custom end block logic can b...(ALL_OF_THE)
x/cadence/client/cli/query.go (1)
31-31: Fix spacing in CLI command description
The string reads “contract s” due to an extra space. It should be “contracts”.x/fantoken/module.go (2)
125-128: Incorrect module reference in commentThe comment refers to the "epochs" module instead of "fantoken" module, which appears to be a copy-paste error.
-// GenerateGenesisState creates a randomized GenState of the epochs module. +// GenerateGenesisState creates a randomized GenState of the fantoken module.
135-138: Empty weighted operations implementationThe WeightedOperations function returns nil, indicating no simulation operations are defined yet. Consider implementing weighted operations for more comprehensive simulation testing.
x/cadence/keeper/querier.go (1)
24-24: Minor wording nit“ContractModules returns contract addresses which are using the cadence”
reads awkwardly. Consider “that use the cadence module” for clarity.x/smart-account/ante/ante_test.go (1)
163-178: Ensure authenticator IDs are deterministic across testsA handful of tests now rely on
id == 1/2. If any new tests add
authenticators earlier in the suite, these fixed expectations will break.
Consider asserting> 0or performing the add/remove inSetupTest()to
guarantee ordering, keeping tests independent.app/keepers/keepers.go (1)
230-240:WithUnorderedTransactions(false)disables tx-index randomisationExplicitly opting-out is fine, just make sure it matches consensus across
validators running older binaries that might rely on the default (trueat
v0.53). A coordinated upgrade note is advised.x/cadence/keeper/msg_server.go (3)
28-30: Minor doc‐comment typo (“contract s.”) – please tidy up for godoc correctness
There is an extra space and trailing “s” in “cadence contract s.” which leaks into generated docs (go vet’sdoccheck will warn).-// RegisterCadenceContract handles incoming transactions to register cadence contract s. +// RegisterCadenceContract handles incoming transactions to register cadence contracts.
37-38: Return value is always allocated even when keeper returns an error – verify semantic intent
RegisterCadenceContract(and the two functions below) build a non-nil response and return it together with an error from the keeper.
Most Cosmos-SDK handlers returnnil, erron failure. Returning a non-nil proto here whenerr != nilmight surprise callers or produce misleading ABCI events.If the keeper error should abort the tx completely consider:
- return &types.MsgRegisterCadenceContractResponse{}, k.RegisterContract(...) + if err := k.RegisterContract(ctx, req.SenderAddress, req.ContractAddress); err != nil { + return nil, err + } + return &types.MsgRegisterCadenceContractResponse{}, nil
52-54: Same doc typo as above – also fix forUnjailCadenceContract.x/cadence/keeper/keeper_test.go (1)
24-27: Unused test fields may trip linters
bk,wkandqueryClientare set but never referenced inside this file. Static analysis (go vet,golangci-lint) will flag them as dead code. If you don’t need them, please drop the struct fields/initialisation or add TODOs explaining forthcoming usage.x/cadence/keeper/msg_server_test.go (1)
103-106: State clean-up inside the loop risks masking errors
RemoveContractis called unconditionally at the end of every sub-test, even when registration/unregistration failed earlier. IfRemoveContractitself returns an error (e.g. “not found”) it will be silently ignored, possibly hiding real issues.Consider asserting the error or moving clean-up into
t.Cleanup(...)blocks so failures are surfaced:s.T().Cleanup(func() { s.Require().NoError(s.App.CadenceKeeper.RemoveContract(s.Ctx, contractAddress)) s.Require().NoError(s.App.CadenceKeeper.RemoveContract(s.Ctx, contractAddressWithAdmin)) })app/export_custom.go (3)
69-77: Usestruct{}{}instead ofboolfor set-like map – saves memory & communicates intent-allowedAddrsMap := make(map[string]bool) +allowedAddrsMap := make(map[string]struct{}) ... -allowedAddrsMap[addr] = true +allowedAddrsMap[addr] = struct{}{}A tiny improvement, but it’s idiomatic Go for “membership” maps.
210-228: Iterator must be closed viadeferto avoid resource leakage
KVStoreReversePrefixIteratorshould be closed whether or not a panic happens. Placedefer iter.Close()immediately after it’s created.iter := storetypes.KVStoreReversePrefixIterator(store, stakingtypes.ValidatorsKey) +defer func() { + if err := iter.Close(); err != nil { + app.Logger().Error("iterator close error", "err", err) + } +}()This guarantees the underlying iterator resources are released even on early returns.
241-247: Shadowed loop variableerrmay hide errors
Inside several iterator callbacks you assign to the outererrvariable; however, if an earlier iteration setserr, later successful iterations may overwrite it and you won’t detect the failure. Capture/return the error explicitly or break on first failure.cmd/bitsongd/cmd/custom_export.go (1)
22-27:FlagForZeroHeight&FlagNewOperatorAddrare dead codeBoth constants are defined but never registered or referenced, which is noise for maintainers and triggers
go vet’sunusedlinter.- FlagForZeroHeight = "for-zero-height" - FlagNewOperatorAddr = "modules-to-export" + // (Removed – not in use)cmd/bitsongd/cmd/root.go (1)
472-516:customAppExportduplicatesappExport– DRY violationThe two functions are ~90 % identical, differing only in the final
Export…call. A small higher-order helper eliminates duplication and future drift:-func customAppExport(...){ /* 90 lines */ } +func makeExporter( + exportFn func(*bitsong.BitsongApp, bool, []string) (servertypes.ExportedApp, error), +) servertypes.AppExporter { + return func(logger log.Logger, db cosmosdb.DB, traceStore io.Writer, height int64, + _ bool, jailAllowedAddrs []string, appOpts servertypes.AppOptions, + _ []string, + ) (servertypes.ExportedApp, error) { + wasmApp, err := buildBitsongApp(logger, db, traceStore, height, appOpts) + if err != nil { return servertypes.ExportedApp{}, err } + return exportFn(wasmApp, true, jailAllowedAddrs) + } +} + +var customAppExport = makeExporter((*bitsong.BitsongApp).CustomExportAppStateAndValidators)Besides readability, this prevents subtle discrepancies (e.g. flag overrides) between the two exporters.
app/upgrades/v022/upgrades_test.go (4)
85-87: Remove debug print statements before production.These debug print statements should be removed before merging to production as they will pollute logs when tests are run.
- fmt.Println("~~~~~~~~~ POST UPGRADE DEBUG ~~~~~~~~~~~~") - fmt.Printf("delAddr: %v\n", delAddr) - fmt.Printf("valAddr: %v\n", valAddr)
179-179: Remove debug print statement.This debug print statement should be removed before production.
- fmt.Printf("err: %v\n", err)
207-208: Remove debug print statement.This debug print statement should be removed before production.
- fmt.Printf("val.OperatorAddress: %v\n", val.OperatorAddress)
216-216: Remove debug print statement.This debug print statement should be removed before production.
- fmt.Printf("controlValAddr: %v\n", controlValAddr)proto/bitsong/cadence/v1/query.proto (1)
63-63: Correct the comment for QueryParamsResponse.The comment incorrectly describes this message as "QueryCadenceContractsResponse" when it should be "QueryParamsResponse".
-// QueryCadenceContractsResponse is the response type for the Query/CadenceContracts RPC method. +// QueryParamsResponse is the response type for the Query/Params RPC method.app/upgrades/v021/upgrades.go (1)
300-304: Avoidfmt.Printfin production upgrade logic – use the app logger instead
fmt.Printfwrites directly tostdout, bypassing node log configuration, log levels and structured fields.
PrefersdkCtx.Logger().Info/Debug(orloggerdeclared at line 28) so operators can filter/redirect output and so the messages are tagged with block-height and module context.- fmt.Printf("del: %q", del.GetDelegatorAddr()) - fmt.Printf("val: %q", del.GetValidatorAddr()) - fmt.Printf("stake: %q", stake) - fmt.Printf("currentStake: %q", currentStake) + logger := sdkCtx.Logger().With( + "delegator", del.GetDelegatorAddr(), + "validator", del.GetValidatorAddr(), + ) + logger.Debug("delegation stake delta", + "stake_from_distribution", stake, + "current_stake", currentStake, + )app/upgrades/v022/upgrades.go (1)
206-219: Error from debug-file writing is swallowed
PrintConditionalJsonLogsignores (and only prints) errors fromjson.MarshalIndentandos.WriteFile.
During live-net upgrades it is safer to surface a failure so operators know the node did not write the audit file, instead of silently continuing.- if err != nil { - fmt.Printf("Failed to write debugging log, continuing with upgrade... %s\n", fileName) - } + if err != nil { + return fmt.Errorf("unable to write %s: %w", fileName, err) + }and bubble the error up to the caller; the upgrade handler can decide whether to abort or just log.
server/start.go (2)
558-566: Error message references the wrong keyThe validation for
brokenValidatorserrors withfmt.Errorf("expected comma-separated list of validators to patch info for %s", KeyNewChainID)but the missing value belongs to
KeyBrokenValidator, notKeyNewChainID.-return nil, fmt.Errorf("expected comma-separated list of validators to patch info for %s", KeyNewChainID) +return nil, fmt.Errorf("expected comma-separated list of validators to patch info for %s", KeyBrokenValidator)
828-841:ParseValidatorInfosis unused – dead code?The helper is not referenced anywhere in this file (or the rest of the PR).
If you plan to process--new-vals-json, wire this function up; otherwise delete it to keep the surface minimal.Dead code increases cognitive load and can drift out-of-sync with expectations.
go.mod (1)
246-254: Debug-onlyreplaceinstructions should not live ingo.modLines 246-254 are commented hints for cloning local forks and using
replacedirectives. While helpful for development, shipping them ingo.modencourages accidental uncommenting and pollutes the canonical build spec.Please move these notes to
DEVELOPMENT.md(or a similar doc) and keepgo.modstrictly machine-consumable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
go.sumis excluded by!**/*.sumtests/ict/go.sumis excluded by!**/*.sumx/cadence/keeper/testdata/clock_example.wasmis excluded by!**/*.wasmx/cadence/keeper/testdata/cw_testburn.wasmis excluded by!**/*.wasmx/cadence/types/cadence.pb.gois excluded by!**/*.pb.gox/cadence/types/genesis.pb.gois excluded by!**/*.pb.gox/cadence/types/query.pb.gois excluded by!**/*.pb.gox/cadence/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/cadence/types/tx.pb.gois excluded by!**/*.pb.gox/cadence/types/tx.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (97)
Dockerfile(1 hunks)Makefile(2 hunks)app/app.go(25 hunks)app/export.go(7 hunks)app/export_custom.go(1 hunks)app/helpers/mock.go(1 hunks)app/keepers/keepers.go(7 hunks)app/keepers/keys.go(2 hunks)app/modules.go(6 hunks)app/posthandler.go(0 hunks)app/test_helper.go(1 hunks)app/testing/test_suite.go(1 hunks)app/upgrades/v010/constants.go(0 hunks)app/upgrades/v010/upgrades.go(0 hunks)app/upgrades/v011/constants.go(0 hunks)app/upgrades/v011/upgrades.go(0 hunks)app/upgrades/v013/constants.go(0 hunks)app/upgrades/v013/upgrades.go(0 hunks)app/upgrades/v014/constants.go(0 hunks)app/upgrades/v014/upgrades.go(0 hunks)app/upgrades/v015/constants.go(0 hunks)app/upgrades/v015/upgrades.go(0 hunks)app/upgrades/v016/constants.go(0 hunks)app/upgrades/v016/upgrades.go(0 hunks)app/upgrades/v018/constants.go(0 hunks)app/upgrades/v018/upgrades.go(0 hunks)app/upgrades/v020/constants.go(0 hunks)app/upgrades/v020/upgrade_test.go(0 hunks)app/upgrades/v020/upgrades.go(0 hunks)app/upgrades/v021/constants.go(2 hunks)app/upgrades/v021/upgrade_test.go(7 hunks)app/upgrades/v021/upgrades.go(4 hunks)app/upgrades/v022/constants.go(1 hunks)app/upgrades/v022/upgrades.go(1 hunks)app/upgrades/v022/upgrades_test.go(1 hunks)cmd/bitsongd/cmd/custom_export.go(1 hunks)cmd/bitsongd/cmd/genesis.go(2 hunks)cmd/bitsongd/cmd/root.go(6 hunks)go.mod(4 hunks)proto/README.md(1 hunks)proto/bitsong/cadance/v1/query.proto(0 hunks)proto/bitsong/cadance/v1/tx.proto(0 hunks)proto/bitsong/cadence/v1/cadence.proto(1 hunks)proto/bitsong/cadence/v1/genesis.proto(1 hunks)proto/bitsong/cadence/v1/query.proto(1 hunks)proto/bitsong/cadence/v1/tx.proto(1 hunks)server/start.go(1 hunks)server/types.go(1 hunks)server/util.go(1 hunks)tests/ict/go.mod(10 hunks)x/README.md(1 hunks)x/cadence/README.md(1 hunks)x/cadence/abci.go(2 hunks)x/cadence/abci_test.go(15 hunks)x/cadence/client/cli/query.go(4 hunks)x/cadence/client/cli/tx.go(7 hunks)x/cadence/genesis.go(1 hunks)x/cadence/genesis_test.go(2 hunks)x/cadence/keeper/cadance.go(10 hunks)x/cadence/keeper/keeper.go(4 hunks)x/cadence/keeper/keeper_test.go(6 hunks)x/cadence/keeper/msg_server.go(2 hunks)x/cadence/keeper/msg_server_test.go(8 hunks)x/cadence/keeper/querier.go(3 hunks)x/cadence/keeper/querier_test.go(6 hunks)x/cadence/module.go(2 hunks)x/cadence/module_test.go(1 hunks)x/cadence/spec/01_concepts.md(3 hunks)x/cadence/spec/02_state.md(3 hunks)x/cadence/spec/03_integration.md(5 hunks)x/cadence/spec/README.md(1 hunks)x/cadence/types/codec.go(1 hunks)x/cadence/types/codec_test.go(1 hunks)x/cadence/types/msgs.go(1 hunks)x/cadence/types/params_test.go(1 hunks)x/fantoken/handler_test.go(4 hunks)x/fantoken/keeper/fees.go(4 hunks)x/fantoken/keeper/keeper.go(4 hunks)x/fantoken/keeper/keeper_test.go(1 hunks)x/fantoken/module.go(2 hunks)x/fantoken/simulation/genesis.go(1 hunks)x/fantoken/types/expected_keepers.go(1 hunks)x/smart-account/ante/ante.go(1 hunks)x/smart-account/ante/ante_test.go(14 hunks)x/smart-account/ante/circuit_breaker_test.go(1 hunks)x/smart-account/ante/pubkey_test.go(2 hunks)x/smart-account/authenticator/base_test.go(3 hunks)x/smart-account/authenticator/composition_test.go(4 hunks)x/smart-account/authenticator/cosmwasm_test.go(9 hunks)x/smart-account/authenticator/message_filter_test.go(2 hunks)x/smart-account/authenticator/signature_authenticator_test.go(2 hunks)x/smart-account/authenticator/spend_limits_test.go(4 hunks)x/smart-account/integration_test.go(24 hunks)x/smart-account/keeper/genesis_test.go(6 hunks)x/smart-account/keeper/keeper_test.go(8 hunks)x/smart-account/keeper/msg_server_test.go(7 hunks)x/smart-account/post/post_test.go(4 hunks)
💤 Files with no reviewable changes (20)
- app/posthandler.go
- app/upgrades/v013/constants.go
- app/upgrades/v018/constants.go
- app/upgrades/v011/constants.go
- app/upgrades/v016/constants.go
- app/upgrades/v015/upgrades.go
- app/upgrades/v015/constants.go
- app/upgrades/v016/upgrades.go
- app/upgrades/v020/constants.go
- app/upgrades/v014/upgrades.go
- app/upgrades/v014/constants.go
- app/upgrades/v010/constants.go
- app/upgrades/v013/upgrades.go
- app/upgrades/v010/upgrades.go
- proto/bitsong/cadance/v1/query.proto
- app/upgrades/v011/upgrades.go
- app/upgrades/v018/upgrades.go
- app/upgrades/v020/upgrade_test.go
- proto/bitsong/cadance/v1/tx.proto
- app/upgrades/v020/upgrades.go
🧰 Additional context used
🧬 Code Graph Analysis (21)
x/smart-account/ante/circuit_breaker_test.go (2)
app/app.go (1)
BitsongApp(172-193)app/upgrades/v021/constants.go (1)
IsSmartAccountActive(18-18)
x/cadence/module.go (1)
x/cadence/types/keys.go (1)
ModuleName(11-11)
x/smart-account/authenticator/signature_authenticator_test.go (2)
app/app.go (1)
BitsongApp(172-193)x/fantoken/types/expected_keepers.go (1)
AccountKeeper(22-24)
x/smart-account/post/post_test.go (2)
app/app.go (1)
BitsongApp(172-193)x/fantoken/types/expected_keepers.go (1)
AccountKeeper(22-24)
x/fantoken/handler_test.go (1)
x/fantoken/handler.go (1)
NewProposalHandler(56-66)
x/cadence/abci_test.go (4)
x/fantoken/types/expected_keepers.go (1)
BankKeeper(27-37)x/cadence/module.go (1)
ModuleName(27-27)x/cadence/types/keys.go (1)
ModuleName(11-11)x/cadence/abci.go (1)
EndBlocker(21-66)
x/cadence/client/cli/query.go (2)
x/cadence/types/query.pb.go (6)
QueryCadenceContracts(35-38)QueryCadenceContracts(42-42)QueryCadenceContracts(43-45)QueryCadenceContract(136-139)QueryCadenceContract(143-143)QueryCadenceContract(144-146)x/cadence/types/cadence.pb.go (3)
CadenceContract(27-32)CadenceContract(36-36)CadenceContract(37-39)
x/cadence/types/codec.go (1)
x/cadence/types/tx.pb.go (12)
MsgRegisterCadenceContract(36-41)MsgRegisterCadenceContract(45-45)MsgRegisterCadenceContract(46-48)MsgUnregisterCadenceContract(129-134)MsgUnregisterCadenceContract(138-138)MsgUnregisterCadenceContract(139-141)MsgUnjailCadenceContract(222-227)MsgUnjailCadenceContract(231-231)MsgUnjailCadenceContract(232-234)MsgUpdateParams(317-324)MsgUpdateParams(328-328)MsgUpdateParams(329-331)
x/fantoken/keeper/keeper_test.go (1)
x/fantoken/types/expected_keepers.go (1)
BankKeeper(27-37)
x/cadence/client/cli/tx.go (1)
x/cadence/types/tx.pb.go (9)
MsgRegisterCadenceContract(36-41)MsgRegisterCadenceContract(45-45)MsgRegisterCadenceContract(46-48)MsgUnregisterCadenceContract(129-134)MsgUnregisterCadenceContract(138-138)MsgUnregisterCadenceContract(139-141)MsgUnjailCadenceContract(222-227)MsgUnjailCadenceContract(231-231)MsgUnjailCadenceContract(232-234)
x/smart-account/authenticator/message_filter_test.go (2)
app/app.go (1)
BitsongApp(172-193)x/fantoken/types/expected_keepers.go (1)
AccountKeeper(22-24)
x/smart-account/authenticator/composition_test.go (2)
app/app.go (1)
BitsongApp(172-193)x/fantoken/types/expected_keepers.go (1)
AccountKeeper(22-24)
x/fantoken/keeper/keeper.go (1)
x/fantoken/types/expected_keepers.go (2)
ProtocolPoolKeeper(10-12)ParamSubspace(15-20)
x/fantoken/simulation/genesis.go (4)
x/fantoken/types/fantoken.pb.go (6)
FanToken(74-81)FanToken(84-84)FanToken(85-87)Metadata(28-38)Metadata(42-42)Metadata(43-45)app/genesis.go (1)
GenesisState(17-17)x/fantoken/types/genesis.pb.go (3)
GenesisState(28-31)GenesisState(35-35)GenesisState(36-38)x/fantoken/types/keys.go (1)
ModuleName(9-9)
x/cadence/keeper/querier.go (3)
x/cadence/types/query.pb.go (12)
QueryCadenceContracts(35-38)QueryCadenceContracts(42-42)QueryCadenceContracts(43-45)QueryCadenceContractsResponse(81-86)QueryCadenceContractsResponse(90-90)QueryCadenceContractsResponse(91-93)QueryCadenceContract(136-139)QueryCadenceContract(143-143)QueryCadenceContract(144-146)QueryCadenceContractResponse(182-185)QueryCadenceContractResponse(189-189)QueryCadenceContractResponse(190-192)x/cadence/types/cadence.pb.go (3)
CadenceContract(27-32)CadenceContract(36-36)CadenceContract(37-39)x/cadence/types/keys.go (1)
ErrInvalidAddress(23-23)
x/cadence/types/msgs.go (3)
x/cadence/types/tx.pb.go (12)
MsgRegisterCadenceContract(36-41)MsgRegisterCadenceContract(45-45)MsgRegisterCadenceContract(46-48)MsgUnregisterCadenceContract(129-134)MsgUnregisterCadenceContract(138-138)MsgUnregisterCadenceContract(139-141)MsgUnjailCadenceContract(222-227)MsgUnjailCadenceContract(231-231)MsgUnjailCadenceContract(232-234)MsgUpdateParams(317-324)MsgUpdateParams(328-328)MsgUpdateParams(329-331)x/cadence/types/keys.go (1)
RouterKey(13-13)x/cadence/types/codec.go (1)
AminoCdc(13-13)
app/keepers/keepers.go (3)
x/fantoken/keeper/keeper.go (2)
Keeper(17-26)NewKeeper(28-54)x/fantoken/types/expected_keepers.go (3)
ProtocolPoolKeeper(10-12)AccountKeeper(22-24)BankKeeper(27-37)app/app.go (1)
Bech32Prefix(87-87)
x/cadence/keeper/msg_server.go (2)
x/cadence/keeper/keeper.go (1)
Keeper(18-26)x/cadence/types/tx.pb.go (19)
MsgServer(538-553)MsgRegisterCadenceContract(36-41)MsgRegisterCadenceContract(45-45)MsgRegisterCadenceContract(46-48)MsgRegisterCadenceContractResponse(92-93)MsgRegisterCadenceContractResponse(97-97)MsgRegisterCadenceContractResponse(98-100)MsgUnregisterCadenceContract(129-134)MsgUnregisterCadenceContract(138-138)MsgUnregisterCadenceContract(139-141)MsgUnregisterCadenceContractResponse(185-186)MsgUnregisterCadenceContractResponse(190-190)MsgUnregisterCadenceContractResponse(191-193)MsgUnjailCadenceContract(222-227)MsgUnjailCadenceContract(231-231)MsgUnjailCadenceContract(232-234)MsgUnjailCadenceContractResponse(278-279)MsgUnjailCadenceContractResponse(283-283)MsgUnjailCadenceContractResponse(284-286)
cmd/bitsongd/cmd/custom_export.go (1)
server/start.go (1)
KeyIsTestnet(104-104)
app/upgrades/v021/upgrades.go (3)
app/upgrades/v022/upgrades.go (1)
CustomCalculateDelegationRewards(221-290)app/keepers/keepers.go (1)
AppKeepers(121-167)x/fantoken/types/expected_keepers.go (1)
BankKeeper(27-37)
server/start.go (2)
app/config.go (1)
DefaultConfig(21-46)server/types.go (1)
ValidatorInfo(3-8)
🪛 LanguageTool
x/cadence/spec/02_state.md
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...eight. It simply contains the gas limit parameter which is used to determine the maximum ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~47-~47: Did you mean “there is”?
Context: ... in state. - Jailing a contract updates the is_jailed field of a CadenceContract objec...
(THE_THERE)
[uncategorized] ~48-~48: Did you mean “there is”?
Context: ...n state. - Unjailing a contract updates the is_jailed field of a CadenceContract objec...
(THE_THERE)
[uncategorized] ~49-~49: You might be missing the article “the” here.
Context: ...t deletes a CadenceContract object from state.
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
x/cadence/spec/03_integration.md
[style] ~32-~32: Consider removing “of” to be more concise
Context: ...ceEndBlock` Sudo message. This is where all of the contract's custom end block logic can b...
(ALL_OF_THE)
🪛 Buf (1.47.2)
proto/bitsong/cadence/v1/genesis.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/cadence/v1/tx.proto
6-6: import "google/api/annotations.proto": file does not exist
(COMPILE)
proto/bitsong/cadence/v1/query.proto
4-4: import "cosmos/base/query/v1beta1/pagination.proto": file does not exist
(COMPILE)
🪛 golangci-lint (1.64.8)
app/export.go
40-40: app.StakingKeeper undefined (type *BitsongApp has no field or method StakingKeeper)
(typecheck)
app/app.go
272-272: app.GetKVStoreKey undefined (type *BitsongApp has no field or method GetKVStoreKey)
(typecheck)
278-278: app.BankKeeper undefined (type *BitsongApp has no field or method BankKeeper)
(typecheck)
317-317: app.CrisisKeeper undefined (type *BitsongApp has no field or method CrisisKeeper)
(typecheck)
472-472: app.UpgradeKeeper undefined (type *BitsongApp has no field or method UpgradeKeeper)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e-tests (e2e-pfm)
- GitHub Check: e2e-tests (e2e-basic)
🔇 Additional comments (176)
proto/README.md (1)
5-6: Updated Protobuf compiler and gRPC Gateway versions
The bump ofGOLANG_PROTOBUF_VERSIONto1.36.6(and correspondingGRPC_GATEWAY_VERSIONto1.16.0) aligns with the upgradedgoogle.golang.org/protobufdependency. Ensure that any CI scripts, developer setup documentation, and Docker images are updated to use these versions to avoid mismatches during code generation.Makefile (1)
33-33: Removed redundantBUILDDIRdeclaration
Consolidating theBUILDDIRassignment avoids duplication and simplifies maintenance. No further changes required.x/cadence/module_test.go (1)
1-1: Corrected test package name typo
Renamingcadance_testtocadence_testensures consistency with the module's corrected spelling and import paths.x/README.md (1)
10-10: Fixed module name in module list
Changingcadancetocadencein the README aligns with the refactored module directory and import paths.x/cadence/README.md (1)
1-1: Corrected module title and link spelling
Updating "Cadance" to "Cadence" in the module README maintains consistency across documentation and code.Also applies to: 5-5
x/cadence/spec/README.md (1)
1-1: Spelling correction confirmed.
The module name and all internal references have been consistently updated from “cadance” to “cadence” in the README documentation.Also applies to: 5-5, 7-7
x/cadence/genesis.go (2)
1-1: Package name updated.
Thepackagedeclaration now correctly readscadence, aligning with the directory structure and module naming.
9-10: Import paths updated.
Updated all imports togithub.com/bitsongofficial/go-bitsong/x/cadence/...to reflect the renamed module without any lingering typos.x/cadence/types/params_test.go (1)
8-8: Import path corrected.
The test now importsgithub.com/bitsongofficial/go-bitsong/x/cadence/types, fixing the typo in the path.x/cadence/abci.go (2)
1-1: Package declaration fixed.
Renamed the package tocadenceto match the updated module name.
14-15: Import statements fixed.
All imports now correctly point tox/cadence/keeperandx/cadence/types. No stalecadancereferences remain.proto/bitsong/cadence/v1/cadence.proto (3)
2-2: Proto package name corrected.
Updated tobitsong.cadence.v1to align with the renamed module.
4-4: Go package option updated.
Thego_packageoption now referencesgithub.com/bitsongofficial/go-bitsong/x/cadence/typescorrectly.
8-8: Message name updated.
Renamed the protobuf message toCadenceContract, resolving the typo inCadanceContract.Dockerfile (1)
41-41: Upgrade Alpine base image to 3.17
This bump aligns the runtime image with updated dependencies and upstream security patches.app/upgrades/v021/constants.go (2)
6-6: Fix import alias for cadence types
Updated the import alias tocadencetypes, matching the corrected module name (x/cadence/types).
35-35: Use correct store key alias for cadence
Switched tocadencetypes.StoreKeyin theAddedslice to reflect the proper module name.x/cadence/keeper/keeper.go (2)
14-14: Import path corrected to x/cadence/types
Replaced the misspelled module path with the correctly spelledcadencepackage.
17-17: Update doc comments to use "cadence" spelling
All documentation comments have been updated to reference the correctly spelled module name.Also applies to: 49-49, 54-54, 67-67, 84-84, 89-89
x/cadence/spec/01_concepts.md (3)
13-13: Correct register command example
Changed the CLI example to usetx cadence register(was misspelled). Consider clarifying thebsdalias versus thebtsgdbinary for consistency.
[approve_code_changes, suggest_nitpick]
25-25: Fix unjail command syntax
Updated the transaction example totx cadence unjailto match the corrected module name.
37-37: Fix unregister command syntax
Updated the transaction example totx cadence unregisterto match the corrected module name.x/cadence/types/codec_test.go (1)
28-31: Update proto Msg URLs to use correct module name
Fixed the interface registry entries from/bitsong.cadance.v1/...to/bitsong.cadence.v1/....x/fantoken/types/expected_keepers.go (2)
10-10: Interface Rename to Align with New Module ArchitectureThe interface name change from
DistrKeepertoProtocolPoolKeeperreflects the architectural shift mentioned in the PR objectives. This is part of the introduction of the newprotocolpoolmodule, which is taking over the responsibility of community pool funding.
10-12:Details
✅ Verification successful
Verify usage consistency across codebase
While the interface renaming looks correct, it's important to ensure that all references to this interface have been updated throughout the codebase, particularly in the fantoken keeper implementation and any tests.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to DistrKeeper in the codebase echo "Checking for any remaining references to DistrKeeper..." rg "DistrKeeper" --type go # Check for proper usage of ProtocolPoolKeeper in fantoken keeper echo "Checking usage of ProtocolPoolKeeper in fantoken keeper..." rg "ProtocolPoolKeeper" "x/fantoken/keeper"Length of output: 10706
All ProtocolPoolKeeper references are up-to-date
- No remaining
DistrKeeperreferences in the fantoken module or its testsProtocolPoolKeeperis correctly imported and used inx/fantoken/keeper/keeper.goNo further changes required.
x/cadence/module.go (2)
1-1: Module name spelling correction from "cadance" to "cadence"The package name and import paths have been correctly updated to fix the spelling of the module name. This aligns with the broader effort to rename the module throughout the codebase.
Also applies to: 21-23, 29-29
27-28:Details
❓ Verification inconclusive
Note package vs module name discrepancy
While the package is named
cadence, I notice thattypes.ModuleNameis defined as"cade"(from the relevant code snippet), not"cadence". This is likely intentional but worth noting the shortened form used for the module name constant.
🏁 Script executed:
#!/bin/bash # Check the value and usage of ModuleName across the codebase echo "Checking ModuleName in cadence types..." rg "ModuleName.*=.*\"cade\"" "x/cadence" # Check how the module name is used in store keys echo "Checking store key usage..." rg "StoreKey.*=.*ModuleName" "x/cadence"Length of output: 325
ModuleName constant mismatch: “cade” vs package name “cadence”
I noticed that in
x/cadence/types/keys.go, the module constant is defined as:const ( ModuleName = "cade" StoreKey = ModuleName … )while the Go package is named
cadence. This abbreviation may be unintentional and could lead to confusion when registering the module or working with CLI/store routes.Please confirm whether “cade” is deliberate. If not, please:
- Update
ModuleNameto"cadence"inx/cadence/types/keys.go- Ensure
StoreKey = ModuleNameand all references totypes.ModuleName(e.g., inx/cadence/module.go, CLI flags, route definitions) continue to work as expected after the changex/cadence/spec/02_state.md (1)
5-5: Documentation updated to reflect module name correctionAll references to "cadance" have been properly updated to "cadence" in the documentation, maintaining consistency with the code changes.
Also applies to: 10-10, 20-20, 46-49
server/types.go (1)
3-8: New ValidatorInfo struct for testnet configurationThis new struct supports the enhanced testnet initialization mentioned in the PR objectives. It allows for more detailed validator configuration during testnet setup, including delegation counts, token amounts, and jailed status.
app/testing/test_suite.go (1)
149-149: Simplified keeper access path looks good.The change from
s.App.AppKeepers.BankKeepertos.App.BankKeeperaligns with the broader refactoring across the codebase to simplify keeper references by accessing them directly from theBitsongAppstruct rather than through the intermediateAppKeepersstruct.x/cadence/spec/03_integration.md (1)
3-3: Spelling correction from "Cadance" to "Cadence" looks good.All instances of the incorrect spelling "Cadance" have been properly updated to "Cadence" throughout the documentation and code examples. This ensures consistency with the module name and aligns with the broader renaming effort across the codebase.
Also applies to: 5-5, 15-15, 22-22, 32-32, 42-42, 49-49, 66-66, 73-73
x/smart-account/ante/circuit_breaker_test.go (1)
135-135: Simplified keeper access path looks good.The changes from
s.BitsongApp.AppKeepers.SmartAccountKeepertos.BitsongApp.SmartAccountKeeperalign with the broader refactoring across the codebase to simplify keeper references by accessing them directly from theBitsongAppstruct rather than through the intermediateAppKeepersstruct.Also applies to: 142-142, 144-144, 152-152, 154-154
x/cadence/genesis_test.go (2)
1-1: Package and import updates for spelling correction look good.The package name has been properly updated from
cadance_testtocadence_test, along with corresponding import paths and struct names to reflect the correct spelling. This aligns with the broader effort to fix the module name spelling throughout the codebase.Also applies to: 9-10, 14-14, 17-17, 21-21
29-29: Default genesis state and keeper access references updated correctly.The references to
cadence.DefaultGenesisState()and the keeper access viasuite.App.CadenceKeeperhave been properly updated to reflect both the correct spelling and the simplified keeper access pattern.Also applies to: 58-59, 61-61, 65-65
app/keepers/keys.go (3)
8-8: Correctly updated module name from "cadance" to "cadence"This change properly fixes the typo in the module name from "cadance" to "cadence" in the import statement, ensuring consistency throughout the codebase.
13-13: Added protocolpool module importThis import adds support for the new protocolpool module mentioned in the PR objectives. This is an essential addition that enables the community pool funding functionality refactoring.
60-62: Added store keys for cadence and protocolpool modulesThe store keys for the cadence module (with corrected spelling) and the new protocolpool module are properly registered in the KV store keys list. This ensures that these modules can correctly access their persistent storage.
x/fantoken/keeper/fees.go (1)
18-18:Details
✅ Verification successful
Community pool funding redirected from distrKeeper to poolKeeper
The PR migrates community pool funding calls from the distribution module to the new protocol pool module across all fee deduction methods. This change aligns with the broader refactoring mentioned in the PR objectives where
poolKeeperreplacesdistrKeeper.Also applies to: 31-31, 44-44
🏁 Script executed:
#!/bin/bash # Verify that distrKeeper is consistently replaced with poolKeeper across the codebase echo "Checking for any remaining references to distrKeeper.FundCommunityPool:" rg "distrKeeper\.FundCommunityPool" --type go echo "Verifying new poolKeeper.FundCommunityPool references:" rg "poolKeeper\.FundCommunityPool" --type goLength of output: 691
Community pool funding calls fully migrated to poolKeeper
All references to
distrKeeper.FundCommunityPoolhave been removed and replaced withpoolKeeper.FundCommunityPool.
Verified locations inx/fantoken/keeper/fees.go:
- Line 18
- Line 31
- Line 44
No further changes required.
x/smart-account/ante/pubkey_test.go (2)
112-112: Simplified AccountKeeper reference in EmitPubKeyDecoratorEvents initializationThese changes simplify the keeper references by removing the intermediate
AppKeepersstruct, directly accessingAccountKeeperfromBitsongApp. This aligns with the broader refactoring effort mentioned in the PR objectives.Also applies to: 144-144
152-152: Simplified AccountKeeper reference in GetPubKey callThis change maintains consistency with the keeper reference simplification pattern applied throughout the codebase.
proto/bitsong/cadence/v1/genesis.proto (2)
2-2: Corrected "cadance" to "cadence" in protobuf definitionsThese changes correctly fix the spelling of the module name from "cadance" to "cadence" in the package name, import paths, and Go package option. This ensures consistency across the entire codebase and aligns with the broader refactoring mentioned in the PR objectives.
Also applies to: 6-6, 8-8
4-4:Details
✅ Verification successful
Verify the gogoproto import path
Static analysis indicates the file "gogoproto/gogo.proto" doesn't exist. This might be a configuration issue with the static analysis tool, but it's worth verifying that the import path is correct and the protobuf compilation works as expected.
🏁 Script executed:
#!/bin/bash # Check if the gogoproto/gogo.proto file exists in the expected locations echo "Checking if the protobuf files can be compiled successfully:" buf build proto/ echo "Checking for gogoproto paths in the repository:" find . -type d -name "gogoproto" -o -name "proto" | xargs ls -laLength of output: 1559
proto/bitsong/cadence/v1/genesis.proto – “gogoproto/gogo.proto” import validated
The import resolves to third_party/proto/gogoproto/gogo.proto and
buf build proto/completes without error. No changes are required.🧰 Tools
🪛 Buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
app/test_helper.go (1)
168-168: Updated keeper reference to match AppKeepers embeddingThe code now accesses
BankKeeperdirectly from theappinstance instead of through theAppKeepersfield, consistent with the application-wide refactoring whereAppKeepersis now embedded in theBitsongAppstruct.x/smart-account/authenticator/message_filter_test.go (2)
174-174: Updated keeper reference to match AppKeepers embeddingAccess to the
AccountKeeperhas been simplified by referencing it directly from theBitsongAppinstance instead of through the intermediaryAppKeepersfield, aligning with the application-wide refactoring.
373-373: Updated keeper reference to match AppKeepers embeddingAccess to the
AccountKeeperhas been simplified by referencing it directly from theBitsongAppinstance instead of through the intermediaryAppKeepersfield, aligning with the application-wide refactoring.x/smart-account/authenticator/base_test.go (3)
76-76: Updated keeper reference to match AppKeepers embeddingAccess to the
AccountKeeperhas been simplified by referencing it directly from theBitsongAppinstance instead of through the intermediaryAppKeepersfield, aligning with the application-wide refactoring.
114-114: Updated keeper reference to match AppKeepers embeddingAccess to the
AccountKeeperhas been simplified by referencing it directly from theBitsongAppinstance instead of through the intermediaryAppKeepersfield, aligning with the application-wide refactoring.
158-158: Updated keeper reference to match AppKeepers embeddingDirect reference to
BankKeeperfrom theBitsongAppinstance simplifies the code and aligns with the application-wide refactoring whereAppKeepersis now embedded in theBitsongAppstruct.x/smart-account/authenticator/signature_authenticator_test.go (1)
264-264: Updated keeper reference to match AppKeepers embeddingAccess to the
AccountKeeperhas been simplified by referencing it directly from theBitsongAppinstance instead of through the intermediaryAppKeepersfield, aligning with the application-wide refactoring.x/fantoken/keeper/keeper_test.go (1)
46-48: Keeper access simplification looks goodThe code now directly accesses keepers from the app instance rather than through the AppKeepers indirection, and renames
apptoAppfor consistency. This aligns with the application-wide refactoring to simplify keeper references.app/helpers/mock.go (1)
27-27: Function name updated for CometBFT compatibilityThe code correctly updates the public key conversion function from
ToTmPubKeyInterfacetoToCmtPubKeyInterfaceto align with the CometBFT upgrade (v0.38.17) mentioned in the PR objectives.x/cadence/types/codec.go (2)
25-28: Module name spelling corrected from "cadance" to "cadence"The registration of concrete message types now uses the correct spelling "cadence" instead of "cadance", both in the message type names and in the registration strings.
34-37: Message implementation names correctly updatedThe interface registration now uses the properly spelled message implementations, consistent with the changes made to the concrete type registrations above.
x/smart-account/keeper/genesis_test.go (7)
24-31: Direct keeper access simplifies code structureThe code now directly references
SmartAccountKeeperfrom the app instance rather than throughAppKeepers, which is consistent with the application-wide refactoring to embed keepers directly in theBitsongAppstruct.
33-40: Consistent keeper access pattern appliedThe direct keeper access pattern is consistently applied here for the second call to
AddAuthenticatorWithId.
42-44: Simplified getter access for authenticator dataThe direct access pattern is correctly applied to the getter method for authenticator data.
46-55: Error handling works with simplified keeper accessThe error handling for the invalid public key case remains unchanged and works correctly with the simplified keeper access pattern.
56-66: Direct authenticator manager access simplifies codeBoth the authenticator manager and smart account keeper are now directly accessed from the app instance, maintaining the error handling for the non-registered authenticator type.
81-89: Loop operation with direct keeper accessThe AddAuthenticator call within the loop now correctly uses the direct keeper access pattern while maintaining the same functionality and error checking.
91-95: Genesis data retrieval with simplified access patternThe GetAllAuthenticatorData method is now directly accessed from the keeper, maintaining the existing assertions and validations.
x/fantoken/handler_test.go (6)
26-27: Approved: Simplified keeper access pattern.The change from
suite.App.AppKeepers.ParamsKeepertosuite.App.ParamsKeeperimproves code clarity by removing an unnecessary layer of indirection.
55-56: Approved: Direct keeper access.Directly accessing
FanTokenKeeperfrom the App struct simplifies the code without changing functionality.
70-71: Approved: Consistent simplification pattern.Passing
suite.App.FanTokenKeeperdirectly toNewProposalHandleris consistent with the refactoring pattern applied throughout the codebase.
73-74: Approved: Direct keeper access.Simplifying keeper access for parameter validation.
81-82: Approved: Consistent pattern in test methods.Maintaining the simplified keeper access pattern in
TestProposalHandlerFailed.
99-100: Approved: Final instance of simplified keeper access.Consistently applying the direct keeper access pattern throughout the test file.
x/smart-account/keeper/keeper_test.go (5)
49-64: Approved: Simplified keeper access in TestKeeper_AddAuthenticator.Direct access to
SmartAccountKeeperthroughs.Appimproves code clarity by removing the intermediateAppKeepersaccessor while maintaining the same functionality.
67-82: Approved: Direct access to AuthenticatorManager.The code now directly references
s.App.AuthenticatorManagerinstead of going throughAppKeepers, consistent with the architectural refactoring in the PR.
94-113: Approved: Simplified keeper access in TestKeeper_GetAuthenticatorDataForAccount.Consistent application of the direct keeper access pattern for both
SmartAccountKeeperandAuthenticatorManager.
118-150: Approved: Direct keeper references in TestKeeper_GetAndSetAuthenticatorId.All instances of keeper access have been consistently simplified, improving code readability without changing functionality.
162-207: Approved: Consistent pattern in TestKeeper_GetSelectedAuthenticatorForAccount.The direct keeper access pattern has been correctly applied throughout this test method, maintaining consistency with the broader refactoring.
x/cadence/abci_test.go (2)
1-1: Approved: Corrected module spelling from "cadance" to "cadence".Consistent spelling corrections applied to package name, imports, variable names, and comments, aligning with the PR's broader initiative to correct the module name throughout the codebase.
Also applies to: 15-16, 36-36, 239-239
55-55: Approved: Simplified keeper access pattern.All instances of keeper access have been updated to directly reference
s.App.WasmKeeperands.App.CadenceKeeperinstead of going through the intermediateAppKeepersstruct, consistent with the architectural refactoring throughout the codebase.Also applies to: 64-64, 79-79, 107-109, 112-112, 126-127, 148-148, 159-159, 163-163, 179-179, 186-186, 204-205, 213-213, 224-224, 235-235, 242-242
x/cadence/client/cli/query.go (4)
9-9: Corrected import path: approving change
Import path updated to use the correctly spelled “cadence” module.
45-47: Renamed query method to CadenceContracts
Updated call toqueryClient.CadenceContractsmatches the fixed import and type names.
73-75: Renamed request struct to QueryCadenceContract
Correctly referencestypes.QueryCadenceContractinstead of the old misspelling.
77-78: Renamed query method to CadenceContract
Single-contract query call aligns with the corrected service naming.x/smart-account/keeper/msg_server_test.go (9)
15-15: Use direct SmartAccountKeeper reference
RefactoredMsgServerinstantiation to remove the intermediateAppKeeperslayer. No logic change.
51-51: Use direct SmartAccountKeeper reference in failure test
Consistently removedAppKeepersforMsgServercreation.
76-76: Use direct SmartAccountKeeper reference in removal test
SimplifiedMsgServerinstantiation; behaviour unchanged.
106-108: Dereference keeper for active-state test
Assignedak := *s.App.SmartAccountKeeperand passed toNewMsgServerImpl; refactor only.
122-123: DirectGetParamscall
ReplacedAppKeepersaccess withs.App.SmartAccountKeeper.GetParams; no functional change.
126-128: DirectSetParamscall
RemovedAppKeepersindirection fromSetParams; test logic remains intact.
170-171: Direct governor lookup
AccessedCircuitBreakerGovernordirectly onSmartAccountKeeper; purely structural.
188-188: Use direct SmartAccountKeeper in inactive test
MsgServerinstantiation refactored; behavior unchanged.
191-192: DirectSetParamscall in inactive test
RemovedAppKeepersprefix; no logic alteration.app/upgrades/v021/upgrade_test.go (11)
30-31: Initialize preModule with direct UpgradeKeeper
Replaces the oldAppKeepersreference. No logic changes in setup.
39-40: DirectGetAllValidatorscall
Accessess.App.StakingKeeperdirectly; structural refactor only.
45-46: Fund accounts via direct helper
FundAcccall unchanged; keeper access refactored.
54-55: DirectSetValidatorinvocation
Structural refactor of keeper reference; logic identical.
58-59: DirectGetValidatorDelegationscall
Refactored to use direct keeper; no behavioural change.
88-90: Direct ICQKeeper params check
RemovedAppKeeperslayer when accessingICQKeeper; test asserts unchanged.
94-98: Direct SmartAccountKeeper params check
Simplified access toSmartAccountKeeperandAuthenticatorManager; functionality unaffected.
100-102: Direct IBC client params check
Replaced nested keeper reference with directs.App.IBCKeeper.ClientKeeper; no logic change.
103-106: Direct CadenceKeeper params check
Access toCadenceKeeperstreamlined; test assertions remain valid.
171-172: ScheduleUpgrade via direct UpgradeKeeper
RemovedAppKeepersindirection; schedules upgrade as before.
173-174: Retrieve upgrade plan via direct UpgradeKeeper
Structural refactor; functionality unchanged.x/smart-account/authenticator/spend_limits_test.go (7)
83-84: Instantiate CosmwasmAuthenticator directly
RemovedAppKeepersprefix forContractKeeperandAccountKeeper; logic preserved.
86-87: Register authenticator on direct manager
SmartAccountKeeper.AuthenticatorManageraccessed withoutAppKeepers; no change in behaviour.
88-89: Create DeductFeeDecorator with direct keepers
PassedAccountKeeper,BankKeeper, andFeeGrantKeeperdirectly; structural only.
91-93: Build AuthenticatorAnteDecorator directly
Uses directSmartAccountKeeper; decorator logic unchanged.
99-100: Build AuthenticatorPostDecorator directly
RemovedAppKeepersfrom arguments; functionality remains the same.
203-204: Create GovPermissionKeeper directly
Simplified towasmkeeper.NewGovPermissionKeeper(btsgApp.WasmKeeper); no logic change.
216-217: Create DefaultPermissionKeeper directly
Removed intermediate reference; test helpers unaffected.x/smart-account/authenticator/composition_test.go (4)
68-68: Instantiate spy authenticator with direct KVStoreKey
Refactored to useBitsongApp.GetKVStoreKey()directly; no logic impact.
204-204: Use direct AccountKeeper in TestAnyOf initialization
RemovedAppKeepersprefix; test operations unchanged.
346-346: Use direct AccountKeeper in TestAllOf initialization
Structural refactor only.
444-444: Use direct AccountKeeper in TestComposedAuthenticator
Consistency refactor; behaviour remains correct.x/cadence/client/cli/tx.go (8)
10-10: Consistent module name correction from "cadance" to "cadence".The import path has been updated to use the correct "cadence" spelling, which is consistent with the module renaming throughout the codebase.
25-27: Function name corrections to maintain consistency with module renaming.The function names have been correctly updated to use "Cadence" instead of "Cadance" to match the module's proper spelling.
32-38: Documentation and description updates for consistency.The documentation comments and command descriptions now correctly refer to "cadence" instead of "cadance".
49-52: Message type correction to match renamed types.The message type has been updated to use the correctly spelled
MsgRegisterCadenceContracttype.
66-72: Documentation and description updates for unregister command.The documentation comments and command descriptions for the unregister command now correctly refer to "cadence" instead of "cadance".
83-86: Message type correction for unregister command.The message type has been updated to use the correctly spelled
MsgUnregisterCadenceContracttype.
100-106: Documentation and description updates for unjail command.The documentation comments and command descriptions for the unjail command now correctly refer to "cadence" instead of "cadance".
117-120: Message type correction for unjail command.The message type has been updated to use the correctly spelled
MsgUnjailCadenceContracttype.x/smart-account/integration_test.go (3)
61-61: Refactored app field name to App for better code style.The field name has been capitalized to
Appfromapp, maintaining consistency with Go's naming conventions for exported fields and improving code readability.
84-86: Direct keeper access from App rather than through AppKeepers.Simplified the code by accessing keepers directly from the App instance instead of through an intermediate AppKeepers struct, as part of a broader refactoring of keeper access patterns.
104-105: Simplified keeper access pattern throughout test suite.Modified all keeper references to access them directly from the App instance rather than through an intermediate AppKeepers struct. This change is part of a broader refactoring to simplify the keeper access patterns throughout the codebase.
Also applies to: 125-126, 153-154, 185-185, 278-280, 304-309, 311-313, 338-343, 358-364, 374-384, 407-408, 426-427, 511-512, 526-527, 551-554, 560-580, 596-606, 708-716
cmd/bitsongd/cmd/genesis.go (2)
132-135: Updated governance parameter access pattern.Changed to access MinDeposit parameter through
GovParams.Paramsrather thanGovParams.DepositParams, reflecting a structural change in governance parameter access that consolidates parameters under a single field.
174-174: Simplified governance parameter assignment.The change consolidates governance parameter assignments by replacing separate
DepositParams,TallyParams, andVotingParamsassignments with a single assignment of the entireParamsfield, simplifying parameter handling.x/smart-account/authenticator/cosmwasm_test.go (4)
50-50: Direct keeper access in CosmwasmAuthenticator initialization.Simplified CosmwasmAuthenticator initialization by accessing keepers directly from the BitsongApp instance rather than through the intermediate AppKeepers struct.
383-385: Direct BankKeeper access for coin minting and sending.Modified BankKeeper access to use direct reference from the BitsongApp instance, as part of the broader keeper access pattern refactoring.
443-445: Direct AccountKeeper access for authentication requests.Updated to access the AccountKeeper directly from the BitsongApp instance when generating authentication requests, maintaining consistency with the new keeper access pattern.
457-458: Direct keeper access for contract operations.Modified all WasmKeeper and AccountKeeper access in contract-related operations to use direct references from the BitsongApp instance, completing the refactoring of keeper access patterns in this test file.
Also applies to: 470-471, 480-480, 489-490
x/cadence/keeper/querier_test.go (6)
8-8: Import path correctly updated tocadenceThe import path now correctly references "cadence" instead of "cadance", which is consistent with the module renaming throughout the codebase.
37-38: Keeper reference simplifiedThe reference to the Cadence keeper has been updated to use the direct keeper reference (
s.App.CadenceKeeper) instead of going through the intermediateAppKeepersstruct. This aligns with the broader refactoring to simplify keeper access.
52-53: Function name corrected to use "Cadence"Function name has been updated to use the correct module name "Cadence" instead of "Cadance".
85-86: Method names and references updated to "Cadence"All references to contract registration, querying, and response fields have been correctly updated to use "Cadence" instead of "Cadance". Keeper access has also been simplified to use direct references.
Also applies to: 89-90, 94-97, 101-102
108-109: All jailed contract test references updated to "Cadence"Function name and method references for testing jailed contracts have been corrected to use "Cadence" throughout the test file.
Also applies to: 141-143, 146-147, 151-154, 158-159
165-166: Query contract test references updated to "Cadence"All contract query test function names, keeper method calls, and response field references have been updated to use the correct "Cadence" naming.
Also applies to: 173-176, 179-182, 218-220, 225-226
app/export.go (3)
75-76: Simplified keeper references for Crisis, Staking, and Distribution keepersMultiple keeper references have been updated to access the keepers directly from the app instance rather than through the intermediate
AppKeepersstruct. This is part of the broader refactoring to simplify keeper references across the codebase.Also applies to: 80-86, 89-93, 103-104, 107-111, 117-132
144-146: Remaining keeper references simplifiedAll remaining references to keepers in the export functions have been updated to use direct access, improving code clarity and consistency with the rest of the codebase.
Also applies to: 154-160, 163-169, 179-190, 195-197, 202-209
40-43:Details
✅ Verification successful
Direct keeper reference used for StakingKeeper
Changed from
app.AppKeepers.StakingKeeperto direct access viaapp.StakingKeeper, which is now possible becauseAppKeepersis embedded directly in theBitsongAppstruct. This simplifies the code and makes it more readable.
🏁 Script executed:
#!/bin/bash # Verify that StakingKeeper is properly defined in BitsongApp struct cat app/app.go | grep -A 5 "BitsongApp struct"Length of output: 181
No action needed: Direct keeper reference is valid
The
BitsongAppstruct embedskeepers.AppKeepers, soapp.StakingKeepercorrectly resolves toapp.AppKeepers.StakingKeeper. This change is safe and improves readability.🧰 Tools
🪛 golangci-lint (1.64.8)
40-40: app.StakingKeeper undefined (type *BitsongApp has no field or method StakingKeeper)
(typecheck)
x/smart-account/post/post_test.go (4)
80-82: Simplified keeper references in AuthenticatorPostDecorator initializationUpdated to access the SmartAccountKeeper and AccountKeeper directly from the BitsongApp instance instead of through the intermediate AppKeepers struct. This follows the broader refactoring pattern in the codebase.
113-121: Direct SmartAccountKeeper reference in AddAuthenticator callUpdated to access the SmartAccountKeeper directly from the BitsongApp instance, simplifying the code and improving readability without changing functionality.
122-129: Simplified keeper reference in second AddAuthenticator callConsistent with the other changes, updated to access SmartAccountKeeper directly from the BitsongApp instance.
186-188: Direct references to AuthenticatorManager and SmartAccountKeeperUpdated to access both the AuthenticatorManager and SmartAccountKeeper directly from the BitsongApp instance, maintaining consistency with the rest of the codebase.
x/fantoken/simulation/genesis.go (4)
1-15: Well-structured imports for the new simulation packageThe imports are organized correctly, including necessary packages for simulation functionality and math operations. The structure follows standard Go conventions.
16-39: Properly implemented parameter randomizationThe implementation correctly randomizes module parameters (IssueFee, MintFee, BurnFee) with sensible ranges using the simulation framework's random number generator. The bond denomination is correctly obtained from the simulation state.
40-77: Fan token generation implementation is robustThe code generates between 1 and 5 random fan tokens with appropriate properties:
- Random denominations with a "ft" prefix
- Reasonable max supply ranges
- Random minter accounts from the simulation accounts
- Properly formatted metadata with random names, symbols, and optional URIs
- Consistent authority assignment
This provides good coverage for simulation testing scenarios.
78-86: Correct genesis state construction and storageThe function properly constructs a GenesisState with the randomized parameters and fan tokens, and correctly marshals and stores it in the simulation state's genesis map using the appropriate module name key.
x/fantoken/keeper/keeper.go (5)
6-6: Appropriate imports added for CollectionsThe addition of the Collections import from Cosmos SDK aligns with modern state management practices in the SDK ecosystem.
22-24: Refactoring from distribution to protocol pool keeperThe replacement of
distrKeeperwithpoolKeeperof typeProtocolPoolKeeperreflects a shift in responsibility for community pool funding, and adding theSchemafield supports modern state management with collections.
34-34: Updated function signature for NewKeeperProperly updated parameter from
distrKeepertopoolKeeperto match the struct field changes.
51-51: Consistent assignment in Keeper initializationThe assignment of
poolKeeperin the Keeper constructor maintains consistency with the struct field changes.
46-54:Details
✅ Verification successful
Verify fee deduction methods use poolKeeper
While the keeper initialization correctly assigns the new
poolKeeper, ensure the fee deduction methods (deductIssueFee,deductMintFee, anddeductBurnFee) have been updated to usepoolKeeper.FundCommunityPoolinstead of the previousdistrKeeper.FundCommunityPool.
🏁 Script executed:
#!/bin/bash # Description: Check if fee deduction methods have been updated to use poolKeeper # Search for fee deduction methods in the fantoken module rg -A 5 "deduct(Issue|Mint|Burn)Fee" --glob "x/fantoken/keeper/fees.go"Length of output: 1482
🏁 Script executed:
#!/bin/bash # Check for any usage of FundCommunityPool in fees.go rg -n "FundCommunityPool" --glob "x/fantoken/keeper/fees.go" # Display full implementations of the fee deduction methods rg -A30 "func (k Keeper) deduct(Issue|Mint|Burn)Fee" --glob "x/fantoken/keeper/fees.go"Length of output: 480
Fee deduction methods use poolKeeper.FundCommunityPool
Verified that in x/fantoken/keeper/fees.go the methods now call k.poolKeeper.FundCommunityPool for all fees:
- Line 18: return k.poolKeeper.FundCommunityPool(ctx, sdk.Coins{params.IssueFee}, authority)
- Line 31: return k.poolKeeper.FundCommunityPool(ctx, sdk.Coins{params.MintFee}, authority)
- Line 44: return k.poolKeeper.FundCommunityPool(ctx, sdk.Coins{params.BurnFee}, authority)
No further changes required.
x/fantoken/module.go (3)
17-17: Added simulation package importCorrectly added the fantoken simulation package import to support new simulation capabilities.
24-24: Added simulation types importProperly imported the Cosmos SDK simulation types to support the new simulation methods.
130-133: Storage decoder registration using collections SchemaGood integration with the new Schema field added to the keeper, allowing proper simulation testing of the module state.
app/modules.go (8)
12-16: Fixed module name spelling from 'cadance' to 'cadence'Consistently corrected the import paths for the cadence module, fixing a previous spelling error throughout the codebase.
47-48: Added protocolpool module importsProperly added imports for the new protocolpool module from the Cosmos SDK that will replace the distribution module for community pool funding.
121-151: Direct keeper access pattern implementationRefactored module initialization to directly access keepers from the app struct rather than through an intermediate
AppKeepersstruct, simplifying the architecture and improving code clarity.
149-149: Added protocolpool module to application modulesProperly integrated the new protocolpool module with the AccountKeeper and BankKeeper dependencies, ensuring it's available for other modules like fantoken that now depend on it.
158-159: Updated lifecycle hooks for protocolpool moduleAdded the protocolpool module to begin and end blockers with a critical note that it must run before the bank module in end blockers, ensuring proper execution order for pool-related operations.
Also applies to: 170-173
206-207: Updated init blockers with correct module namesFixed cadence module name spelling and added the protocolpool module to the init blockers, ensuring proper module initialization order.
219-235: Updated simulation modules with direct keeper accessRefactored simulation module setup to consistently use direct keeper references from the app struct, aligning with the broader architecture simplification.
240-251: Simplified keeper getter methodsUpdated getter methods to directly return keepers from the app struct, maintaining consistency with the new direct access pattern throughout the codebase.
server/util.go (3)
13-16: Database initialization utilityGood implementation of a database initialization helper that standardizes the DB creation process across the application.
18-27: Trace writer utility with proper error handlingWell-implemented utility for managing trace log files with appropriate file open flags and permissions.
29-34: In-place testnet creation command integrationThis function properly integrates the testnet creator command into the application's CLI, supporting the in-place testnet functionality mentioned in the PR objectives.
x/smart-account/ante/ante_test.go (1)
86-94: Good refactor – direct keeper access improves readabilitySwapping
AppKeepers.*for direct fields simplifies the tests and mirrors the
production code after the embedding change. Nice cleanup.x/cadence/types/msgs.go (2)
11-12: Constant rename looks correctThe JSON payload matches the new module name. No functional concerns.
30-34: Route/Type helpers updated – verify downstream handlersDown-stream message routers & protobuf service definitions must be updated to
expect the new type strings (register_cadence_contract, etc.). A quick grep
is recommended to catch any missed cases.app/keepers/keepers.go (1)
106-119: Module-account permission map updated – double-check genesis
protocolpooltypes.ModuleNameand
protocolpooltypes.ProtocolPoolEscrowAccountnow appear inmaccPermsbut the
corresponding module accounts must be created during genesis initialisation
(InitGenesis) or migrations, otherwise the app will panic on first mint/send
attempt. Verify the new module’sInitGenesishandles this.cmd/bitsongd/cmd/root.go (2)
188-191: Gas-price tweak looks benignLowering the default
MinGasPricesfrom0.001ubtsgto0.00069ubtsgis fine; just ensure all validator configs are regenerated or they will still reject txs below the old floor.
398-404: Comma-separated validator list never split
brokenValis passed downstream as a single string, yet the commented code suggests the callee expects a slice. Verify the signature ofInitBitsongAppForTestnet. If a slice is required:-brokenVal, ok := appOpts.Get(testnetserver.KeyBrokenValidator).(string) +brokenValStr, ok := appOpts.Get(testnetserver.KeyBrokenValidator).(string) ... -return bitsong.InitBitsongAppForTestnet(..., brokenVal) +brokenVals := strings.Split(brokenValStr, ",") +return bitsong.InitBitsongAppForTestnet(..., brokenVals)app/upgrades/v022/upgrades_test.go (1)
220-220:⚠️ Potential issueFix error handling in GetValidatorHistoricalRewards.
The function returns a reward object and an error (not a boolean), but you're using
foundas if it's a boolean. This will cause a compilation error.- rewards, found := s.App.DistrKeeper.GetValidatorHistoricalRewards(s.Ctx, controlValAddr, 1) - s.Require().NoError(found, "Control validator rewards should exist") + rewards, err := s.App.DistrKeeper.GetValidatorHistoricalRewards(s.Ctx, controlValAddr, 1) + s.Require().NoError(err, "Control validator rewards should exist")Likely an incorrect or invalid review comment.
proto/bitsong/cadence/v1/query.proto (1)
4-4:Details
✅ Verification successful
Verify the import path for pagination proto.
Static analysis indicates that the import "cosmos/base/query/v1beta1/pagination.proto" file may not exist. Please verify this import path is correct in your development environment.
🏁 Script executed:
#!/bin/bash # Check if the imported proto file exists in your project dependencies fd -t f "pagination.proto" | grep -i "cosmos/base/query"Length of output: 115
Import path for pagination.proto verified
The file
third_party/proto/cosmos/base/query/v1beta1/pagination.protoexists, so the importimport "cosmos/base/query/v1beta1/pagination.proto";is correct.
🧰 Tools
🪛 Buf (1.47.2)
4-4: import "cosmos/base/query/v1beta1/pagination.proto": file does not exist
(COMPILE)
x/cadence/keeper/cadance.go (2)
8-8: LGTM! Correctly updated import path.The import path has been properly updated to use "cadence" instead of "cadance".
13-14: LGTM! Properly renamed module references.All references to "cadance" have been correctly updated to "cadence" in comments, function names, and type references.
Also applies to: 18-19, 23-24, 37-38, 43-44, 65-66, 93-93, 119-121, 125-126, 135-136, 148-149, 154-155, 171-172, 192-192, 195-196
app/upgrades/v022/constants.go (3)
10-14: LGTM! Well-defined constants for the upgrade.The upgrade constants are properly defined with clear naming conventions.
16-22: LGTM! Properly configured upgrade handler.The upgrade configuration correctly includes the handler and store upgrades for the new protocol pool module.
24-60: LGTM! Comprehensive data structures for upgrade tracking.The data structures are well-designed to track various aspects of the upgrade process, including delegation patches, zero-share delegations, and slashing events. This will be valuable for debugging and auditing the upgrade.
app/upgrades/v022/upgrades.go (1)
52-92: Delegation removal lacks hooks & emits no eventsWhen zero-share delegations are deleted via
k.StakingKeeper.RemoveDelegation, staking hooks (AfterDelegationRemoved) are not automatically triggered by that call.
If other modules (e.g. x/distribution) rely on those hooks, state divergence can occur.Please ensure either:
- The keeper method already invokes hooks (verify in the local fork), or
- Manually invoke
k.StakingKeeper.Hooks().AfterDelegationRemoved(...)after successful removal.go.mod (1)
3-4: Go toolchain version1.23.2is in the futureOfficial Go releases are currently 1.22.x. Setting an unreleased minor version breaks reproducible builds for all downstream users and CI systems.
Unless you require a development snapshot, downgrade to the latest stable:
-go 1.23.2 +go 1.22Then run
go mod tidyto refresh module checksums.
| // CadenceContract returns the cadence contract information | ||
| func (q Querier) CadenceContract(stdCtx context.Context, req *types.QueryCadenceContract) (*types.QueryCadenceContractResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(stdCtx) | ||
|
|
||
| // Ensure the contract address is valid | ||
| if _, err := sdk.AccAddressFromBech32(req.ContractAddress); err != nil { | ||
| return nil, sdkerrors.ErrInvalidAddress | ||
| } | ||
|
|
||
| contract, err := q.keeper.GetCadanceContract(ctx, req.ContractAddress) | ||
| contract, err := q.keeper.GetCadenceContract(ctx, req.ContractAddress) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &types.QueryCadanceContractResponse{ | ||
| CadanceContract: *contract, | ||
| return &types.QueryCadenceContractResponse{ | ||
| CadenceContract: *contract, | ||
| }, nil |
There was a problem hiding this comment.
Possible nil-pointer dereference & missing request validation
- As with the previous query,
reqcan benil. GetCadenceContractreturns(*CadenceContract, error). If it returns a
nilcontract with anilerror, the subsequent*contractdereference
will panic.
Suggested defensive pattern:
+ if req == nil {
+ return nil, status.Error(codes.InvalidArgument, "request cannot be nil")
+ }
if _, err := sdk.AccAddressFromBech32(req.ContractAddress); err != nil {
return nil, sdkerrors.ErrInvalidAddress
}
- contract, err := q.keeper.GetCadenceContract(ctx, req.ContractAddress)
+ contract, err := q.keeper.GetCadenceContract(ctx, req.ContractAddress)
if err != nil {
return nil, err
}
+ if contract == nil {
+ return nil, status.Error(codes.NotFound, "contract not found")
+ }🤖 Prompt for AI Agents (early access)
In x/cadence/keeper/querier.go around lines 36 to 52, the function does not
check if the request object is nil before accessing its fields, and it
dereferences the contract pointer without verifying it is non-nil, risking a
nil-pointer panic. Fix this by first validating that req is not nil and
returning an appropriate error if it is. Then, after calling GetCadenceContract,
check if the returned contract pointer is nil before dereferencing it, and
handle that case by returning a suitable error to avoid panics.
| // debug upgradeHandler | ||
| err := v022.CustomV022PatchLogic(ctx, &app.AppKeepers, true) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Panic / log.Fatal combo yields inconsistent termination semantics
This function mixes log.Fatal (which os.Exits — no defer runs) and panic (which unwinds the stack). Choose one strategy or, better, propagate errors to the caller so the CLI can decide what to do.
Suggestion: turn the function into customTestUpgradeHandlerLogicViaExport(...) error and bubble errors up through CustomExportAppStateAndValidators, keeping the CLI responsible for exiting.
🤖 Prompt for AI Agents (early access)
In app/export_custom.go around lines 78 to 83, the code currently panics on
error, mixing panic and log.Fatal which causes inconsistent termination
behavior. Refactor the function to return an error instead of panicking, rename
it to customTestUpgradeHandlerLogicViaExport(...) error, and propagate the error
up through CustomExportAppStateAndValidators so the CLI caller can handle the
error and decide whether to exit or not.
| addStartNodeFlags(cmd, opts) | ||
| cmd.Flags().String(KeyTriggerTestnetUpgrade, "", "If set (example: \"v21\"), triggers the v21 upgrade handler to run on the first block of the testnet") | ||
| cmd.Flags().Bool("skip-confirmation", false, "Skip the confirmation prompt") | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
broken-vals flag referenced but never declared
The long help text mentions a --broken-vals flag, yet only --trigger-testnet-upgrade and --skip-confirmation are added.
If you intend to support a flag (instead of the third positional argument), add it here:
cmd.Flags().String(KeyBrokenValidator, "", "Comma-separated list of validator operator addresses whose delegations must be retained with minimal power")Otherwise, remove the reference from the help text to avoid ambiguity.
🤖 Prompt for AI Agents (early access)
In server/start.go around lines 214 to 218, the help text references a
--broken-vals flag that is not declared in the command flags. To fix this, add
the missing flag declaration using cmd.Flags().String with the appropriate key
and description for broken validator addresses. Alternatively, if the flag is
not needed, remove its mention from the help text to prevent confusion.
| // This state usually occurs when we gracefully stop the node. | ||
| err = blockStore.DeleteLatestBlock() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| block = blockStore.LoadBlock(blockStore.Height()) |
There was a problem hiding this comment.
Potential double call to DeleteLatestBlock
blockStore.DeleteLatestBlock() is executed in the branch starting at line 675 and then unconditionally executed again at line 742.
If the earlier branch was taken the second call will attempt to delete an already-deleted height, leading to a panic (ErrDeletionHeightMismatch) on CometBFT ≥ 0.38.14.
Guard the second deletion:
-// Save the block with extended commit
-blockStore.DeleteLatestBlock()
+// Save the block with extended commit – ensure we removed the old head once
+if blockStore.Height() >= state.LastBlockHeight {
+ if err := blockStore.DeleteLatestBlock(); err != nil {
+ return nil, err
+ }
+}
blockStore.SaveBlockWithExtendedCommit(block, blockParts, ec)Also applies to: 742-744
🤖 Prompt for AI Agents (early access)
In server/start.go around lines 674 to 679 and 742 to 744, the method
blockStore.DeleteLatestBlock() is called twice unconditionally, which can cause
a panic if the block was already deleted in the earlier branch. To fix this, add
a conditional check before the second call to DeleteLatestBlock() to ensure it
is only called if the block has not been deleted yet, preventing the
ErrDeletionHeightMismatch panic on CometBFT ≥ 0.38.14.
| // Split the comma-separated list of validators | ||
| // validatorAddrs := strings.Split(brokenValidators, ",") | ||
| // for _, valAddrStr := range validatorAddrs { | ||
|
|
||
| // Trim any whitespace | ||
| // valAddrStr = strings.TrimSpace(valAddrStr) | ||
|
|
||
| // Decode the validator address | ||
|
|
||
| // Create a validator with very low voting power (1) | ||
| // brokenVal := &cmttypes.Validator{ | ||
| // Address: valAddr, | ||
| // PubKey: nil, // You might want to fetch the actual pubkey if needed | ||
| // VotingPower: 1, // Minimal voting power to keep the validator in the set | ||
| // } | ||
| // newValidators = append(newValidators, brokenVal) | ||
| // } | ||
| } |
There was a problem hiding this comment.
brokenValidators list is parsed but silently ignored
All logic that should convert each address to a low-power Validator is commented out, making the brokenValidator argument/flag ineffective. This is easy to miss in testing and will surprise operators expecting those validators to be preserved.
Either:
- Re-enable the code (and add proper pub-key lookup & error handling), or
- Remove the argument/flag entirely to keep behaviour explicit.
Leaving dead code in place is a maintainability smell.
🤖 Prompt for AI Agents (early access)
In server/start.go around lines 758 to 775, the code that parses the
brokenValidators list and converts each address into a low-power Validator is
commented out, causing the brokenValidators argument/flag to have no effect. To
fix this, either uncomment and complete the code by adding proper public key
lookup and error handling for each validator address, ensuring each Validator is
created with minimal voting power and appended to newValidators, or remove the
brokenValidators argument/flag entirely to avoid confusion and maintain explicit
behavior.
| for _, height := range []int64{blockStore.Height(), blockStore.Height() - 1, blockStore.Height() + 1} { | ||
| err = stateDB.Set([]byte(fmt.Sprintf("validatorsKey:%v", height)), buf) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Direct writes to stateDB use Set instead of SetSync
stateDB.Set buffers the write in the underlying batch but does not fsync.
Because stateStore.Save(state) calls stateDB.SetSync for its own keys, your additional manual writes risk being lost if the node crashes before the next commit.
Replace with the synchronous variant (or explicitly stateDB.Commit()):
-err = stateDB.Set([]byte(fmt.Sprintf("validatorsKey:%v", height)), buf)
+err = stateDB.SetSync([]byte(fmt.Sprintf("validatorsKey:%v", height)), buf)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, height := range []int64{blockStore.Height(), blockStore.Height() - 1, blockStore.Height() + 1} { | |
| err = stateDB.Set([]byte(fmt.Sprintf("validatorsKey:%v", height)), buf) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| for _, height := range []int64{ | |
| blockStore.Height(), | |
| blockStore.Height() - 1, | |
| blockStore.Height() + 1, | |
| } { | |
| - err = stateDB.Set([]byte(fmt.Sprintf("validatorsKey:%v", height)), buf) | |
| + err = stateDB.SetSync([]byte(fmt.Sprintf("validatorsKey:%v", height)), buf) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } |
🤖 Prompt for AI Agents (early access)
In server/start.go around lines 809 to 814, the code uses stateDB.Set for
writing, which buffers writes without fsync, risking data loss on crash. Replace
stateDB.Set calls with stateDB.SetSync to ensure synchronous writes or
explicitly call stateDB.Commit() after the loop to flush the batch. This change
guarantees durability consistent with stateStore.Save behavior.
| Use: "in-place-testnet [newChainID] [newOperatorAddress] ", | ||
| Short: "Create and start a testnet from current local state", | ||
| Long: `Create and start a testnet from current local state. | ||
| After utilizing this command the network will start. If the network is stopped, | ||
| the normal "start" command should be used. Re-using this command on state that | ||
| has already been modified by this command could result in unexpected behavior. | ||
| If the --broken-vals flag is set, the server expects a bitsong1val... addr that has broken delegations. | ||
| The server will retain this p |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mismatch between Use declaration, arg validation and docstring (and a truncated sentence)
Use advertises two positional args while cobra.ExactArgs(3) enforces three (newChainID, newOperatorAddress, and brokenValidator).
In addition, the docstring refers to a --broken-vals flag (which is never registered) and line 139 ends abruptly: “The server will retain this p”.
Please keep the public interface self-consistent to avoid user confusion:
- Use: "in-place-testnet [newChainID] [newOperatorAddress] ",
+ Use: "in-place-testnet [newChainID] [newOperatorAddress] [brokenValidatorAddrs]",…and finish the truncated sentence or drop it if redundant.
Finally, either register a --broken-vals flag or remove the reference altogether.
📝 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.
| Use: "in-place-testnet [newChainID] [newOperatorAddress] ", | |
| Short: "Create and start a testnet from current local state", | |
| Long: `Create and start a testnet from current local state. | |
| After utilizing this command the network will start. If the network is stopped, | |
| the normal "start" command should be used. Re-using this command on state that | |
| has already been modified by this command could result in unexpected behavior. | |
| If the --broken-vals flag is set, the server expects a bitsong1val... addr that has broken delegations. | |
| The server will retain this p | |
| Use: "in-place-testnet [newChainID] [newOperatorAddress] [brokenValidatorAddrs]", | |
| Short: "Create and start a testnet from current local state", | |
| Long: `Create and start a testnet from current local state. | |
| After utilizing this command the network will start. If the network is stopped, | |
| the normal "start" command should be used. Re-using this command on state that | |
| has already been modified by this command could result in unexpected behavior. | |
| If the --broken-vals flag is set, the server expects a bitsong1val... addr that has broken delegations. | |
| The server will retain this p |
🤖 Prompt for AI Agents (early access)
In server/start.go around lines 131 to 139, fix the inconsistency between the
`Use` declaration, argument validation, and documentation by ensuring the number
of positional arguments in `Use` matches the count enforced by
`cobra.ExactArgs`. Either update `Use` to include three arguments or adjust the
argument validation to expect two. Complete or remove the truncated sentence in
the docstring on line 139. Also, either register the `--broken-vals` flag
properly if it is needed or remove all references to it from the documentation
to maintain clarity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
app/upgrades/v021/upgrades.go (3)
357-365: Panic upon exceeding reference count may be risky.Panic on line 361 might be too severe in production environments, where a more graceful error handling strategy could be preferable. Consider returning an error or logging a critical state instead of forcing a node crash.
func incrementReferenceCount(ctx sdk.Context, k *keepers.AppKeepers, valAddr sdk.ValAddress, period uint64) { historical, _ := k.DistrKeeper.GetValidatorHistoricalRewards(ctx, valAddr, period) if historical.ReferenceCount > 2 { - panic("reference count should never exceed 2") + ctx.Logger().Error("exceeded reference count limit", "valAddr", valAddr.String(), "period", period) + return } historical.ReferenceCount++ k.DistrKeeper.SetValidatorHistoricalRewards(ctx, valAddr, period, historical) }
309-309: 🛠️ Refactor suggestionReplace panic with graceful error handling.
Panic will cause the node to crash, which might be too severe in a production environment. Consider returning an error instead.
- panic(fmt.Sprintln("current stake is not delgator from slashed validator, and is more than maximum margin of error")) + ctx.Logger().Error("stake calculation error: current stake exceeds margin of error", + "delegator", del.GetDelegatorAddr(), + "validator", del.GetValidatorAddr(), + "stake", stake, + "currentStake", currentStake, + "margin", marginOfErr) + return rewards, false
326-328: 🛠️ Refactor suggestionReplace panic with error return for negative stake.
Similar to the previous comment, consider handling this validation error gracefully.
// sanity check if stake.IsNegative() { - panic("stake should not be negative") + ctx.Logger().Error("negative stake detected", "stake", stake) + return sdk.DecCoins{} }
🧹 Nitpick comments (2)
app/upgrades/v021/upgrades.go (2)
368-368: Fix typo in function name.The function name
CustommTokensFromSharesTruncatedhas an extra 'm' which appears to be a typo.-func CustommTokensFromSharesTruncated(t math.Int, ds math.LegacyDec, shares math.LegacyDec) math.LegacyDec { +func CustomTokensFromSharesTruncated(t math.Int, ds math.LegacyDec, shares math.LegacyDec) math.LegacyDec {
304-304: Clarify margin of error calculation.The code
math.LegacyNewDecWithPrec(50, 3)creates a decimal value of 0.05 (or 5%), but the parameter name might be confusing to readers. Consider adding a clearer comment or using a more explicit calculation.- marginOfErr := currentStake.Mul(math.LegacyNewDecWithPrec(50, 3)) // 5.0% + // 5.0% margin of error (0.05 = 50 * 10^-3) + marginOfErr := currentStake.Mul(math.LegacyNewDecWithPrec(50, 3))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)app/upgrades/v021/upgrades.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e-tests (e2e-basic)
- GitHub Check: e2e-tests (e2e-pfm)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/ict/basic_start_test.go (2)
581-588: Helper function added for protocol pool funding.This helper function provides a clean abstraction for the protocol pool funding operation, consistent with the PR's goal of replacing distribution module functionality.
Note that the function comment still references "DistributionFundCommunityPool" but the function name is "ProtocolPoolFundCommunityPool".
-// DistributionFundCommunityPool funds the community pool with the specified amount of coins. +// ProtocolPoolFundCommunityPool funds the community pool with the specified amount of coins. func ProtocolPoolFundCommunityPool(ctx context.Context, node *cosmos.ChainNode, keyName, amount string) error {
590-594: Helper function added for protocol pool queries.This helper function correctly implements the query functionality using the protocol pool gRPC client.
The function comment should be updated to match the actual function name.
-// DistributionCommunityPool returns the community pool +// ProtocolPoolQueryCommunityPool returns the community pool func ProtocolPoolQueryCommunityPool(ctx context.Context, node *cosmos.ChainNode) (*sdk.Coins, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/ict/basic_start_test.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e-tests (e2e-pfm)
- GitHub Check: e2e-tests (e2e-basic)
🔇 Additional comments (4)
tests/ict/basic_start_test.go (4)
15-15: Good addition of protocolpool module import.This change correctly adds the required import for the new protocolpool module, aligning with the PR's objective to replace distribution module functionality with the protocol pool.
304-304: Function replacement aligns with module migration.The change from
node.DistributionFundCommunityPoolto the newProtocolPoolFundCommunityPoolhelper function correctly implements the migration from the distribution module to the protocol pool module as specified in the PR objectives.
317-317: Query function replacement is consistent with module changes.The replacement of
chain.DistributionQueryCommunityPoolwithProtocolPoolQueryCommunityPoolmaintains consistency with the module migration strategy.
319-319:Details
✅ Verification successful
Verify type conversion from Dec to Int.
The change from using
LegacyNewDectoNewIntfor amount comparison represents a shift in type handling. While the change is likely correct as part of the SDK update, ensure that the comparison semantics remain consistent between decimal and integer types.
🏁 Script executed:
#!/bin/bash # Verify how comparison is handled in other places where protocol pool amounts are used rg -A 2 -B 2 "AmountOf.*NewInt" --glob "*.go"Length of output: 370
No action needed: using NewInt for comparison is correct
The
coins.AmountOf(...)method returns ansdkmath.Int, so comparing it againstsdkmath.NewInt(int64(amount))is type‐correct and preserves the intended semantics.
|
@angelorc This PR is ready to merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
246-254: Remove or guard debug instructions
Leftover debug clone commands and local replace hints (// run: git clone ...,// uncomment:) should be removed or gated behind a build tag before merging to avoid confusion in production builds.
📜 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 (1)
go.mod(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e-tests (e2e-pfm)
- GitHub Check: e2e-tests (e2e-basic)
🔇 Additional comments (5)
go.mod (5)
3-3: Confirm Go version alignment
The Go toolchain version has been bumped to 1.23.2 to match the updated Cosmos-SDK (v0.53.0) and CometBFT (v0.38.17) requirements. Ensure your CI and local development environments support Go 1.23.2.
6-43: Approve direct dependency upgrades
Core Cosmos and CometBFT dependencies (e.g.,cosmossdk.io/api,cosmossdk.io/collections,cometbft/cometbft,cosmos-sdk, as well as logging, math, store, tx, cobra, viper, Prometheus, etc.) have all been updated to versions that align with the v022 upgrade and the newprotocolpoolmodule. Changes look consistent with PR objectives.
53-231: Approve indirect dependency additions/updates
A large set of indirect dependencies has been refreshed (e.g.,cel.dev/expr,cosmossdk.io/depinject/schema, telemetry, DataDog, cloud provider SDKs, bitset, sonic, cockroachdb internals, security libraries, etc.). These appear required by the refactored modules and new CLI/debug tools.
234-244: Validate replace directives for security fixes
Thereplaceblock correctly pins deprecated or vulnerable modules (keyring,jwt-go,gin-gonic/gin,goleveldb) to safe versions. This aligns with known Cosmos SDK advisories.
255-255: Approve exclusion of legacy gogo/protobuf
Excluding the problematicv1.3.3ofgithub.com/gogo/protobufis intentional to avoid known store/query failures. This exclusion is valid.
3 participants
Context & Purpose
This PR addresses several critical issues experienced in #274, #276, and #277, primarily related to delegation stores and in-place-testnet functionality. The changes are necessary to restore normal network operation and correct delegation reward calculations.
1. CometBFT & Cosmos-SDK Version Alignment
x/protocol-poolto prepare for making use of block inflations for other than just staking rewards (ensured replaced x/distribution msgs with x/protocol-pools being used in x/fantoken)2. In-place Testnet Functionality Fix
3. Debug Tools: Custom Export CLI
This modified export function runs the upgradeHandler logic on a node's local state, and includes validation via crisis invariant checking and delegation rewards verification.
bitsongd custom-exportcommand to help debug v022 upgradeHandler4. V022 UpgradeHandler Implementation
Implemented custom upgradeHandler logic with several critical fixes:
Inject 2 missing slashing events in x/distr store to resolve crisis invariant verified via unit tests (skew tokens->shares to indicate 1 slash, but did not set a slash event in unit test store, replicating issue in mainnet state)
Solution for incorrect reference count issues verified via unit test (there never can be more than two, so we set test vals ref count to 2 prior to upgrade height, and then attempt to claim tokens after delegation height. Test will panic when attempting to claim post upgrade if upgrade handler does not resolve any broken ref count )
Remove any 0 share delegations in store verified via unit tests
Any delegations with improper tokens->shares calculation (within 10% range) has rewards claimed and sent to their proper withdraw destination .
Once the upgradehandler logic is run, we validate the state with the crisis invariant feature, as well as reiterating through delegations and querying their rewards with the normal function from the distribution keeper.
Additional Logging
A json log is created and any validator or store object being modified is logged for verification purposes during testing, as well as a debugging log for this specific upgrade unit tests
Summary by CodeRabbit
New Features
Improvements
AppKeepersstruct for improved maintainability.Bug Fixes
Chores
Documentation