v0.21 - [email protected], wasmvm@v2, ibc-go@v8 #265
Conversation
…y include impacted delegators in upgrade, ugrade go simulation test layout
e2e localbitsong without initial state
…-bitsong into v0.20.1-upgrade-rc
…ring ValidateAuthenticatorFeePayer, configure expidited props,
…x/accounts, PreComitter, add v021 upgrade unit tests
📝 WalkthroughWalkthroughThis pull request delivers extensive updates across workflows, Docker configurations, Makefiles, application code, and module definitions. The Go version and several GitHub Action versions have been upgraded, and base images have been updated throughout Docker and CI files. Major modules such as “cadance,” “smart-account,” and “fantoken” receive new features and refactorings—including new protobuf definitions, keeper methods, CLI commands, and tests—while dependencies and error-handling mechanisms are modernized (e.g., shifting from sdk.Int to math.Int and using updated error wrappers). Integration tests and documentation are also revised. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant AD as AuthenticatorDecorator
participant SAK as SmartAccountKeeper
participant NH as NextHandler
C->>AD: Submit transaction (tx)
AD->>AD: Validate fee payer (must be first signer)
AD->>AD: Retrieve selected authenticators for each message
AD->>SAK: (Optional) Validate smart account credentials
AD->>NH: Pass transaction to next AnteHandler
NH-->>AD: Return result (success/failure)
AD-->>C: Respond with final transaction outcome
sequenceDiagram
participant EB as EndBlocker
participant CK as CadanceKeeper
participant EC as ExecuteContract
participant HE as handleError
participant C as Contract
EB->>CK: Retrieve all registered contracts
loop For each Contract
CK->>C: Check contract status (jailed or not)
alt Not Jailed
EB->>EC: Execute contract (end-block message)
alt Execution Error
EC->>HE: Report error and determine if out-of-gas
HE->>CK: Set contract as jailed if needed
else Execution OK
EC-->>CK: Update contract state
end
else
CK->>EB: Skip jailed contract
end
end
EB-->>CK: End block processing complete
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (5)
x/cadance/types/msgs_test.go (1)
34-34:⚠️ Potential issueHandle potential error from AccAddressFromBech32.
The error from
AccAddressFromBech32is being ignored. This could mask potential issues with invalid addresses.-acc, _ := sdk.AccAddressFromBech32(p.Authority) +acc, err := sdk.AccAddressFromBech32(p.Authority) +suite.Require().NoError(err, "failed to parse authority address")x/cadance/types/msgs.go (3)
16-19:⚠️ Potential issueFix inconsistent type names in constants.
The constant names use "FeePay" while the actual message types use "Cadance". This inconsistency could lead to confusion.
Apply this diff to maintain consistency:
- TypeMsgRegisterFeePayContract = "register_cadance_contract" - TypeMsgUnregisterFeePayContract = "unregister_cadance_contract" - TypeMsgUnjailFeePayContract = "unjail_cadance_contract" + TypeMsgRegisterCadanceContract = "register_cadance_contract" + TypeMsgUnregisterCadanceContract = "unregister_cadance_contract" + TypeMsgUnjailCadanceContract = "unjail_cadance_contract"
54-55:⚠️ Potential issueFix incorrect type string in MsgUnregisterCadanceContract.
The Type() method returns TypeMsgRegisterFeePayContract instead of TypeMsgUnregisterFeePayContract.
Apply this diff to fix the type string:
-func (msg MsgUnregisterCadanceContract) Type() string { return TypeMsgRegisterFeePayContract } +func (msg MsgUnregisterCadanceContract) Type() string { return TypeMsgUnregisterCadanceContract }
76-77:⚠️ Potential issueFix incorrect type string in MsgUnjailCadanceContract.
The Type() method returns TypeMsgRegisterFeePayContract instead of TypeMsgUnjailFeePayContract.
Apply this diff to fix the type string:
-func (msg MsgUnjailCadanceContract) Type() string { return TypeMsgRegisterFeePayContract } +func (msg MsgUnjailCadanceContract) Type() string { return TypeMsgUnjailCadanceContract }app/upgrades/v018/upgrades.go (1)
122-130:⚠️ Potential issueUnify context usage for keeper operations.
The code mixes
ctxandsdkCtxwhen handling parameters:
- Line 122: Uses
sdkCtxto get mint params- Line 124: Uses
ctxto set mint params- Line 127-130: Uses
ctxfor bank paramsThis inconsistency could lead to state inconsistencies.
Apply these changes to use
sdkCtxconsistently:-keepers.MintKeeper.Params.Set(ctx, mintParams) +keepers.MintKeeper.Params.Set(sdkCtx, mintParams) -bankParams := keepers.BankKeeper.GetParams(ctx) +bankParams := keepers.BankKeeper.GetParams(sdkCtx) -keepers.BankKeeper.SetParams(ctx, bankParams) +keepers.BankKeeper.SetParams(sdkCtx, bankParams)
🧹 Nitpick comments (121)
x/cadance/types/params_test.go (1)
11-52: Consider enhancing test cases for better clarity and coverage.While the test cases are well-structured, consider these improvements:
- Add comments explaining the minimum gas threshold value
- Include edge cases around the minimum gas threshold
- Make test case descriptions more specific (e.g., "Success - Meets min Gas" is used twice)
Example improvement:
testCases := []struct { name string params types.Params success bool }{ { - "Success - Default", + "Success - Default params should be valid", types.DefaultParams(), true, }, { - "Success - Meets min Gas", + "Success - Exact minimum required gas (100_000)", types.NewParams(100_000), true, }, { - "Success - Meets min Gas", + "Success - Above minimum gas requirement", types.NewParams(500_000), true, }, + { + "Fail - Just below minimum gas requirement", + types.NewParams(99_999), + false, + }, // ... rest of the cases }tests/bitsongibctesting/chain.go (6)
38-39: Consider avoiding a shared global slice for testing directories.
Using a package-levelTestingDirectoriescan complicate concurrency or parallel test runs, and may become a maintenance burden. A more robust approach is to manage directories at the individual test level, ensuring they are created and cleaned up under each test’s lifecycle.
40-48: Use temporary test directories and ensure cleanup.
The code creates a random directory name (rand.Int()) for test data. If tests run in parallel or crash, leftover directories may accumulate. Consider using a managed approach—such ast.TempDir()if atesting.Tinstance is available—or another cleanup mechanism to ensure proper removal of these directories after tests complete.
161-224: Refactor repeated code for obtaining account numbers and sequences.
The block of code extractingaccountNumbersandaccountSequencesfrom the signers is nearly identical to logic found in other functions. Consider consolidating it into a shared helper to avoid duplication and reduce maintenance overhead.
226-301: Consider unifying transaction signing flows.
Similar transaction signing logic appears in multiple functions (SendMsgsNoCheck,SendMsgsFromPrivKeys, etc.). If possible, centralizing or extracting common pieces will help ensure consistent handling of error cases, signatures, and block commits.
410-410: Address the TODO to set the fee payer.
There's a placeholder comment indicating that the fee payer is not yet set. If fee payer logic is required, implementing it sooner can prevent unexpected behavior or misconfigurations in fee-handling.
600-674: Consider standardizing or streamlining compound signature handling.
Serializing compound signatures as JSON in theSignaturefield is unconventional and might not be fully compatible with standard Cosmos SDK multi-sign workflows. If possible, adopting an existing multi-sig pattern (e.g.,[sdk.MultiSignature]or a custom extension) may reduce potential interoperability and parsing complexities later.tests/ict/polytone_suite.go (4)
53-55: Consider using a non-Nop reporter for enhanced logs.
Currently, the suite uses a NopReporter which suppresses logs. If you want more detailed logging, consider switching to a different reporter.
127-132: Remove or uncomment stale code if not needed.
These lines are commented out, suggesting they're no longer in use. If they're not part of debug or future usage, removing them helps keep the code clean.
133-134: Different error handling approach.
Elsewhere in this file, errors are returned directly instead of using require. Verify if this discrepancy is intentional.
281-283: Improved error handling.
Switching fromrequire.NoErrorto returning errors is more flexible for controlling flow. Consider adding context or wrapping errors to aid debugging, especially in complex scenarios.Also applies to: 285-287, 290-292
app/upgrades/v020/upgrades.go (3)
70-71: Consider removing commented-out code.
This double-commented line might create confusion.
98-100: New patch function name references V018 in a V020 upgrade.
Consider aligning function naming with the current upgrade (e.g.,V020ManualDelegationRewardsPatch) to reduce confusion.
207-210: Usage offmt.Printffor debugging.
Consider switching to structured logging viasdkCtx.Logger()for consistency and to avoid stdout noise in production.btsgutils/btsgcli/tx_cmd_wrap.go (3)
15-19: Improve command description clarity.The
TxIndexCmdfunction simply sets a short description. Consider adding more insights or usage examples for users, especially if the module has complex commands.
21-37: Document the contract forTxCliDesc.Although you have a comment regarding the contract (
len(args) = NumArgs), additional clarifications (e.g., ifNumArgsincludes or excludes the signer field) could help maintainers more easily understand the structure and usage constraints.
80-117: Consider advanced error wrapping or structured logs for CLI transactions.While the command runs and broadcasts transactions, failures are wrapped in
errbut without structured logs or advanced metadata. For debugging CLI transaction failures, especially with complex flags, adding structured logs with relevant command context or more robust error wrapping (e.g., including the module name or subcommand in the error message) could simplify troubleshooting.x/cadance/abci.go (3)
38-44: Skip jailed contracts earlier for efficiency.You skip jailed contracts inside the loop. If the number of jailed contracts is large, consider skipping them at retrieval time or offering a separate structure for active vs. jailed contracts. This avoids extra loop iterations.
48-50: Clarify reusederrvariable.
handleErrorreuses the sameerrfrom the prior operation. If multiple operations rely on the sameerrvariable, consider renaming or resetting it to minimize confusion about which operation triggered the error.
114-124: Expand out-of-gas detection logic if needed.
IsOutOfGasErroronly checks specific store errors. If future store or VM errors trigger out-of-gas–like conditions, they might not be detected. Evaluate whether you need to handle additional error types for more comprehensive coverage.x/cadance/module.go (4)
48-52: Consider test coverage for default parameters.
DefaultGenesissets default params but it's unclear if there's a test to confirm that these defaults remain valid over time (e.g., with new required parameters). Ensure test coverage for default parameter usage.
135-138: Review query server for performance.Registering a new query server is straightforward, but if you expect high query loads, ensure the underlying
keeper.NewQuerier(a.keeper)is efficient. If queries become expensive, implementing caching or additional indexing in the keeper might be beneficial.
145-148: Clarify the module identity inEndBlock.The function doc mentions "fantoken" instead of "cadance." Update the comment to reflect this module’s identity to maintain clarity and consistency in your codebase.
-// EndBlock returns the end blocker for the fantoken module. It returns no validator updates. +// EndBlock returns the end blocker for the cadance module. It returns no validator updates.
154-156: Inject a compile-time check for consensus version.To avoid confusion about the correct version after large refactors, you might add a simple compile-time check that references
ConsensusVersionfrom a central location or a commented constant, ensuring developers consciously update it for every major PR.cmd/bitsongd/cmd/init.go (1)
124-124: Handle potential error from GetBool
Ignoring the error fromcmd.Flags().GetBool(FlagOverwrite)may cause unforeseen issues if flag retrieval fails. Consider checking and handling the returned error.app/upgrades/v021/upgrades.go (1)
36-73: Ensure thorough error handling for staking and distribution operations
k.StakingKeeper.GetAllValidators(ctx)andk.StakingKeeper.GetValidatorDelegations(ctx, valAddr)discard possible errors. Handle those errors to prevent silent failures when reapplying the v018 patch logic.btsgutils/btsgcli/query_cmd_wrap.go (4)
17-40: Global variable and descriptor struct
lastQueryModuleNameis a global variable that could invite concurrency risks if used in parallel. TheQueryDescriptorfields are well-defined, but consider thread-safety updates if necessary.
50-59: Use of global variable in QueryIndexCmd
ModifyinglastQueryModuleNamemight cause collisions in concurrent scenarios. A more scoped approach would be safer.
61-65: Unused return value from descriptor factory
The second return value (Q) is discarded. Validate if this is intentional or if you need the resultingproto.Message.
71-89: Reflection-based descriptor preparation
Using generics and reflection for pagination and function name detection is flexible but keep an eye on performance overhead in production scenarios.app/ante_handler.go (1)
160-164: IntroducingNewCircuitBreakerDecorator.
The circuit breaker merges classic and authenticator flows. Check that any fallback or gating logic is thoroughly covered by integration tests, ensuring neither path conflicts with the other.Potentially separate the circuit-breaker logic from aggregator logic in future for clarity.
x/fantoken/module.go (2)
11-11: Commented out simulation import.
If you’re moving away from the older simulation approach or waiting to re-implement it, keep track to remove or re-enable it as needed.
173-177: Commented-out simulation methods.
Keeping them commented might hinder clarity. If these are deprecated, consider removing the code. Otherwise, note aTODOcomment if it’s to be restored.app/export.go (1)
88-92: Validating & retrieving all delegations.
EnsuringGetAllDelegationsreturns no error is important. The panic on error might be acceptable in the zero-height context, but consider whether you should gracefully handle partial data to allow partial state export.x/cadance/keeper/cadance.go (2)
11-11: Minor grammar fix in documentation.
The comment says "Store Keys for cadance contract s (both jailed and unjailed)." Consider removing the trailing "s" or clarifying the singular/plural usage to avoid confusion.-// Store Keys for cadance contract s (both jailed and unjailed) +// Store keys for cadance contracts (both jailed and unjailed)
178-184: Consider returning a no-op instead of error if jail status is unchanged.
Currently, if a contract is already in the requested jail status, an error is returned. Depending on app requirements, you might prefer returning success silently if no state change is needed.x/smart-account/ante/ante.go (2)
1-34: Segregate imports by priority.
Consider grouping standard library, external, and local imports for better readability.-import ( - "bytes" - "fmt" - ... - smartaccountkeeper "github.com/bitsongofficial/go-bitsong/x/smart-account/keeper" - ... -) +import ( + // Standard library + "bytes" + "fmt" + ... + + // External packages + smartaccountkeeper "github.com/bitsongofficial/go-bitsong/x/smart-account/keeper" + ... +)
128-145: Consider splitting out message authentication logic.
The loop for message authentication is lengthy. Extracting the logic into smaller functions can improve readability and testability.btsgutils/btsgcli/parsers.go (1)
1-468: Recommend adding tests for your CLI parsing methods.The parsing logic here handles a wide variety of argument and flag types, including tricky edge cases like negative integers and structured coin input. Ensuring broad test coverage would boost confidence in the correctness of these parsers and help catch regressions or overlooked corner cases.
app/app.go (1)
507-509: Return pointer to the module manager instead of copying.Returning
*app.mmrather than*app.mmby value would prevent accidental divergence between the original manager and its consumer. If you intend to keep it read-only, returning by value might be intentional, but otherwise, consider returning*module.Managerdirectly.app/keepers/keepers.go (2)
210-215: TODO regarding skipUpgradeHeights from config.
You have a TODO comment here. Consider finalizing the approach to fetch from external config.
471-473: Modifying Gov config and creating GovKeeper.
Verify ifMaxMetadataLen = 10200is the desired final limit or should be configurable.x/cadance/module_test.go (1)
1-7: NewCadanceModuleSuitetest scaffold.
This suite extendsKeeperTestHelper, which is a good step toward comprehensive Cadance module testing. However, consider adding actual test functions to verify Cadance-specific functionality.x/cadance/types/errors.go (1)
7-11: LGTM! Well-structured error definitions.The error types are well-defined with clear messages and sequential codes. They comprehensively cover the contract jailing states.
Consider adding godoc comments for each error variable to provide more context about when these errors are returned. For example:
var ( + // ErrContractJailed is returned when attempting to execute operations on a jailed contract ErrContractJailed = errorsmod.Register(ModuleName, 1, "contract is jailed") + // ErrContractNotJailed is returned when attempting to unjail a contract that is not jailed ErrContractNotJailed = errorsmod.Register(ModuleName, 2, "contract is not jailed") + // ErrContractAlreadyJailed is returned when attempting to jail an already jailed contract ErrContractAlreadyJailed = errorsmod.Register(ModuleName, 3, "contract is already jailed") )btsgutils/generic_helper.go (1)
8-17: Consider adding error handling for reflection edge cases.While the implementation is generally sound, it could benefit from handling potential reflection edge cases:
- The type assertion could panic if reflection creates an incompatible type
- No handling for cases where reflection fails to create a new instance
Consider this safer implementation:
-func MakeNew[T any]() T { +func MakeNew[T any]() (T, error) { var v T if typ := reflect.TypeOf(v); typ.Kind() == reflect.Ptr { elem := typ.Elem() + // Handle cases where elem is invalid + if elem == nil { + return v, fmt.Errorf("invalid type: %v", typ) + } //nolint:forcetypeassert - return reflect.New(elem).Interface().(T) // must use reflect + // Use safe type assertion + if instance, ok := reflect.New(elem).Interface().(T); ok { + return instance, nil + } + return v, fmt.Errorf("type assertion failed for: %v", typ) } else { - return *new(T) // v is not ptr, alloc with new + return *new(T), nil // v is not ptr, alloc with new } }app/upgrades/v016/upgrades.go (1)
13-13: Document the unused plan parameter.The
_planparameter is unused in the upgrade handler. Consider adding a comment explaining why it's not used or remove the underscore prefix if it might be used in the future.- return func(ctx context.Context, _plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { + // plan parameter is not used as this upgrade doesn't require any specific plan parameters + return func(ctx context.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {app/params/amino.go (1)
15-15: Consider making address prefixes configurable.While the current implementation works, consider making the address prefixes configurable through constants or configuration to improve maintainability.
+const ( + AccAddressPrefix = "bitsong" + ValAddressPrefix = "bitsongvaloper" +) func MakeEncodingConfig() EncodingConfig { cdc := codec.New() - interfaceRegistry := testutil.CodecOptions{AccAddressPrefix: "bitsong", ValAddressPrefix: "bitsongvaloper"}.NewInterfaceRegistry() + interfaceRegistry := testutil.CodecOptions{AccAddressPrefix: AccAddressPrefix, ValAddressPrefix: ValAddressPrefix}.NewInterfaceRegistry()x/cadance/types/params.go (1)
12-12: Consider defining gas limit constants.The gas limit values (100,000) are used in multiple places. Consider defining them as package constants for better maintainability.
+const ( + DefaultContractGasLimit = 100_000 + MinimumContractGasLimit = 100_000 +) func DefaultParams() Params { return Params{ - ContractGasLimit: 100_000, + ContractGasLimit: DefaultContractGasLimit, } } func (p Params) Validate() error { - minimumGas := uint64(100_000) + minimumGas := uint64(MinimumContractGasLimit)Also applies to: 27-27
x/cadance/types/msgs_test.go (1)
20-22: Consider using test constants for better maintainability.The hardcoded address in
SetupTestwould be more maintainable if defined as a package-level test constant.+const ( + testGovModule = "juno10d07y265gmmuvt4z0w9aw880jnsr700jvss730" +) func (suite *MsgsTestSuite) SetupTest() { - suite.govModule = "juno10d07y265gmmuvt4z0w9aw880jnsr700jvss730" + suite.govModule = testGovModule }x/cadance/types/codec_test.go (1)
20-33: Consider adding negative test cases.While the test verifies successful interface registration, it would be beneficial to add negative test cases to ensure proper error handling when registering invalid implementations.
Consider adding test cases for:
- Attempting to register duplicate implementations
- Registering invalid message types
- Verifying proper error messages
app/posthandler.go (1)
12-29: Enhance documentation for the circuit breaker pattern.While the code comment mentions the circuit breaker pattern, it would be helpful to document:
- The purpose and benefits of using this pattern
- The conditions under which the circuit breaker activates
- The impact on transaction processing
scripts/makefiles/format.mk (1)
27-29: Consider DRYing up the find commands.The find commands have duplicated patterns that could be extracted into variables for better maintainability.
Consider refactoring like this:
+FIND_GO_FILES := find . -name '*.go' -type f -not -path "*.git*" -not -path "*/mocks/*" -not -name '*.pb.go' -not -name '*.pulsar.go' -not -name '*.gw.go' + format-format: - @find . -name '*.go' -type f -not -path "*.git*" -not -path "*/mocks/*" -not -name '*.pb.go' -not -name '*.pulsar.go' -not -name '*.gw.go' | xargs go run mvdan.cc/gofumpt -w . - @find . -name '*.go' -type f -not -path "*.git*" -not -path "*/mocks/*" -not -name '*.pb.go' -not -name '*.pulsar.go' -not -name '*.gw.go' | xargs go run github.com/client9/misspell/cmd/misspell -w - @find . -name '*.go' -type f -not -path "*.git*" -not -path "/*mocks/*" -not -name '*.pb.go' -not -name '*.pulsar.go' -not -name '*.gw.go' | xargs go run golang.org/x/tools/cmd/goimports -w -local github.com/bitsongofficial/go-bitsong + @$(FIND_GO_FILES) | xargs go run mvdan.cc/gofumpt -w . + @$(FIND_GO_FILES) | xargs go run github.com/client9/misspell/cmd/misspell -w + @$(FIND_GO_FILES) | xargs go run golang.org/x/tools/cmd/goimports -w -local github.com/bitsongofficial/go-bitsongbtsgutils/btsgcli/dynamic.go (1)
40-47: Consider optimizing string conversion performance.The current implementation creates a new regex pattern for each function call. Consider making it a package-level constant to improve performance.
+var pascalToKebabRegex = regexp.MustCompile(`([a-z0-9])([A-Z])`) + // pascalToKebab converts PascalCase string to kebab-case string func pascalToKebab(s string) string { - reg := regexp.MustCompile(`([a-z0-9])([A-Z])`) - s = reg.ReplaceAllString(s, `${1}-${2}`) + s = pascalToKebabRegex.ReplaceAllString(s, `${1}-${2}`)x/fantoken/types/expected_keepers.go (1)
34-36: Consider removing commented-out interface methods.These commented-out methods in the BankKeeper interface should either be implemented or removed to maintain code cleanliness.
x/cadance/genesis_test.go (1)
21-50: Test cases look good but could be enhanced.The test cases cover basic scenarios well, including default genesis, custom genesis, and invalid gas amount. Consider adding edge cases for ContractGasLimit.
Add test cases for:
{ "Success - Custom Genesis", types.GenesisState{ Params: types.Params{ ContractGasLimit: 500_000, }, }, true, }, + { + "Success - Maximum Gas Limit", + types.GenesisState{ + Params: types.Params{ + ContractGasLimit: uint64(^uint64(0)), // max uint64 + }, + }, + true, + },x/cadance/genesis.go (2)
53-55: Consider using more graceful error handling instead of panic.While panicking during genesis initialization is common, consider using a more graceful error handling approach that allows for better error recovery and testing.
- if err := ValidateGenesis(data); err != nil { - panic(err) - } + if err := ValidateGenesis(data); err != nil { + return fmt.Errorf("failed to validate genesis state: %w", err) + } - if err := k.SetParams(ctx, data.Params); err != nil { - panic(err) - } + if err := k.SetParams(ctx, data.Params); err != nil { + return fmt.Errorf("failed to set params: %w", err) + }Also applies to: 58-60
13-14: Add detailed godoc comments for exported functions.The exported functions lack comprehensive documentation. Add detailed godoc comments to improve code maintainability.
+// NewGenesisState creates a new GenesisState object with the given parameters. +// It is used to initialize the module's state during genesis. func NewGenesisState(params types.Params) *types.GenesisState { +// DefaultGenesisState returns a default GenesisState object with default parameters. +// This is used when no custom genesis state is provided. func DefaultGenesisState() *types.GenesisState { +// GetGenesisStateFromAppState retrieves the Cadance module's GenesisState from the +// provided raw application genesis state. If no state exists, returns an empty state. func GetGenesisStateFromAppState(cdc codec.Codec, appState map[string]json.RawMessage) *types.GenesisState { +// ExportGenesis returns a GenesisState for a given context and keeper. +// It retrieves the current state of the module for genesis export. func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {Also applies to: 20-21, 25-27, 63-64
x/cadance/keeper/querier.go (2)
24-34: Consider adding pagination validation.While the implementation is correct, consider validating the pagination parameters before making the keeper call to ensure they are within acceptable bounds.
func (q Querier) CadanceContracts(stdCtx context.Context, req *types.QueryCadanceContracts) (*types.QueryCadanceContractsResponse, error) { ctx := sdk.UnwrapSDKContext(stdCtx) + if req.Pagination != nil { + if req.Pagination.Limit > types.MaxPageLimit { + return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "page limit %d exceeds maximum %d", req.Pagination.Limit, types.MaxPageLimit) + } + } + contracts, err := q.keeper.GetPaginatedContracts(ctx, req.Pagination)
55-64: Consider adding context timeout handling.The implementation should handle context cancellation to ensure proper resource cleanup.
func (q Querier) Params(stdCtx context.Context, _ *types.QueryParamsRequest) (*types.QueryParamsResponse, error) { + if err := stdCtx.Err(); err != nil { + return nil, err + } ctx := sdk.UnwrapSDKContext(stdCtx)app/config.go (1)
23-35: Consider making chainId configurable.While extracting the chainId to a variable improves consistency, consider making it configurable through environment variables or configuration files for better flexibility across different environments.
+const defaultChainId = "bitsong-test-1" + +func getChainId() string { + if envChainId := os.Getenv("BITSONG_CHAIN_ID"); envChainId != "" { + return envChainId + } + return defaultChainId +} func DefaultConfig() network.Config { encCfg := MakeEncodingConfig() - chainId := "bitsong-test-1" + chainId := getChainId()btsgutils/btsgcli/flag_advice.go (1)
73-77: Consider using errors.Join for multiple flag errors.Instead of panicking on the first error, consider collecting all flag errors and returning them together.
+var errs []error fs.VisitAll(func(flag *pflag.Flag) { err := cmd.MarkFlagRequired(flag.Name) if err != nil { - panic(err) + errs = append(errs, fmt.Errorf("flag %s: %w", flag.Name, err)) } }) +if len(errs) > 0 { + panic(errors.Join(errs...)) +}scripts/makefiles/proto.mk (1)
5-7: Consider using ARG in Dockerfile instead of Makefile variables.While the current implementation works, consider moving the version and image name to a Dockerfile using ARG for better Docker integration and version management.
-protoVer=0.15.1 -protoImageName=ghcr.io/cosmos/proto-builder:$(protoVer) -protoImage=$(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace $(protoImageName)Create a new Dockerfile:
ARG PROTO_VERSION=0.15.1 FROM ghcr.io/cosmos/proto-builder:${PROTO_VERSION} WORKDIR /workspacex/cadance/keeper/keeper.go (1)
17-26: Add field documentation for the Keeper struct.Each field in the Keeper struct should be documented to improve code maintainability.
// Keeper of the cadance store type Keeper struct { + // storeKey defines the store key for persistent storage storeKey storetypes.StoreKey + // cdc defines the codec for encoding and decoding binary data cdc codec.BinaryCodec + // wasmKeeper provides access to the wasm module's functionality wasmKeeper wasmkeeper.Keeper + // contractKeeper provides permissioned access to contract operations contractKeeper *wasmkeeper.PermissionedKeeper + // authority defines the module's authority for governance operations authority string }x/fantoken/keeper/fantoken.go (1)
3-12: Consider grouping imports by domain.The imports could be better organized by grouping standard library, Cosmos SDK, and project-specific imports.
import ( + // Standard library + "fmt" + + // Cosmos SDK "cosmossdk.io/math" storetypes "cosmossdk.io/store/types" db "github.com/cosmos/cosmos-db" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" gogotypes "github.com/cosmos/gogoproto/types" + // Project specific "github.com/bitsongofficial/go-bitsong/x/fantoken/types" )tests/ict/polytone_types.go (1)
8-13: Consider automating type synchronization.Instead of manually copying types, consider using a code generation tool or git submodules to keep the types in sync with the source repository.
This would help prevent drift between the source and copied types, reducing maintenance overhead and potential bugs.
x/cadance/keeper/msg_server.go (1)
28-38: Consider adding event emission for contract registrationThe
RegisterCadanceContracthandler should emit events to allow external systems to track contract registrations.func (k msgServer) RegisterCadanceContract(goCtx context.Context, req *types.MsgRegisterCadanceContract) (*types.MsgRegisterCadanceContractResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) // Validate request if err := req.ValidateBasic(); err != nil { return nil, err } + // Emit event + if err := ctx.EventManager().EmitTypedEvent( + &types.EventRegisterCadanceContract{ + SenderAddress: req.SenderAddress, + ContractAddress: req.ContractAddress, + }, + ); err != nil { + return nil, err + } + return &types.MsgRegisterCadanceContractResponse{}, k.RegisterContract(ctx, req.SenderAddress, req.ContractAddress) }x/cadance/client/cli/query.go (1)
28-58: Add output format flag to contracts queryConsider adding a flag to allow different output formats (e.g., text, json, yaml) for better integration with scripts.
func GetCmdShowContracts() *cobra.Command { cmd := &cobra.Command{ Use: "contracts", Short: "Show addresses of all current cadance contracts", Args: cobra.ExactArgs(0), RunE: func(cmd *cobra.Command, _ []string) error { clientCtx, err := client.GetClientQueryContext(cmd) if err != nil { return err } + outputFormat, err := cmd.Flags().GetString("output") + if err != nil { + return err + } + queryClient := types.NewQueryClient(clientCtx) pageReq, err := client.ReadPageRequest(cmd.Flags()) @@ -49,11 +54,21 @@ return err } + switch outputFormat { + case "json": + return clientCtx.PrintProto(res) + case "text": + for _, contract := range res.Contracts { + fmt.Fprintln(cmd.OutOrStdout(), contract.Address) + } + return nil + default: + return fmt.Errorf("unsupported output format %s", outputFormat) + } return clientCtx.PrintProto(res) }, } flags.AddQueryFlagsToCmd(cmd) + cmd.Flags().String("output", "json", "Output format (json|text)") flags.AddPaginationFlagsToCmd(cmd, "contracts") return cmd }x/fantoken/types/validation.go (1)
81-95: Add minimum fee validationThe
ValidateFeesfunction should include checks for minimum fee amounts to prevent dust attacks.func ValidateFees(issueFee, mintFee, burnFee sdk.Coin) error { + minFee := sdk.NewCoin(issueFee.Denom, math.NewInt(1000)) // Example minimum fee + if err := issueFee.Validate(); err != nil { return err } + if issueFee.IsLT(minFee) { + return errors.Wrapf(ErrInvalidFee, "issue fee %s is less than minimum required %s", issueFee, minFee) + } if err := mintFee.Validate(); err != nil { return err } + if mintFee.IsLT(minFee) { + return errors.Wrapf(ErrInvalidFee, "mint fee %s is less than minimum required %s", mintFee, minFee) + } if err := burnFee.Validate(); err != nil { return err } + if burnFee.IsLT(minFee) { + return errors.Wrapf(ErrInvalidFee, "burn fee %s is less than minimum required %s", burnFee, minFee) + } return nil }tests/ict/polytone_test.go (2)
1-1: Consider adding license header.The source attribution comment is good, but consider adding a license header to maintain consistency with other files in the codebase.
83-98: Add error handling for JSON marshaling edge cases.The
HelloMessagefunction could benefit from additional error handling for JSON marshaling edge cases, such as handling circular references or invalid UTF-8 strings.x/fantoken/handler_test.go (2)
26-26: Remove commented-out code.The commented-out line should be removed as it's no longer needed with the new testing framework.
- // suite.ctx = suite.app.BaseApp.NewContext(false)
53-77: Add test cases for additional scenarios.The test coverage could be enhanced by adding test cases for:
- Multiple parameter changes in a single proposal
- Invalid parameter values (not just negative values)
- Boundary conditions for fee values
Also applies to: 79-101
x/fantoken/keeper/grpc_query.go (2)
31-31: Consider using more descriptive error messages.The error message "empty denom" could be more descriptive to help with debugging.
- return nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "empty denom") + return nil, errors.Wrap(sdkerrors.ErrInvalidRequest, "fantoken denom cannot be empty")
95-96: Optimize slice allocation.The slice append operation could be optimized by pre-allocating the result slice.
- var result []*types.FanToken - result = append(result, fantokens...) + result := make([]*types.FanToken, len(fantokens)) + copy(result, fantokens)x/fantoken/types/fantoken.go (2)
81-89: Enhance validation for FanToken.The
Validatemethod could be enhanced to include additional checks:
- Validate that MaxSupply is positive
- Ensure URI format is valid if present
112-130: Add length validation for metadata fields.The
Validatemethod for Metadata should include length validation for name, symbol, and URI fields to prevent excessively long values.app/upgrades/v010/upgrades.go (1)
78-79: Update to use math.NewInt for consistency.The code mixes old and new math package usage. Consider updating
sdk.NewCoinsandsdk.NewCointo use the new math package consistently.-mintCoins := sdk.NewCoins(sdk.NewCoin(appparams.DefaultBondDenom, math.NewInt(CassiniMintAmount))) +mintCoins := sdk.NewCoins(sdk.NewCoin(appparams.DefaultBondDenom, math.NewInt(CassiniMintAmount)))btsgutils/slice_helper.go (1)
45-55: Optimize map initialization in ContainsDuplicate.The map is initialized with size 0 but will grow to accommodate all elements. Consider initializing with len(arr) for better performance.
-visited := make(map[any]bool, 0) +visited := make(map[any]bool, len(arr))x/cadance/client/cli/tx.go (2)
16-22: Update module description in CLI command.The command uses "Clock subcommands" in the description but the module name is "cadance". Consider updating for consistency.
- Short: "Clock subcommands.", + Short: "Cadance module subcommands",
37-39: Fix typos in command description.Remove extra spaces in the description strings.
- Short: "Register a cadance contract .", - Long: "Register a cadance contract . Sender must be admin of the contract.", + Short: "Register a cadance contract", + Long: "Register a cadance contract. Sender must be admin of the contract.",x/cadance/types/msgs.go (1)
47-48: Handle potential errors in GetSigners methods.The GetSigners methods ignore potential errors from AccAddressFromBech32. While these errors should never occur due to ValidateBasic checks, it's good practice to handle them for robustness.
Consider handling the errors or adding a comment explaining why they're ignored:
func (msg MsgRegisterCadanceContract) GetSigners() []sdk.AccAddress { - from, _ := sdk.AccAddressFromBech32(msg.SenderAddress) + // Error is already checked in ValidateBasic + from, err := sdk.AccAddressFromBech32(msg.SenderAddress) + if err != nil { + panic(err) + } return []sdk.AccAddress{from} }Also applies to: 69-70, 91-92, 119-120
app/upgrades/v021/upgrade_test.go (1)
61-61: Remove debug print statement.Debug print statements should not be committed to the codebase.
-fmt.Printf("real dels: %q", dels)x/cadance/keeper/querier_test.go (1)
108-162: Consider refactoring to reduce code duplication.This test function shares significant code with
TestQueryCadanceContracts. Consider extracting common test setup and validation logic into helper functions.Example refactor:
func (s *IntegrationTestSuite) setupContractTestCases(addr sdk.AccAddress) []struct { desc string contracts []string } { return []struct { desc string contracts []string }{ { desc: "On empty", contracts: []string(nil), }, // ... other cases } } func (s *IntegrationTestSuite) validateAndCleanupContracts(contracts []string, expectJailed bool) { resp, err := s.queryClient.CadanceContracts(s.Ctx, &types.QueryCadanceContracts{}) s.Require().NoError(err) s.Require().NotNil(resp) for _, contract := range resp.CadanceContracts { s.Require().Contains(contracts, contract.ContractAddress) s.Require().Equal(expectJailed, contract.IsJailed) } for _, contract := range contracts { s.App.AppKeepers.CadanceKeeper.RemoveContract(s.Ctx, contract) } }x/cadance/abci_test.go (1)
35-40: Consider adding documentation for embedded test contracts.The embedded WASM contracts should be documented to explain their purpose and behavior.
app/test_helper.go (1)
74-74: Consider adding documentation for genesis state initialization.The default genesis state bytes and custom chain ID setup should be documented to explain their purpose and usage.
Also applies to: 81-108
x/fantoken/keeper/keeper.go (1)
247-249: Consider adding more context to the error message.The error message for disabled minting could provide more information about why minting is disabled and potential solutions.
- return errors.Wrapf(types.ErrInvalidMinter, "the minting is disabled") + return errors.Wrapf(types.ErrInvalidMinter, "the minting is disabled for this fantoken as it has reached its final supply state")x/cadance/keeper/msg_server_test.go (3)
103-105: Consider adding cleanup after each test case.The test removes contracts at the end of each test case, but consider using
deferat the beginning of the test for better cleanup guarantee in case of test failures.s.Run(tc.desc, func() { + defer func() { + s.App.AppKeepers.CadanceKeeper.RemoveContract(s.Ctx, contractAddress) + s.App.AppKeepers.CadanceKeeper.RemoveContract(s.Ctx, contractAddressWithAdmin) + }() // Set params params := types.DefaultParams()
13-107: Well-structured test cases with comprehensive coverage.The test function covers essential scenarios including:
- Successful contract registration
- Admin permission checks
- Invalid address handling
- Jailed contract handling
Consider adding these additional test cases:
- Contract with empty admin
- Maximum number of contracts per address (if there's a limit)
192-225: Consider adding more edge cases to duplicate registration tests.The test covers basic duplicate registration scenarios, but could be enhanced with:
- Attempting to register an unregistered contract after unregistering it
- Testing duplicate operations with different senders
proto/buf.gen.swagger.yaml (1)
5-5: Add newline at end of file.Add a newline character at the end of the file to follow YAML best practices.
opt: logtostderr=true,fqn_for_swagger_name=true,simple_operation_ids=true +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
scripts/protocgen.sh (1)
21-22: Commented go mod Tidy Command.
Thego mod tidy --compat=1.20command is commented out. Reassess if it is needed in the build process to keep dependencies in check or if it can be permanently removed.x/cadance/spec/02_state.md (3)
1-7: Markdown and Header FormattingThe file begins with clear markdown headers. Ensure that headings start at the beginning of the line to satisfy markdownlint rules, though the current formatting is mostly in-line with the document requirements.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Headings must start at the beginning of the line
null(MD023, heading-start-left)
18-40: Genesis & Params Section ClarificationThe
GenesisStateandParamsdefinitions are clearly illustrated with appropriate gogoproto annotations. Minor stylistic improvements in the accompanying text (for example, a slight grammatical refinement) could enhance clarity, but overall the presentation is solid.🧰 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)
42-49: State Transitions DocumentationThe state transitions are documented clearly. Consider rephrasing the bullet points for enhanced readability (for example, "updates the jail status of the contract" instead of "updates the is_jailed field"), but the current descriptions effectively convey the intended behavior.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: Did you mean “there is”?
Context: ... in state. - Jailing a contract updates the is_jailed field of a CadanceContract objec...(THE_THERE)
[uncategorized] ~48-~48: Did you mean “there is”?
Context: ...n state. - Unjailing a contract updates the is_jailed field of a CadanceContract objec...(THE_THERE)
proto/bitsong/smartaccount/v1beta1/tx.proto (1)
21-64: Message Structures: Consistency and Naming Considerations.
All message types (e.g.,MsgAddAuthenticator,MsgRemoveAuthenticator,MsgSetActiveState, and theTxExtension) are clearly defined. One small nitpick: consider renaming the fieldidinMsgRemoveAuthenticator(line 41) toauthenticator_idfor consistency with theGetAuthenticatorRequestmessage inquery.protoif that fits your domain model.x/cadance/spec/01_concepts.md (2)
12-18: Registering a Contract: Transaction Example Clarity.
The instructions for registering a contract, including the bash command example, are clear. Consider reviewing the wording for "setup"—if used as a verb it might be clearer as “set up” (e.g., "Developers can set up their contract with x/Clock by registering it...").
20-30: Unjailing and Unregistering a Contract: Consistent Guidance.
The sections on unjailing and unregistering a contract provide practical examples and notes regarding sender authorization. Again, check the usage of “setup” in the note (line 30) and consider replacing it with “set up” if that is your intent.🧰 Tools
🪛 LanguageTool
[grammar] ~30-~30: Make sure that the noun ‘setup’ is correct. Did you mean the past participle “set up”?
Context: ...tegration.md) to ensure the contract is setup with a Sudo message. ## Unregistering...(BE_VB_OR_NN)
tests/localbitsong/scripts/add_keys.sh (1)
1-14: Enhance script robustness with safety flags.
Consider adding safety options (e.g.set -euo pipefail) and a check for the availability of thebitsongdcommand at the beginning of this test script. This improves error detection and robustness during key recovery operations.proto/bitsong/cadance/v1/query.proto (1)
59-66: Correct response comment for QueryParamsResponse.
The comment aboveQueryParamsResponseappears to be a copy-paste error referencing "QueryCadanceContractsResponse." Please update it to accurately describeQueryParamsResponse(e.g. "QueryParamsResponse is the response type for the Query/Params RPC method.").x/cadance/spec/03_integration.md (1)
32-36: Improve documentation clarity with minor text adjustments.
In the section describing the CosmWasm integration, consider:
- Rewording “This is where all of the contract's custom end block logic can be performed” to “This is where the contract's custom end block logic is performed.”
- Using “auto-compounding” (with a hyphen) for clarity if that aligns with your style guide.
🧰 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)
[uncategorized] ~36-~36: Possible missing comma found.
Context: ... the example below, at the end of every block thevalConfig variable will increase...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~36-~36: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...ea and perform actions such as cleanup, auto compounding, etc. ```rust // msg.rs #[cw_serde] pu...(AUTO_HYPHEN)
tests/localbitsong/scripts/setup.sh (1)
6-6: Consider Quoting Literal Assignments
For improved shell-script robustness, consider quoting literal assignments. For example, change:MONIKER=valto
MONIKER="val"This practice helps avoid issues in more complex scenarios.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 6-6: Use var=$(command) to assign output (or quote to assign string).
(SC2209)
tests/localbitsong/README.md (3)
1-9: Comprehensive Overview with Scope for Style Refinement
The README delivers a clear introduction and details the two testing configurations for LocalBitSong. To further enhance readability, consider varying sentence beginnings (e.g., avoid repetitive use of “LocalBitSong”) or rephrasing certain parts.🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ts for a standard testing environment. LocalBitSong comes in two flavors: 1. No initial st...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
78-80: Terminology Consistency in Account Descriptions
In the “LocalBitSong Accounts and Keys” section, ensure that terms like “pre-configured” are used consistently throughout the document to maintain clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~80-~80: Do not mix variants of the same word (‘pre-configure’ and ‘preconfigure’) within a single text.
Context: ...Song Accounts and Keys LocalBitSong is pre-configured with one validator and 9 accounts with ...(EN_WORD_COHERENCY)
106-111: FAQ Section Clarity and Grammar
The FAQ section is informative and straightforward. A minor review of sentence structure might help to further polish the text for a smoother reading experience.x/smart-account/README.md (17)
15-19: Phrasing Adjustment ("opt in"):
The text currently uses “opt-in into using authenticators.” Since “opt in” is the verb form, consider revising it to “opt in to using authenticators” for correct usage.🧰 Tools
🪛 LanguageTool
[grammar] ~16-~16: The word “opt-in” is a noun. The verb is spelled with a space.
Context: ...e module is enabled, the user needs to opt-in into using authenticators by selecting ...(NOUN_VERB_CONFUSION)
86-87: Avoid Repetitive Phrasing:
The description of how theSignatureVerificationauthenticator uses a public key is repeated in nearby sentences. Consider varying the wording to enhance readability.🧰 Tools
🪛 LanguageTool
[style] ~86-~86: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r work for a specific account, you just need to feed it the right information. For exam...(REP_NEED_TO_VB)
[style] ~87-~87: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r example, theSignatureVerificationneeds to know which public key to check when ver...(REP_NEED_TO_VB)
300-300: Hyphenation for Compound Adjective:
On line 300, “user specific information” should be hyphenated as “user-specific information” for clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~300-~300: When ‘user-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...his allows for contract reuse even when user specific information is needed. This field is st...(SPECIFIC_HYPHEN)
301-301: Sentence Variation Suggestion:
Line 301 begins with the same word as the preceding sentence, which may affect the flow. Consider starting the sentence with a synonym or restructuring it to avoid repetition.🧰 Tools
🪛 LanguageTool
[style] ~301-~301: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...is standardized and must be json bytes. This way it can be easily parsed and display...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
307-307: Contract Interface Description Improvement:
The explanation of the contract’s authenticator hooks (starting on line 307) repeats similar phrasing found earlier. Consider rewording this section for better variety and clarity.🧰 Tools
🪛 LanguageTool
[style] ~307-~307: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: .... ### Contract Interface The contract needs to implement 5 authenticator hooks which a...(REP_NEED_TO_VB)
[style] ~307-~307: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...thenticator through sudo entrypoint, it needs to handle the following messages: ```rs #...(REP_NEED_TO_VB)
370-371: Punctuation Enhancement:
In the section discussing the backup authentication method, consider adding a comma after introductory elements (for example, after “This way” on line 371) to improve readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~371-~371: Possible missing comma found.
Context: ...the classic authentication method. This way existing applications don't need to be ...(AI_HYDRA_LEO_MISSING_COMMA)
380-383: Grammatical Clarity:
In the section on transaction restrictions (e.g. one signer per message), ensure phrases like “which account a message is associated with” are complete and grammatically correct. A minor rephrasing might help clarify the intended meaning.🧰 Tools
🪛 LanguageTool
[grammar] ~382-~382: The verb “account” is usually followed by “for”.
Context: ...triction here as it makes it more clear which account a message is associated with. ### The ...(ACCOUNTS_FOR)
423-424: Hyphenation in Compound Modifiers:
When describing the composition logic, consider hyphenating compound adjectives such as “top level authenticator” to “top-level authenticator” to improve clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~424-~424: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...This is done by combining the id of the top level authenticator with the position of the ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
425-427: Typo Correction for Possessive Pronoun:
In the description of composite authenticator IDs (around line 426), “it's authenticator” should be corrected to “its authenticator” to use the proper possessive form.🧰 Tools
🪛 LanguageTool
[uncategorized] ~426-~426: Did you mean “its” (the possessive pronoun)?
Context: ...smAuthenticator(contract1, params))` as it's authenticator, and it has a global id o...(ITS_PREMIUM)
440-440: Verb Agreement Correction:
Ensure you use the correct singular form (e.g., “has” instead of “have”) when referring to a singular entity in this context.🧰 Tools
🪛 LanguageTool
[grammar] ~440-~440: With the singular noun ‘one’, use the verb inflections “has” or “had”.
Context: ...the transaction only one of the methods have been called on each. For example, if t...(SINGULAR_NOUN_VERB_AGREEMENT)
445-446: Punctuation Suggestion:
There appear to be missing commas in the sentences around lines 445–446. Adding commas can improve readability and properly separate clauses.🧰 Tools
🪛 LanguageTool
[uncategorized] ~445-~445: Possible missing comma found.
Context: ...rmExecution on B. This is the expected behaviour and authenticator authors should be awa...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~446-~446: Possible missing comma found.
Context: ...re is a proposal to improve this in the future tracked in [#8373](https://github.com/...(AI_HYDRA_LEO_MISSING_COMMA)
460-461: Comma Usage for Compound Sentences:
Before “and” connecting two independent clauses, a comma can help improve clarity. Please review the sentence on line 460 and consider adding the necessary comma.🧰 Tools
🪛 LanguageTool
[uncategorized] ~460-~460: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...uthenticators. The message will succeed and its state changes be committed iff `con...(COMMA_COMPOUND_SENTENCE_2)
468-471: Consistency in Verb Agreement:
Review the description of confirm execution logic (lines 468–471) to ensure that singular/mass noun verbs agree correctly.🧰 Tools
🪛 LanguageTool
[grammar] ~471-~471: Consider using third-person verb forms for singular and mass nouns.
Context: ...age execution. Similarly, when calling confirm on an AnyOf authenticator, we call conf...(MASS_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
468-468: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
468-468: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
471-471: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
471-471: Emphasis style
Expected: asterisk; Actual: underscore(MD049, emphasis-style)
477-477: Conciseness Improvement:
The phrase “At the moment” could be replaced with a more succinct alternative to enhance conciseness.🧰 Tools
🪛 LanguageTool
[style] ~477-~477: For conciseness, consider replacing this expression with an adverb.
Context: ...ted. ### Selecting sub-authenticators At the moment, we do not support selecting sub-authen...(AT_THE_MOMENT)
491-491: Punctuation for Clarity:
Consider inserting a comma after “AnyOf” on line 491 to improve the sentence’s flow.🧰 Tools
🪛 LanguageTool
[typographical] ~491-~491: Consider adding a comma.
Context: ...this authenticator were a simpleAnyOfthere would be no way of providing a signatur...(IF_THERE_COMMA)
502-502: Hyphenation in Modifier Phrasing:
The header “One Click trading” should likely be rendered as “One-Click trading” to correctly format the compound modifier.🧰 Tools
🪛 LanguageTool
[uncategorized] ~502-~502: When ‘One-Click’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...re some examples of how to do that: ## One Click trading A hot key can be configured so...(ONE_CLICK_HYPHEN)
538-539: Markdown Styling Consistency:
Multiple unordered lists across the document show inconsistent indentation and bullet styles. Please adjust these lists according to markdownlint guidelines (preferably using asterisks and consistent indentation) for improved formatting consistency.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
538-538: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
539-539: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (40)
go.sumis excluded by!**/*.sumproto/buf.lockis excluded by!**/*.locktests/cw-orch/Cargo.lockis excluded by!**/*.locktests/ict/contracts/cw_template.wasmis excluded by!**/*.wasmtests/ict/contracts/polytone_listener.wasmis excluded by!**/*.wasmtests/ict/contracts/polytone_note.wasmis excluded by!**/*.wasmtests/ict/contracts/polytone_proxy.wasmis excluded by!**/*.wasmtests/ict/contracts/polytone_tester.wasmis excluded by!**/*.wasmtests/ict/contracts/polytone_voice.wasmis excluded by!**/*.wasmtests/ict/go.sumis excluded by!**/*.sumx/cadance/keeper/testdata/clock_example.wasmis excluded by!**/*.wasmx/cadance/keeper/testdata/cw_testburn.wasmis excluded by!**/*.wasmx/cadance/types/cadance.pb.gois excluded by!**/*.pb.gox/cadance/types/genesis.pb.gois excluded by!**/*.pb.gox/cadance/types/query.pb.gois excluded by!**/*.pb.gox/cadance/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/cadance/types/tx.pb.gois excluded by!**/*.pb.gox/cadance/types/tx.pb.gw.gois excluded by!**/*.pb.gw.gox/fantoken/types/events.pb.gois excluded by!**/*.pb.gox/fantoken/types/fantoken.pb.gois excluded by!**/*.pb.gox/fantoken/types/genesis.pb.gois excluded by!**/*.pb.gox/fantoken/types/gov.pb.gois excluded by!**/*.pb.gox/fantoken/types/params.pb.gois excluded by!**/*.pb.gox/fantoken/types/query.pb.gois excluded by!**/*.pb.gox/fantoken/types/tx.pb.gois excluded by!**/*.pb.gox/smart-account/images/authentication_flow.jpgis excluded by!**/*.jpgx/smart-account/images/authenticator_manager.jpgis excluded by!**/*.jpgx/smart-account/images/circuit_breaker.jpgis excluded by!**/*.jpgx/smart-account/images/keeper.jpgis excluded by!**/*.jpgx/smart-account/testutils/bytecode/cosigner_authenticator.wasmis excluded by!**/*.wasmx/smart-account/testutils/bytecode/echo.wasmis excluded by!**/*.wasmx/smart-account/testutils/bytecode/spend_limit_v1.0.0-alpha.1.wasmis excluded by!**/*.wasmx/smart-account/testutils/contracts/cosigner-authenticator/Cargo.lockis excluded by!**/*.lockx/smart-account/testutils/contracts/echo/Cargo.lockis excluded by!**/*.lockx/smart-account/types/genesis.pb.gois excluded by!**/*.pb.gox/smart-account/types/models.pb.gois excluded by!**/*.pb.gox/smart-account/types/params.pb.gois excluded by!**/*.pb.gox/smart-account/types/query.pb.gois excluded by!**/*.pb.gox/smart-account/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/smart-account/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (153)
.github/workflows/interchaintest-e2e.yml(2 hunks).github/workflows/push-docker-image.yml(1 hunks).github/workflows/release.yml(5 hunks)Dockerfile(2 hunks)Makefile(1 hunks)README.md(2 hunks)app/ante_handler.go(7 hunks)app/app.go(21 hunks)app/config.go(3 hunks)app/encoding.go(1 hunks)app/export.go(6 hunks)app/genesis.go(1 hunks)app/keepers/keepers.go(7 hunks)app/keepers/keys.go(1 hunks)app/keepers/querier.go(1 hunks)app/modules.go(6 hunks)app/params/amino.go(1 hunks)app/params/config.go(2 hunks)app/params/proto.go(1 hunks)app/posthandler.go(1 hunks)app/test_helper.go(7 hunks)app/testing/test_suite.go(3 hunks)app/types.go(1 hunks)app/upgrades/types.go(3 hunks)app/upgrades/v010/constants.go(1 hunks)app/upgrades/v010/upgrades.go(3 hunks)app/upgrades/v011/constants.go(1 hunks)app/upgrades/v011/upgrades.go(1 hunks)app/upgrades/v013/constants.go(1 hunks)app/upgrades/v013/upgrades.go(1 hunks)app/upgrades/v014/constants.go(1 hunks)app/upgrades/v014/upgrades.go(1 hunks)app/upgrades/v015/constants.go(1 hunks)app/upgrades/v015/upgrades.go(1 hunks)app/upgrades/v016/constants.go(1 hunks)app/upgrades/v016/upgrades.go(1 hunks)app/upgrades/v018/constants.go(1 hunks)app/upgrades/v018/upgrades.go(5 hunks)app/upgrades/v020/constants.go(1 hunks)app/upgrades/v020/upgrade_test.go(4 hunks)app/upgrades/v020/upgrades.go(10 hunks)app/upgrades/v021/constants.go(1 hunks)app/upgrades/v021/upgrade_test.go(1 hunks)app/upgrades/v021/upgrades.go(1 hunks)btsgutils/btsgcli/dynamic.go(1 hunks)btsgutils/btsgcli/flag_advice.go(1 hunks)btsgutils/btsgcli/index_cmd.go(1 hunks)btsgutils/btsgcli/parsers.go(1 hunks)btsgutils/btsgcli/query_cmd_wrap.go(1 hunks)btsgutils/btsgcli/string_formatter.go(1 hunks)btsgutils/btsgcli/tx_cmd_wrap.go(1 hunks)btsgutils/generic_helper.go(1 hunks)btsgutils/slice_helper.go(1 hunks)cmd/bitsongd/cmd/genesis.go(4 hunks)cmd/bitsongd/cmd/init.go(4 hunks)cmd/bitsongd/cmd/init_from_state.go(1 hunks)cmd/bitsongd/cmd/root.go(10 hunks)e2e/ict/README.md(0 hunks)e2e/ict/polytone_test.go(0 hunks)e2e/localbitsong/README.md(0 hunks)go.mod(1 hunks)proto/bitsong/cadance/v1/cadance.proto(1 hunks)proto/bitsong/cadance/v1/genesis.proto(1 hunks)proto/bitsong/cadance/v1/query.proto(1 hunks)proto/bitsong/cadance/v1/tx.proto(1 hunks)proto/bitsong/fantoken/v1beta1/fantoken.proto(1 hunks)proto/bitsong/fantoken/v1beta1/tx.proto(2 hunks)proto/bitsong/smartaccount/v1beta1/genesis.proto(1 hunks)proto/bitsong/smartaccount/v1beta1/models.proto(1 hunks)proto/bitsong/smartaccount/v1beta1/params.proto(1 hunks)proto/bitsong/smartaccount/v1beta1/query.proto(1 hunks)proto/bitsong/smartaccount/v1beta1/tx.proto(1 hunks)proto/buf.gen.pulsar.yaml(1 hunks)proto/buf.gen.swagger.yaml(1 hunks)proto/buf.yaml(1 hunks)scripts/generate-swagger-docs.sh(1 hunks)scripts/makefiles/e2e.mk(1 hunks)scripts/makefiles/format.mk(1 hunks)scripts/makefiles/proto.mk(1 hunks)scripts/makefiles/tests.mk(1 hunks)scripts/polytone.sh(1 hunks)scripts/protoc-swagger-gen.sh(0 hunks)scripts/protocgen-pulsar.sh(1 hunks)scripts/protocgen.sh(1 hunks)scripts/test_node.sh(2 hunks)swagger/config.json(1 hunks)tests/bitsongibctesting/chain.go(1 hunks)tests/ict/basic_start_test.go(1 hunks)tests/ict/basic_upgrade_test.go(5 hunks)tests/ict/conformance/cosmwasm.go(1 hunks)tests/ict/go.mod(1 hunks)tests/ict/helpers/cosmwasm.go(1 hunks)tests/ict/module_pfm_test.go(1 hunks)tests/ict/polytone_suite.go(6 hunks)tests/ict/polytone_test.go(1 hunks)tests/ict/polytone_types.go(1 hunks)tests/ict/setup.go(2 hunks)tests/ict/utils.go(1 hunks)tests/ict/workflow/cosmwasm.go(1 hunks)tests/localbitsong/README.md(1 hunks)tests/localbitsong/docker-compose.yml(1 hunks)tests/localbitsong/scripts/add_keys.sh(1 hunks)tests/localbitsong/scripts/setup.sh(4 hunks)third_party/proto/ibc/core/client/v1/client.proto(1 hunks)x/README.md(1 hunks)x/cadance/README.md(1 hunks)x/cadance/abci.go(1 hunks)x/cadance/abci_test.go(1 hunks)x/cadance/client/cli/query.go(1 hunks)x/cadance/client/cli/tx.go(1 hunks)x/cadance/genesis.go(1 hunks)x/cadance/genesis_test.go(1 hunks)x/cadance/keeper/cadance.go(1 hunks)x/cadance/keeper/keeper.go(1 hunks)x/cadance/keeper/keeper_test.go(1 hunks)x/cadance/keeper/msg_server.go(1 hunks)x/cadance/keeper/msg_server_test.go(1 hunks)x/cadance/keeper/querier.go(1 hunks)x/cadance/keeper/querier_test.go(1 hunks)x/cadance/module.go(1 hunks)x/cadance/module_test.go(1 hunks)x/cadance/spec/01_concepts.md(1 hunks)x/cadance/spec/02_state.md(1 hunks)x/cadance/spec/03_integration.md(1 hunks)x/cadance/spec/README.md(1 hunks)x/cadance/types/codec.go(1 hunks)x/cadance/types/codec_test.go(1 hunks)x/cadance/types/errors.go(1 hunks)x/cadance/types/keys.go(1 hunks)x/cadance/types/msgs.go(1 hunks)x/cadance/types/msgs_test.go(1 hunks)x/cadance/types/params.go(1 hunks)x/cadance/types/params_test.go(1 hunks)x/fantoken/client/cli/cli_test.go(12 hunks)x/fantoken/client/cli/tx.go(3 hunks)x/fantoken/handler.go(3 hunks)x/fantoken/handler_test.go(5 hunks)x/fantoken/keeper/fantoken.go(5 hunks)x/fantoken/keeper/grpc_query.go(2 hunks)x/fantoken/keeper/keeper.go(12 hunks)x/fantoken/keeper/keeper_test.go(5 hunks)x/fantoken/keeper/msg_server.go(2 hunks)x/fantoken/keeper/store.go(2 hunks)x/fantoken/module.go(5 hunks)x/fantoken/types/errors.go(1 hunks)x/fantoken/types/expected_keepers.go(2 hunks)x/fantoken/types/fantoken.go(5 hunks)x/fantoken/types/genesis_test.go(9 hunks)x/fantoken/types/msgs.go(9 hunks)x/fantoken/types/params.go(2 hunks)x/fantoken/types/validation.go(4 hunks)x/smart-account/README.md(1 hunks)x/smart-account/ante/ante.go(1 hunks)
💤 Files with no reviewable changes (4)
- e2e/ict/README.md
- scripts/protoc-swagger-gen.sh
- e2e/localbitsong/README.md
- e2e/ict/polytone_test.go
✅ Files skipped from review due to trivial changes (10)
- scripts/makefiles/e2e.mk
- third_party/proto/ibc/core/client/v1/client.proto
- tests/ict/utils.go
- x/fantoken/handler.go
- app/upgrades/v018/constants.go
- tests/ict/workflow/cosmwasm.go
- x/fantoken/types/errors.go
- tests/ict/helpers/cosmwasm.go
- tests/ict/conformance/cosmwasm.go
- cmd/bitsongd/cmd/init_from_state.go
🧰 Additional context used
📓 Learnings (1)
x/fantoken/client/cli/tx.go (2)
Learnt from: hard-nett
PR: bitsongofficial/go-bitsong#265
File: x/fantoken/client/cli/tx.go:451-454
Timestamp: 2025-02-04T15:11:55.240Z
Learning: In cosmos-sdk v0.50.x (as used in BitSong), NewMsgSubmitProposal does not have ValidateBasic() as an interface function, reflecting the SDK's modern protobuf-based message system. The validation check in GetCmdUpdateFantokenFees should not be included.
Learnt from: hard-nett
PR: bitsongofficial/go-bitsong#265
File: x/fantoken/client/cli/tx.go:451-454
Timestamp: 2025-02-04T15:11:55.240Z
Learning: The cosmos-sdk gov module's NewMsgSubmitProposal does not have ValidateBasic() as an interface function, so validation checks should not be performed on the returned message.
🪛 YAMLlint (1.35.1)
proto/buf.gen.pulsar.yaml
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
proto/buf.gen.swagger.yaml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Buf (1.47.2)
proto/bitsong/cadance/v1/genesis.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/smartaccount/v1beta1/params.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/smartaccount/v1beta1/genesis.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/smartaccount/v1beta1/query.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/cadance/v1/query.proto
4-4: import "cosmos/base/query/v1beta1/pagination.proto": file does not exist
(COMPILE)
proto/bitsong/cadance/v1/tx.proto
6-6: import "google/api/annotations.proto": file does not exist
(COMPILE)
proto/bitsong/smartaccount/v1beta1/tx.proto
4-4: import "cosmos/msg/v1/msg.proto": file does not exist
(COMPILE)
🪛 LanguageTool
x/cadance/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)
[uncategorized] ~36-~36: Possible missing comma found.
Context: ... the example below, at the end of every block the val Config variable will increase...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~36-~36: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...ea and perform actions such as cleanup, auto compounding, etc. ```rust // msg.rs #[cw_serde] pu...
(AUTO_HYPHEN)
tests/localbitsong/README.md
[style] ~4-~4: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ts for a standard testing environment. LocalBitSong comes in two flavors: 1. No initial st...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~62-~62: The verb ‘Ensure’ does not usually follow articles like ‘an’. Check that ‘Ensure’ is spelled correctly; using ‘Ensure’ as a noun may be non-standard.
Context: ...e already on your keyring, you will get an "Error: aborted" - Ensure you use the name of the account as list...
(A_INFINITIVE)
[uncategorized] ~80-~80: Do not mix variants of the same word (‘pre-configure’ and ‘preconfigure’) within a single text.
Context: ...Song Accounts and Keys LocalBitSong is pre-configured with one validator and 9 accounts with ...
(EN_WORD_COHERENCY)
[grammar] ~91-~91: The word ‘misery’ is a noun or an adjective. A verb or adverb is missing or misspelled here, or maybe a comma is missing.
Context: ... | `noble width taxi input there patrol clown public spell aunt wish punch moment will misery eight excess arena pen turtle minimum g...
(PRP_MD_NN)
[grammar] ~92-~92: The verb ‘rely’ requires the preposition ‘on’ (or ‘upon’).
Context: ... | `cream sport mango believe inhale text fish rely elegant below earth april wall rug ritu...
(RELY_ON)
[grammar] ~93-~93: Use “a hundred”, or use a number before ‘hundred’.
Context: ... | index light average senior silent limit usual local involve delay update rack cause inmate wall render magnet common feature laundry exact casual resource hundred | | lo-test10 | `bitsong1gulaxnc...
(NODT_DOZEN)
x/cadance/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 CadanceContract objec...
(THE_THERE)
[uncategorized] ~48-~48: Did you mean “there is”?
Context: ...n state. - Unjailing a contract updates the is_jailed field of a CadanceContract objec...
(THE_THERE)
x/cadance/spec/01_concepts.md
[grammar] ~6-~6: The word “setup” is a noun. The verb is spelled with a space.
Context: ... need for external bots. Developers can setup their contract with x/Clock by register...
(NOUN_VERB_CONFUSION)
[grammar] ~30-~30: Make sure that the noun ‘setup’ is correct. Did you mean the past participle “set up”?
Context: ...tegration.md) to ensure the contract is setup with a Sudo message. ## Unregistering...
(BE_VB_OR_NN)
x/smart-account/README.md
[grammar] ~16-~16: The word “opt-in” is a noun. The verb is spelled with a space.
Context: ...e module is enabled, the user needs to opt-in into using authenticators by selecting ...
(NOUN_VERB_CONFUSION)
[style] ~86-~86: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r work for a specific account, you just need to feed it the right information. For exam...
(REP_NEED_TO_VB)
[style] ~87-~87: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r example, the SignatureVerification needs to know which public key to check when ver...
(REP_NEED_TO_VB)
[uncategorized] ~300-~300: When ‘user-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...his allows for contract reuse even when user specific information is needed. This field is st...
(SPECIFIC_HYPHEN)
[style] ~301-~301: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...is standardized and must be json bytes. This way it can be easily parsed and display...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~307-~307: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: .... ### Contract Interface The contract needs to implement 5 authenticator hooks which a...
(REP_NEED_TO_VB)
[style] ~307-~307: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...thenticator through sudo entrypoint, it needs to handle the following messages: ```rs #...
(REP_NEED_TO_VB)
[style] ~369-~369: For conciseness, consider replacing this expression with an adverb.
Context: ...ction will be rejected by this module. At the moment, there is a backup authentication metho...
(AT_THE_MOMENT)
[uncategorized] ~371-~371: Possible missing comma found.
Context: ...the classic authentication method. This way existing applications don't need to be ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~382-~382: The verb “account” is usually followed by “for”.
Context: ...triction here as it makes it more clear which account a message is associated with. ### The ...
(ACCOUNTS_FOR)
[uncategorized] ~424-~424: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...This is done by combining the id of the top level authenticator with the position of the ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~426-~426: Did you mean “its” (the possessive pronoun)?
Context: ...smAuthenticator(contract1, params))` as it's authenticator, and it has a global id o...
(ITS_PREMIUM)
[grammar] ~440-~440: With the singular noun ‘one’, use the verb inflections “has” or “had”.
Context: ...the transaction only one of the methods have been called on each. For example, if t...
(SINGULAR_NOUN_VERB_AGREEMENT)
[uncategorized] ~445-~445: Possible missing comma found.
Context: ...rmExecution on B. This is the expected behaviour and authenticator authors should be awa...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~446-~446: Possible missing comma found.
Context: ...re is a proposal to improve this in the future tracked in [#8373](https://github.com/...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~460-~460: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...uthenticators. The message will succeed and its state changes be committed iff `con...
(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~471-~471: Consider using third-person verb forms for singular and mass nouns.
Context: ...age execution. Similarly, when calling confirm on an AnyOf authenticator, we call conf...
(MASS_AGREEMENT)
[uncategorized] ~473-~473: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...uthenticators. The message will succeed and its state changes be committed iff `con...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~477-~477: For conciseness, consider replacing this expression with an adverb.
Context: ...ted. ### Selecting sub-authenticators At the moment, we do not support selecting sub-authen...
(AT_THE_MOMENT)
[typographical] ~491-~491: Consider adding a comma.
Context: ...this authenticator were a simple AnyOf there would be no way of providing a signatur...
(IF_THERE_COMMA)
[uncategorized] ~502-~502: When ‘One-Click’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...re some examples of how to do that: ## One Click trading A hot key can be configured so...
(ONE_CLICK_HYPHEN)
🪛 Shellcheck (0.10.0)
tests/localbitsong/scripts/setup.sh
[warning] 6-6: Use var=$(command) to assign output (or quote to assign string).
(SC2209)
scripts/protocgen.sh
[warning] 8-8: For loops over find output are fragile. Use find -exec or a while read loop.
(SC2044)
scripts/protocgen-pulsar.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 markdownlint-cli2 (0.17.2)
x/cadance/spec/02_state.md
1-1: Headings must start at the beginning of the line
null
(MD023, heading-start-left)
x/smart-account/README.md
31-31: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
32-32: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
33-33: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 4; Actual: 5
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
218-218: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
219-219: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
220-220: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
221-221: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
343-343: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
344-344: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
345-345: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
468-468: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
468-468: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
471-471: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
471-471: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
486-486: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
487-487: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
538-538: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
539-539: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 golangci-lint (1.62.2)
cmd/bitsongd/cmd/root.go
21-21: "github.com/cosmos/cosmos-sdk/types/module" imported and not used
(typecheck)
🔇 Additional comments (317)
x/cadance/types/params_test.go (2)
1-9: LGTM! Well-organized imports and package declaration.The package declaration and imports follow Go best practices with a separate test package and proper organization of imports.
54-63: LGTM! Clean and effective test execution logic.The test execution follows best practices with proper use of table-driven testing pattern and clear assertions.
tests/bitsongibctesting/chain.go (3)
84-116: Verify the sequence increment logic on failure scenarios.
SendMsgsNoCheckincrementsSenderAccount's sequence even if the transaction fails. If this is intended, it must be well-documented or tested to avoid confusion. In many implementations, a failed transaction leaves account sequence unchanged.
303-348: Implementation looks solid, with appropriate error checks.
TheSignAndDeliverWithAuthenticatorfunction properly handles transaction signing and block finalization. Returning a typed error for block finalization failures keeps error handling consistent.
443-518: Validate multiple signers vs. multiple messages.
SendMsgsFromPrivKeysWithAuthenticatorAndCompoundSigsassumes each message has the same single “top-level” signer. For multi-signer messages, ensure the logic supports or warns about the mismatch of signers if the code ever needs to handle multiple message signers in a single transaction.tests/ict/polytone_suite.go (6)
13-19: Check for breaking changes in upgraded dependencies.
The import statements have been updated to new major versions (wasmvm v2 and interchaintest v8). Please verify if there's any backward-incompatible changes that need additional adjustments.Would you like me to perform a web search for known issues with these major version upgrades?
41-43: Fields introduced for contract references.
These newly introduced struct fields look consistent with the usage context.
97-97: Build approach for the relayer.
No issues found with this updated build invocation.
112-112: Ensure correct network references in InterchainBuildOptions.
Double-check that theClientandNetworkIDvalues are correct for your environment.Also applies to: 114-115
125-126: User funding for test accounts.
Allocating 10,000,000,000 of funds to each user might be high, but it's acceptable for testing.
277-278: Potential mismatch in chain references.
The function usess.QueryTesterCallbackHistory(&s.ChainA)directly, ignoring the passedchainargument. Confirm that referencings.ChainAis intended rather thanchain.app/upgrades/v020/upgrades.go (29)
4-4: No concerns on importingcontext.
8-8: No issues with importingupgradetypesfromcosmossdk.io/x/upgrade/types.
10-10: No issues with importinggithub.com/bitsongofficial/go-bitsong/app/upgrades.
17-18: Parameterbpmis unused.
If it’s intended for future usage, consider adding a TODO. Otherwise, remove it to avoid confusion.
19-20: Initialization ofsdkCtxand logger looks good.
30-33: Loop initialization pattern is acceptable.
34-37: Validator delegation retrieval logic looks fine.
38-42: Proper error handling for validator retrieval.
No issues here.
45-48: Returningnil, errwitherrunset is a bug.
This code tries to return an uninitializederrwhenhasInfois false.
50-51: Calculation of validator period (endingPeriod) is correct.
66-67: Fetching all delegations follows the same pattern as above.
No issues noted.
72-73: Ignoring potential error fromValidator(...).
Validate that skipping error checking here is intentional.
75-78: CallingCalculateDelegationRewardsfor each delegation.
No issues noted.
104-112: Rounding error logging is a good defensive measure.
115-116: Truncation to integer rewards is correctly implemented.
119-125: Overwritingerrfor withdraw address retrieval and send operation.
This error reuse is the same overshadowing issue flagged in a previous review.
127-132: Updating outstanding rewards & community pool is logically correct.
134-139: Error fromGetDelegatorStartingInfo(...)is ignored.
Ensure that ignoring it is intended, or handle it if necessary.
142-151: Handling zero final rewards by logging and providing a zero coin is correct.
154-170: Validator and delegation retrieval ignore possible errors.
Consider capturing and validating errors to avoid silent failures.
172-173: No issues returningnilafter patch completion.
177-180: Silent return on error.
Currently, the function just returns empty values on error, providing no warnings or logs about what went wrong. Confirm that this behavior is desired.
191-191: Iteration over slash events is properly invoked.
212-212: Margin of error for stake adjustments.
No immediate concerns; appears domain-specific.
226-227: Function signature for delegated rewards is consistent.
239-240: Ignoring potential error fromGetValidatorHistoricalRewards.
If there's a possibility of error, consider handling or logging it.
252-252: Ignoring potential error fromGetValidatorHistoricalRewardsfor reference count.
Confirm this is intentional.
267-267: Same pattern of ignoring the error fromGetValidatorHistoricalRewards.
276-276: Typo: double “m” inCustommTokensFromSharesTruncated.
Consider renaming to remove the extra “m.”btsgutils/btsgcli/tx_cmd_wrap.go (1)
57-62: Validate computedNumArgs.Here,
desc.NumArgs = ParseNumFields[M]() - 1 - len(desc.CustomFlagOverrides) - len(desc.CustomFieldParsers)could lead to off-by-one or negative values if the generically parsed fields or custom flags differ from expectations. Consider adding checks or validations to ensureNumArgsremains non-negative and consistent with your CLI usage.x/cadance/abci.go (1)
52-59: Confirm gas meter usage with nested contexts.Using
childCtx := ctx.WithGasMeter(...)can result in ambiguous gas usage when the top-levelctxgas meter differs. Ensure that either the parent context is unaffected or that nested contexts are clearly documented to avoid accidental double-charging or incorrect gas computations elsewhere.x/cadance/module.go (2)
30-30: Ensure consensus version is incremented on every breaking change.It's easy to forget to update
ConsensusVersionfor subsequent changes. A quick internal or CI check could prevent accidental omissions and confusion in the upgrade process.
116-121: ValidateSetParamssuccess.Within
InitGenesis, consider verifying thatSetParamssucceeded or further verifying the returned error if any. AlthoughSetParamspresumably returns no error, future changes could introduce new validations. Checking the result helps avoid silent failures.cmd/bitsongd/cmd/init.go (9)
9-9: No concerns adding "strings" import
This import is used to join seed strings.
28-28: Approved import of "genutiltypes"
This import is useful for reading genesis state from the file.
83-86: Empty seed list remains an issue
You are initializing theseedsslice with an empty string. This was flagged in a previous review. Please either populate this list with actual seed nodes or remove it if unused.
88-90: Verify updated P2P settings
Configuring P2P seeds from an empty string may cause connectivity issues. Also, ensure the newly set inbound (80) and outbound (60) peer limits meet your use case and do not inadvertently block peers.
127-130: Good practice handling genesis file read errors
Reading the genesis file and returning a wrapped error is a proper way to diagnose issues if the file is malformed.
138-139: Verify chainID check for mainnet
Currently, an empty chain ID is treated as mainnet. Confirm this logic is intended, as it might cause unexpected behaviors if the chain ID is neglected or accidentally omitted.
146-148: Approved: Proper JSON marshalling of appState
Returning a wrapped error ensures easier debugging if marshaling fails.
152-153: Verify impact of removing the validator set
Explicitly settingConsensus.Validatorsto nil removes the initial validator definitions. Verify that this is the expected genesis configuration.
158-158: Looks good finalizing print info
No immediate concerns with the final assignment totoPrint.app/upgrades/v021/upgrades.go (6)
1-2: Package declaration
No issues; standard package naming for versioned upgrade logic.
3-22: Imports
No concerns; imports align with your updated modules and upgrade logic.
24-35: Initial upgrade handler logic
WrappingRunMigrationsin a closure and logging the version map before proceeding is a clean approach.
75-99: Verify finalization of consensus parameters
Enabling vote extensions atsdkCtx.BlockHeight() + 1is abrupt. Validate that the block time and evidence parameters fully align with your chain’s operational requirements.
100-129: Comprehensive module parameters update
Updating params for x/cadance, x/smart-account, IBC WASM, and expedited governance in one pass is well-structured. Ensure thorough testing to confirm the expected behavior in production.
132-139: Review interchain query allowlist
AddingSmartContractStateto the ICQ allowlist is convenient but be mindful of any security implications from enabling these cross-chain queries.btsgutils/btsgcli/query_cmd_wrap.go (9)
1-2: Package declaration
No issues: the newbtsgclipackage is suitably named.
3-15: Imports
These imports (grpc, cobra, pflag, etc.) are common for building advanced CLI functionality.
42-49: Implementation ofDescriptorinterface
GetCustomFlagOverridesstraightforwardly returns the map. This meets the interface requirements cleanly.
67-69: Formatting the extended description
Wraps module-specific metadata intodesc.Long. Looks correct.
91-120: CLI construction using reflection
Generates a Cobra command fromdesc, enforcing a strict argument count. Confirm thatcobra.ExactArgs(desc.numArgs)suits all your use cases, including optional arguments.
122-135: SimpleQueryCmd
A concise wrapper aroundBuildQueryClifor straightforward query command usage.
137-145: Convenient function for retrieving module params
Automatically sets the query function name to "Params", fitting well with standard param patterns.
147-170: Avoid using panic in production code
Similar to a previous comment, consider returning an error instead of panicking when reflection yields unexpected results.
172-194: Query logic workflow
Retrieves the query client, parses arguments, invokes reflection-based call, and prints the result. Error checks appear sound.app/ante_handler.go (10)
4-6: Use of new cosmos-sdk packages is appropriate.
These lines reflect the transition from legacy imports to the more recentcosmossdk.iopackages for store, errors, and math. This is consistent with the new architecture and should help keep the codebase current.
16-17: IBC imports upgraded to version 8.
Upgrading to the latest IBC version helps ensure ongoing compatibility with the ecosystem. Ensure that any new version-specific features or deprecations are accounted for in related IBC logic.Would be good to run integration tests to confirm IBC functionality remains stable.
19-20: New smart-account keepers and decorators.
Great addition of these imports for thesmart-accountfeature. Verify that all relevant handlers and keeper logic satisfy your security and performance requirements.
28-28: HandlerOptions struct fields updated.
SmartAccount *smartaccountkeeper.Keeperadded.TxCounterStoreKeyswitched tocorestoretypes.KVStoreService.Cdcswitched tocodec.Codecfromcodec.BinaryCodec.These changes are consistent with the new store and codec APIs. Ensure all call sites are updated accordingly, especially the use of
Cdcfor marshalling/unmarshalling.Also applies to: 31-31, 33-33
50-50: Switch tomath.LegacyNewDecWithPrec.
Usingmath.LegacyNewDecWithPrecis correct for the new cosmos math package. Keep watch for any differences in precision behavior or potential impact on existing validation.
59-59: Updated error wrapping approach.
Replacingsdkerrors.Wrapwitherrors.Wrapis consistent with the new error package. Confirm that the new error hierarchy remains compatible with existing logic that checks error codes/types.Also applies to: 68-68, 80-80
110-110: Ante handler function signature looks good.
No issues here. It's fine to keep the standard(sdk.AnteHandler, error)signature.
112-112: Improved error handling witherrors.Wrap.
Instead of returning nil or a panic on missing keepers, the code now correctly returns a wrapped error. This is a more robust approach for diagnosing configuration errors.Also applies to: 115-115, 118-118
126-139: Old & new signature verification flows.
- A new decorator
deductFeeDecoratoris integrated.- You assemble the old chain of decorators for the “classic” signature verification.
Ensure disabling the old flow or switching is well-documented so the user knows which path is active.
140-148: New authenticator flow.
- Embedding fee deduction and signature checks into the new
smartaccountante.NewAuthenticatorDecorator.- Retaining the incremental sequence decorator.
Ensure you have thorough tests confirming this new path’s correctness, especially multi-sig or smart-account features.
[approve]x/fantoken/module.go (5)
8-9: New imports forappmoduleand updated math usage.
Introduces references tocosmossdk.io/mathandcosmossdk.io/core/appmodule. This aligns the fantoken module with the latest Cosmos SDK architecture.
27-34: Reorganized module references and interfaces.
- Replaces or adds references like
AppModule,module.HasGenesis,appmodule.HasBeginBlocker, etc.- The
AppModuleandAppModuleBasicreferences to the newappmodulearchitecture indicate a cleaner modular approach.
109-110: Marker functions forIsAppModuleandIsOnePerModuleType.
These no-op methods are effectively type signatures for the updated appmodule framework. Watch for potential future expansions of these marker functions.Also applies to: 112-112
137-137: UpdatedInitGenesisto remove ValidatorUpdate return.
Genesis init no longer returns validator updates, aligning with the simplified pattern. Make sure the rest of the app/chain flow properly handles the updated method signature.Also applies to: 143-143
166-171: SimulatorGenesisState method introduced.
This method sets up default genesis states for simulation.
- Setting
ftDefaultGen.Params.IssueFeeto 10 $STAKES for testing is a neat approach.- Ensure it aligns with the rest of the simulator parameters so you get consistent test coverage.
app/export.go (5)
7-7: Store types import.
Introducingstypes "cosmossdk.io/store/types"is consistent with the new cosmos store approach.
21-21: Context creation withouttmproto.Header.
Switching toapp.NewContext(true)is standard in newer versions. Confirm any removed header data isn’t needed for zero-height reinitialization.
31-34: Enhanced error handling on genesis export.
Previously, errors could go undetected. Now the function returns errors properly—good improvement for debugging.
79-81: Withdrawing validator commission withValAddressusage.
Usingsdk.ValAddress(val.GetOperator())ensures typed addresses. This is consistent with the cosmos patterns.
[approve]
174-175: Switching toKVStoreReversePrefixIterator.
- This approach allows iteration in descending order. Confirm that new iteration logic doesn’t miss or reorder validators incorrectly for your use case.
- The added error check for retrieving the validator ensures you quickly detect missing or corrupted store data.
[approve]Also applies to: 179-180
x/cadance/keeper/cadance.go (2)
54-54: Make unmarshal errors more descriptive.
When unmarshaling fails, wrap the error to include the contract address for easier troubleshooting.
203-230: Check for uninitializedk.wasmKeeper.
Ifk.wasmKeeperis nil, callingHasContractInfowill panic. Add a nil check to avoid runtime errors.x/smart-account/ante/ante.go (1)
53-99: Graceful out-of-gas handling looks solid.
Catching the panic and wrapping it is a recommended approach. Ensure you have sufficient logging for production debugging.app/modules.go (2)
154-159: Check for repeated module names in BeginBlockers.
It seems some modules like “authtypes.ModuleName” or “ibcwasmtypes.ModuleName” appear multiple times in different block order arrays. Verify if duplication is intentional or if it might cause confusion.
231-249: Validate newly added accessor methods in tests.
The methods likeGetStakingKeeperandGetTxConfigcan silently fail or become outdated if not tested. Ensure you have adequate unit tests or integration tests verifying their behavior.cmd/bitsongd/cmd/root.go (4)
21-21: False-positive from static analysis regarding unused import.The import statement for
"github.com/cosmos/cosmos-sdk/types/module"is legitimately used around lines 404-405 to accessmodule.HasName. You can safely ignore the static analysis warning in this case.🧰 Tools
🪛 golangci-lint (1.62.2)
21-21: "github.com/cosmos/cosmos-sdk/types/module" imported and not used
(typecheck)
125-127: Panicking on CLI enhancement error.Panicking here is acceptable for command initialization, as it immediately halts the CLI setup if the auto CLI logic fails. Just be mindful that this completely terminates the process. If you would prefer a more graceful fallback, consider returning the error or logging it.
132-141: Temp directory is removed before usage.
This mirrors a concern noted in a previous review comment: thedefer os.RemoveAll(dir)call immediately removes the directory upon function return, rendering the returned path invalid. Removing thedeferstatement or relocating cleanup to a suitably scoped location would prevent unexpected errors.
235-235: Migration topruning.Cmdlooks correct.Switching from
pruning.PruningCmdtopruning.Cmdis consistent with the new Cosmos SDK approach for pruning. No issues found.app/app.go (2)
193-201: Avoid panicking ininit.This aligns with a past suggestion: panics in
init()can abruptly terminate the application. For better robustness, consider gracefully handling the error (e.g., falling back to a tmp directory or returning an error in a higher layer).
424-435: Infinite gas meter usage in PreBlocker is potentially non-deterministic.You are explicitly resetting the gas meter to an infinite meter. Although it might be necessary for complex data hydration, ensure that you understand and accept the impact on deterministic block execution, as it can mask unintended expensive computations.
app/keepers/keepers.go (51)
6-6: Switch to new feegrant import path looks correct.
The change to"cosmossdk.io/x/feegrant"aligns with Cosmos SDK v0.50 import conventions.
11-11: Adoption ofruntimepackage from new SDK structure.
No issues. This is consistent with the updated SDK layout.
15-20: Updated imports for store, evidence, feegrant, and upgrade modules.
These changes correctly point to the newcosmossdk.iopaths.
23-23: Import ofappparamsfrom the local repo.
No concerns; it references the correct local package for app parameters.
27-29: New imports for smart-account module.
Reflects integration with the smart account feature. No issues observed.
30-32: Introduced address codec, server types, and sdk references from new structure.
All consistent with the updated SDK layout.
45-46: Cadance keeper and types imports.
These additions look correct for the new Cadance module integration.
59-67: Imports for packet-forward-middleware and async-icq v8.
Matches the IBC v8 suite. No issues found.
68-70: Added ibchooks references for v8.
This is correct for IBC hooks integration.
71-73: IBC wasm client and fee keeper from ibc-go v8.
Correct references for the new IBC-wasm modules.
77-82: Transitions to ibcclient from ibc-go v8.
No issues. Aligns with the v8 update.
86-97: Expanded wasm capabilities array.
Enabling support for CosmWasm 2.0/2.1 and custom “bitsong” is valid.
119-119: NewParamsKeeperfield inAppKeepers.
Straightforward addition for handling parameters.
183-183: Initializing theParamsKeeper.
Looks correct.
186-192: Setting up theConsensusParamsKeeperand linking tobApp.
Implementation seems consistent with the new cosmic SDK approach.
195-200: Capability keeper and scoping for new modules.
Ensures correct compartmentalization of capabilities.
202-203: Crisis keeper initialization withinvCheckPeriod.
Please ensure the configured period (fromappOpts) is as intended for production.
217-220: Upgrade keeper setup.
The approach for skip heights and the upgrade store is consistent with standard patterns.
223-227: NewAccountKeepercreation.
Configuration looks good, including the bech32 prefix.
229-229: BaseBankKeeperinitialization.
No concerns; uses the correct references for blocked addresses and logs.
234-235: AuthManager instantiation.
Clear approach for grouping authenticators.
246-248: FeeGrantKeeper creation.
Matches the newfeegrantimport path and references.
249-258: SmartAccountKeeper introduced.
Integration withFeeGrantKeeperand authenticator manager looks consistent.
259-261: AuthzKeeper addition.
No issues found in the initialization.
263-268: StakingKeeper setup.
Ensures correct references for new address codecs and the module address.
269-272: DistrKeeper creation.
Aligns with standard distribution keeper initialization.
274-275: SlashingKeeper creation.
Looks fine; references cdc and correct store key.
278-280: MintKeeper creation.
Properly references staking, account, and bank keepers.
288-288: Assigning stakingKeeper.
No further remarks.
290-290: InstantiatingIBCKeeper.
Standard configuration for IBC in v8.
295-299: ibchooks keeper creation.
This sets up special IBC hooks logic. Implementation looks correct.
301-309: IBCWasamClientKeeper creation with config.
Initialization approach appears correct for WASM clients.
311-317: ICS4 middleware hooking.
Integrates IBC hooks with the ICS4 wrapper properly.
319-328: PacketForwardKeeper with a nil transfer keeper initially.
You later set the transfer keeper, so this pattern is acceptable.
340-350: Composing transfer stack with multiple middleware layers.
Implementation is standard for IBC middlewares.
353-353: Comment aboutforwardTimeout.
No functional addition here. Skipping detailed review.
355-366: ICQKeeper creation and module setup.
No immediate concerns. This integrates the async ICQ keeper.
368-371: Extending IBC router withicqModule.
The routes are properly added.
392-392: Defining acceptedStargateQueries.
Allows whitelisted stargate queries from within contracts.
417-418: Appending stargate query support to wasm options.
Solid approach to unify configured queries with the WASM engine.
422-422: WasmKeeper instantiation.
Correct usage of the newly consolidated references and wasm config.
441-443: Contract keeper for ICS4 wrappers and hooking toIcs20WasmHooks.
No issues. Good bridging with WASM hooks.
445-451: CadanceKeeper instantiation, referencing WASM and contract keeper.
This integration into the new module appears correct.
453-456: RegisteringCosmwasmAuthenticator.
Moves cross-verification logic into the AuthenticatorManager.
457-459: IBC route to handle Wasm module.
Wires WASM to IBC—no issues noted.
461-461: Sealing IBC router.
Standard final step to confirm the IBC router configuration.
463-469: Registering new routes ingovRouter.
Supports proposals for params, IBC clients, and fan tokens.
485-485: Set legacy Gov router.
Ensures backward compatibility with Gov v1beta1.
495-497: Building IBC param key table and registering connection params.
Correct approach to handle specialized IBC parameters.
506-507: Defining subspaces foribcexportedandibctransfertypes.
Parameter scoping is well-defined.
509-511: Registering param subspace for ICQ, IBC hooks, and smart-account.
Uses dedicated param key tables for each. No issues.app/upgrades/v013/constants.go (1)
4-4: Switched store import tocosmossdk.io/store/types.
Transition aligns with the new module path structure.app/upgrades/v014/constants.go (1)
4-4: LGTM! Import path updated for cosmos-sdk v0.50 compatibility.The change from
github.com/cosmos/cosmos-sdk/store/typestocosmossdk.io/store/typesaligns with the cosmos-sdk v0.50 module restructuring.✅ Verification successful
LGTM – Upgrade Verified
The new import path (
cosmossdk.io/store/types) is correct for cosmos-sdk v0.50, as confirmed by the latest documentation and migration guides. The change aligns with the SDK's module restructuring.
- The updated import corresponds with the new module structure.
- It replaces the old import path with the one required for cosmos-sdk v0.50 compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What is the correct import path for store types in cosmos-sdk v0.50?Length of output: 3082
app/upgrades/v016/constants.go (1)
4-4: LGTM! Import path updated for cosmos-sdk v0.50 compatibility.The change from
github.com/cosmos/cosmos-sdk/store/typestocosmossdk.io/store/typesaligns with the cosmos-sdk v0.50 module restructuring.app/upgrades/v015/constants.go (1)
4-4: LGTM! Import path updated for cosmos-sdk v0.50 compatibility.The change from
github.com/cosmos/cosmos-sdk/store/typestocosmossdk.io/store/typesaligns with the cosmos-sdk v0.50 module restructuring.app/upgrades/v020/constants.go (1)
4-4: Verify compatibility with the updated Cosmos SDK import.The import path change from
github.com/cosmos/cosmos-sdk/store/typestocosmossdk.io/store/typesaligns with the PR objective to upgrade to [email protected]. Let's verify the compatibility:✅ Verification successful
Verification Complete: Updated Cosmos SDK Import is Compatible
- The
go.modfile confirms dependencies use updatedcosmossdk.io/*modules alongside the required cosmos-sdk version, ensuring compatibility.- No occurrences of the old import path (
github.com/cosmos/cosmos-sdk/store/types) were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check go.mod for cosmos-sdk version and related dependencies echo "Checking cosmos-sdk version..." grep "cosmossdk.io\|cosmos-sdk" go.mod echo -e "\nChecking for potential import conflicts..." # Look for any files still using the old import path rg "github.com/cosmos/cosmos-sdk/store/types"Length of output: 822
app/upgrades/v011/constants.go (1)
4-4: LGTM! Import path update is consistent.The import path change matches the update in v020, maintaining consistency across upgrade handlers.
app/upgrades/v016/upgrades.go (1)
12-14: Verify context type changes across all upgrade handlers.The switch from
sdk.Contexttocontext.Contextand addition ofbpm upgrades.BaseAppParamManagerparameter are breaking changes that need verification across all upgrade handlers.✅ Verification successful
Consistent Context Usage in Upgrade Handlers Verified
- The upgrade handler in
app/upgrades/v016/upgrades.gohas been updated to usecontext.Contextand includes the newbpm upgrades.BaseAppParamManagerparameter.- Searches for upgrade handler signatures (using patterns like
Create*UpgradeHandler) did not reveal any handlers still usingsdk.Context.- The remaining occurrences of
sdk.Context(in files likeapp/upgrades/types.go,app/upgrades/v020/upgrades.go, andapp/upgrades/v021/upgrades.go) appear in supporting helper functions rather than in functions that construct or implement upgrade handlers.Based on this verification, the breaking changes have been applied consistently in all upgrade handler definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent context type usage across upgrade handlers echo "Checking for any remaining sdk.Context usage in upgrade handlers..." rg "sdk\.Context" "app/upgrades" echo -e "\nVerifying BaseAppParamManager parameter in all upgrade handlers..." ast-grep --pattern 'func Create*UpgradeHandler($$$) upgradetypes.UpgradeHandler'Length of output: 1577
app/upgrades/v014/upgrades.go (2)
4-10: LGTM! Import changes align with [email protected].The updated import paths and package structure reflect the modularization in [email protected], particularly the move to
cosmossdk.ionamespace.
12-15: LGTM! Function signature changes align with [email protected].The changes to use
context.Contextand addbpmparameter align with the SDK's move away from custom context types and enhanced parameter management.Let's verify these changes are consistent across other upgrade handlers:
✅ Verification successful
Consistent Upgrade Handler Signatures Verified
The upgrade handler functions across versions (v010, v011, v013, v014, v015, and v016) consistently employ the updated signature usingcontext.Contextalong with the addedbpmparameter. Even in versions v018, v020, and v021—which introduce additional context processing withsdk.UnwrapSDKContext—the uniform usage ofcontext.Contextand the inclusion of thebpmparameter are maintained. This confirms that the changes in v014 are aligned with the updates in [email protected].🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent context and parameter usage across upgrade handlers # Search for upgrade handler function signatures rg -A 3 'func Create.*UpgradeHandler.*\(' 'app/upgrades'Length of output: 4053
app/upgrades/v015/upgrades.go (1)
4-10: LGTM! Changes maintain consistency with v014.The changes mirror those in v014/upgrades.go, maintaining consistency in the upgrade handlers' implementation.
Also applies to: 12-15
app/keepers/querier.go (2)
4-5: LGTM! Import change aligns with [email protected].The change to use
cosmossdk.io/store/typesreflects the SDK's modularization of store-related functionality.
9-11: LGTM! Type changes improve error handling.The transition to
storetypespackage and updated Query method signature with explicit error return improve error handling capabilities.Let's verify the impact of these changes on query implementations:
Also applies to: 13-13, 15-17, 19-21
✅ Verification successful
Verification: Query Implementations Updated Correctly
The search output confirms that the
Querymethod inapp/keepers/querier.gois now defined with the updated signature returning an error, and the change is consistently applied across the codebase without introducing broken implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for query implementations that might need updates # Search for Query method implementations rg -A 3 'func.*Query.*\(' --type goLength of output: 76220
app/encoding.go (2)
10-12: LGTM! Getter function provides controlled access.The
GetEncodingConfig()function provides a clean interface to access the encoding configuration.
8-9: Consider implications of global state.While global
encodingConfigis a common pattern in Cosmos SDK apps, it could make testing more challenging and might need special handling in concurrent scenarios.Let's check if there are any test files that might be affected:
app/upgrades/v010/constants.go (2)
4-5: Import paths updated to use cosmossdk.io domain.The changes align with Cosmos SDK v0.50's move to the
cosmossdk.iodomain.
8-8: Packetforward middleware upgraded to v8.The upgrade aligns with the PR objective to update ibc-go to v8.
app/params/proto.go (1)
15-15: Address prefix configuration issue.Same issue as in
amino.go- consider making address prefixes configurable.x/cadance/types/params.go (1)
25-36: LGTM! Well-structured error handling and validation.The implementation:
- Uses proper error wrapping with descriptive messages
- Performs appropriate validation checks
- Has clear documentation
tests/ict/basic_start_test.go (1)
9-10: LGTM! Import paths updated correctly.The import paths have been correctly updated from v7 to v8 for the interchaintest packages.
scripts/makefiles/tests.mk (2)
15-16: Remove duplicate line.The line
test-raceis duplicated.
3-32: LGTM! Well-structured test suite implementation.The test suite implementation is comprehensive and well-organized with:
- Clear help documentation
- Essential test targets (unit, race, cover, benchmark)
- Proper test flags including ledger mock tags
x/cadance/types/keys.go (2)
20-20: Verify the codespace value.Please confirm if "bitsong-global" is the intended codespace value. Typically, module-specific codespaces are used (e.g., "cade").
22-31: LGTM! Well-structured error definitions.The error definitions are well-organized with:
- Clear and descriptive error messages
- Proper error registration with unique codes
- Consistent error naming convention
app/upgrades/v013/upgrades.go (2)
26-29: Verify the wasm params configuration.Please confirm if these are the intended wasm params:
CodeUploadAccess: wasmtypes.AllowNobodyInstantiateDefaultPermission: wasmtypes.AccessTypeEverybodyThese settings seem contradictory as they allow everyone to instantiate contracts but nobody to upload code.
14-32: LGTM! Proper context and logger handling.The changes correctly:
- Update the function signature with BaseAppParamManager
- Handle context conversion using UnwrapSDKContext
- Initialize logger with proper upgrade name
btsgutils/btsgcli/string_formatter.go (2)
33-45: Consider using errors instead of panic for template errors.The function currently panics on template parsing and execution errors. Using panic for error handling is generally discouraged.
33-45: Add template sanitization to prevent potential injection.The template parsing is done without any sanitization of the input string, which could potentially lead to template injection if the input is not trusted.
Consider adding template function map with safe functions only:
-func FormatLongDesc(longString string, meta *LongMetadata) string { +func FormatLongDesc(longString string, meta *LongMetadata) string { + funcMap := template.FuncMap{ + "html": template.HTMLEscapeString, + "js": template.JSEscapeString, + } - template, err := template.New("long_description").Parse(longString) + template, err := template.New("long_description").Funcs(funcMap).Parse(longString)app/types.go (1)
22-28: LGTM! Improved error handling in interface methods.The changes to BeginBlocker, EndBlocker, and InitChainer signatures to include error returns align with Go's error handling best practices.
x/cadance/types/codec.go (2)
24-29: LGTM! Proper message registration.The message types are correctly registered with the LegacyAmino codec following Cosmos SDK patterns.
31-41: LGTM! Complete interface registration.The interface registration is comprehensive, including all message types and the message service descriptor.
btsgutils/btsgcli/dynamic.go (1)
21-38: Add error handling for reflection operations.The function uses reflection but doesn't handle potential errors that could occur during field access.
app/upgrades/types.go (2)
4-5: LGTM! Import paths updated for cosmos-sdk v0.50.The import paths have been correctly updated to use the new
cosmossdk.ionamespace.
30-31: Verify consensus parameter handling in dependent code.The method signatures have changed from pointer types to value types for ConsensusParams. This change might affect performance and memory usage for large parameter sets.
✅ Verification successful
Consensus parameter handling in dependent code is correct.
- The usage in
app/export.godirectly callsapp.BaseApp.GetConsensusParams(ctx)and assigns the returned value, confirming that the system now consistently uses value types.- There are no lingering dependencies expecting pointer types for
tmproto.ConsensusParams.- Based on current evidence, the change from pointer types to value types does not introduce any performance or memory issues given the size and usage of consensus parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of these methods to ensure they handle value types correctly rg -A 3 "GetConsensusParams|StoreConsensusParams"Length of output: 379
app/upgrades/v011/upgrades.go (1)
34-36: Address commented-out code for merkledrop fees.The commented code for updating merkledrop fees suggests incomplete implementation. Either implement the feature or remove the commented code.
Would you like me to help implement the merkledrop fees update or should we remove this commented section?
x/fantoken/types/expected_keepers.go (1)
11-11: LGTM! Context type updates for cosmos-sdk v0.50.The interface method signatures have been correctly updated to use
context.Contextinstead ofsdk.Context, aligning with the cosmos-sdk v0.50 changes.Also applies to: 28-32
x/fantoken/keeper/store.go (1)
6-6: Error handling update aligns with cosmos-sdk v0.50 but needs consistency across module.The switch from
sdkerrorstocosmossdk.io/errorsis correctly implemented. However, this change needs to be applied consistently across the entire fantoken module.Also applies to: 35-35
app/upgrades/v021/constants.go (2)
31-37: Store upgrades look good but verify module dependencies.The added store keys align with the upgrade requirements. However, let's verify the compatibility of these modules with cosmos-sdk v0.50.
✅ Verification successful
Module dependencies verified for cosmos-sdk v0.50 upgrade
- Confirmed that
github.com/cosmos/cosmos-sdk v0.50.11is present in go.mod.- IBc-related dependencies (e.g.,
github.com/cosmos/ibc-go/v8) and other cosmos modules are correctly aligned.- No compatibility issues were found with the upgrade dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check module compatibility in go.mod grep -A 1 "require (" go.mod grep -A 5 "require (" go.mod | grep "cosmos-sdk" grep -A 5 "require (" go.mod | grep "ibc-go"Length of output: 214
Script:
#!/bin/bash # Display the entire go.mod file to inspect all module dependencies echo "==== Content of go.mod ====" cat go.mod # Search for any mention of cosmos-related dependencies. echo "==== Searching for 'cosmos' in go.mod ====" grep -i "cosmos" go.mod # Search for dependencies using the new cosmos SDK namespace. echo "==== Searching for 'cosmossdk.io' in go.mod ====" grep "cosmossdk.io" go.mod # Search for any mention of IBC-related dependencies. echo "==== Searching for 'ibc' in go.mod ====" grep -i "ibc" go.modLength of output: 13018
17-19: Verify the gas limit for unauthenticated transactions.The 120,000 gas limit for smart account transactions seems reasonable, but let's verify against cosmos-sdk v0.50 recommendations.
x/cadance/keeper/querier.go (1)
12-22: LGTM! Clean implementation of the Querier struct and constructor.The implementation follows the standard Cosmos SDK patterns for query servers. The interface compliance check with
types.QueryServeris a good practice.app/config.go (2)
6-6: LGTM! Updated to use cosmos-db.The update from cometbft-db to cosmos-db aligns with the latest Cosmos SDK practices.
48-56: Verify the chain ID validation.Ensure that the chain ID is properly validated before being used in the application constructor.
x/fantoken/types/params.go (1)
6-6: LGTM! Updated to use cosmossdk.io/math.The update to use the new math package from cosmossdk.io aligns with the latest Cosmos SDK practices.
scripts/makefiles/proto.mk (1)
36-38: Verify the new proto-pulsar-gen command's integration.The new command for generating Dep-Inj Protobuf files needs verification to ensure it's properly integrated with the build system.
✅ Verification successful
Proto-pulsar-gen integration verified with a minor permission note
- The target in
proto.mkcorrectly invokes the script viash, so integration with the build system is effective.- The file
scripts/protocgen-pulsar.shexists, but it isn’t marked as executable. Since the command explicitly calls it withsh, the missing executable flag does not break the build.- If you ever intend to run the script directly (without an explicit shell), you may need to update its permissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the protocgen-pulsar.sh script exists and is executable if [ -f "./scripts/protocgen-pulsar.sh" ]; then echo "Script exists" if [ -x "./scripts/protocgen-pulsar.sh" ]; then echo "Script is executable" else echo "Script is not executable" fi else echo "Script does not exist" fiLength of output: 181
x/cadance/keeper/keeper.go (1)
54-65: 🛠️ Refactor suggestionAdd validation for empty parameters in SetParams.
The SetParams method should validate that the parameters are not nil before validation.
// SetParams sets the x/cadance module parameters. func (k Keeper) SetParams(ctx sdk.Context, p types.Params) error { + if &p == nil { + return fmt.Errorf("parameters cannot be nil") + } + if err := p.Validate(); err != nil { return err }Likely invalid or redundant comment.
tests/ict/polytone_types.go (1)
4-6: Verify compatibility with wasmvm v2.The upgrade to wasmvm v2 might introduce breaking changes. Ensure all type definitions are compatible with the new version.
✅ Verification successful
Compatibility with wasmvm v2 Verified
The test file’s import of
github.com/CosmWasm/wasmvm/v2/typesis aligned with the expected location for wasmvm v2. Although the release notes indicate several breaking changes (in serialization, deprecated queries, mock API modifications, consensus/upgraded compiler changes, and IBC modifications), none of these specifically affect the basic type definitions imported in this file. Therefore, no modifications are required in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What are the breaking changes in CosmWasm/wasmvm v2?Length of output: 4944
tests/ict/polytone_test.go (1)
19-82: Add test coverage for edge cases and error scenarios.The test function covers basic connection rules but could be enhanced with additional test cases:
- Test with invalid addresses
- Test with malformed messages
- Test error handling for failed channel creation
Also, there's a known issue with channel creation that's currently commented out. Consider creating a tracking issue for this.
Would you like me to:
- Generate additional test cases?
- Create an issue to track the channel creation bug?
app/upgrades/v010/upgrades.go (3)
4-19: LGTM! Import paths updated to use cosmossdk.io packages.The migration from legacy SDK packages to cosmossdk.io packages aligns with the Cosmos SDK v0.50 upgrade.
21-26: Verify the BaseAppParamManager integration.The function signature has been updated to include
bpm upgrades.BaseAppParamManager, but it's not being used in the function. Consider if this parameter is necessary.
51-55: LGTM! Improved error handling for validator operations.The code now properly handles potential errors from
GetAllValidators.app/keepers/keys.go (1)
34-61: LGTM! Store keys properly organized.The store keys are well-organized and include new modules (cadance, smart-account) while maintaining a clear structure.
x/cadance/client/cli/tx.go (1)
49-52: LGTM! Proper message construction and validation.The code properly constructs the message and validates it before broadcasting.
app/genesis.go (1)
96-96: Verify security implications of allowing unrestricted WASM code uploads.Changing
CodeUploadAccessfromAllowNobodytoAllowEverybodyallows any user to upload WASM code. This could potentially introduce security risks if not properly managed through other security measures.Please confirm:
- Is this change intentional as part of the upgrade to wasmvm@v2?
- Are there other security measures in place to mitigate potential risks from malicious WASM code uploads?
- Has this configuration been tested in a staging environment?
app/upgrades/v020/upgrade_test.go (1)
84-102: Verify test coverage for reward calculations.The commented out test section suggests that reward calculation tests are not being executed. This could lead to potential issues going undetected.
Please clarify:
- Why were these tests commented out?
- Is there an alternative test coverage for reward calculations?
- When will these tests be re-enabled?
x/fantoken/keeper/msg_server.go (1)
111-111: LGTM! Error handling updated correctly.The transition to use
errors.Wrapffromcosmossdk.io/errorsis implemented correctly, maintaining consistent error handling across the codebase.x/fantoken/types/genesis_test.go (1)
1-192: LGTM! Migration from sdk.Int to math.Int is correctly implemented.The changes consistently replace
sdk.NewIntandsdk.ZeroIntwith theirmathpackage equivalents throughout the test cases, maintaining the same functionality while aligning with the updated dependencies.tests/ict/setup.go (1)
89-89: Verify the impact of commenting out WASM interface registration.The WASM interface registration has been commented out. This might affect the ability to handle WASM-related messages and queries in tests.
✅ Verification successful
WASM Registration Has Minimal Impact
The search shows that the only WASM registration occurs in the commented line within
tests/ict/setup.go(including a similar commented registration for fantokentypes), and no WASM message handlers were detected elsewhere in the codebase. This indicates that currently, commenting outwasmtypes.RegisterInterfaces(cfg.InterfaceRegistry)does not affect any active WASM functionality in our tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if WASM interfaces are registered elsewhere or if this causes any issues. # Search for other WASM interface registrations rg -A 5 'wasmtypes\.RegisterInterfaces' # Search for WASM message handlers to see what might be affected ast-grep --pattern 'func $_(msg wasmtypes.$_) $_ { $$$ }'Length of output: 350
app/testing/test_suite.go (2)
41-44: LGTM! Improved test state management.The addition of
hasUsedAbciflag helps optimize test runs by controlling when to reuse or recreate the app instance, reducing unnecessary setup overhead.
60-70: LGTM! Enhanced test isolation.The setup now uses a temporary directory that is automatically cleaned up after tests, ensuring proper isolation between test runs.
x/cadance/keeper/keeper_test.go (2)
53-55: Remove redundant test function.This test function is redundant as it's identical to
TestIntegrationTestSuiteat lines 30-32.-func TestKeeperTestSuite(t *testing.T) { - suite.Run(t, new(IntegrationTestSuite)) -}
22-43: LGTM! Well-structured test suite with good abstractions.The test suite is well-organized with:
- Clear setup of dependencies and test environment
- Helper methods that encapsulate common operations
- Good error handling and assertions
Also applies to: 112-134
app/upgrades/v021/upgrade_test.go (3)
1-34: LGTM! Well-structured test suite setup.The test suite is properly initialized with necessary dependencies and helper methods.
35-166: LGTM! Comprehensive test coverage.The test cases thoroughly validate various upgrade scenarios including:
- App module parameters
- Zero shares with jailed validators
- Distribution reward invariants
168-177: LGTM! Well-implemented helper function.The
dummyUpgradefunction provides a clean way to simulate upgrades in tests.app/upgrades/v018/upgrades.go (2)
4-28: LGTM! Import updates align with dependency upgrades.The import statements have been properly updated to use:
- IBC v8 packages
- New context and math packages
30-31: LGTM! Function signature updated for new parameter management.The handler now accepts a BaseAppParamManager and uses standard context.Context.
x/cadance/keeper/querier_test.go (3)
12-49: LGTM! Well-structured parameter test cases.The test cases thoroughly validate parameter querying with different gas limits.
52-105: LGTM! Comprehensive contract query testing.The test cases effectively validate contract querying for various scenarios with proper cleanup.
165-232: LGTM! Thorough contract query validation.The test cases effectively validate individual contract queries with proper error handling for both valid and invalid scenarios.
tests/ict/basic_upgrade_test.go (3)
17-24: LGTM! Import paths updated for new versions.The import statements have been properly updated to use:
- Interchaintest v8
- New cosmossdk.io paths
177-197: LGTM! Function signature updated for new proposal submission.The handler now accepts a chainUser parameter for proposal building.
155-159: LGTM! Type adjustments for library compatibility.The proposal ID type has been properly updated to uint64 for consistency with the interchaintest library.
x/fantoken/keeper/keeper_test.go (3)
22-23: LGTM! Consistent use of math package for integer operations.The transition from
sdk.Inttomath.IntforinitAmtandmaxSupplyaligns with the keeper implementation.Also applies to: 27-27
32-34: LGTM! Enhanced test suite structure.The test suite now embeds
apptesting.KeeperTestHelper, providing better test setup and helper functions.
42-49: LGTM! Simplified test setup.The setup process is streamlined using the helper's
Setup()method and properly initializes the test context.x/cadance/abci_test.go (2)
22-24: LGTM! Well-structured test suite.The test suite properly embeds
apptesting.KeeperTestHelperfor comprehensive testing capabilities.
191-218: LGTM! Comprehensive performance testing.The test effectively validates the system's behavior with a large number of contracts, ensuring scalability.
app/test_helper.go (1)
69-72: LGTM! Improved test setup flexibility.The
Setupfunction now accepts a boolean parameter and uses a custom home directory, enhancing test configuration options.x/fantoken/keeper/keeper.go (2)
3-14: LGTM! Updated dependencies for better error handling.The transition to
cosmossdk.io/errorsandcosmossdk.io/mathimproves error handling and mathematical operations.
60-63: LGTM! Consistent error handling pattern.The error wrapping using
errors.Wrapfprovides better context and maintains consistency across the codebase.Also applies to: 67-68, 72-73
tests/ict/module_pfm_test.go (2)
19-19: Verify compatibility with ibc-go v8.The upgrade from ibc-go v7 to v8 might introduce breaking changes in the transfer types.
10-15: Verify compatibility with updated interchaintest v8.The upgrade from interchaintest v7 to v8 might introduce breaking changes.
✅ Verification successful
Compatibility with interchaintest v8 looks correctly addressed in this file
- The imports have been updated to use the v8 module paths.
- The file no longer refers to any v7 paths, so at least on the import level it is compatible.
- As noted in the breaking changes, ensure that any legacy API calls (for example, querying bank balances via legacy RPC methods) have been refactored to use the new native command interfaces if applicable in the rest of your test code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What are the breaking changes between interchaintest v7 and v8?Length of output: 5982
x/fantoken/types/msgs.go (2)
3-10: Good migration to cosmossdk.io packages.The change from
sdk.Inttomath.Intand the use ofcosmossdk.io/errorsaligns with the latest Cosmos SDK best practices.
35-44: Verify maxSupply type compatibility.The change from
sdk.Inttomath.IntinNewMsgIssueparameters needs verification for backward compatibility.✅ Verification successful
MaxSupply type compatibility verified.
The search for any usage of
sdk.Intin fantoken-related files returned no results, which indicates that the change to usingmath.Intis consistent throughout the codebase. This confirms backward compatibility for themaxSupplyparameter inNewMsgIssue.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining sdk.Int usage in fantoken related files rg "sdk\.Int" --type go | grep -i "fantoken"Length of output: 44
cmd/bitsongd/cmd/genesis.go (3)
98-107: Well-structured governance parameters with clear comments.The governance parameters are well-documented with clear percentage values in comments.
89-93: Consider using non-legacy decimal methods.The use of
math.LegacyMustNewDecFromStrmight be deprecated in future versions.✅ Verification successful
Action: Replace legacy decimal parsing methods in
cmd/bitsongd/cmd/genesis.goThe review comment is accurate. Instead of using the deprecated
math.LegacyMustNewDecFromStr, you should migrate to the non-legacy functions provided by the Cosmos SDK, for example:
- Use
math.NewDecFromStrfrom thecosmossdk.io/mathpackage.- Note that
math.NewDecFromStrreturns an error rather than panicking, so adjust error handling accordingly or wrap it in a helper method if identical behavior is needed.Steps:
- Update lines 89–93 of
cmd/bitsongd/cmd/genesis.goto use the new function.- Add proper error handling where necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
Are there non-legacy alternatives to math.LegacyMustNewDecFromStr in Cosmos SDK?Length of output: 4184
144-144: Verify impact of PrepareGenesis signature change.The change from
*tmtypes.GenesisDocto*genutiltypes.AppGenesismight affect existing code.✅ Verification successful
I'm not seeing any references to the old type in relation to the
PrepareGenesisfunction. To be extra sure, let's search for all call sites forPrepareGenesisas well as any remaining references totmtypes.GenesisDocin the repository.
Impact of PrepareGenesis Signature Change Verified
- The only active call site for
PrepareGenesis(incmd/bitsongd/cmd/genesis.goline 254) already uses the updated signature with*genutiltypes.AppGenesis.- The only occurrence of
*tmtypes.GenesisDocwas in commented-out code incmd/bitsongd/cmd/init_from_state.go.No active code references the old signature, so the change does not impact the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uses of PrepareGenesis rg "PrepareGenesis.*tmtypes\.GenesisDoc"Length of output: 42
Script:
#!/bin/bash echo "Searching for call sites to PrepareGenesis..." rg -n "PrepareGenesis\(" echo "" echo "Searching for any references to tmtypes.GenesisDoc..." rg -n "\*tmtypes\.GenesisDoc"Length of output: 821
x/fantoken/client/cli/tx.go (2)
8-8: LGTM!The import of
cosmossdk.io/mathaligns with the migration to use the new math package from cosmos-sdk.
451-453: LGTM!The removal of
ValidateBasic()check is correct as it's not an interface function in cosmos-sdk v0.50.x.x/fantoken/client/cli/cli_test.go (4)
8-8: LGTM!The import of
cosmossdk.io/mathand update ofmaxSupplyto usemath.NewIntare consistent with the migration to the new math package.Also applies to: 28-28
60-60: LGTM!The function signature update to use
math.Intis consistent with the migration to the new math package.
70-70: LGTM!The fee calculations have been consistently updated to use
math.NewIntacross all command functions.Also applies to: 94-94, 114-114, 135-135, 157-157, 178-178, 200-200
426-428: LGTM!The test assertions have been correctly updated to use
math.NewIntandmath.ZeroInt.Also applies to: 464-464
proto/buf.gen.swagger.yaml (1)
1-5: LGTM!The Swagger configuration is well-structured with appropriate options for generating API documentation.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
x/cadance/README.md (1)
1-5: LGTM!The documentation clearly explains the purpose of the Cadance module and provides a link to detailed specifications.
proto/buf.gen.pulsar.yaml (1)
1-17: New YAML Configuration Looks Good.
The file correctly defines the version, management settings, and plugin configurations for generating Pulsar-related code.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
proto/bitsong/cadance/v1/cadance.proto (1)
1-13: New Proto File Addition Looks Good.
The new protocol buffer file is well-structured with proper proto3 syntax and clearly defines theCadanceContractmessage to manage contract address and jail status, which aligns with the intended Cadance module enhancements.x/README.md (1)
9-12: Modules List Updated Correctly.
The updated module list now reflects the removal of the deprecatedmerkledropmodule and the addition of the newcadanceandsmart-accountsmodules. The links to their respective READMEs ensure easy access to further documentation.tests/localbitsong/docker-compose.yml (1)
10-11: Docker Build Arguments Updated.
The update forRUNNER_IMAGEandGO_VERSIONensures consistency with the upgraded Go version (1.23.0) across the project’s Docker and CI configurations.scripts/protocgen.sh (3)
2-2: Simplified Error Handling.
Replacingset -eo pipefailwithset -esimplifies the error handling within the script. Ensure that this change does not mask commands whose failure you’d rather detect.
17-20: File Reorganization Commands – Double Check Impact.
The commands that copy files fromgithub.com/bitsongofficial/go-bitsong/*to the current directory and then remove thegithub.comdirectory should be carefully verified. Ensure that these operations do not accidentally delete or overwrite important files.
6-9: For-Loop over Find Output: Potential Fragility.
Using a for-loop to iterate overfindresults (especially withxargs) can be error-prone when filenames contain spaces or special characters. Consider refactoring this loop with a more robust construct such as a while-read loop. For example:- proto_dirs=$(find ./bitsong -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq) - for dir in $proto_dirs; do - for file in $(find "${dir}" -maxdepth 1 -name '*.proto'); do - if grep go_package $file &> /dev/null ; then - buf generate --template buf.gen.gogo.yaml $file - fi - done - done + while IFS= read -r -d '' dir; do + while IFS= read -r -d '' file; do + if grep -q go_package "$file"; then + buf generate --template buf.gen.gogo.yaml "$file" + fi + done < <(find "$dir" -maxdepth 1 -name '*.proto' -print0) + done < <(find ./bitsong -type d -name 'bitsong' -print0)This approach is more robust and less likely to break with unusual filenames.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 8-8: For loops over find output are fragile. Use find -exec or a while read loop.
(SC2044)
proto/buf.yaml (1)
21-23: New linting rules added for package validation.
The addition ofPACKAGE_SAME_GO_PACKAGE,PACKAGE_SAME_DIRECTORY, andPACKAGE_DIRECTORY_MATCHhelps enforce organization consistency. Please ensure these rules are compatible with the rest of your project’s structure and that your team is aware of the newly enforced constraints.x/cadance/spec/README.md (1)
1-16: New module documentation added.
The README is well structured and provides a clear overview of thex/cadancemodule, outlining its purpose and contents. In future iterations, consider adding usage examples or specific code snippets to further aid developers in integrating the module.proto/bitsong/cadance/v1/genesis.proto (2)
1-27: New genesis protobuf definitions added.
The definitions forGenesisStateandParamsare clear and follow proto3 standards. Just ensure the imported files—includinggogoproto/gogo.proto—are accessible to avoid issues during code generation.🧰 Tools
🪛 Buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
4-4:⚠️ Potential issueVerify imported file path.
The import statementimport "gogoproto/gogo.proto";may lead to compilation issues if the file does not exist at the specified location. Please double-check the path or update it to point to the correct location (e.g., using the repository of gogo/protobuf if applicable).🧰 Tools
🪛 Buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/bitsong/smartaccount/v1beta1/models.proto (1)
1-24: New AccountAuthenticator message added.
The protobuf messageAccountAuthenticatoris clearly defined with detailed comments on each field. Ensure that any consumers of this message are updated to interpret the overloadedconfigfield correctly based on the authenticator type.swagger/config.json (2)
25-29: Renamed tag from "Gaia REST" to "CometBft RPC".
The updated tag name better reflects the current consensus and service naming. Confirm that all documentation and integrations referencing the old tag are updated accordingly to avoid confusion.
30-34: Renamed operationId for UpgradedConsensusState.
Changing the operationId from"UpgradedConsensusState"to"sdk/UpgradedConsensusState"improves clarity regarding its origin. Please verify that downstream tooling and API consumers are aligned with this update.proto/bitsong/smartaccount/v1beta1/params.proto (2)
1-7: Dependency Check and File SetupThe header—including the syntax, package declaration, and go package option—appears correct. However, the import statement for
"gogoproto/gogo.proto"is triggering static analysis warnings. Please verify that this dependency is correctly included in your project or provided via your dependency management system.🧰 Tools
🪛 Buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
8-26: Robust Params Message DefinitionThe
Paramsmessage is clearly defined with well-documented fields and appropriate YAML annotations. All field names and tags follow a consistent naming convention. Ensure that any downstream code consuming these parameters is updated to reflect the changes.scripts/polytone.sh (1)
4-4: Updated WASM Directory PathThe directory path for storing the WASM files has been updated to
"tests/ict/contracts/". Please confirm that all related scripts, CI configurations, and documentation reference this new path to ensure consistency.proto/bitsong/fantoken/v1beta1/fantoken.proto (1)
33-37: Updated Custom Type for max_supplyThe change in the custom type now uses
"cosmossdk.io/math.Int"instead of the previous"github.com/cosmos/cosmos-sdk/types.Int". This update aligns with the broader refactoring for integer handling. Please double-check that the corresponding Go modules and protobuf generators properly support this new type.proto/bitsong/smartaccount/v1beta1/genesis.proto (3)
1-7: File Header and ImportsThe package declaration, syntax, and import statements are standard. As with
params.proto, please ensure the"gogoproto/gogo.proto"import is correctly available in the environment.🧰 Tools
🪛 Buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
10-21: AuthenticatorData Message StructureThe
AuthenticatorDatamessage is well-structured and documented. It clearly specifies the purpose of storing account addresses and their associated authenticators. If applicable, consider adding any validation constraints in the application logic that consumes these messages.
23-35: GenesisState Message CompletenessThe
GenesisStatemessage is comprehensive, capturing required parameters, the next available authenticator ID, and a list of authenticator data. Ensure consistency across modules that depend on these genesis definitions.x/cadance/spec/02_state.md (1)
7-16: CadanceContract Definition in State ObjectsThe code snippet defining
CadanceContractis concise and well-commented. It effectively explains the purpose of storing a contract’s address and its jail status.proto/bitsong/smartaccount/v1beta1/query.proto (3)
1-9: Dependency Imports: Verify the import paths.
Static analysis indicates that the import on line 4 (import "gogoproto/gogo.proto";) may not be found. Please confirm that the intended file is available or update the path (for example, it might need to begithub.com/gogo/protobuf/gogoproto/gogo.protoif that is where it resides).🧰 Tools
🪛 Buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
12-30: Service Definition: Check RPC HTTP Options.
The newQueryservice with its three RPC methods (Params,GetAuthenticator, andGetAuthenticators) is clearly defined and the HTTP options appear consistent with expected RESTful endpoints. Just ensure that the endpoints (with path parameters) match the upstream routing conventions.
32-58: Message Definitions: Clarity and Consistency.
All the request and response message definitions (includingQueryParamsRequest,QueryParamsResponse,GetAuthenticatorsRequest/Response, andGetAuthenticatorRequest/Response) are defined correctly with clear field numbering. Verify that the message typeParams(used insideQueryParamsResponse) is defined in the importedparams.protoand that its usage aligns with your application logic.proto/bitsong/smartaccount/v1beta1/tx.proto (2)
1-8: Import Path Verification for TX Module.
Line 4’s import (import "cosmos/msg/v1/msg.proto";) is reported by static analysis as not existing. Please verify if the intended proto file is located at this path or if it requires an update. This is important to ensure that message signing definitions are properly included.🧰 Tools
🪛 Buf (1.47.2)
4-4: import "cosmos/msg/v1/msg.proto": file does not exist
(COMPILE)
9-19: Msg Service RPC Definitions: Clean and Concise.
TheMsgservice definition with RPC methods for adding, removing, and setting the active state of authenticators is well structured. The use of HTTP method options is omitted here (which is typical for transaction messages), so ensure that the intended behavior is consistent with your transaction handling.Dockerfile (3)
3-3: Base Image Upgrade: Alignment with Project Updates.
The base image upgrade fromgolang:1.22-alpinetogolang:1.23-alpineis correctly applied and aligns with both the PR title and other build files.
21-27: CosmWasm Library Download & Checksum Verification.
The revised multi-command block that downloads thelibwasmvm_muslcfile and then verifies its checksum is a neat improvement. A few suggestions:
• Ensure that the target directory (/lib) exists and that the container user has proper permissions to write there.
• Consider adding a clearer error message or exit condition if the checksum verification fails.
29-39: Build and Static Linking Checks.
The subsequent commands that copy the project code, build the binary with static linking, and validate the build (usingfileandgrep) are well thought out. Ensure that any future changes in the build process remain compatible with your static linking goals.x/cadance/spec/01_concepts.md (1)
4-7: New "Clock" Module Overview: Clarity and Detail.
The introduction of the “Clock” module with a detailed explanation of its purpose, execution conditions, and failure handling is clear and comprehensive. The explanation fits well with the broader Cadance module updates.🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: The word “setup” is a noun. The verb is spelled with a space.
Context: ... need for external bots. Developers can setup their contract with x/Clock by register...(NOUN_VERB_CONFUSION)
scripts/protocgen-pulsar.sh (2)
3-31: Protoc Installation and Cleanup Logic: Clear and Functional.
The functionprotoc_install_gopulsarinstalls the required Go packages and the subsequent commands clean the API directory before generation. The commented-out sed fixes are preserved for reference. This script is straightforward and meets its intended purpose.
1-1: Shebang Missing: Specify the Interpreter.
Adding an explicit shebang (for example,#!/bin/bash) at the top of the script will clarify the intended interpreter and improve compatibility with static analysis (ShellCheck) and execution in different environments.Proposed Diff:
-# this script is for generating protobuf files for the new google.golang.org/protobuf API +#!/bin/bash +# this script is for generating protobuf files for the new google.golang.org/protobuf API✅ Verification successful
Shebang Missing: Add Shebang for Clarity
The file currently starts with a comment line rather than an explicit shebang. Adding a shebang (e.g.,
#!/bin/bash) will indicate the intended interpreter and improve compatibility with tools like ShellCheck.
- File:
scripts/protocgen-pulsar.sh- Recommended Change: Insert
#!/bin/bashat the top of the file.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
proto/bitsong/cadance/v1/query.proto (1)
4-4: Verify import path for pagination.proto.
The static analysis tool flagged the import of"cosmos/base/query/v1beta1/pagination.proto"as potentially problematic. Please confirm that this file exists at the specified path or update the import path to match the current project structure and dependency versions.🧰 Tools
🪛 Buf (1.47.2)
4-4: import "cosmos/base/query/v1beta1/pagination.proto": file does not exist
(COMPILE)
.github/workflows/interchaintest-e2e.yml (2)
17-17: Confirm Go version update.
The environment variableGO_VERSIONhas been updated to1.23.0, which aligns with the broader upgrade. Please ensure that all build and test jobs are compatible with this version.
77-82: Consider updating artifact download action version.
While theactions/upload-artifactaction was updated to v4, theactions/download-artifactstep still uses v3. For consistency and potential improvements in functionality or security, consider updating it to v4 if there are no compatibility issues.🧰 Tools
🪛 actionlint (1.7.4)
78-78: the runner of "actions/download-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/push-docker-image.yml (1)
21-26: Base image updates look good.
The changes updatingRUNNER_BASE_IMAGE_DISTROLESS,RUNNER_BASE_IMAGE_NONROOT, andRUNNER_BASE_IMAGE_ALPINEreflect a shift to newer, more secure images. Please verify these images are fully compatible with your Docker build process.scripts/generate-swagger-docs.sh (1)
9-9: IBC Version Update Confirmation
The retrieval of the IBC version has been updated to useibc-go/v8, which aligns with the current dependency update. Please verify that the version string returned byis compatible with the downstream Swagger generation (especially the URL in line 12).
✅ Verification successful
IBC Version Retrieval Verified
The command now returns the version stringv8.5.2. This version tag is formatted as expected for a GitHub release and should integrate correctly into the URL constructed in line 12 of the Swagger generation script. No compatibility issues have been detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
go list -m -f '{{ .Version }}' github.com/cosmos/ibc-go/v8Length of output: 67
proto/bitsong/cadance/v1/tx.proto (1)
1-100: New Cadance Module Protocol Definitions
This new Protocol Buffers file for thebitsong.cadance.v1module introduces RPC service endpoints for managing cadance contracts and updating module parameters. Please ensure that:
- The
Paramstype referenced inMsgUpdateParamsis defined (or imported) elsewhere in the project.- The HTTP annotations attached to the RPC methods correctly generate the intended REST endpoints.
Overall, the definitions are clear and comprehensive.🧰 Tools
🪛 Buf (1.47.2)
6-6: import "google/api/annotations.proto": file does not exist
(COMPILE)
.github/workflows/release.yml (7)
24-24: Update Go Version in Release Job
The Go version in the release job is now specified as 1.23. Ensure that this version is fully compatible with all build steps and dependencies in the release process.
52-53: Update Go Version in Build-MacOS Job
The macOS build job now uses Go 1.23. This update should be validated with all macOS-targeted builds and tests to prevent regressions.
83-84: Update Go Version in Build-Linux Job
The Linux build job has been updated to use Go 1.23. Please confirm that this change works across bothamd64andarm64architectures without issues.
128-128: Update Go Version in Build-Windows Job
The Windows job now uses Go 1.23. Ensure that Windows-specific build and test processes perform as expected with this version update.
59-59: Artifact Upload Action Version Update (macOS)
The macOS job now employsactions/upload-artifact@v4instead of v3. Verify that the updated action handles the artifact packaging and upload correctly.
98-104: Artifact Upload Action Version Update for Linux Job
The Linux job has updated the artifact upload steps to useactions/upload-artifact@v4for bothamd64andarm64binaries. Double-check that the artifact paths remain accurate after this upgrade.
136-136: Artifact Upload Action Version Update (Windows)
In the Windows build job, the upload artifact step now usesactions/upload-artifact@v4. Confirm that this change does not affect how the Windows binaries are collected and stored.README.md (2)
19-19: Update Go Version Requirement in Documentation
The note now specifies that BitSong requires Go 1.23.x+. This change ensures consistency with the updated build pipelines. Please verify that all contributors are aware of this upgraded minimum requirement.
31-31: Update Go Installation Command
The installation command has been updated to download Go version 1.23.5. Ensure that 1.23.5 is the recommended stable release that aligns with all aspects of the project.scripts/test_node.sh (3)
62-62: Add Expedited Voting Period Configuration
A new genesis configuration line sets theexpedited_voting_periodto "6s" in the governance parameters. Please verify that this value is suitable for the intended expedited voting process in your test environment.
74-75: Update Genesis Account Allocation
The genesis account addition commands now include specific coin allocations (e.g.,10000000ubtsg,1000utest). Confirm that the comma-separated format is correctly parsed by your application's genesis initialization logic.
78-78: Include Sequence Flag in Genesis Transaction
The genesis transaction command now includes the--sequence 1flag. Ensure that this change aligns with your expected transaction sequencing and does not conflict with nonce management for genesis accounts.proto/bitsong/fantoken/v1beta1/tx.proto (2)
2-2: Package Versioning Updated
Updating the package declaration tobitsong.fantoken.v1beta1reinforces proper semantic versioning. Ensure that any modules or client code depending on this package are updated accordingly.
43-47: Custom Type Update for max_supply
The change of the(gogoproto.customtype)formax_supplyfrom the previous type to"cosmossdk.io/math.Int"aligns with the broader migration to the new math package. Please verify that all usages ofmax_supplyin the codebase and downstream modules correctly handle the new type.Makefile (2)
3-11: Refined Build Script Includes
The Makefile now removes the obsoleteinclude contrib/devtools/Makefileand incorporates updated build script files such asscripts/makefiles/format.mk,scripts/makefiles/proto.mk, andscripts/makefiles/tests.mk. This helps modernize and modularize the build process. Please ensure that these included files exist and perform as expected.
17-29: Enhanced Help Command Messaging
The help target has been updated to display clearer instructions (“To run commands, use:”) along with new entries likemake formatandmake test. This makes the available commands more discoverable.tests/localbitsong/scripts/setup.sh (4)
6-9: Set Validator Identifier and Mnemonic for Testing
Introducing theMONIKERvariable (set to"val") and hardcoding the mnemonic simplifies the setup for LocalBitSong. Remember that hardcoding sensitive values is acceptable only for local testing environments.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 6-6: Use var=$(command) to assign output (or quote to assign string).
(SC2209)
53-66: Hardcoded Genesis Accounts for Local Testnet
The addition of multiple genesis account commands with fixed addresses streamlines the initialization for testing. Verify that these addresses and their associated balances are intended strictly for a non-production, controlled testnet environment.
67-68: Consistent Use of MONIKER in Key Recovery and Genesis Tx
Switching to$MONIKERfor adding keys and generating the genesis transaction enhances consistency across the script. This update minimizes the risk of misreferencing a previously used variable (such as$VAL).
136-136: Initialization Command Updated to Use MONIKER
The command initializing the blockchain now correctly references$MONIKERrather than$VAL, ensuring consistency. This update reduces confusion and potential errors during recovery.go.mod (4)
3-3: Go Version Upgraded
The Go version has been updated to1.23.0, which is in line with the other dependency upgrades. Double-check this version against project requirements and CI configurations.
5-46: Updated Direct Dependencies
The primaryrequireblock reflects significant updates to variouscosmossdk.iomodules along with other dependencies such asgithub.com/cosmos/cosmos-sdk v0.50.11and updates to packages likegithub.com/gorilla/muxandgithub.com/prometheus/client_golang. These changes appear consistent with the migration and modernization effort. Please verify compatibility with your application code and ensure all new version constraints are met.
48-210: Comprehensive Indirect Dependency Refresh
The indirect dependencies have been extensively updated. This helps improve security, performance, and compatibility across the board. It is advisable to run a full build and test suite to catch any subtle version conflicts. Also, confirm that the new dependencies do not introduce unexpected behavior in the application.
212-219: Replacement and Exclusion Directives Reviewed
Thereplaceblock reverts the mapping forgithub.com/confio/ics23/goto point togithub.com/cosmos/cosmos-sdk/ics23/go v0.8.0, and the exclusion ofgithub.com/gogo/protobuf v1.3.3is now in place. Ensure these directives precisely match upstream expectations and resolve any conflicts with your build process.tests/ict/go.mod (8)
3-3: Go Version Update:
The module’s Go version has been updated from a previous version to 1.23.0. Please verify that all build tools and CI pipelines are compatible with this change.
6-7: ChainSafe Replace Directive Update:
Thereplaceblock now directs both
•github.com/ChainSafe/go-schnorrkel→ versionv0.0.0-20200405005733-88cbf1b4c40dand
•github.com/ChainSafe/go-schnorrkel/1→ versionv1.0.0.
Please ensure that these new versions are compatible with the rest of the codebase and that any API changes are handled accordingly.
12-18: Primary Dependency Version Updates:
In the firstrequireblock, key dependencies have been updated:
•github.com/CosmWasm/wasmvm/v2is now atv2.1.5.
•github.com/cosmos/cosmos-sdkis updated tov0.50.11.
•github.com/cosmos/ibc-go/v8is now atv8.3.1.
• Docker (github.com/docker/docker) and interchaintest (github.com/strangelove-ventures/interchaintest/v8) packages, as well asgo.uber.org/zaphave also been bumped.These updates should be carefully tested to verify backward compatibility with your application’s API assumptions.
23-30: Bulk Updates in Indirect Dependencies (Part 1):
The secondrequireblock introduces several updated indirect dependencies from providers such ascloud.google.comand new Cosmos modules (e.g.cosmossdk.io/api,collections,core, anddepinject).
Make sure that these indirect dependency changes do not introduce any unforeseen conflicts, and that integration tests cover the affected areas.
33-40: Cosmos Module Enhancements:
Additional modules (includingcosmossdk.io/store,x/circuit,x/evidence,x/feegrant,x/tx, andx/upgrade) have been updated. These changes likely reflect adaptations to the newer Cosmos SDK version. Please verify that the usage of these modules in the rest of the codebase aligns with any API or behavioral changes in this update.
42-46: Additional Dependency Updates:
Updates include:
•github.com/BurntSushi/tomlupdated tov1.4.0.
• The ChainSafe replacement for/1and an update togithub.com/ComposableFi/go-subkey/v2are included.Review these changes to ensure that any changes in function signatures or behavior in these libraries are properly handled.
47-266: Comprehensive Indirect Dependency Refresh:
There is a major update in a long stretch of indirect dependencies (lines 47–266), touching numerous libraries (e.g. gRPC, opentelemetry, various GitHub packages, Google libraries, and more).
Because this is a sweeping update, please run a full integration test suite and double-check that no transitive dependency mismatch or API breakage has occurred.
268-269: Explicit Exclude Directive:
The file explicitly excludesgithub.com/gogo/protobuf v1.3.3. Ensure that no part of the codebase or its transitive dependencies require this version unexpectedly.x/smart-account/README.md (2)
1-9: New Module Documentation Added:
This new README provides a comprehensive introduction and architectural overview for thex/smart-accountmodule. The general explanation and introductory sections are clear and informative.
541-550: Overall Documentation Quality:
This README is very comprehensive and well-organized. Aside from the minor phrasing, punctuation, and formatting improvements noted above, the documentation effectively explains the architecture, workflow, and interfaces of the smart-account module. Great work!
| IssueFee: sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(1_000_000_000)), | ||
| MintFee: sdk.NewCoin(sdk.DefaultBondDenom, math.ZeroInt()), | ||
| BurnFee: sdk.NewCoin(sdk.DefaultBondDenom, math.ZeroInt()), |
There was a problem hiding this comment.
💡 Codebase verification
Fee Amounts Inconsistency Detected Across Environments
- x/fantoken/types/params.go and cmd/bitsongd/cmd/genesis.go: The default IssueFee is set to 1,000,000,000.
- x/fantoken/module.go: The module’s default genesis parameters set the IssueFee to 10,000,000, which is significantly lower.
- The MintFee and BurnFee defaulting to zero remain consistent.
It appears that the IssueFee value differs between the module defaults and the other environments. Please verify if this discrepancy is intentional (for example, to accommodate testnet versus mainnet settings) or if the module defaults should be updated to match the 1,000,000,000 value.
🔗 Analysis chain
Verify the fee amounts across environments.
The default fee amounts should be verified to ensure they are appropriate across different environments (testnet, mainnet).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for fee configurations across different environments
rg -A 5 'IssueFee.*=.*NewInt|MintFee.*=.*ZeroInt|BurnFee.*=.*ZeroInt'Length of output: 1878
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
app/keepers/keepers.go (2)
89-101: Document WasmVM capability requirements.The wasmCapabilities list has been expanded to include newer CosmWasm versions, but lacks documentation about compatibility requirements.
Consider adding a comment explaining:
- Minimum required CosmWasm version
- Impact of enabling/disabling specific capabilities
- Security implications of the "bitsong" custom capability
238-247: Document authenticator initialization order.The authenticator initialization sequence is critical but lacks documentation about the required order and dependencies between authenticators.
Add comments explaining:
- Why this specific order of authenticator registration is required
- Dependencies between authenticators (e.g., why AllOf/AnyOf need to be registered after basic authenticators)
- Impact of changing the order
app/upgrades/v021/upgrade_test.go (3)
22-22: Consider increasing the dummy upgrade height.The current upgrade height of 5 blocks is quite low and might not provide enough time for the chain to stabilize before the upgrade. Consider increasing this to a more realistic value (e.g., 100) to better simulate real-world conditions.
-const dummyUpgradeHeight = 5 +const dummyUpgradeHeight = 100
82-82: Fix typo in comment.The comment contains a typo: "cheeck" should be "check".
- // cheeck icq params + // check icq params
38-161: Enhance test coverage with additional assertions.The test cases could be strengthened by:
- Verifying that slashed validators have the correct token amount after slashing
- Adding assertions for validator status (jailed/unjailed) in post-upgrade checks
- Validating that the upgrade plan is properly cleaned up after execution
app/upgrades/v021/upgrades.go (3)
30-34: Enhance error logging for migrations.Consider adding more detailed logging for migration failures to aid in debugging.
logger.Info(fmt.Sprintf("pre migrate version map: %v", vm)) versionMap, err := mm.RunMigrations(ctx, configurator, vm) if err != nil { + logger.Error("failed to run migrations", "error", err) return nil, err }
36-36: Clarify the upstream reversion comment.The comment about reverting to upstream could be more explicit about why this is necessary.
-// reapplies v018 patch after removing delegations with 0 power, letting us revert back upstream to cosmos-sdk library +// Remove delegations with 0 power and reapply v018 patch to align with upstream cosmos-sdk library's delegation handling
111-111: Define gas limit as a constant.The gas limit value should be defined as a constant for better maintainability.
+const defaultContractGasLimit = 1000000 // 1MB // set x/cadance params cadanceParams := cadancetypes.DefaultParams() -cadanceParams.ContractGasLimit = 1000000 // 1mb +cadanceParams.ContractGasLimit = defaultContractGasLimit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/app.go(21 hunks)app/keepers/keepers.go(6 hunks)app/upgrades/v021/constants.go(1 hunks)app/upgrades/v021/upgrade_test.go(1 hunks)app/upgrades/v021/upgrades.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v021/constants.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-docker
🔇 Additional comments (6)
app/app.go (3)
203-209: Verify the increased Wasm size limits.The function increases Wasm blob size limit to 7MB without clear justification. This could potentially impact node performance and security.
Please confirm:
- Why was the size limit increased to 7MB specifically?
- Have performance implications been considered?
- Are there any security considerations with larger Wasm files?
334-345: LGTM! Clean IBC middleware setup.The transfer stack setup is well-structured and follows IBC-go v8 best practices.
193-201: 🛠️ Refactor suggestionConsider handling home directory error gracefully.
The
init()function panics if it fails to get the user's home directory. This could make the application fail abruptly during startup.Consider this safer approach:
func init() { userHomeDir, err := os.UserHomeDir() if err != nil { - panic(err) + fmt.Fprintf(os.Stderr, "failed to get user home directory: %v\n", err) + userHomeDir = os.TempDir() // Fallback to system temp directory } DefaultNodeHome = filepath.Join(userHomeDir, ".bitsongd") }Likely invalid or redundant comment.
app/keepers/keepers.go (1)
469-471: LGTM! Proper CosmWasm authenticator registration.The CosmWasm authenticator is correctly registered after the ContractKeeper is initialized.
app/upgrades/v021/upgrade_test.go (1)
168-169: Handle potential error from GetUpgradePlan.The error from GetUpgradePlan is checked but not handled. Consider adding assertions to verify the plan's content.
- _, err = s.App.AppKeepers.UpgradeKeeper.GetUpgradePlan(s.Ctx) - s.Require().NoError(err) + plan, err = s.App.AppKeepers.UpgradeKeeper.GetUpgradePlan(s.Ctx) + s.Require().NoError(err) + s.Require().Equal("v021", plan.Name) + s.Require().Equal(dummyUpgradeHeight, plan.Height)✅ Verification successful
Error: No shell scripts or web queries found
Enhance test validation of the upgrade plan
The current test only checks that there is no error when retrieving the upgrade plan. To ensure the upgrade was properly scheduled, update the test to retrieve the plan and confirm key fields (like the upgrade's name and height) match expectations.
- In
app/upgrades/v021/upgrade_test.go, modify the call to capture the plan returned byGetUpgradePlan.- Add assertions using
s.Require().Equal()to verify the plan's name (e.g.,"v021") and the expected upgrade height.app/upgrades/v021/upgrades.go (1)
90-94: Address TODO comment about block time.The TODO comment about block time tuning should be addressed before merging.
Please clarify if the default block time values are appropriate for BitSong's requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/upgrades/v021/upgrades.go(1 hunks)
🔇 Additional comments (4)
app/upgrades/v021/upgrades.go (4)
1-22: LGTM! Modern import paths are used correctly.The imports are well-organized and use the latest paths (e.g.,
cosmossdk.io).
49-51: Improve error handling in delegation cleanup.The error handling combines existence check and error check in a way that could mask the actual error.
- if exists, err := k.DistrKeeper.HasDelegatorStartingInfo(ctx, valAddr, sdk.AccAddress(del.DelegatorAddress)); err != nil || !exists { + exists, err := k.DistrKeeper.HasDelegatorStartingInfo(ctx, valAddr, sdk.AccAddress(del.DelegatorAddress)) + if err != nil { + logger.Error("failed to check delegator starting info", "error", err) return nil, err - } + } + if !exists { + logger.Info("no starting info found for delegator", "delegator", del.DelegatorAddress) + continue + }
90-93: Address the TODO comment about block time.The TODO comment indicates that the values are specific to 1.5s block time and may need tuning. This should be addressed before the upgrade.
Please confirm the desired block time for BitSong and update these values accordingly.
146-147: Handle the error from icqKeeper.SetParamsThe use of
//nolint:errcheckhere is masking a potential error.-//nolint:errcheck -icqKeeper.SetParams(ctx, icqparams) +if err := icqKeeper.SetParams(ctx, icqparams); err != nil { + return err +}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/bitsongd/cmd/root.go (1)
132-141:⚠️ Potential issueFix: Temporary directory is removed before usage.
The
defer os.RemoveAll(dir)statement executes whentempDir()returns, causing the directory to be deleted before it can be used by the application.Apply this diff to fix the issue:
func tempDir() string { dir, err := os.MkdirTemp("", "bitsongd") if err != nil { panic(fmt.Sprintf("failed creating temp directory: %s", err.Error())) } - defer os.RemoveAll(dir) return dir }
🧹 Nitpick comments (1)
cmd/bitsongd/cmd/root.go (1)
64-74: Add cleanup for temporary app resources.The temporary app is created for autocli configuration but its resources are not cleaned up, which could lead to memory leaks in long-running processes.
Consider adding cleanup after autocli configuration:
appOptions := make(simtestutil.AppOptionsMap, 0) tempDir := tempDir() tempApp := bitsong.NewBitsongApp( log.NewNopLogger(), cosmosdb.NewMemDB(), nil, true, tempDir, appOptions, []wasmkeeper.Option{}, ) + defer func() { + if err := tempApp.Close(); err != nil { + panic(err) + } + os.RemoveAll(tempDir) + }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/ict/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
.github/workflows/interchaintest-e2e.yml(3 hunks)app/genesis.go(1 hunks)cmd/bitsongd/cmd/root.go(10 hunks)tests/ict/go.mod(1 hunks)tests/ict/setup.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/genesis.go
- .github/workflows/interchaintest-e2e.yml
- tests/ict/go.mod
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/bitsongd/cmd/root.go
21-21: "github.com/cosmos/cosmos-sdk/types/module" imported and not used
(typecheck)
🔇 Additional comments (4)
tests/ict/setup.go (3)
102-102: LGTM! Documentation update is clear and accurate.The function documentation has been updated to be more specific and descriptive.
8-11: Verify compatibility with updated dependencies.The upgrade to interchaintest v8 and ibc-go v8 aligns with the PR objectives. Let's verify compatibility with other dependencies.
Also applies to: 17-17
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check go.mod for potential version conflicts and compatibility issues # Check direct dependencies grep -A 5 "require (" go.mod # Check for any replace directives that might affect these packages grep -A 5 "replace (" go.modLength of output: 674
Dependency compatibility verified.
The go.mod output shows no conflicts with the updated interchaintest-related imports; the updated dependencies in tests/ict/setup.go appear compatible with the existing module configuration.
- Imports Updated: The changes to interchaintest v8 and related packages align with the repository’s dependency management.
- No Version Conflicts: The output from go.mod confirms that there are no conflicting versions or replace directives affecting these dependencies.
89-89: Verify impact of disabled WASM types registration.The WASM types registration has been commented out. Please verify if this is related to the wasmvm@v2 upgrade mentioned in the PR objectives and confirm that this won't break any functionality.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for WASM-related functionality that might be affected # Search for WASM type usage rg -l "wasmtypes\." --type go # Search for WASM message handling ast-grep --pattern 'MsgStore$_' --lang goLength of output: 499
Review Response: WASM Types Registration Verification
Our search confirms that while multiple parts of the code reference
wasmtypes., there isn’t a corresponding dependency on an explicit call towasmtypes.RegisterInterfaces. This suggests that the upgrade towasmvm@v2likely replaces or renders the explicit registration unnecessary. As long as the intended registration occurs via alternative mechanisms or is integrated within updated modules (as implied by the PR objectives), commenting out this registration does not appear to break functionality.cmd/bitsongd/cmd/root.go (1)
399-417: 🛠️ Refactor suggestionAdd error handling for type assertions.
The type assertions in
autoCliOpts()could panic if modules don't implement the expected interfaces.Consider adding error handling:
func autoCliOpts(initClientCtx client.Context, tempApp *bitsong.BitsongApp) autocli.AppOptions { modules := make(map[string]appmodule.AppModule, 0) for _, m := range tempApp.ModuleManager().Modules { - if moduleWithName, ok := m.(module.HasName); ok { + moduleWithName, ok := m.(module.HasName) + if !ok { + continue + } moduleName := moduleWithName.Name() - if appModule, ok := moduleWithName.(appmodule.AppModule); ok { - modules[moduleName] = appModule - } + appModule, ok := moduleWithName.(appmodule.AppModule) + if !ok { + continue + } + modules[moduleName] = appModule } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
go.mod (1)
218-218: Document the reason for excluding gogo/protobuf v1.3.3.Add a comment explaining why this specific version is excluded to help future maintainers.
+// exclude due to known vulnerabilities and compatibility issues with cosmos-sdk v0.50 exclude github.com/gogo/protobuf v1.3.3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(1 hunks)cmd/bitsongd/cmd/init_from_state.go(0 hunks)go.mod(1 hunks)scripts/makefiles/build.mk(2 hunks)
💤 Files with no reviewable changes (1)
- cmd/bitsongd/cmd/init_from_state.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🔇 Additional comments (2)
go.mod (2)
3-3: Verify Go 1.23.0 compatibility with your CI/CD pipeline.Go 1.23.0 is a very recent version. Ensure your CI/CD pipeline and deployment environments support this version.
17-30: Verify compatibility between major version upgrades.The PR includes significant version upgrades:
- wasmd v0.53.2
- wasmvm/v2 v2.1.5
- cosmos-sdk v0.50.11
- ibc-go/v8 v8.5.2
Please ensure these versions are compatible with each other and test thoroughly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/upgrades/v020/upgrades.go (3)
46-47:⚠️ Potential issueCritical: Uninitialized error variable returned.
The code returns an uninitialized error variable which will be nil, masking the actual issue when delegator starting info is missing.
Apply this fix:
-if !hasInfo { - return nil, err +if !hasInfo { + return nil, fmt.Errorf("delegator starting info not found for validator %s and delegator %s", valAddr, del.GetDelegatorAddr()) }
58-60:⚠️ Potential issueCritical: Unhandled error from V018ManualDelegationRewardsPatch.
The error returned by V018ManualDelegationRewardsPatch is ignored, which could mask critical issues during the upgrade process.
Apply this fix:
if patched { - V018ManualDelegationRewardsPatch(sdkCtx, rewardsRaw, outstanding, k, val, del, endingPeriod) + if err := V018ManualDelegationRewardsPatch(sdkCtx, rewardsRaw, outstanding, k, val, del, endingPeriod); err != nil { + return nil, fmt.Errorf("failed to patch delegation rewards: %w", err) + } }
120-121:⚠️ Potential issueCritical: Error variable shadowing in reward distribution.
The error from GetDelegatorWithdrawAddr is shadowed by the subsequent error assignment, potentially masking issues with fetching the withdraw address.
Apply this fix:
-withdrawAddr, err := k.DistrKeeper.GetDelegatorWithdrawAddr(sdkCtx, sdk.AccAddress(del.GetDelegatorAddr())) -err = k.BankKeeper.SendCoinsFromModuleToAccount(sdkCtx, distrtypes.ModuleName, withdrawAddr, finalRewards) +withdrawAddr, err := k.DistrKeeper.GetDelegatorWithdrawAddr(sdkCtx, sdk.AccAddress(del.GetDelegatorAddr())) +if err != nil { + return fmt.Errorf("failed to get delegator withdraw address: %w", err) +} +if err := k.BankKeeper.SendCoinsFromModuleToAccount(sdkCtx, distrtypes.ModuleName, withdrawAddr, finalRewards); err != nil { + return fmt.Errorf("failed to send coins to delegator: %w", err) +}
🧹 Nitpick comments (2)
app/upgrades/v020/upgrades.go (2)
212-212: Define margin of error as a constant.Magic numbers should be defined as named constants for better maintainability and clarity.
Add this at the package level:
+const ( + // StakeMarginOfErrorPercent represents the 5% margin of error allowed for stake calculations + StakeMarginOfErrorPercent = 50 +)Then update the line:
-marginOfErr := currentStake.Mul(math.LegacyNewDecWithPrec(50, 3)) // 5.0% +marginOfErr := currentStake.Mul(math.LegacyNewDecWithPrec(StakeMarginOfErrorPercent, 3))
276-276: Fix typo in function name.The function name has a double 'm' in "Customm".
Apply this fix:
-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 {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/upgrades/v020/upgrades.go(11 hunks)app/upgrades/v021/upgrades.go(1 hunks)
🔇 Additional comments (5)
app/upgrades/v021/upgrades.go (4)
49-51: Improve error handling in delegation cleanup.The error handling combines existence check and error check in a way that could mask the actual error.
- if exists, err := k.DistrKeeper.HasDelegatorStartingInfo(sdkCtx, valAddr, sdk.AccAddress(del.DelegatorAddress)); err != nil || !exists { + exists, err := k.DistrKeeper.HasDelegatorStartingInfo(sdkCtx, valAddr, sdk.AccAddress(del.DelegatorAddress)) + if err != nil { + logger.Error("failed to check delegator starting info", "error", err) return nil, err - } + } + if !exists { + logger.Info("no starting info found for delegator", "delegator", del.DelegatorAddress) + continue + }
90-93: Address TODO comment about block time values.The TODO comment indicates that the block time values need to be tuned specifically for Bitsong. Please determine and set the appropriate values based on Bitsong's desired block time.
Would you like me to help determine appropriate values based on Bitsong's requirements?
119-119: Add error handling for keeper operations.Several keeper operations are missing error handling:
SmartAccountKeeper.SetParamsIBCKeeper.ClientKeeper.SetParamsGovKeeper.Params.SetAdd error handling similar to other keeper operations:
-k.SmartAccountKeeper.SetParams(sdkCtx, authenticatorParams) +if err := k.SmartAccountKeeper.SetParams(sdkCtx, authenticatorParams); err != nil { + return nil, err +} -k.IBCKeeper.ClientKeeper.SetParams(sdkCtx, params) +if err := k.IBCKeeper.ClientKeeper.SetParams(sdkCtx, params); err != nil { + return nil, err +} -k.GovKeeper.Params.Set(sdkCtx, govparams) +if err := k.GovKeeper.Params.Set(sdkCtx, govparams); err != nil { + return nil, err +}Also applies to: 126-126, 134-134
146-147: Handle the error from icqKeeper.SetParamsThe use of
//nolint:errcheckhere is masking a potential error fromicqKeeper.SetParams(ctx, icqparams).-//nolint:errcheck -icqKeeper.SetParams(ctx, icqparams) +if err := icqKeeper.SetParams(ctx, icqparams); err != nil { + return err +}app/upgrades/v020/upgrades.go (1)
4-10: LGTM! Good modernization of imports.The switch to
cosmossdk.ioimports and addition of the BaseAppParamManager parameter follows best practices for Cosmos SDK v0.50+.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
app/upgrades/v021/upgrades.go (3)
49-51:⚠️ Potential issueImprove error handling in delegation cleanup.
The error handling combines existence check and error check in a way that could mask the actual error.
Apply this diff to fix the error handling:
- if exists, err := k.DistrKeeper.HasDelegatorStartingInfo(sdkCtx, valAddr, sdk.AccAddress(del.DelegatorAddress)); err != nil || !exists { + exists, err := k.DistrKeeper.HasDelegatorStartingInfo(sdkCtx, valAddr, sdk.AccAddress(del.DelegatorAddress)) + if err != nil { + logger.Error("failed to check delegator starting info", "error", err) return nil, err - } + } + if !exists { + logger.Info("no starting info found for delegator", "delegator", del.DelegatorAddress) + continue + }
119-119:⚠️ Potential issueAdd error handling for keeper operations.
Several keeper operations are missing error handling.
Apply this diff to add error handling:
-k.SmartAccountKeeper.SetParams(sdkCtx, authenticatorParams) +if err := k.SmartAccountKeeper.SetParams(sdkCtx, authenticatorParams); err != nil { + return nil, err +} -k.IBCKeeper.ClientKeeper.SetParams(sdkCtx, params) +if err := k.IBCKeeper.ClientKeeper.SetParams(sdkCtx, params); err != nil { + return nil, err +} -k.GovKeeper.Params.Set(sdkCtx, govparams) +if err := k.GovKeeper.Params.Set(sdkCtx, govparams); err != nil { + return nil, err +}Also applies to: 126-126, 134-134
146-147:⚠️ Potential issueHandle the error from icqKeeper.SetParams.
The use of
//nolint:errcheckis masking a potential error.Apply this diff to handle the error:
-//nolint:errcheck -icqKeeper.SetParams(ctx, icqparams) +if err := icqKeeper.SetParams(ctx, icqparams); err != nil { + return err +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/upgrades/v021/upgrade_test.go(1 hunks)app/upgrades/v021/upgrades.go(1 hunks)cmd/bitsongd/cmd/init.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades/v021/upgrade_test.go
🔇 Additional comments (6)
app/upgrades/v021/upgrades.go (3)
1-22: LGTM!The imports are well-organized and all are actively used in the code.
29-34: LGTM!The migration execution is properly implemented with appropriate error handling.
90-94: Verify block time values.The TODO comment indicates that the evidence parameters need to be tuned based on bitsong's desired block time. These values are currently commented out.
Please:
- Confirm the desired block time for bitsong
- Calculate and set appropriate values for:
MaxAgeNumBlocksMaxAgeDurationcmd/bitsongd/cmd/init.go (3)
28-28: LGTM: Import addition is appropriate.The new import
genutiltypesis correctly added and used for enhanced genesis file handling.
81-92: LGTM: P2P configuration improvements.The changes address previous concerns about empty seed lists and optimize peer limits:
- Seed nodes from reputable providers are now included
- Peer limits are adjusted for better network stability (80 inbound, 60 outbound)
152-154: Verify the implications of setting validators to nil.Setting
genDoc.Consensus.Validatorsto nil might affect the network's ability to reach consensus. Please verify that this is the intended behavior and doesn't impact network initialization.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for validator initialization patterns in the codebase rg -A 5 'Consensus\.Validators.*=.*nil'Length of output: 397
Consensus Initialization Behavior: Setting Validators to nil Is Intentional
The search confirms that the only instance of setting
genDoc.Consensus.Validatorsto nil is in the genesis file initialization (seecmd/bitsongd/cmd/init.go). This appears to be an intended design choice. Clearing the validators here is typical in such blockchain setups where the initial validator set is later determined by the staking module or through gentx transactions. There is no additional code that suggests an unintended impact on network consensus.
- Location Verified:
cmd/bitsongd/cmd/init.go(lines 152–154)- Implication: Clearing the validators in the genesis doc is intended, allowing the validator set to be determined downstream during network startup without conflict.
…itsongofficial/go-bitsong into hard-nett/v0.21-smart-accounts
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/upgrades/v020/upgrades.go (1)
217-225: 🛠️ Refactor suggestionImprove panic message and error handling.
The panic message could be more descriptive, and the error could be returned instead of panicking.
Apply this fix:
if stake.GT(currentStake) { marginOfErr := currentStake.Mul(math.LegacyNewDecWithPrec(50, 3)) // 5.0% if stake.LTE(currentStake.Add(marginOfErr)) { stake = currentStake patched = true } else { - panic(fmt.Sprintln("current stake is not delgator from slashed validator, and is more than maximum margin of error")) + return sdk.DecCoins{}, fmt.Errorf( + "stake (%s) exceeds current stake (%s) by more than margin of error (%s) for delegator %s and validator %s", + stake, currentStake, marginOfErr, del.GetDelegatorAddr(), del.GetValidatorAddr(), + ) } }
♻️ Duplicate comments (6)
app/upgrades/v020/upgrades.go (3)
46-48:⚠️ Potential issueFix uninitialized error return.
The error variable is uninitialized when
hasInfois false, which could lead to unclear error messages.Apply this fix:
-if !hasInfo { - return nil, err +if !hasInfo { + return nil, fmt.Errorf("delegator starting info not found for validator %s and delegator %s", valAddr, del.GetDelegatorAddr()) }
126-128:⚠️ Potential issueFix error variable shadowing.
The error from
GetDelegatorWithdrawAddris being shadowed by the subsequent assignment toerr.Apply this fix:
-withdrawAddr, err := k.DistrKeeper.GetDelegatorWithdrawAddr(sdkCtx, sdk.AccAddress(del.GetDelegatorAddr())) -err = k.BankKeeper.SendCoinsFromModuleToAccount(sdkCtx, distrtypes.ModuleName, withdrawAddr, finalRewards) +withdrawAddr, err := k.DistrKeeper.GetDelegatorWithdrawAddr(sdkCtx, sdk.AccAddress(del.GetDelegatorAddr())) +if err != nil { + return err +} +if err := k.BankKeeper.SendCoinsFromModuleToAccount(sdkCtx, distrtypes.ModuleName, withdrawAddr, finalRewards); err != nil { + return err +}
213-216:⚠️ Potential issueRemove debug print statements.
Debug print statements should not be present in production code.
Remove these lines:
-fmt.Printf("del: %q", del.GetDelegatorAddr()) -fmt.Printf("val: %q", del.GetValidatorAddr()) -fmt.Printf("stake: %q", stake) -fmt.Printf("currentStake: %q", currentStake)app/upgrades/v021/upgrades.go (3)
61-64:⚠️ Potential issueFix incorrect error handling order.
The error is checked after using the result, which could lead to a panic if there's an error.
Apply this diff to fix the error handling order:
- hasInfo, err := k.DistrKeeper.HasDelegatorStartingInfo(sdkCtx, sdk.ValAddress(valAddr), sdk.AccAddress(del.GetDelegatorAddr())) - if !hasInfo { - return nil, err - } + hasInfo, err := k.DistrKeeper.HasDelegatorStartingInfo(sdkCtx, sdk.ValAddress(valAddr), sdk.AccAddress(del.GetDelegatorAddr())) + if err != nil { + return nil, err + } + if !hasInfo { + logger.Info("no starting info found for delegator", "delegator", del.GetDelegatorAddr()) + continue + }
123-123:⚠️ Potential issueAdd error handling for keeper operations.
Several keeper operations are missing error handling.
Apply this diff to add proper error handling:
-k.SmartAccountKeeper.SetParams(sdkCtx, authenticatorParams) +if err := k.SmartAccountKeeper.SetParams(sdkCtx, authenticatorParams); err != nil { + return nil, err +} -k.IBCKeeper.ClientKeeper.SetParams(sdkCtx, params) +if err := k.IBCKeeper.ClientKeeper.SetParams(sdkCtx, params); err != nil { + return nil, err +} -k.GovKeeper.Params.Set(sdkCtx, govparams) +if err := k.GovKeeper.Params.Set(sdkCtx, govparams); err != nil { + return nil, err +}Also applies to: 130-130, 138-138
150-151:⚠️ Potential issueHandle the error from icqKeeper.SetParams.
The use of
//nolint:errcheckis masking a potential error.Apply this diff to handle the error properly:
-//nolint:errcheck -icqKeeper.SetParams(ctx, icqparams) +if err := icqKeeper.SetParams(ctx, icqparams); err != nil { + return err +}
🧹 Nitpick comments (10)
x/cadance/types/msgs.go (1)
46-49: Improve error handling in GetSigners().The function ignores potential errors from
AccAddressFromBech32. SinceValidateBasic()already validates the address, this is not a critical issue, but it would be good practice to handle or document why we're ignoring the error.func (msg MsgRegisterCadanceContract) GetSigners() []sdk.AccAddress { - from, _ := sdk.AccAddressFromBech32(msg.SenderAddress) + // We can safely ignore the error here as ValidateBasic() ensures the address is valid + from, err := sdk.AccAddressFromBech32(msg.SenderAddress) + if err != nil { + // This should never happen as ValidateBasic() ensures the address is valid + panic(err) + } return []sdk.AccAddress{from} }x/cadance/keeper/keeper_test.go (5)
34-43: Remove commented code.The commented code at lines 39-40 should be removed if it's no longer needed. If it's meant for future use, consider adding a TODO comment explaining why it's kept.
- // encCfg := app.MakeEncodingConfig() - // types.RegisterInterfaces(encCfg.InterfaceRegistry)
45-51: Add error documentation for FundAccount method.Consider adding error documentation to clarify possible error scenarios.
+// FundAccount mints the specified amount of coins and sends them to the given address. +// Returns an error if minting or sending coins fails. func (s *IntegrationTestSuite) FundAccount(ctx sdk.Context, addr sdk.AccAddress, amounts sdk.Coins) error {
57-81: Consider breaking down StoreCode into smaller functions.The StoreCode method is quite complex and handles multiple responsibilities. Consider extracting the parameter setup and code storage into separate helper methods for better maintainability.
+// setupWasmParams configures WASM parameters for testing +func (s *IntegrationTestSuite) setupWasmParams() error { + params := s.App.AppKeepers.WasmKeeper.GetParams(s.Ctx) + params.InstantiateDefaultPermission = wasmtypes.AccessTypeEverybody + return s.App.AppKeepers.WasmKeeper.SetParams(s.Ctx, params) +} + +// storeWasmCode stores the WASM bytecode and returns the code ID +func (s *IntegrationTestSuite) storeWasmCode(sender string, code []byte) (uint64, error) { + msg := wasmtypes.MsgStoreCodeFixture(func(m *wasmtypes.MsgStoreCode) { + m.WASMByteCode = code + m.Sender = sender + }) + rsp, err := s.App.MsgServiceRouter().Handler(msg)(s.Ctx, msg) + if err != nil { + return 0, err + } + var result wasmtypes.MsgStoreCodeResponse + if err := s.App.AppCodec().Unmarshal(rsp.Data, &result); err != nil { + return 0, err + } + return result.CodeID, nil +}
83-106: Add input validation to InstantiateContract.Consider validating the sender and admin addresses before proceeding with contract instantiation.
func (s *IntegrationTestSuite) InstantiateContract(sender string, admin string) string { + // Validate addresses + if _, err := sdk.AccAddressFromBech32(sender); err != nil { + panic(fmt.Sprintf("invalid sender address: %s", err)) + } + if admin != "" { + if _, err := sdk.AccAddressFromBech32(admin); err != nil { + panic(fmt.Sprintf("invalid admin address: %s", err)) + } + }
108-130: Add error documentation for helper methods.The helper methods for contract management should include documentation about possible error scenarios.
+// RegisterCadanceContract registers a contract with the Cadance module. +// Returns an error if the contract is already registered or if the addresses are invalid. func (s *IntegrationTestSuite) RegisterCadanceContract(senderAddress string, contractAddress string) { +// UnregisterCadanceContract removes a contract from the Cadance module. +// Returns an error if the contract is not registered or if the addresses are invalid. func (s *IntegrationTestSuite) UnregisterCadanceContract(senderAddress string, contractAddress string) { +// JailCadanceContract sets the jail status of a contract to true. +// Returns an error if the contract does not exist or is already jailed. func (s *IntegrationTestSuite) JailCadanceContract(contractAddress string) { +// UnjailCadanceContract sets the jail status of a contract to false. +// Returns an error if the contract does not exist, is not jailed, or if the sender is not authorized. func (s *IntegrationTestSuite) UnjailCadanceContract(senderAddress string, contractAddress string) {app/upgrades/v020/upgrades.go (1)
282-282: Fix typo in function name.The function name has a double "m" which appears to be a typo.
Apply this fix:
-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 {app/app.go (3)
84-84: Consider removing unused variable.The
EmptyWasmOptsvariable is declared but never used in the codebase.-var EmptyWasmOpts []wasmkeeper.Option
203-209: Document the rationale for increased wasm size limits.The function increases wasm size limits but lacks documentation explaining why 7mb was chosen and potential implications.
func overrideWasmVariables() { - // Override Wasm size limitation from WASMD. + // Override WASMD's default wasm size limits to accommodate larger contracts. + // The 7mb limit was chosen based on: + // - Typical contract sizes in the ecosystem + // - Memory constraints of validator nodes + // Note: Increasing this limit requires validators to have adequate memory. wasmtypes.MaxWasmSize = 7 * 1024 * 1024 // 7mb wasm blob wasmtypes.MaxProposalWasmSize = wasmtypes.MaxWasmSize }
252-256: Make IBC wasm capabilities configurable.The supported capabilities are hardcoded. Consider making them configurable through app options.
+ // Default capabilities if not specified in app options + defaultCapabilities := []string{"iterator", "stargate", "abort"} + capabilities := cast.ToStringSlice(appOpts.Get("ibc-wasm.capabilities")) + if len(capabilities) == 0 { + capabilities = defaultCapabilities + } ibcWasmConfig := wasmlctypes.WasmConfig{ DataDir: filepath.Join(homePath, "ibc_08-wasm"), - SupportedCapabilities: []string{"iterator", "stargate", "abort"}, + SupportedCapabilities: capabilities, ContractDebugMode: false, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/interchaintest-e2e.yml(3 hunks)app/app.go(21 hunks)app/upgrades/v020/upgrades.go(11 hunks)app/upgrades/v021/upgrades.go(1 hunks)proto/buf.gen.pulsar.yaml(1 hunks)tests/ict/setup.go(3 hunks)x/cadance/keeper/cadance.go(1 hunks)x/cadance/keeper/keeper_test.go(1 hunks)x/cadance/types/msgs.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- proto/buf.gen.pulsar.yaml
- .github/workflows/interchaintest-e2e.yml
- x/cadance/keeper/cadance.go
🔇 Additional comments (17)
x/cadance/types/msgs.go (6)
1-27: LGTM! Well-structured constants and interface declarations.The imports, constants, and interface implementations are well-organized and properly documented.
68-71: Same error handling improvement needed in GetSigners().This implementation has the same error handling pattern as MsgRegisterCadanceContract.GetSigners().
90-93: Same error handling improvement needed in GetSigners().This implementation has the same error handling pattern as MsgRegisterCadanceContract.GetSigners().
95-130: LGTM! Well-implemented parameter update message.The constructor and ValidateBasic implementation are well-structured with proper error handling.
118-121: Same error handling improvement needed in GetSigners().This implementation has the same error handling pattern as MsgRegisterCadanceContract.GetSigners().
132-141: LGTM! Well-implemented address validation helper.The helper function properly validates multiple addresses and provides good error context.
x/cadance/keeper/keeper_test.go (2)
1-20: LGTM! Well-organized imports and package structure.The imports are properly organized and all necessary dependencies are included.
22-32: LGTM! Well-structured test suite definition.The test suite is properly defined with all necessary fields and follows testing best practices.
app/upgrades/v020/upgrades.go (2)
4-4: LGTM! Import changes align with SDK updates.The imports have been updated to use the new
cosmossdk.iopackages, which is the correct approach for the SDK v0.50 upgrade.Also applies to: 8-8, 10-10
17-19: LGTM! Context handling follows best practices.The upgrade handler correctly:
- Accepts the new BaseAppParamManager parameter
- Uses context.Context in the signature
- Properly unwraps the SDK context
app/app.go (1)
194-201: Improve error handling in init.Panicking in init can abruptly terminate the application. Consider falling back to a default path.
func init() { userHomeDir, err := os.UserHomeDir() if err != nil { - panic(err) + fmt.Fprintf(os.Stderr, "failed to get user home directory: %v\n", err) + userHomeDir = "/tmp" } DefaultNodeHome = filepath.Join(userHomeDir, ".bitsongd") }tests/ict/setup.go (4)
8-11: LGTM! Consistent version upgrades.The import paths have been correctly updated to use v8 for both interchaintest and ibc-go packages, aligning with the PR's dependency upgrade objectives.
Also applies to: 17-17
22-22: Fix typo in variable name and verify value.The variable name contains a typo: "Expidited" should be "Expedited". Also, the value matches
VotingPeriod- please verify if this is intentional.- ExpeditedVoting = "15s" + ExpeditedVoting = "15s"
102-102: LGTM! Improved function documentation.The updated comment provides a clearer description of the function's purpose.
89-89: Verify if commenting out WASM types registration is intentional.Given that the PR introduces new WASM-related features (wasm ibc light client), please verify if commenting out the WASM types registration is intentional and won't affect the new functionality.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other WASM type registrations that might have replaced this rg -i "wasm.*register.*interface" --type goLength of output: 120
I'll further investigate by searching for any uncommented usage of
wasmtypes.RegisterInterfacesor relatedwasmtypesinvocations in the codebase. This will help us determine if the registration was moved or replaced elsewhere.
🏁 Script executed:
#!/bin/bash # Search for all occurrences of wasmtypes usage in Go files rg -i "wasmtypes" --type goLength of output: 7192
WASM types registration in tests/ict/setup.go is intentionally commented out.
- The search for active calls to
wasmtypes.RegisterInterfacesacross the repository returns only the commented-out call in tests/ict/setup.go.- Other parts of the code reference and utilize
wasmtypes(and related modules) without an explicit interface registration call.- This suggests the registration mechanism has been migrated or is handled elsewhere, aligning with the new WASM ibc light client functionality.
app/upgrades/v021/upgrades.go (2)
147-147: Review commented code for potential security implications.The commented code references a whitelist configuration that might be important for security.
Please verify if the commented whitelist configuration should be included:
// icqparams.AllowQueries = wasmbinding.GetStargateWhitelistedPaths()✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for whitelist configurations in the codebase rg -A 5 'GetStargateWhitelistedPaths|AllowQueries' --type goLength of output: 1020
Confirmed: Retaining the Manual Whitelist is the Correct Approach
- The test in
app/upgrades/v021/upgrade_test.goasserts that exactly one allowed query exists, matching the manually appended/cosmwasm.wasm.v1.Query/SmartContractState.- Including the commented-out assignment using
wasmbinding.GetStargateWhitelistedPaths()would likely add additional entries, potentially deviating from the intended security configuration and breaking the test expectation.Thus, the commented whitelist configuration should remain excluded.
94-97: Address TODO comment and commented code.The TODO comment and commented code need to be addressed before the upgrade is deployed.
Please verify:
- What is the desired block time for Bitsong?
- Should the commented evidence parameters be enabled?
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for block time configuration in the codebase rg -l 'block.*time|blocktime' --type goLength of output: 119
Action: Update Consensus Evidence Parameters
Based on our search of the codebase:
- The only references to block time (in both the upgrade and testing files) are consistent with the default 1.5‑second setting. There’s no indication elsewhere that Bitsong intends to use a different interval.
- The commented evidence parameters (with MaxAgeNumBlocks = 1,000,000 and MaxAgeDuration = 14 days) were specifically tuned for a 1.5‑second block time.
Recommendation:
Uncomment (and if necessary, update the accompanying comments) the evidence parameter assignments so that the consensus parameters support safe comet pruning for a 1.5‑second block time. If Bitsong ever changes its desired block interval from 1.5 seconds, these values should be recalculated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/bitsongd/cmd/init.go (2)
101-102: Consider removing or implementing the commented consensus timeout.The commented consensus timeout configuration suggests an incomplete implementation. Either remove the comment or implement the timeout setting if it's needed.
210-254: Enhance genesis file download security.While the implementation is solid, consider adding these security measures:
- Verify the downloaded genesis file's checksum
- Use HTTPS certificate verification
- Add rate limiting for retry attempts
Example implementation for checksum verification:
// Read the response body body, err := io.ReadAll(resp.Body) if err != nil { return errors.Wrap(err, "failed to read genesis file response body") } +// Verify checksum +expectedChecksum := "YOUR-EXPECTED-CHECKSUM" +actualChecksum := fmt.Sprintf("%x", sha256.Sum256(body)) +if actualChecksum != expectedChecksum { + return errors.New("genesis file checksum verification failed") +} // Write the body to the destination genesis fileapp/test_helper.go (2)
171-262: Review commented staking parameters configuration.The staking parameters configuration (lines 222-230) is commented out. This might limit the ability to customize validator settings in tests.
Please confirm if the default parameters are sufficient for all test cases:
Also, consider adding test coverage for different validator configurations:
func GenesisStateWithValSet(app *BitsongApp) GenesisState { + return GenesisStateWithValSetAndParams(app, stakingtypes.DefaultParams()) +} + +func GenesisStateWithValSetAndParams(app *BitsongApp, stakingParams stakingtypes.Params) GenesisState { // ... existing code ... - stakingGenesis := stakingtypes.NewGenesisState(stakingtypes.DefaultParams(), validators, delegations) + stakingGenesis := stakingtypes.NewGenesisState(stakingParams, validators, delegations)❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for tests that might need custom staking parameters rg "MinCommissionRate|MaxValidators|MaxEntries|HistoricalEntries" test_Length of output: 119
The search for test files explicitly referencing custom staking parameters (e.g., "MinCommissionRate", "MaxValidators", etc.) did not return any results. This suggests that none of the existing tests rely on a customized staking configuration—default parameters appear to be used throughout. However, because the search was limited to filenames matching "test_" and similar patterns and no _test.go files were queried, please manually verify whether any tests (or future scenarios) require alternative staking configurations.
- Confirm that no tests are designed to evaluate custom validator settings.
- If future tests demand parameter flexibility, consider refactoring (e.g., adding a GenesisStateWithValSetAndParams function).
74-108: Consider parameterizing the chain ID in SetupWithCustomHome.The chain ID is hardcoded to "bitsong-2b" in
SetupWithCustomHome. Consider making it a parameter for better flexibility in tests.-func SetupWithCustomHome(isCheckTx bool, dir string) *BitsongApp { - return SetupWithCustomHomeAndChainId(isCheckTx, dir, "bitsong-2b") +func SetupWithCustomHome(isCheckTx bool, dir string, chainId string) *BitsongApp { + return SetupWithCustomHomeAndChainId(isCheckTx, dir, chainId)Also, verify that the genesis state caching is thread-safe for parallel tests.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for parallel test execution rg -l "t.Parallel" | xargs rg "SetupWithCustomHome"Length of output: 49
Action: Revisit Chain ID Parameterization & Thread Safety in Genesis State Caching
- The helper function
SetupWithCustomHomeindeed hardcodes the chain ID ("bitsong-2b"). Parameterizing this value—as suggested—would improve the flexibility of tests that might require different chain configurations.- Regarding the genesis state caching via the package-level variable
defaultGenesisStatebytes: while our recent grep for parallel test invocations (via"t.Parallel") did not yield any references toSetupWithCustomHome, this absence suggests that such tests aren’t currently running concurrently. However, note that as written the caching logic is not thread-safe. If tests are later modified to run in parallel, a race condition could occur. It is advisable to either add synchronization (e.g., a mutex) or ensure that tests invoking this function do not run concurrently.Please verify manually whether any tests might eventually run in parallel contexts that use these helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/helpers/mock.go(3 hunks)app/test_helper.go(7 hunks)cmd/bitsongd/cmd/genesis.go(6 hunks)cmd/bitsongd/cmd/init.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e-tests (e2e-polytone)
- GitHub Check: e2e-tests (e2e-pfm)
- GitHub Check: e2e-tests (e2e-basic)
🔇 Additional comments (11)
app/helpers/mock.go (3)
6-6: LGTM! Import alias update is consistent with the PR objectives.The change from
tmtypestocmttypesaligns with the broader dependency updates mentioned in the PR objectives.
13-13: LGTM! Type assertion updated correctly.The variable type update from
tmtypes.PrivValidatortocmttypes.PrivValidatorcorrectly reflects the import alias change while maintaining the same interface implementation check.
32-32: LGTM! Method calls updated correctly.The updates to use
cmttypes.VoteSignBytesandcmttypes.ProposalSignBytescorrectly reflect the import alias change while maintaining the same functionality.Also applies to: 43-43
cmd/bitsongd/cmd/init.go (2)
84-92: LGTM! P2P configuration is well-structured.The P2P configuration has been improved with:
- A diverse set of reliable seed nodes
- Balanced peer limits for network stability
137-185: LGTM! Robust genesis file handling implementation.The implementation addresses previous concerns about mainnet genesis file handling with:
- Clear mainnet detection logic
- Graceful fallback to local testing on download failures
- Proper handling of large mainnet app states
app/test_helper.go (3)
9-15: LGTM! Import changes align with Cosmos SDK v0.50.The updates to use
cosmossdk.iopackages and the switch tobaseappfrombamare consistent with the Cosmos SDK v0.50 migration.
70-72: LGTM! Simplified setup function.The change to use
isCheckTxinstead oftesting.Tis more appropriate as the parameter directly relates to the app's initialization mode.
114-160: Verify the impact of commented BeginBlock/EndBlock calls.The BeginBlock and EndBlock calls are commented out. This might affect block processing simulation in tests.
Could you clarify if this is intentional and if there are any implications for test coverage?
Also, verify if any tests depend on these block boundary events:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for tests that might depend on block boundary events rg -A 5 "BeginBlock|EndBlock" test_Length of output: 83
Let's run a revised shell script command to search for any occurrences of
BeginBlockorEndBlockin files with the Go test suffix (_test.go) to better cover the test code in the repository:
🏁 Script executed:
#!/bin/bash # Search for tests using BeginBlock or EndBlock in Go test files rg -A 5 "BeginBlock|EndBlock" --glob "*_test.go"Length of output: 6702
Clarification on Block Boundary Simulation in Tests
The commented-out BeginBlock/EndBlock calls in the SignCheckDeliver helper appear to be intentional. Our search of Go test files revealed that block boundary events are simulated explicitly in other parts of the test suite—for instance, in the
EndBlockerTestSuitein x/cadance/abci_test.go and upgrade tests in app/upgrades/v020/upgrade_test.go. These tests invoke dedicated functions (e.g., viacallEndBlockeror explicitBeginBlockercalls) to simulate block transitions.
- Intentional Change: The removal of BeginBlock/EndBlock from SignCheckDeliver is intentional, as block simulation is handled elsewhere.
- Test Coverage: Tests that depend on block boundary events do not utilize SignCheckDeliver for that purpose, reducing any potential risk.
cmd/bitsongd/cmd/genesis.go (3)
8-11: Migration to new math and consensus types packages.The changes reflect the migration from legacy SDK types to the new
cosmossdk.io/mathpackage and fromtmtypestocmttypes, aligning with the Cosmos SDK v0.50 upgrade.
31-34: Type update for ConsensusParams.The
ConsensusParamsfield type has been updated from*tmtypes.ConsensusParamsto*cmttypes.ConsensusParams, maintaining compatibility with the latest Cosmos SDK.
144-150: Updated PrepareGenesis signature for better type safety.The function signature change from
*tmtypes.GenesisDocto*genutiltypes.AppGenesisimproves type safety and aligns with the Cosmos SDK v0.50 upgrade.
Summary by CodeRabbit
New Features
x/cadance: cron service style module for automated smart contract execution at the end of each block.x/smart-accounts: programmable account transaction authentication support module.wasm ibc light client: ibc module for creating custom light client verification workflows in CosmWasm contracts.x/icq: interchain queries allow deterministic ibc queries.vote extensions: enables support for validators to include offline data via vote extensions.cosmossdk.iopackages.Upgrades & Improvements
cadanceandsmart-accounts.cosmossdk.iopackages for improved consistency and functionality.Summary by CodeRabbit
New Features
Improvements
errorspackage and replaced integer handling with themathpackage for consistency.