Bump golangci-lint to v2.1.6. #2363
Conversation
WalkthroughBumps golangci-lint and its GitHub Action; rewrites .golangci.yml; adds many lint suppressions and path sanitizations; rewires app init to use App-level routers/services; introduces new antewrapper constructors/AnteHandle implementations; adds flatfees CLI and validations; multiple keeper/handler call-site refactors and mock/test helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as flatfees CLI
participant GRPC as App gRPC Router
participant Keeper as FlatFeesKeeper
CLI->>GRPC: CalculateTxFees(request)
GRPC->>Keeper: CalculateTxFees(ctx, txBytes, flags)
Keeper-->>GRPC: fee response
GRPC-->>CLI: prints response
sequenceDiagram
autonumber
participant App as App
participant Ante as ProvSetUpContextDecorator
participant Deduct as DeductUpFrontCostDecorator
participant Next as Next AnteHandler
participant Post as FlatFeePostHandler
App->>Ante: AnteHandle(ctx, tx, simulate)
Ante->>Deduct: (maybe) check/deduct up-front costs
Deduct->>Next: next(ctx', tx, simulate)
Next-->>Ante: continue
Ante->>Post: finalize fees / post-handling
Post-->>App: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/devtools/Makefile (1)
41-43: Consider updating developer docs and changelog
Since this version bump affects the linting toolchain, ensure any references in README, CONTRIBUTING.md, changelog entries, or other developer guides are updated accordingly to prevent stale documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/lint.yml(1 hunks)contrib/devtools/Makefile(1 hunks)
🔇 Additional comments (3)
contrib/devtools/Makefile (1)
41-43: Tool version bump looks good
TheGOLANGCI_LINT_VERSIONhas been correctly updated tov2.1.6, matching the workflow change in.github/workflows/lint.yml..github/workflows/lint.yml (2)
34-34: Upgrade GitHub Action to v8
Thegolangci/golangci-lint-actionhas been bumped fromv6tov8—this aligns with the PR objective to keep our CI workflows current.
38-38: Synchronize lint tool version
Theversioninput is now set tov2.1.6, in lockstep with the Makefile. Great to see consistency across tooling.
.changelog/unreleased/dependencies/2346-upgrade-golangci-lint.md
Outdated
Show resolved
Hide resolved
nagarajdivine
force-pushed
the
chore/updage-golang-lint
branch
from
cd82c51 to
c79a0be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
x/exchange/keeper/market.go (1)
1058-1062: Add deprecated shim, fix docstring, and verify permission
- Introduce a
CanTransferCommitmentsForMarketshim (marked Deprecated) that delegates to the newCanTransferCommitmentForMarketto avoid breaking downstream callers.- Update the doc comment on
CanTransferCommitmentForMarketto “transfer a commitment” instead of “transfer commitments” for consistency.- Confirm that
exchange.Permission_cancelis the correct permission for commitment transfers—or switch to a more specific permission constant if one exists.testutil/ibc/isc4_middleware_mock.go (1)
4-10: Fix non-versioned ibc-go capability imports to use v8 module path
- testutil/ibc/isc4_middleware_mock.go: change
github.com/cosmos/ibc-go/modules/capability/types
to
github.com/cosmos/ibc-go/v8/modules/capability/types- app/app.go (112–114), x/oracle/module/module_ibc.go (11), x/oracle/keeper/*.go, x/ibchooks/…, x/ibcratelimit/… and other files all import
github.com/cosmos/ibc-go/modules/capability/...; update each to the v8 path.- Verify no remaining non-versioned ibc-go imports before merging.
cmd/provenanced/config/client.go (1)
79-97: ValidateBasic: mismatch between allowed broadcast modes and error message.The error mentions 'block' but it’s not accepted. Either accept 'block' or remove it from the message. Recommending acceptance to match SDK expectations.
-// ValidateBasic does simple stateless validation of this Msg. +// ValidateBasic does simple stateless validation of this config. func (c ClientConfig) ValidateBasic() error { @@ - switch c.BroadcastMode { - case "sync", "async": - default: - return errors.New("unknown broadcast-mode (must be one of 'sync' 'async' 'block')") - } + switch c.BroadcastMode { + case "sync", "async", "block": + default: + return errors.New("unknown broadcast-mode (must be one of 'sync', 'async', 'block')") + }internal/ibc/ibc.go (1)
1-55: Split runtime changes into a dedicated PR
This PR should only bump golangci/golangci-lint-action to v8 and golangci-lint to v2.1.6; it now also adds new internal/ibc APIs and updates consumers in x/ibcratelimit and x/ibchooks. Isolate the IBC error-API additions and related logging changes into a separate PR to keep tooling bumps focused..golangci.yml (1)
75-194: Blocking: v2 moved linters-settings → linters.settings (and formatters.settings)With version: "2", this whole block must be migrated; keys like misspell.ignore-words and govet.shadow changed semantics/locations. Today much of this will be ignored/misapplied.
Minimal actions:
- Move this block under linters.settings; move gci settings under formatters.settings.
- Replace misspell.ignore-words → misspell.ignore-rules.
- Replace govet.shadow: true → govet.enable: [shadow].
Example (pattern):
linters: settings: depguard: rules: { ... } # move as-is errcheck: exclude-functions: - (fmt.State).Write govet: enable: - shadow settings: printf: funcs: - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf dogsled: max-blank-identifiers: 3 misspell: locale: US ignore-rules: - cancelled formatters: settings: gci: custom-order: true sections: - standard - default - prefix(github.com/provenance-io) - blank - dotTip: run the official migrator to do this safely:
- golangci-lint migrate -c .golangci.yml
Migration refs and key changes. (golangci-lint.run)app/test_helpers.go (1)
464-484: Revise TestAddr error handling for bech32 mismatches
- Replace the generic error on string mismatch with a detailed message:
return nil, fmt.Errorf("bech32 string mismatch: got %q, expected %q", bech, bechexpected)- On byte-array mismatch, don’t return the stale
err(which is nil); return
fmt.Errorf("bech32→bytes mismatch: bech32=%X hex=%X", bechres, res)Optional: callers like
createIncrementalAccountscurrently ignore TestAddr’s error—handle or panic to surface test failures.testutil/ibc/testchain.go (1)
84-92: Don’t hard-code bech32 prefix; derive from configHard-coding "cosmos" will break if the app uses a different prefix. Use the SDK’s address string instead.
-func (chain *TestChain) RegisterRateLimiterContract(suite *suite.Suite, addr []byte) { - addrStr, err := sdk.Bech32ifyAddressBytes("cosmos", addr) - suite.Require().NoError(err) +func (chain *TestChain) RegisterRateLimiterContract(suite *suite.Suite, addr []byte) { + addrStr := sdk.AccAddress(addr).String() provenanceApp := chain.GetProvenanceApp() provenanceApp.RateLimitingKeeper.SetParams(chain.GetContext(), ibcratelimit.Params{ ContractAddress: addrStr, }) }internal/provwasm/simulation.go (3)
166-203: Critical: only node is funded; later txs are signed by feebucket/consumer and will likely fail for feesStore/instantiate use
feebucketas sender; execute usesconsumer. Without “stake” on those accounts,RandomFeescan panic (“no fees”). Fund all signers up-front.Apply this diff to fund all required accounts in one place:
@@ - // fund the node account to do all of these txs - fundErr := testutil.FundAccount(ctx, bk, node.Address, sdk.NewCoins(sdk.Coin{ - Denom: "stake", - Amount: sdkmath.NewInt(1_000_000_000_000_000), - })) - - if fundErr != nil { - return simtypes.NoOpMsg("provwasm", "", "unable to fund account"), nil, nil - } + // fund all signers (node, feebucket, consumer) with stake for fees + for _, addr := range []sdk.AccAddress{node.Address, feebucket.Address, consumer.Address} { + if err := testutil.FundAccount(ctx, bk, addr, sdk.NewCoins(sdk.NewCoin( + "stake", sdkmath.NewInt(1_000_000_000_000_000), + ))); err != nil { + return simtypes.NoOpMsg("provwasm", "", "unable to fund account"), nil, nil + } + }
264-286: Essential: instantiate uses hard-coded CodeID=1; extract CodeID from store response and pass it forwardRelying on
CodeID: 1will break if any prior store occurred. Parse the store tx response and captureCodeID, then pass it into instantiate.Apply this diff to plumb CodeID:
@@ - msg2, ops, _, storeErr := Dispatch(r, app, ctx, simState, ak, bk, feebucket, chainID, msg, nil, nil) - - ops = append(ops, simtypes.FutureOperation{Op: SimulateMsgInstantiateContract(simState, ak, bk, nk, node, feebucket, merchant, consumer, name), BlockHeight: int(ctx.BlockHeight()) + 1}) + msg2, ops, sdkResp, storeErr := Dispatch(r, app, ctx, simState, ak, bk, feebucket, chainID, msg, nil, nil) + + var codeID uint64 + for _, resp := range sdkResp.MsgResponses { + if resp.TypeUrl == "/cosmwasm.wasm.v1.MsgStoreCodeResponse" { + if sc, ok := resp.GetCachedValue().(*types.MsgStoreCodeResponse); ok { + codeID = sc.CodeID + break + } + } + } + if codeID == 0 { + app.Logger().Error("store code", "MsgResponses", sdkResp.MsgResponses) + panic("no store code response found") + } + + ops = append(ops, simtypes.FutureOperation{ + Op: SimulateMsgInstantiateContract(simState, ak, bk, nk, node, feebucket, merchant, consumer, name, codeID), + BlockHeight: int(ctx.BlockHeight()) + 1, + })Additionally, consider embedding the wasm (
embed.FS) or resolving its path robustly; the relative./sim_contracts/tutorial.wasmis brittle in CI.
287-352: Plumb CodeID into instantiate; remove constant 1Use the CodeID from store.
Apply this diff:
-// SimulateMsgInstantiateContract simulates contract instantiation. -func SimulateMsgInstantiateContract(simState module.SimulationState, ak authkeeper.AccountKeeperI, bk bankkeeper.ViewKeeper, nk namekeeper.Keeper, node, feebucket, merchant, consumer simtypes.Account, name string) simtypes.Operation { +// SimulateMsgInstantiateContract simulates contract instantiation. +func SimulateMsgInstantiateContract(simState module.SimulationState, ak authkeeper.AccountKeeperI, bk bankkeeper.ViewKeeper, nk namekeeper.Keeper, node, feebucket, merchant, consumer simtypes.Account, name string, codeID uint64) simtypes.Operation { @@ - msg := &types.MsgInstantiateContract{ + msg := &types.MsgInstantiateContract{ Sender: feebucket.Address.String(), Admin: feebucket.Address.String(), - CodeID: 1, + CodeID: codeID, Label: label, Msg: []byte(m), Funds: amount, }Also, good job validating the instantiate response and extracting the contract address.
x/attribute/types/msgs.go (1)
78-95: Strengthen value validation (nil vs empty).Constructor ensures non-nil, but direct callers could pass empty; deletion path requires non-empty and this should, too.
Apply:
- if msg.Value == nil { - return fmt.Errorf("invalid value: nil") - } + if len(msg.Value) == 0 { + return fmt.Errorf("empty value") + }internal/antewrapper/decorator_up_front_cost.go (1)
109-118: Convert BankKeeper calls to use ctx.Context()
- internal/antewrapper/decorator_up_front_cost.go:117: replace
bk.GetBalance(ctx, addr, coin.Denom)
with
bk.GetBalance(ctx.Context(), addr, coin.Denom)- internal/antewrapper/utils.go:250: replace
bankKeeper.SendCoinsFromAccountToModule(ctx, addr, authtypes.FeeCollectorName, fee)
with
bankKeeper.SendCoinsFromAccountToModule(ctx.Context(), addr, authtypes.FeeCollectorName, fee)internal/antewrapper/decorator_context_setup.go (3)
51-54: Bug: losing context changes by calling WithGasMeter on ctx instead of newCtx.This discards state set by ante.SetGasMeter. Use newCtx.WithGasMeter.
-// Now wrap that gas meter in our flat-fee gas meter. -newCtx = ctx.WithGasMeter(NewFlatFeeGasMeter(newCtx.GasMeter(), newCtx.Logger(), d.ffk)) +// Now wrap that gas meter in our flat-fee gas meter. +newCtx = newCtx.WithGasMeter(NewFlatFeeGasMeter(newCtx.GasMeter(), newCtx.Logger(), d.ffk))
59-69: Possible nil deref: ctx.ConsensusParams() may be nil. Add guard.Protect access to .Block via a cp != nil check.
-if bp := ctx.ConsensusParams().Block; bp != nil { - maxBlockGas := bp.GetMaxGas() +if cp := ctx.ConsensusParams(); cp != nil { + if bp := cp.Block; bp != nil { + maxBlockGas := bp.GetMaxGas() if maxBlockGas > 0 { if gasWanted > uint64(maxBlockGas) { return newCtx, sdkerrors.ErrInvalidGasLimit.Wrapf("tx gas limit %d exceeds block max gas %d", gasWanted, maxBlockGas) } if txGasLimitShouldApply(ctx.ChainID(), tx.GetMsgs()) && gasWanted > TxGasLimit { return newCtx, sdkerrors.ErrInvalidGasLimit.Wrapf("tx gas limit %d exceeds tx max gas %d", gasWanted, TxGasLimit) } - } - } + } + } }
80-82: Incorrect “gasWanted” reported in OutOfGas error.Message shows meter limit instead of requested gas. Use gasWanted.
-err = sdkerrors.ErrOutOfGas.Wrapf("out of gas in location: %v; gasWanted: %d, gasUsed: %d", - rType.Descriptor, newCtx.GasMeter().Limit(), newCtx.GasMeter().GasConsumed()) +err = sdkerrors.ErrOutOfGas.Wrapf("out of gas in location: %v; gasWanted: %d, gasUsed: %d", + rType.Descriptor, gasWanted, newCtx.GasMeter().GasConsumed())x/attribute/types/events.go (1)
121-133: Encode ValueHash: Replacestring(attribute.Hash())withbase64.StdEncoding.EncodeToString(attribute.Hash())inx/attribute/types/events.gofor safe, readable output and consistency with other attribute events.
♻️ Duplicate comments (1)
.golangci.yml (1)
43-47: usetesting enable: good; deprecations acknowledgedEnabling usetesting is valid in v2. Commenting out stylecheck/gosimple/tenv is correct: gosimple+stylecheck were merged into staticcheck, and tenv is removed in v2.
Refs: linters availability and v2 changes. (golangci-lint.run)
🧹 Nitpick comments (73)
x/exchange/client/cli/flags.go (2)
26-26: Explain the nolint.Add a short rationale for
//nolint:revive(e.g., public CLI flag name stability) to avoid future removal.
940-964: Trim and normalize tokens before merge to avoid whitespace-parse failures.pflag’s StringSlice split doesn’t trim; merged coins like
"acct:1nhash, 2nhash"may carry leading spaces causing parse errors downstream. Trim each token before colon-check/merge.Apply:
func ReadFlagAccountAmountsOrDefault(flagSet *pflag.FlagSet, name string, def []exchange.AccountAmount) ([]exchange.AccountAmount, error) { rawVals, err := flagSet.GetStringSlice(name) if len(rawVals) == 0 || err != nil { return def, err } // Slice flags are automatically split on commas. But here, we need commas for separating coin // entries in a coins string. So, add any entries without a colon to the previous entry. vals := make([]string, 0, len(rawVals)) for i, val := range rawVals { + val = strings.TrimSpace(val) if i == 0 || strings.Contains(val, ":") { vals = append(vals, val) } else { - vals[len(vals)-1] += "," + val + vals[len(vals)-1] += "," + strings.TrimSpace(val) } } rv, err := ParseAccountAmounts(vals) if err != nil { return def, err } return rv, nil }I can also add table-driven tests covering spaces, multiple commas, and first-token-without-colon cases.
testutil/ibc/isc4_middleware_mock.go (2)
17-28: Optional: make mock behavior configurableReturning a constant sequence (1) is fine; consider fields to control sequence/error for broader test scenarios.
Example:
-type ICS4WrapperMock struct{} +type ICS4WrapperMock struct{ + SequenceToReturn uint64 + ErrToReturn error +} func (m *ICS4WrapperMock) SendPacket( _ sdk.Context, _ *capabilitytypes.Capability, _ string, _ string, _ ibcclienttypes.Height, _ uint64, _ []byte, ) (sequence uint64, err error) { - return 1, nil + if m.SequenceToReturn == 0 { + return 1, m.ErrToReturn + } + return m.SequenceToReturn, m.ErrToReturn }
41-45: Nit: group blank identifiers on one line for readabilitySame effect, less vertical churn.
-func (m *ICS4WrapperMock) GetAppVersion( - _ sdk.Context, - _, - _ string, -) (string, bool) { +func (m *ICS4WrapperMock) GetAppVersion( + _ sdk.Context, + _, _ string, +) (string, bool) {x/flatfees/client/cli/query.go (5)
41-44: Nit: help text grammar for the flagTighten the help string.
- cmd.Flags().Bool(FlagDoNotConvert, false, "Do not convert msg fee cost into the fee denom, return them as they are defined") + cmd.Flags().Bool(FlagDoNotConvert, false, "Do not convert message fees to the fee denom; return them as defined.")
65-69: Prefer cmd.Context() over context.Background()Honors cancellation (e.g., Ctrl-C) and timeouts; improves UX without behavior change.
- response, err := queryClient.Params(context.Background(), &types.QueryParamsRequest{}) + response, err := queryClient.Params(cmd.Context(), &types.QueryParamsRequest{})- response, err := queryClient.AllMsgFees(context.Background(), req) + response, err := queryClient.AllMsgFees(cmd.Context(), req)- response, err := queryClient.MsgFee(context.Background(), req) + response, err := queryClient.MsgFee(cmd.Context(), req)- resp, err := queryClient.CalculateTxFees( - context.Background(), + resp, err := queryClient.CalculateTxFees( + cmd.Context(), &types.QueryCalculateTxFeesRequest{TxBytes: txBytes, GasAdjustment: float32(gasAdj)}, )Also applies to: 105-110, 144-149, 196-203
133-136: Remove redundant args checkcobra.ExactArgs(1) already guarantees one non-empty arg.
- if len(args) == 0 || len(args[0]) == 0 { - return errors.New("no msg-type-url provided") - }Also drop the now-unused import:
import ( "context" - "errors" "fmt"
190-192: Fix error message: we're encoding, not decodingAccurate error text helps troubleshooting.
- if err != nil { - return fmt.Errorf("error decoding tx bytes: %w", err) - } + if err != nil { + return fmt.Errorf("error encoding tx bytes: %w", err) + }
206-213: Optional: reduce flag surface for this query commandAddTxFlagsToCmd brings many tx/broadcast flags that don’t apply here and can confuse users. You only need gas-adjustment; consider defining just that and keeping the query flags you added.
- flags.AddTxFlagsToCmd(cmd) + // Only need gas adjustment for simulation; avoid surfacing unrelated tx flags. + cmd.Flags().Float64(flags.FlagGasAdjustment, 1.0, "gas adjustment factor for simulation")internal/pioconfig/provenanceconfig.go (1)
39-43: Use decimal format for ProvMinGasPrices (DecCoins style).Many Cosmos components expect gas prices as decimals (e.g., 0.0nhash). Emit a decimal to avoid parsing surprises.
Apply:
- ProvMinGasPrices: fmt.Sprintf("%d%s", defaultMinGasPricesAmount, feeAndBondDenom), + ProvMinGasPrices: fmt.Sprintf("%d.0%s", defaultMinGasPricesAmount, feeAndBondDenom),and
- ProvMinGasPrices: fmt.Sprintf("%d%s", defaultMinGasPricesAmount, defaultFeeDenom), + ProvMinGasPrices: fmt.Sprintf("%d.0%s", defaultMinGasPricesAmount, defaultFeeDenom),Also applies to: 53-56
cmd/provenanced/config/client.go (3)
14-25: Fix typo in comment.“loades” → “loaded”.
-// - The way the SDK loads the config into viper stomps on previously loades stuff. +// - The way the SDK loads the config into viper stomps on previously loaded stuff.
27-27: Add a reason to the nolint directive.Helps satisfy nolintlint and future readers.
- DefaultChainID = "" //nolint:revive + DefaultChainID = "" //nolint:revive // intentionally empty; set via config/tests at runtime
64-67: Doc fix: “output format”, not “writer”.-// SetOutput sets the output writer. +// SetOutput sets the output format.internal/ibc/ibc.go (5)
12-13: Use correct Go initialism casing and keep module label semantic.
- Exported identifiers should capitalize initialisms: IBC vs Ibc (revive/stylecheck).
- “module” log field should be the module name (e.g., "ibc"); keep the event type separate.
Apply:
-// IbcAcknowledgementErrorType is the error type for IBC acknowledgements. -const IbcAcknowledgementErrorType = "ibc-acknowledgement-error" +// IBCAcknowledgementErrorType is the event type for IBC acknowledgement errors. +const IBCAcknowledgementErrorType = "ibc-acknowledgement-error" - logger := ctx.Logger().With("module", IbcAcknowledgementErrorType) + logger := ctx.Logger().With("module", "ibc", "event", IBCAcknowledgementErrorType) - IbcAcknowledgementErrorType, + IBCAcknowledgementErrorType,Also applies to: 25-25, 40-40
23-23: Fix doc comment to satisfy lint (starts with the identifier; sentence case).-// EmitIBCErrorEvents Emit and Log errors +// EmitIBCErrorEvents emits and logs IBC acknowledgement error events.
26-36: Right-size log levels and prefer structured logs over fmt.Sprintf.
- Logging “no error” at Error level is noisy; use Info.
- Log the primary error once, then per-context as Info with structured fields.
- if err == nil { - logger.Error("no error skipping emit") - return - } + if err == nil { + logger.Info("no error; skipping IBC error event emission") + return + } + // structured log of the primary error + logger.Error("IBC acknowledgement error", "err", err) attributes := make([]sdk.Attribute, len(errorContexts)+1) attributes[0] = sdk.NewAttribute("error", err.Error()) for i, s := range errorContexts { attributes[i+1] = sdk.NewAttribute("error-context", s) - logger.Error(fmt.Sprintf("error-context: %v", s)) + logger.Info("IBC error context", "context", s) }
46-54: Optional guard for empty/non-JSON acknowledgements.Cheap fast-path to avoid unnecessary allocations on empty input.
func IsAckError(acknowledgement []byte) bool { + if len(acknowledgement) == 0 { + return false + } var ackErr channeltypes.Acknowledgement_Error if err := json.Unmarshal(acknowledgement, &ackErr); err == nil && len(ackErr.Error) > 0 { return true } return false }
15-21: Make EmitIBCErrorEvents variadic and update its callers
- In internal/ibc/ibc.go, change
func EmitIBCErrorEvents(ctx sdk.Context, err error, errorContexts []string)
to
func EmitIBCErrorEvents(ctx sdk.Context, err error, errorContexts ...string)- In NewEmitErrorAcknowledgement (line 18), call
EmitIBCErrorEvents(ctx, err, errorContexts...)- In internal/ibc/ibc_test.go (line 120), change
ibc.EmitIBCErrorEvents(ctx, tc.err, tc.errCtx)
to
ibc.EmitIBCErrorEvents(ctx, tc.err, tc.errCtx...)internal/handlers/staking_restrictions_hooks.go (3)
28-33: Enforce RestrictionOptions invariants on constructionAdd a light sanitize step so MinBondedCapPercent ∈ [0,1], MaxBondedCapPercent ∈ (0,1], and Min ≤ Max. This prevents misconfiguration from silently weakening/over-tightening limits.
Apply within constructor:
func NewStakingRestrictionHooks(k *stakingkeeper.Keeper, opts RestrictionOptions) StakingRestrictionHooks { - return StakingRestrictionHooks{k, opts} + opts = sanitizeRestrictionOptions(opts) + return StakingRestrictionHooks{k, opts} }Add helper (outside this hunk):
func sanitizeRestrictionOptions(opts RestrictionOptions) RestrictionOptions { if opts.MaxConcentrationMultiple <= 0 { opts.MaxConcentrationMultiple = DefaultConcentrationMultiple } if opts.MaxBondedCapPercent <= 0 || opts.MaxBondedCapPercent > 1 { opts.MaxBondedCapPercent = DefaultMaxCapPercent } if opts.MinBondedCapPercent < 0 || opts.MinBondedCapPercent > 1 { opts.MinBondedCapPercent = DefaultMinCapPercent } if opts.MinBondedCapPercent > opts.MaxBondedCapPercent { opts.MinBondedCapPercent = opts.MaxBondedCapPercent } return opts }
61-80: Avoid float truncation when computing maxBond; use integer basis points
int64(maxValidatorPercent * 100)floors to whole percent and depends on float32 rounding. Compute deterministically in basis points (1/10000) with rounding.Apply:
- maxBond := currentBond.Quo(sdkmath.NewInt(100)).MulRaw(int64(maxValidatorPercent * 100)) + // Use basis points to avoid float truncation and 1% granularity. + denom := int64(10000) + scaled := int64(math.Round(float64(maxValidatorPercent) * float64(denom))) + maxBond := currentBond.MulRaw(scaled).Quo(sdkmath.NewInt(denom))And add import:
import ( "context" "math" // ... )Also applies to: 87-88
92-97: Fix error message: prints total bonded instead of validator bondedThe message says “validator bonded tokens of %d” but prints currentBond (network total). Use validator.GetBondedTokens() for accuracy; also consider 2 decimal places for percent.
- "validator bonded tokens of %d exceeds max of %d (%.1f%%) for %d validators", - currentBond.BigInt(), + "validator bonded tokens of %d exceeds max of %d (%.2f%%) for %d validators", + validator.GetBondedTokens().BigInt(), maxBond.BigInt(), - maxValidatorPercent*100, + maxValidatorPercent*100, valCount,.golangci.yml (4)
1-1: Quote the config version per v2 schemaThe v2 schema declares version as a string. Quote it to avoid validation hiccups.
Apply:
-version: 2 +version: "2"Source: golangci-lint v2 config schema. (golangci-lint.run)
13-13: Depguard disabled but rules configured — confirm intentYou’ve commented out depguard in enable while keeping extensive rules below. If enforcement is desired, uncomment it; otherwise drop the rules to avoid confusion. Note that list-mode: lax largely allows imports unless explicitly denied; consider strict for allowlist enforcement.
Refs: depguard settings and list modes. (golangci-lint.run)
47-51: Formatters block is valid; avoid gofmt+goimports overlapRunning both gofmt and goimports is redundant (goimports already formats). Prefer goimports + gci to reduce churn.
formatters: enable: - - gofmt - goimports - gciDocs: formatters config. (golangci-lint.run)
79-110: Depguard allow list: prefix semantics + mode choiceReminder (from prior learning): allow entries are prefixes unless they end with
$. Broad prefixes like cosmossdk.io or github.com/cosmos permit all subpackages. If you want exact packages, add $ . If stricter control is desired, switch list-mode to strict.Refs: depguard matching rules and previous lesson. (golangci-lint.run)
x/hold/client/cli/query.go (5)
41-77: Improve invalid-address error message.Wrap with context to aid users.
- if _, err = sdk.AccAddressFromBech32(args[0]); err != nil { - return sdkerrors.ErrInvalidAddress.Wrap(err.Error()) - } + if _, err = sdk.AccAddressFromBech32(args[0]); err != nil { + return sdkerrors.ErrInvalidAddress. + Wrapf("invalid bech32 address %q: %v", args[0], err) + }
41-77: Minor simplification: avoid predeclaring res.Keeps scope tight.
- var res *hold.GetHoldsResponse - queryClient := hold.NewQueryClient(clientCtx) - res, err = queryClient.GetHolds(cmd.Context(), &req) + queryClient := hold.NewQueryClient(clientCtx) + res, err := queryClient.GetHolds(cmd.Context(), &req)
41-47: Help text consistency.Short strings: either end all with a period or none. “all” command below has no period.
- Short: "Get the funds that are on hold for an address.", + Short: "Get the funds that are on hold for an address",
21-21: Example address type is misleading and likely encodes garbage.sdk.AccAddress is []byte; casting an arbitrary string yields an invalid address. Use a string placeholder or a valid bech32 example.
-var exampleQueryAddr1 = sdk.AccAddress("exampleQueryAddr1___") +// Use a readable placeholder or a chain-appropriate bech32 example. +var exampleQueryAddr1 = "<bech32-account-address>"
82-86: Align Short with previous command.Add a period (or remove from the other). Since above we removed it, mirror that here.
- Short: "Get all funds on hold for all accounts", + Short: "Get all funds on hold for all accounts",app/test_helpers.go (2)
355-356: Unexport if not needed + confirm scope creepIf this strategy type is only used within this file/package, prefer a non-exported name (generateAccountStrategy). Also, given the PR goal is lint infra upgrade, confirm these new helpers are intended in this PR.
517-524: Speed up tests: avoid LoadLatest for encoding-only setupNew uses a fresh memdb; you can skip loading latest state.
tempApp := New( log.NewNopLogger(), dbm.NewMemDB(), nil, - true, + false, simtestutil.NewAppOptionsWithFlagHome(tempDir), )testutil/contracts/wasm.go (4)
35-38: Clarify comment: function returns bytes, it doesn’t “apply” rate limitingMinor wording tweak to avoid implying runtime behavior.
-// RateLimiterWasm applies rate limiting to WASM contract execution. +// RateLimiterWasm returns the rate-limiter contract wasm bytecode.
40-48: Make instantiate access configurable (optional)Hardcoding AccessTypeEverybody is fine for tests, but consider letting callers pass an AccessConfig to exercise different policies.
-func StoreContractCode(app *provenanceapp.App, ctx sdk.Context, wasmCode []byte) (uint64, error) { +func StoreContractCode(app *provenanceapp.App, ctx sdk.Context, wasmCode []byte, access *wasmtypes.AccessConfig) (uint64, error) { govKeeper := wasmkeeper.NewGovPermissionKeeper(app.WasmKeeper) creator := app.AccountKeeper.GetModuleAddress(govtypes.ModuleName) - - accessEveryone := wasmtypes.AccessConfig{Permission: wasmtypes.AccessTypeEverybody} - codeID, _, err := govKeeper.Create(ctx, creator, wasmCode, &accessEveryone) + if access == nil { + access = &wasmtypes.AccessConfig{Permission: wasmtypes.AccessTypeEverybody} + } + codeID, _, err := govKeeper.Create(ctx, creator, wasmCode, access) return codeID, err }
50-56: Prefer []byte for msg to avoid implicit string->bytes conversionsNot critical, but it reduces copies and makes the API consistent with Wasm expectations.
-func InstantiateContract(app *provenanceapp.App, ctx sdk.Context, msg string, codeID uint64) (sdk.AccAddress, error) { +func InstantiateContract(app *provenanceapp.App, ctx sdk.Context, msg []byte, codeID uint64) (sdk.AccAddress, error) { contractKeeper := wasmkeeper.NewDefaultPermissionKeeper(app.WasmKeeper) creator := app.AccountKeeper.GetModuleAddress(govtypes.ModuleName) - addr, _, err := contractKeeper.Instantiate(ctx, codeID, creator, creator, []byte(msg), "contract", nil) + addr, _, err := contractKeeper.Instantiate(ctx, codeID, creator, creator, msg, "contract", nil) return addr, err }
58-63: Return []byte and rename param for clarityQuerySmart returns bytes; returning string risks lossy conversions. Also, “key” suggests a storage key; this is a query message.
-func QueryContract(app *provenanceapp.App, ctx sdk.Context, contract sdk.AccAddress, key []byte) (string, error) { - state, err := app.WasmKeeper.QuerySmart(ctx, contract, key) - return string(state), err +func QueryContract(app *provenanceapp.App, ctx sdk.Context, contract sdk.AccAddress, queryMsg []byte) ([]byte, error) { + return app.WasmKeeper.QuerySmart(ctx, contract, queryMsg) }testutil/ibc/testchain.go (5)
38-45: Use test logging instead of printlnThis integrates with test output and respects -v.
codeID, err := contracts.StoreContractCode(chain.GetProvenanceApp(), chain.GetContext(), contracts.CounterWasm()) suite.Require().NoError(err, "counter contract direct code load failed", err) -println("loaded counter contract with code id: ", codeID) +suite.T().Logf("loaded counter contract with code id: %d", codeID) return codeID
46-53: Use test logging instead of printlncodeID, err := contracts.StoreContractCode(chain.GetProvenanceApp(), chain.GetContext(), contracts.EchoWasm()) suite.Require().NoError(err, "echo contract direct code load failed", err) -println("loaded echo contract with code id: ", codeID) +suite.T().Logf("loaded echo contract with code id: %d", codeID) return codeID
54-61: Use test logging and consistent assertion argsAlso include err in the assertion message args for parity with other helpers.
codeID, err := contracts.StoreContractCode(chain.GetProvenanceApp(), chain.GetContext(), contracts.RateLimiterWasm()) -suite.Require().NoError(err, "rate limiter contract direct code load failed") -println("loaded rate limiter contract with code id: ", codeID) +suite.Require().NoError(err, "rate limiter contract direct code load failed", err) +suite.T().Logf("loaded rate limiter contract with code id: %d", codeID) return codeID
62-69: Use test logging instead of printlnaddr, err := contracts.InstantiateContract(chain.GetProvenanceApp(), chain.GetContext(), msg, codeID) suite.Require().NoError(err, "contract instantiation failed", err) -println("instantiated contract '", codeID, "' with address: ", addr) +suite.T().Logf("instantiated contract %d with address: %s", codeID, addr.String()) return addr
76-83: Use test logging; consider renaming param“key” suggests store key; this is a query message. Also avoid println.
state, err := contracts.QueryContract(chain.GetProvenanceApp(), chain.GetContext(), contract, key) suite.Require().NoError(err, "contract query failed", err) -println("got query result of ", state) +suite.T().Logf("contract query result: %s", state) return statetestutil/mocks/codec.go (4)
1-1: Doc: include UnpackAny in the list of injectable error pointsKeep docs aligned with implementation.
-// Package mocks contains mock codecs used in test utilities. +// Package mocks contains mock codecs used in test utilities. +// Error injection is supported for: MarshalJSON, MustMarshalJSON, UnmarshalJSON, MustUnmarshalJSON, and UnpackAny.
62-74: LGTM: deterministic error injection with pop semanticsConsider DRYing the “pop first error” pattern into a tiny helper to reduce duplication, but not required.
76-87: LGTM: panic path mirrors MustMarshalJSON contractsSame DRY note applies.
88-101: Avoid broad nolint unless necessaryIf govet isn’t flagging a real issue here, drop the directive or scope it with a reason to avoid hiding future warnings.
internal/provwasm/simulation.go (1)
353-375: Handle parse error and align guard with sent fundsTiny polish: check the error from
ParseCoinsNormalized.Apply this diff:
- amount, _ := sdk.ParseCoinsNormalized(fmt.Sprintf("100%s", denom)) + amount, err := sdk.ParseCoinsNormalized(fmt.Sprintf("100%s", denom)) + if err != nil { + panic(err) + }Note: The fee payer here is
consumer; with earlier funding fix, fees won’t fail.x/attribute/client/cli/tx.go (4)
98-103: Handle flag error and enforce the advertised 200-char limit.Help text promises a max of 200 chars, but we don’t validate. Also, ignore of GetString error is unnecessary.
Apply in both commands:
- msg.ConcreteType, _ = cmd.Flags().GetString(FlagConcreteType) + ct, err := cmd.Flags().GetString(FlagConcreteType) + if err != nil { + return err + } + if len(ct) > 200 { + return fmt.Errorf("concrete type too long: %d > 200", len(ct)) + } + msg.ConcreteType = ctAlso applies to: 157-162
201-205: Fix error message to report the original input, not the encoded bytes.On decode failure, deleteValue is nil/garbled; surfacing the original arg is more useful.
- deleteValue, err := encodeAttributeValue(strings.TrimSpace(args[3]), attributeType) + deleteArg := strings.TrimSpace(args[3]) + deleteValue, err := encodeAttributeValue(deleteArg, attributeType) if err != nil { - return fmt.Errorf("error encoding value %s to type %s : %w", deleteValue, attributeType.String(), err) + return fmt.Errorf("error encoding value %q to type %s: %w", deleteArg, attributeType.String(), err) }
184-186: Nit: help text grammar.“with specific name and value the provenance blockchain” → “with a specific name and value from the Provenance blockchain”.
- Short: "Delete an account attribute with specific name and value the provenance blockchain", + Short: "Delete an account attribute with a specific name and value from the Provenance blockchain",
49-51: Nit: comment wording.“creates a command for adding an account attributes” → “creates a command for adding an account attribute”.
-// NewAddAccountAttributeCmd creates a command for adding an account attributes. +// NewAddAccountAttributeCmd creates a command for adding an account attribute. ... -// NewUpdateAccountAttributeCmd creates a command for adding an account attributes. +// NewUpdateAccountAttributeCmd creates a command for updating an account attribute.Also applies to: 107-109
x/attribute/abci.go (2)
13-19: Fix constant name and doc; it currently reads “Attribution” and “retain” while it’s used for deleting attributes.Rename and update the usage/comment for clarity and to avoid leaking a confusing exported API name.
Apply:
-// MaxExpiredAttributionCount defines the max number of expired attributions to retain. -const MaxExpiredAttributionCount = 100_000 +// MaxExpiredAttributeDeleteLimit caps how many expired attributes are deleted per block. +const MaxExpiredAttributeDeleteLimit = 100_000 @@ - deleted := keeper.DeleteExpiredAttributes(ctx, MaxExpiredAttributionCount) + deleted := keeper.DeleteExpiredAttributes(ctx, MaxExpiredAttributeDeleteLimit)Also applies to: 18-26
18-28: Optional: consider making the per-block delete limit a param.A hard-coded exported const is rigid; a module param (with sane default) would allow ops to tune without code changes.
app/prefix.go (1)
27-35: Doc comment mismatches and omissions in Defaults block.
- Comment above ValidatorAddressPrefix mentions ValidatorPubKeyPrefix.
- “mainnet account public key prefix” is misleading once SetConfig(testnet=true) runs.
- Missing docs for ConsNodeAddressPrefix/ConsNodePubKeyPrefix and CoinType vars.
Apply:
- // AccountPubKeyPrefix is the mainnet account public key prefix. + // AccountPubKeyPrefix is the default account public key prefix. AccountPubKeyPrefix = AccountAddressPrefix + "pub" - // ValidatorPubKeyPrefix is the prefix for validator public keys. - ValidatorAddressPrefix = AccountAddressPrefix + "valoper" - ValidatorPubKeyPrefix = AccountAddressPrefix + "valoperpub" - ConsNodeAddressPrefix = AccountAddressPrefix + "valcons" - ConsNodePubKeyPrefix = AccountAddressPrefix + "valconspub" - CoinType = CoinTypeMainNet + // ValidatorAddressPrefix is the prefix for validator operator addresses. + ValidatorAddressPrefix = AccountAddressPrefix + "valoper" + // ValidatorPubKeyPrefix is the prefix for validator operator public keys. + ValidatorPubKeyPrefix = AccountAddressPrefix + "valoperpub" + // ConsNodeAddressPrefix is the prefix for consensus node addresses. + ConsNodeAddressPrefix = AccountAddressPrefix + "valcons" + // ConsNodePubKeyPrefix is the prefix for consensus node public keys. + ConsNodePubKeyPrefix = AccountAddressPrefix + "valconspub" + // CoinType is the current SLIP-44 coin type. + CoinType = CoinTypeMainNetx/attribute/types/msgs.go (1)
68-76: Unify value parameter type with other constructors.Other constructors take []byte; this one takes string and converts. For consistency and to avoid implicit UTF-8 assumptions, accept []byte here too.
Apply:
-func NewMsgUpdateAttributeExpirationRequest(account, name, value string, expirationDate *time.Time, owner sdk.AccAddress) *MsgUpdateAttributeExpirationRequest { +func NewMsgUpdateAttributeExpirationRequest(account, name string, value []byte, expirationDate *time.Time, owner sdk.AccAddress) *MsgUpdateAttributeExpirationRequest { return &MsgUpdateAttributeExpirationRequest{ Account: account, Name: strings.ToLower(strings.TrimSpace(name)), - Value: []byte(value), + Value: value, ExpirationDate: expirationDate, Owner: owner.String(), } }x/hold/hold.go (1)
9-18: Consider rejecting zero amounts if holds must be strictly positive.sdk.Coin.Validate allows zero; if AccountHold requires > 0, add an IsPositive() check.
Apply:
if err := e.Amount.Validate(); err != nil { return fmt.Errorf("invalid amount: %w", err) } + if !e.Amount.IsPositive() { + return fmt.Errorf("invalid amount: must be > 0") + }app/export.go (1)
79-79: Minor: nolint comment is fine; consider referencing the actual Staticcheck code.Optional: add a brief rationale to aid future maintenance.
x/attribute/keeper/msg_server.go (1)
48-51: Prefer consistent method promotion usage across this fileYou switched to
k.SetAttributehere but still call other keeper methods viak.Keeper.*elsewhere. For readability, use one style consistently (prefer promotion).Apply these minimal diffs:
- err = k.Keeper.UpdateAttribute(ctx, originalAttribute, updateAttribute, ownerAddr) + err = k.UpdateAttribute(ctx, originalAttribute, updateAttribute, ownerAddr)- err = k.Keeper.UpdateAttributeExpiration(ctx, attribute, ownerAddr) + err = k.UpdateAttributeExpiration(ctx, attribute, ownerAddr)- err = k.Keeper.DeleteAttribute(ctx, msg.Account, msg.Name, nil, ownerAddr) + err = k.DeleteAttribute(ctx, msg.Account, msg.Name, nil, ownerAddr)- err = k.Keeper.DeleteAttribute(ctx, msg.Account, msg.Name, &msg.Value, ownerAddr) + err = k.DeleteAttribute(ctx, msg.Account, msg.Name, &msg.Value, ownerAddr)- err := k.Keeper.SetAccountData(ctx, msg.Account, msg.Value) + err := k.SetAccountData(ctx, msg.Account, msg.Value)x/exchange/orders.go (1)
552-559: Constructor is fine; optional guard against nil orderFunction works as-is. If you want earlier failure on misuse, add a nil check.
func NewFilledOrder(order *Order, actualPrice sdk.Coin, actualFees sdk.Coins) *FilledOrder { + if order == nil { + panic("NewFilledOrder: order must not be nil") + } return &FilledOrder{ order: order, actualPrice: actualPrice, actualFees: actualFees, } }x/exchange/keeper/grpc_query.go (1)
435-439: Be consistent with promoted vs embedded keeper callsYou changed to
k.CreateMarket(...)here but still usek.Keeper.*elsewhere in this file. Choose one approach for clarity (promotion recommended).x/exchange/genesis.go (1)
13-101: Robust genesis validation; minor collision-proofing nitOverall solid: params/markets/orders/commitments/payments validated with useful errors and max-id check. Small nit: build payment duplicate key with a delimiter that cannot appear to avoid rare collisions.
- id := payment.Source + " " + payment.ExternalId + id := payment.Source + "\x00" + payment.ExternalIdx/flatfees/client/cli/tx.go (2)
169-204: Validate the message client-side before broadcast.Call ValidateBasic to fail fast with clear errors.
func NewCmdUpdateConversionFactor() *cobra.Command { @@ RunE: func(cmd *cobra.Command, args []string) error { clientCtx, err := client.GetClientTxContext(cmd) if err != nil { return err } @@ msg.ConversionFactor, err = ParseConversionFactor(args[0]) if err != nil { return err } + if err := msg.ValidateBasic(); err != nil { + return err + } + return provcli.GenerateOrBroadcastTxCLIAsGovProp(clientCtx, flagSet, msg) }, }
57-63: Split only once and tweak the comment.Avoid unintended splits when values contain '=' and fix “convert” -> “converted”.
-// Both <base> and <convert> must be a single coin string. +// Both <base> and <converted> must be a single coin string. func ParseConversionFactor(arg string) (types.ConversionFactor, error) { - parts := strings.Split(arg, "=") + parts := strings.SplitN(arg, "=", 2) if len(parts) != 2 { return types.ConversionFactor{}, fmt.Errorf("invalid conversion factor %q: expected exactly one equals sign", arg) }internal/antewrapper/decorator_up_front_cost.go (1)
24-27: Return a pointer from the constructor.Prevents copying the struct and aligns with typical Ante decorator construction.
-func NewDeductUpFrontCostDecorator(ak ante.AccountKeeper, bk BankKeeper, fk FeegrantKeeper) DeductUpFrontCostDecorator { - return DeductUpFrontCostDecorator{ak: ak, bk: bk, fk: fk} -} +func NewDeductUpFrontCostDecorator(ak ante.AccountKeeper, bk BankKeeper, fk FeegrantKeeper) *DeductUpFrontCostDecorator { + return &DeductUpFrontCostDecorator{ak: ak, bk: bk, fk: fk} +}x/flatfees/types/flatfees.go (1)
58-68: Simplify cost aggregation.Use variadic Add to avoid the loop.
func NewMsgFee(msgTypeURL string, cost ...sdk.Coin) *MsgFee { rv := &MsgFee{ MsgTypeUrl: msgTypeURL, } - // Adding each cost coin in so that the result is always valid and sorted. - for _, coin := range cost { - rv.Cost = rv.Cost.Add(coin) - } + // Add ensures the result is canonical and sorted. + rv.Cost = rv.Cost.Add(cost...) return rv }x/exchange/module/module.go (1)
151-155: Comment vs implementation mismatch.Comment says “of which there are none,” but this returns simulation.WeightedOperations(). Update the comment or return nil.
-// of which there are none. -func (am AppModule) WeightedOperations(_ module.SimulationState) []simtypes.WeightedOperation { - return simulation.WeightedOperations() -} +// Returns the exchange module operations with their respective weights. +func (am AppModule) WeightedOperations(_ module.SimulationState) []simtypes.WeightedOperation { + return simulation.WeightedOperations() +}x/attribute/types/events.go (5)
14-16: Grammar nit: “a attribute” → “an attribute”.-// EventTypeAttributeExpirationUpdated emitted when a attribute's expiration date is updated. +// EventTypeAttributeExpirationUpdated emitted when an attribute's expiration date is updated.
24-26: Comment label mismatch (name vs value).-// AttributeKeyNameAttribute is the telemetry label for attribute value. +// AttributeKeyNameAttribute is the telemetry label for attribute name.
47-49: Typo: “acount” → “account”.-// EventTelemetryLabelAccount acount telemetry metrics label +// EventTelemetryLabelAccount account telemetry metrics label
41-42: Comment label mismatch (“name” vs “value”).-// EventTelemetryLabelValue name telemetry metrics label +// EventTelemetryLabelValue value telemetry metrics label
55-67: Standardize time formatting to RFC3339 (current .String() is locale/impl-dependent).- if attribute.ExpirationDate != nil { - expirationDate = attribute.ExpirationDate.String() - } + if attribute.ExpirationDate != nil { + expirationDate = attribute.ExpirationDate.UTC().Format(time.RFC3339Nano) + }- if attribute.ExpirationDate != nil { - updated = attribute.ExpirationDate.String() - } + if attribute.ExpirationDate != nil { + updated = attribute.ExpirationDate.UTC().Format(time.RFC3339Nano) + } - if originalExpiration != nil { - original = originalExpiration.String() - } + if originalExpiration != nil { + original = originalExpiration.UTC().Format(time.RFC3339Nano) + }- if attribute.ExpirationDate != nil { - expiredTime = attribute.ExpirationDate.String() - } + if attribute.ExpirationDate != nil { + expiredTime = attribute.ExpirationDate.UTC().Format(time.RFC3339Nano) + }If any consumers parse these strings today, confirm they expect RFC3339.
Also applies to: 83-99, 121-126
x/exchange/msgs.go (3)
376-409: Fix error label: “to address” → “account”The error refers to
m.Accountbut says “to address”. Use “account” for clarity.- if _, err := sdk.AccAddressFromBech32(m.Account); err != nil { - errs = append(errs, fmt.Errorf("invalid to address %q: %w", m.Account, err)) - } + if _, err := sdk.AccAddressFromBech32(m.Account); err != nil { + errs = append(errs, fmt.Errorf("invalid account %q: %w", m.Account, err)) + }
94-118: Validation surface looks thorough; consider minor consistency helpersThe ValidateBasic implementations are comprehensive and consistent (errors.Join, zero checks, address decoding). To reduce repetition and keep error messages uniform:
- Optional: extract helpers like
validateNonZeroMarketID(id uint32) error,validateNonZeroCoin(coin sdk.Coin, field string) error, etc., and reuse.Also applies to: 120-149, 151-160, 162-201, 203-241, 242-271, 343-347, 348-375, 411-433, 434-458, 459-477, 483-499, 500-516, 517-528, 529-543, 544-595, 601-625, 633-637, 638-649, 650-664, 665-693, 694-722, 723-739, 740-749, 750-786, 799-809, 811-815, 816-824
16-49: AllRequestMsgs list: keep as single source of truthGood to centralize message types. Consider keeping it sorted or matching tx.proto order for easier diffs later. Optional only.
SpicyLemon
changed the title
nagarajdivine
force-pushed
the
chore/updage-golang-lint
branch
2 times, most recently
from
85b83bd to
77e62e5
Compare
nagarajdivine
force-pushed
the
chore/updage-golang-lint
branch
from
89885a4 to
1267dbe
Compare
nagarajdivine
force-pushed
the
chore/updage-golang-lint
branch
from
5ea3f1f to
0dcb1b1
Compare
…revive lint warnings
… settings differnce
…rors and updated lint configuration
nagarajdivine
force-pushed
the
chore/updage-golang-lint
branch
from
c09166a to
669d141
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/metadata/keeper/scope.go (1)
922-928: Move defer to immediately after iterator creation.The
defer it.Close()statement on Line 928 is placed after the for loop that uses the iterator (Lines 926-927). While this still works correctly (defer executes when the function returns), Go convention is to place defer statements immediately after resource acquisition for clarity.Apply this diff to move the defer to the conventional location:
func (k Keeper) RemoveNetAssetValues(ctx sdk.Context, scopeID types.MetadataAddress) { store := ctx.KVStore(k.storeKey) it := storetypes.KVStorePrefixIterator(store, types.NetAssetValueKeyPrefix(scopeID)) + defer it.Close() //nolint:errcheck // close error safe to ignore in this context. var keys [][]byte for ; it.Valid(); it.Next() { keys = append(keys, it.Key()) } - defer it.Close() //nolint:errcheck // close error safe to ignore in this context. for _, key := range keys { store.Delete(key) } }
♻️ Duplicate comments (7)
x/msgfees/types/msgs.go (1)
19-37: This critical issue was already flagged in a previous review.All six ValidateBasic() methods unconditionally return
errDep = errors.New("deprecated and unusable"), which will block all message processing for these types. This is inconsistent with the PR's stated objective of updating linting tooling. The previous review comment comprehensively covered this issue and requested verification of whether these messages are actually deprecated or require proper validation implementation.x/attribute/keeper/keeper.go (1)
120-122: Critical: Iterator closed before use.The iterator is closed on line 121 immediately after creation on line 119, but the loop that uses it starts on line 124. This causes the iterator to be closed before it's used, so the loop will never execute and the function won't process any records.
Apply this diff to defer the close:
iterator := storetypes.KVStorePrefixIterator(store, prefix) - - iterator.Close() //nolint:errcheck,gosec // close error safe to ignore in this context - + defer iterator.Close() //nolint:errcheck // close error safe to ignore in this context. // Iterate over records, processing callbacks. for ; iterator.Valid(); iterator.Next() {x/name/keeper/msg_server.go (1)
194-194: Inconsistent use of explicit Keeper reference.Line 194 uses
s.Keeper.CreateRootName(...)instead of the promoted methods.CreateRootName(...), which is inconsistent with the refactoring pattern applied throughout this file (lines 190-191 correctly uses.GetAuthority()).Apply this diff to use the promoted method:
- err := s.Keeper.CreateRootName(ctx, msg.Record.Name, msg.Record.Address, msg.Record.Restricted) + err := s.CreateRootName(ctx, msg.Record.Name, msg.Record.Address, msg.Record.Restricted)app/app.go (2)
497-499: Critical: app.SimulateProv method does not exist.Neither
Appnorbaseapp.BaseAppimplements aSimulateProvmethod, so this code won't compile. You must either passapp.BaseApp.Simulatedirectly or add a delegating method.Option 1 - Pass BaseApp.Simulate directly:
app.FlatFeesKeeper = flatfeeskeeper.NewKeeper( - appCodec, runtime.NewKVStoreService(keys[flatfeestypes.StoreKey]), authtypes.FeeCollectorName, app.SimulateProv, + appCodec, runtime.NewKVStoreService(keys[flatfeestypes.StoreKey]), authtypes.FeeCollectorName, app.BaseApp.Simulate, )Option 2 - Add a delegating method to App (add after line 1226):
func (app *App) SimulateProv(txBytes []byte) (sdk.GasInfo, *sdk.Result, sdk.Context, error) { return app.BaseApp.Simulate(txBytes) }
1257-1257: Critical: app.SimulateProv does not exist.Line 1257 calls
app.SimulateProvwhich doesn't exist (same issue as line 498). This must be fixed before the code will compile.Apply the same fix as suggested for line 498 (either use
app.BaseApp.Simulatedirectly or add theSimulateProvdelegating method)..golangci.yml (2)
7-7: Critical: Use v2 syntaxdefault: noneinstead ofdisable-all.In golangci-lint v2,
disable-all: trueis deprecated. Replace it with the v2-compliant syntax.Apply this diff:
- disable-all: true + default: noneBased on past review comments and golangci-lint v2 migration guide.
42-103: Critical: Move linter settings to top-levellinters-settings:key.The current structure nests per-linter configuration under
linters: settings:, which is invalid in golangci-lint v2. This causesgolangci-lint config verifyto fail, and all these settings (depguard, errcheck, govet, golint, dogsled, misspell, nolintlint) are silently ignored.Apply this diff to move settings to the correct top-level key:
+linters-settings: + depguard: + rules: + main: + list-type: lax + files: + - $all + - "!$test" + allow: + - $gostd + # Primary + - github.com/provenance-io/provenance + - github.com/provlabs/vault + - github.com/CosmWasm + - cosmossdk.io + - github.com/cosmos + - github.com/cometbft + - github.com/armon/go-metrics + - google.golang.org + - github.com/spf13 + - github.com/golang/protobuf + - github.com/google/uuid + - github.com/gorilla/mux + - github.com/grpc-ecosystem/grpc-gateway + - github.com/hashicorp/go-metrics + - github.com/rs/zerolog + - github.com/stretchr/testify + - sigs.k8s.io/yaml + deny: + - pkg: "github.com/cosmos/cosmos-sdk/x/params" + desc: "The params module is no more." + test: + list-type: lax + files: + - "$test" + allow: + - $gostd + - github.com/stretchr/testify + errcheck: + exclude-functions: + - (fmt.State).Write + govet: + shadow: true + printf: + funcs: + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf + golint: + min-confidence: 0 + dogsled: + max-blank-identifiers: 3 + misspell: + locale: US + mode: default + ignore-rules: + - cancelled + nolintlint: + allow-unused: false + require-explanation: false + require-specific: false + linters: disable-all: true enable: - asciicheck - bidichk - bodyclose - copyloopvar - depguard - dogsled - durationcheck - errcheck - errorlint - errname - goconst - gocritic - goheader - gomodguard - goprintffuncname - gosec - govet - importas - ineffassign - makezero - misspell - nakedret - nolintlint - prealloc - predeclared - promlinter - staticcheck - tparallel - unconvert - unparam - unused - whitespace - usetesting - settings: - depguard: - rules: - main: - list-type: lax - files: - - $all - - "!$test" - allow: - - $gostd - # Primary - - github.com/provenance-io/provenance - - github.com/provlabs/vault - - github.com/CosmWasm - - cosmossdk.io - - github.com/cosmos - - github.com/cometbft - - github.com/armon/go-metrics - - google.golang.org - - github.com/spf13 - - github.com/golang/protobuf - - github.com/google/uuid - - github.com/gorilla/mux - - github.com/grpc-ecosystem/grpc-gateway - - github.com/hashicorp/go-metrics - - github.com/rs/zerolog - - github.com/stretchr/testify - - sigs.k8s.io/yaml - deny: - - pkg: "github.com/cosmos/cosmos-sdk/x/params" - desc: "The params module is no more." - test: - list-type: lax - files: - - "$test" - allow: - - $gostd - - github.com/stretchr/testify - errcheck: - exclude-functions: - - (fmt.State).Write - govet: - shadow: true - printf: - funcs: - - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof - - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf - - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf - - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf - golint: - min-confidence: 0 - dogsled: - max-blank-identifiers: 3 - misspell: - locale: US - mode: default - ignore-rules: - - cancelled - nolintlint: - allow-unused: false - require-explanation: false - require-specific: falseAfter applying, run
golangci-lint config verifyto confirm the configuration is valid.Based on past review comments confirming this structure causes verification failures.
🧹 Nitpick comments (2)
x/hold/client/cli/tx.go (1)
135-135: LGTM! Suppression is appropriate for CLI tooling.The
nolintdirective correctly suppresses G304 for this CLI command where users intentionally provide file paths. The error handling is adequate.Optionally, clarify the comment to reflect that this is safe in a CLI context:
- data, err := os.ReadFile(path) //nolint:gosec // G304: error handled + data, err := os.ReadFile(path) //nolint:gosec // G304: user-controlled path in CLI is expectedinternal/antewrapper/decorator_fee_setup.go (1)
35-35: Consider refactoring test-specific logic.The call to
adjustCostsForUnitTestsusingctx.ChainID()to detect test environments couples production code to test infrastructure. Consider using dependency injection (e.g., a flag or config option) or build tags to separate test-specific behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (81)
.changelog/unreleased/dependencies/2346-upgrade-golangci-lint.md(1 hunks).github/workflows/lint.yml(1 hunks).golangci.yml(3 hunks)app/app.go(9 hunks)app/export.go(2 hunks)app/test_helpers.go(1 hunks)cmd/provenanced/cmd/docgen.go(1 hunks)cmd/provenanced/cmd/root.go(3 hunks)cmd/provenanced/config/manager.go(2 hunks)contrib/devtools/Makefile(1 hunks)internal/antewrapper/decorator_context_setup.go(2 hunks)internal/antewrapper/decorator_fee_setup.go(1 hunks)internal/antewrapper/decorator_up_front_cost.go(1 hunks)internal/antewrapper/flat_fee_gas_meter.go(1 hunks)internal/antewrapper/post_handler.go(1 hunks)internal/antewrapper/utils.go(1 hunks)internal/handlers/msg_service_router.go(2 hunks)internal/pioconfig/provenanceconfig.go(2 hunks)testutil/ibc/testchain.go(1 hunks)testutil/mocks/codec.go(3 hunks)testutil/network.go(1 hunks)testutil/testnet.go(1 hunks)x/attribute/client/cli/tx.go(1 hunks)x/attribute/client/cli/utils.go(1 hunks)x/attribute/keeper/keeper.go(7 hunks)x/attribute/keeper/msg_server.go(1 hunks)x/attribute/types/attribute.go(1 hunks)x/attribute/types/keys.go(1 hunks)x/exchange/client/cli/flags.go(3 hunks)x/exchange/keeper/grpc_query.go(1 hunks)x/exchange/keeper/keeper.go(2 hunks)x/exchange/keeper/orders.go(3 hunks)x/flatfees/client/cli/query.go(3 hunks)x/flatfees/client/cli/tx.go(2 hunks)x/flatfees/keeper/keeper.go(1 hunks)x/flatfees/module/module.go(1 hunks)x/flatfees/simulation/genesis.go(1 hunks)x/flatfees/types/codec.go(1 hunks)x/flatfees/types/errors.go(1 hunks)x/flatfees/types/flatfees.go(4 hunks)x/flatfees/types/msgs.go(3 hunks)x/hold/client/cli/tx.go(1 hunks)x/hold/keeper/grpc_query.go(3 hunks)x/hold/keeper/keeper.go(2 hunks)x/ibcratelimit/keeper/rate_limit.go(1 hunks)x/marker/client/cli/tx.go(6 hunks)x/marker/keeper/keeper.go(7 hunks)x/marker/keeper/marker.go(2 hunks)x/marker/keeper/msg_server.go(19 hunks)x/marker/types/si.go(2 hunks)x/marker/types/status.go(1 hunks)x/metadata/keeper/msg_server.go(9 hunks)x/metadata/keeper/objectstore.go(1 hunks)x/metadata/keeper/record.go(1 hunks)x/metadata/keeper/scope.go(7 hunks)x/metadata/keeper/session.go(1 hunks)x/metadata/keeper/specification.go(7 hunks)x/metadata/spec/examples/go/metadata_address.go(2 hunks)x/metadata/types/specification.go(1 hunks)x/msgfees/keeper/query_server.go(2 hunks)x/msgfees/types/msgs.go(1 hunks)x/name/client/cli/tx.go(1 hunks)x/name/keeper/keeper.go(3 hunks)x/name/keeper/msg_server.go(7 hunks)x/oracle/keeper/icq.go(1 hunks)x/oracle/keeper/msg_server.go(1 hunks)x/quarantine/keeper/keeper.go(3 hunks)x/quarantine/testutil/test_helpers.go(1 hunks)x/sanction/keeper/keeper.go(5 hunks)x/sanction/simulation/genesis.go(1 hunks)x/sanction/simulation/operations.go(1 hunks)x/trigger/client/cli/query.go(1 hunks)x/trigger/client/cli/tx.go(4 hunks)x/trigger/keeper/event_detector.go(1 hunks)x/trigger/keeper/event_listener.go(1 hunks)x/trigger/keeper/migrations_v2.go(1 hunks)x/trigger/keeper/queue.go(2 hunks)x/trigger/keeper/trigger.go(1 hunks)x/trigger/simulation/genesis.go(6 hunks)x/trigger/types/trigger.go(2 hunks)x/wasm/types/any.go(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- x/metadata/keeper/record.go
- x/wasm/types/any.go
- internal/antewrapper/utils.go
- x/marker/keeper/marker.go
- x/flatfees/types/codec.go
- x/flatfees/types/errors.go
🚧 Files skipped from review as they are similar to previous changes (53)
- x/attribute/types/attribute.go
- x/trigger/keeper/trigger.go
- x/attribute/client/cli/tx.go
- x/attribute/client/cli/utils.go
- testutil/testnet.go
- x/flatfees/module/module.go
- x/trigger/keeper/queue.go
- cmd/provenanced/cmd/docgen.go
- internal/antewrapper/flat_fee_gas_meter.go
- x/trigger/keeper/event_detector.go
- x/marker/keeper/keeper.go
- x/flatfees/keeper/keeper.go
- x/exchange/keeper/keeper.go
- x/metadata/keeper/objectstore.go
- x/name/keeper/keeper.go
- x/marker/types/si.go
- x/flatfees/client/cli/tx.go
- x/quarantine/testutil/test_helpers.go
- x/metadata/keeper/specification.go
- internal/handlers/msg_service_router.go
- x/marker/types/status.go
- x/ibcratelimit/keeper/rate_limit.go
- x/hold/keeper/grpc_query.go
- x/sanction/simulation/operations.go
- internal/antewrapper/post_handler.go
- .changelog/unreleased/dependencies/2346-upgrade-golangci-lint.md
- x/sanction/simulation/genesis.go
- x/flatfees/client/cli/query.go
- app/export.go
- x/marker/keeper/msg_server.go
- x/exchange/keeper/orders.go
- x/trigger/client/cli/tx.go
- contrib/devtools/Makefile
- x/hold/keeper/keeper.go
- testutil/network.go
- app/test_helpers.go
- x/attribute/types/keys.go
- internal/antewrapper/decorator_context_setup.go
- x/oracle/keeper/msg_server.go
- internal/pioconfig/provenanceconfig.go
- x/exchange/keeper/grpc_query.go
- x/marker/client/cli/tx.go
- x/metadata/keeper/session.go
- x/exchange/client/cli/flags.go
- x/name/client/cli/tx.go
- x/metadata/types/specification.go
- .github/workflows/lint.yml
- x/trigger/types/trigger.go
- cmd/provenanced/config/manager.go
- x/flatfees/simulation/genesis.go
- x/trigger/client/cli/query.go
- x/metadata/keeper/msg_server.go
- x/msgfees/keeper/query_server.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-15T22:53:55.013Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2422
File: x/ledger/keeper/msg_server.go:147-169
Timestamp: 2025-08-15T22:53:55.013Z
Learning: In the Provenance codebase, basic message validation (such as checking for empty required fields, nil checks, etc.) should be implemented in the ValidateBasic() method rather than in the message handlers. Message handlers should assume that ValidateBasic() has already performed basic validation.
Applied to files:
x/msgfees/types/msgs.go
📚 Learning: 2025-09-04T18:02:39.865Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2422
File: x/asset/keeper/msg_server.go:160-165
Timestamp: 2025-09-04T18:02:39.865Z
Learning: In Cosmos SDK message handlers, ValidateBasic() is called before the handler executes, so explicit nil checks in handlers are often redundant if the validation already covers those cases. The MsgCreatePool.ValidateBasic() method validates that Pool is non-nil and valid.
Applied to files:
x/msgfees/types/msgs.go
🧬 Code graph analysis (5)
x/msgfees/types/msgs.go (2)
x/msgfees/types/tx.pb.go (15)
MsgAssessCustomMsgFeeRequest(36-42)MsgAssessCustomMsgFeeRequest(46-46)MsgAssessCustomMsgFeeRequest(47-49)MsgAddMsgFeeProposalRequest(120-131)MsgAddMsgFeeProposalRequest(135-135)MsgAddMsgFeeProposalRequest(136-138)MsgUpdateMsgFeeProposalRequest(244-255)MsgUpdateMsgFeeProposalRequest(259-259)MsgUpdateMsgFeeProposalRequest(260-262)MsgRemoveMsgFeeProposalRequest(368-373)MsgRemoveMsgFeeProposalRequest(377-377)MsgRemoveMsgFeeProposalRequest(378-380)MsgUpdateConversionFeeDenomProposalRequest(566-571)MsgUpdateConversionFeeDenomProposalRequest(579-579)MsgUpdateConversionFeeDenomProposalRequest(580-582)internal/sdk/validator.go (1)
ValidateBasic(31-37)
internal/antewrapper/decorator_up_front_cost.go (1)
internal/antewrapper/utils.go (2)
BankKeeper(74-77)FeegrantKeeper(69-71)
x/oracle/keeper/icq.go (1)
x/oracle/types/keys.go (1)
PortID(19-19)
x/name/keeper/msg_server.go (1)
x/name/types/errors.go (1)
ErrNameNotBound(10-10)
x/trigger/simulation/genesis.go (1)
internal/rand/int.go (1)
IntBetween(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: tests (03)
- GitHub Check: tests (04)
- GitHub Check: test-race (03)
- GitHub Check: tests (00)
- GitHub Check: test-race (04)
- GitHub Check: tests (01)
- GitHub Check: tests (02)
- GitHub Check: test-race (00)
- GitHub Check: test-race (01)
- GitHub Check: test-race (02)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: heighliner-docker
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (48)
x/metadata/spec/examples/go/metadata_address.go (2)
10-11: LGTM: Import ordering improvement.The reordering of the
bech32import is a cosmetic change enforced by the updated golangci-lint configuration. This ensures consistent import grouping across the codebase.
150-151: LGTM: Appropriate lint suppression for fmt.Formatter.The nolint directive is correctly applied here. In
fmt.Formatterimplementations, ignoringWriteerrors is standard practice because the formatter infrastructure handles them internally. The comment clearly explains the rationale for the suppression.x/trigger/simulation/genesis.go (3)
34-34: LGTM! Safe integer conversions in randomization functions.The nolint directives correctly suppress false positives.
IntBetweenalways returns non-negative values within the specified ranges, making theint→uint64conversions safe.Also applies to: 39-39, 50-50
133-133: LGTM! Appropriate suppressions for random ID generation.Both conversions are safe:
- Line 133:
r.Perm(maxCount)returns values in[0, maxCount), all non-negative.- Line 142:
r.Intn(maxCount) + 1yields values in[1, maxCount], always positive.Also applies to: 142-142
174-183: LGTM! Bounds-checked conversion pattern.The nolint directive at line 174 appropriately covers the
int→uint64conversions on lines 175–176 (both values are fromr.Intn(6), i.e.,[0, 5]). The conversion at line 183 (uint64→int) is protected by the overflow check at lines 179–182, so no suppression is needed there.x/oracle/keeper/icq.go (1)
39-39: The int64→uint64 cast is safe becausectx.BlockTime()derives from blockchain timestamps (always ≥ the Unix epoch), sotimeoutTimestampcannot be negative and the//nolintsuppression is appropriate.x/flatfees/types/flatfees.go (4)
35-48: LGTM! Comprehensive cross-field validation.The Params.Validate() method properly validates both nested structures and enforces the cross-field constraint that DefaultCost.Denom must match ConversionFactor.DefinitionAmount.Denom. Error messages provide clear context for debugging.
58-68: LGTM! Constructor ensures valid sorted coins.The NewMsgFee constructor properly aggregates optional cost coins using the Add() method, which ensures the result is always valid and sorted as noted in the comment.
85-100: LGTM! Thorough validation with proper nil handling.The MsgFee.Validate() method includes appropriate nil checks and validates both the MsgTypeUrl and Cost fields with clear error messages.
114-135: LGTM! Proper validation with consistency enforcement.The ConversionFactor.Validate() method correctly enforces non-zero amounts for both fields and includes a consistency check (lines 130-132) ensuring that when denoms match, the amounts must also be equal. This prevents invalid conversion factors like "1musd → 2musd".
x/flatfees/types/msgs.go (3)
16-25: LGTM! Proper authority validation and nested Params validation.The ValidateBasic() method correctly validates the authority address and delegates to Params.Validate() for comprehensive parameter validation. This aligns with the learning that basic validation should be implemented in ValidateBasic() rather than handlers.
Based on learnings.
27-36: LGTM! Proper authority validation and nested ConversionFactor validation.The ValidateBasic() method correctly validates the authority address and delegates to ConversionFactor.Validate() for comprehensive conversion factor validation.
38-74: LGTM! Comprehensive validation with thorough duplicate detection.The ValidateBasic() method implements excellent validation logic:
- Ensures at least one operation is provided (line 44-46)
- Validates each ToSet entry and detects duplicates within ToSet (lines 48-57)
- Validates each ToUnset entry and detects duplicates both within ToUnset and across ToSet/ToUnset (lines 59-71)
The cross-list duplicate detection (lines 67-69) is particularly important to prevent conflicting operations on the same message type in a single request.
x/metadata/keeper/scope.go (3)
21-21: LGTM! Iterator close suppressions are appropriate.The
nolint:errchecksuppressions for iterator close operations in these read-only iteration functions are appropriate. Iterator close errors are typically non-critical in these contexts where iterators are created, fully consumed, and then closed within the function scope.Also applies to: 37-38, 58-59, 906-907
191-195: Excellent improvement! Logging close errors instead of suppressing.This change is better than simply suppressing the error. By wrapping the close in a deferred function that logs on error, you maintain observability while acknowledging that iterator close errors don't warrant propagating up the call stack in this cleanup path.
875-875: LGTM! Safe integer conversion.The
gosecG115 suppression is appropriate here. ConvertingBlockHeight()fromint64touint64is safe in this context because block heights in Cosmos SDK are never negative (they start at 0 and only increment).internal/antewrapper/decorator_fee_setup.go (2)
12-13: LGTM!The doc comment is clear, follows Go conventions, and properly documents the constructor.
17-18: LGTM!The doc comment is clear, follows Go conventions, and properly documents the AnteHandle method.
internal/antewrapper/decorator_up_front_cost.go (2)
24-25: LGTM!The doc comment is clear, follows Go conventions, and properly documents the constructor with its keeper dependencies.
29-30: LGTM!The doc comment is clear, follows Go conventions, and properly documents the AnteHandle method implementation.
x/sanction/keeper/keeper.go (1)
154-154: LGTM! Standard lint suppressions for iterator cleanup.The
nolint:errcheckdirectives on deferred iteratorClose()calls are appropriate. Iterator close errors in read-only operations don't affect correctness, and the explanatory comments document the rationale clearly.Also applies to: 216-216, 243-243, 271-271, 332-332
x/quarantine/keeper/keeper.go (1)
79-79: LGTM! Consistent lint suppressions for iterator cleanup.The
nolint:errcheckdirectives on deferred iteratorClose()calls follow the same pattern as other keeper files. The explanatory comments clearly document why these errors can be safely ignored.Also applies to: 152-152, 340-340
x/attribute/keeper/msg_server.go (1)
48-48: LGTM! Using promoted method is cleaner.The change from
k.Keeper.SetAttributetok.SetAttributeuses the promoted method from the embeddedKeeper. This is more idiomatic and aligns with the broader refactoring pattern applied throughout the PR.x/attribute/keeper/keeper.go (5)
363-363: LGTM! Standard iterator cleanup pattern.The deferred
Close()withnolint:errcheckis appropriate for this read-only iteration.
398-398: LGTM! Proper iterator cleanup with nil guard.The iterator is explicitly closed on line 413 and set to nil, preventing the deferred cleanup from double-closing. The
nolint:errcheck,gosecdirectives are appropriate.Also applies to: 413-413
475-475: LGTM! Standard iterator cleanup.The deferred
Close()withnolint:errcheckfollows the established pattern.
489-489: LGTM! Consistent iterator cleanup.The deferred
Close()withnolint:errcheckis appropriate.
542-542: LGTM! Explicit cleanup after iteration.The immediate
Close()on line 542 is appropriate since it occurs after the iteration loop completes. Thenolint:errcheck,gosecdirective is acceptable here.x/name/keeper/msg_server.go (4)
40-40: LGTM! Consistent use of promoted methods in BindName.All method calls correctly use the promoted methods from the embedded
Keeper(s.GetRecordByName,s.ResolvesTo,s.Normalize,s.NameExists,s.SetNameRecord), making the code cleaner and more idiomatic.Also applies to: 52-52, 58-58, 63-63, 73-73
107-107: LGTM! Consistent promoted method usage in DeleteName.All method calls properly use promoted methods, including the attribute keeper access through
s.attrKeeper.Also applies to: 119-119, 124-124, 129-129, 136-136
165-165: LGTM! Promoted methods used consistently in ModifyName.The method calls correctly use promoted methods for record retrieval, authority checking, and record updates.
Also applies to: 170-171, 179-179
190-191: LGTM! Authority check uses promoted method.The authority check correctly uses
s.GetAuthority()instead ofs.Keeper.GetAuthority().app/app.go (8)
412-412: LGTM! Using promoted streaming services registration.The change from
app.BaseApp.RegisterStreamingServicestoapp.RegisterStreamingServicesuses the promoted method from the embeddedBaseApp, which is cleaner.
470-470: LGTM! Circuit breaker uses promoted method.The change to
app.SetCircuitBreakerproperly uses the promoted method fromBaseApp.
509-509: LGTM! GroupKeeper uses promoted MsgServiceRouter.The change to
app.MsgServiceRouter()properly accesses the promoted method.
625-625: LGTM! ICQKeeper uses promoted GRPCQueryRouter.The change to
app.GRPCQueryRouter()correctly uses the promoted method.
704-704: LGTM! GovKeeper uses promoted MsgServiceRouter.The change to
app.MsgServiceRouter()is consistent with the refactoring pattern.
948-948: LGTM! Configurator uses promoted routers.The change to use
app.MsgServiceRouter()andapp.GRPCQueryRouter()properly accesses the promoted methods.
1252-1252: LGTM! RegisterTxService uses promoted GRPCQueryRouter.The change to
app.GRPCQueryRouter()is consistent with the refactoring pattern.
1284-1284: LGTM! RegisterTendermintService uses promoted GRPCQueryRouter.The change to
app.GRPCQueryRouter()properly uses the promoted method.testutil/ibc/testchain.go (1)
164-164: LGTM! Appropriate use of deterministic RNG in test code.Using a fixed seed (
rand.NewSource(1)) for test transaction generation ensures reproducible test behavior. The nolint suppression is justified here, as cryptographically secure randomness is not required for test utilities.testutil/mocks/codec.go (1)
61-119: Approve: no concurrent MockCodec usage detectedSearch of
*_test.gofiles found not.Parallel()or goroutines usingMockCodec; the error-slice mutation is safe in current tests.x/trigger/keeper/event_listener.go (1)
46-46: LGTM! Lint suppression applied as requested.The
nolint:errcheckdirective appropriately suppresses the error check warning for the deferred iterator close, consistent with the past review guidance and the broader pattern across trigger-related files in this PR.x/trigger/keeper/migrations_v2.go (2)
32-32: LGTM! Blank line improves readability.The added blank line appropriately separates the iterator initialization from the cleanup function definition.
35-35: LGTM! Lint suppression applied as requested.The
nolint:errcheck,gosecdirective appropriately suppresses warnings for the iterator close operation in this cleanup context, consistent with past review guidance.cmd/provenanced/cmd/root.go (3)
62-62: LGTM! Appropriate suppression for best-effort cleanup.The
nolint:errcheckdirective is correct for this deferred temp directory cleanup—failure here is non-critical since the directory is temporary.
271-271: LGTM! Permission 0o755 is safe and intended.The
nolint:gosecdirective correctly suppresses G301 for this standard directory permission, which is appropriate for a snapshot directory.
294-298: LGTM! Safe file operations with appropriate suppressions.Both suppressions are correct:
- Line 294: G304 suppressed because the genesis.json path is constructed from controlled inputs
- Line 298: errcheck suppressed because close errors on read-only file handles during genesis parsing are non-critical
| applyAllowedAddrs := false | ||
|
|
||
| // check if there is a allowed address list | ||
| if len(jailAllowedAddrs) > 0 { | ||
| applyAllowedAddrs = true | ||
| } | ||
| applyAllowedAddrs = len(jailAllowedAddrs) > 0 |
There was a problem hiding this comment.
No need to set this to false and then to something else immediately after.
| ) (*abci.ResponseFinalizeBlock, error) { | ||
| tx, err := simtestutil.GenSignedMockTx( | ||
| rand.New(rand.NewSource(1)), | ||
| rand.New(rand.NewSource(1)), //nolint:gosec // G304 |
There was a problem hiding this comment.
G304 doesn't make sense for this line, is that the right thing to put here?
G304 is about a tained filepath being used, but this is just a random number generator line.
| defer iterator.Close() | ||
|
|
||
| iterator.Close() //nolint:errcheck,gosec // close error safe to ignore in this context | ||
|
|
There was a problem hiding this comment.
issue (blocking): This still needs to be a defer.
| key := AttributeExpirationKeyPrefix | ||
| expireTimeBz := make([]byte, 8) | ||
| binary.BigEndian.PutUint64(expireTimeBz, uint64(expireTime.Unix())) | ||
| binary.BigEndian.PutUint64(expireTimeBz, uint64(expireTime.Unix())) //nolint:gosec // G304: safe fixed size buffer write. |
There was a problem hiding this comment.
Here too, I don't think this is a G304, is it?
Description
closes: Bump golangci-lint and golangci/golangci-lint-action #2346
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Behavioral / Validation
Bug Fixes
Documentation
Chores