Ci/golangci lint v2 #922
Ci/golangci lint v2 #922
Conversation
WalkthroughThis pull request updates and refactors several components across the project. The linting tool configuration is updated via workflow and Makefile modifications. The linter configuration file (.golangci.yml) is restructured, introducing explicit versioning, reordering linter settings, and adding new exclusion and formatter sections. In core application files, method calls are refactored to use the main App instance directly instead of its BaseApp subcomponent. Error handling throughout the codebase is standardized by renaming and updating error constants. Additionally, vesting account operations and related tests are revised to align with the new direct method invocation patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App
participant BaseApp
Client->>App: Request Operation
alt Old Flow
App->>BaseApp: Invoke method (e.g., SetCircuitBreaker, NewContext, GRPCQueryRouter)
BaseApp-->>App: Return result
else New Flow
App->>App: Invoke method directly (e.g., SetCircuitBreaker, NewContext, GRPCQueryRouter)
end
App-->>Client: Response
sequenceDiagram
participant User
participant MsgServer
participant Bank as BankKeeper/AccountKeeper
User->>MsgServer: Create Vesting Account Request
alt Old Flow
MsgServer->>Bank: Call IsSendEnabledCoins, GetAccount, NewAccount, SetAccount, SendCoins
Bank-->>MsgServer: Processed account data
else New Flow
MsgServer->>MsgServer: Directly perform account operations
end
MsgServer-->>User: Vesting Account Created Response
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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. 🪧 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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #922 +/- ##
==========================================
+ Coverage 45.06% 45.08% +0.02%
==========================================
Files 113 113
Lines 6402 6399 -3
==========================================
Hits 2885 2885
+ Misses 3382 3379 -3
Partials 135 135
🚀 New features to boost your workflow:
|
![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)
.golangci.yml (1)
98-102: Updated Issues Reporting Configuration
The issues section now setsuniq-by-line: false, which will list all reported issues without deduplication per line. Verify that this behavior is acceptable for your team's review process, as it may result in multiple similar messages for a single line.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/lint.yml(1 hunks).golangci.yml(2 hunks)Makefile(1 hunks)app/app.go(3 hunks)app/export.go(2 hunks)x/logic/keeper/grpc_query_ask.go(2 hunks)x/logic/keeper/interpreter.go(2 hunks)x/logic/types/errors.go(1 hunks)x/logic/util/prolog.go(3 hunks)x/mint/module_test.go(1 hunks)x/vesting/msg_server.go(9 hunks)x/vesting/types/vesting_account.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/mint/module_test.gox/logic/util/prolog.gox/logic/keeper/interpreter.gox/logic/types/errors.gox/logic/keeper/grpc_query_ask.goapp/export.goapp/app.gox/vesting/types/vesting_account.gox/vesting/msg_server.go
`**/*_test.go`: Review tests to ensure they follow GoConvey style, using a behavioral and data-driven approach.
**/*_test.go: Review tests to ensure they follow GoConvey style, using a behavioral and data-driven approach.
x/mint/module_test.go
🧬 Code Definitions (4)
x/logic/util/prolog.go (1)
x/logic/types/errors.go (2)
ErrInvalidArgument(10-10)ErrLimitExceeded(12-12)
x/logic/keeper/interpreter.go (1)
x/logic/types/errors.go (3)
ErrInternal(14-14)ErrInvalidArgument(10-10)ErrLimitExceeded(12-12)
x/logic/keeper/grpc_query_ask.go (2)
x/logic/types/errors.go (2)
ErrInvalidArgument(10-10)ErrLimitExceeded(12-12)x/logic/prolog/context.go (1)
UnwrapSDKContext(15-24)
x/vesting/msg_server.go (1)
x/vesting/types/vesting_account.go (4)
NewPermanentLockedAccount(478-488)NewPeriodicVestingAccount(279-302)NewBaseVestingAccount(30-40)NewCliffVestingAccountRaw(543-549)
🪛 GitHub Check: codecov/patch
x/logic/util/prolog.go
[warning] 38-38: x/logic/util/prolog.go#L38
Added line #L38 was not covered by tests
[warning] 55-55: x/logic/util/prolog.go#L55
Added line #L55 was not covered by tests
[warning] 61-61: x/logic/util/prolog.go#L61
Added line #L61 was not covered by tests
[warning] 68-68: x/logic/util/prolog.go#L68
Added line #L68 was not covered by tests
[warning] 79-79: x/logic/util/prolog.go#L79
Added line #L79 was not covered by tests
x/logic/keeper/interpreter.go
[warning] 55-55: x/logic/keeper/interpreter.go#L55
Added line #L55 was not covered by tests
app/export.go
[warning] 55-55: app/export.go#L55
Added line #L55 was not covered by tests
[warning] 67-67: app/export.go#L67
Added line #L67 was not covered by tests
app/app.go
[warning] 444-444: app/app.go#L444
Added line #L444 was not covered by tests
[warning] 907-907: app/app.go#L907
Added line #L907 was not covered by tests
[warning] 1139-1139: app/app.go#L1139
Added line #L1139 was not covered by tests
[warning] 1147-1147: app/app.go#L1147
Added line #L1147 was not covered by tests
🔇 Additional comments (47)
x/logic/keeper/interpreter.go (3)
58-58: Consistent error naming convention applied.The change from
types.InvalidArgumenttotypes.ErrInvalidArgumentmaintains consistent error naming patterns throughout the codebase.
186-187: Error type standardization for out-of-gas scenarios.The error type
types.ErrLimitExceedednow properly follows Go's error naming convention with theErrprefix, making the code more readable and maintainable.
55-55:✅ Verification successful
Appropriate error type renaming for improved clarity.
The change from
types.Internaltotypes.ErrInternalfollows standard Go conventions for error naming. This enhances code readability by clearly indicating this variable represents an error.
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for this error path # Test: Check if test coverage exists for this error path ast-grep --pattern 'errorsmod.Wrapf(types.ErrInternal, "error creating interpreter: %v", err.Error())'Length of output: 233
Final Confirmation: Error Naming is Enhanced and Test Coverage Verified
The renaming from
types.Internaltotypes.ErrInternaladheres to standard Go conventions, making it immediately clear that the variable represents an error. The test coverage for this specific error path has been confirmed in line 55 ofx/logic/keeper/interpreter.go.
- File:
x/logic/keeper/interpreter.go- Line: 55
- Change:
return nil, errorsmod.Wrapf(types.ErrInternal, "error creating interpreter: %v", err.Error())- Impact: Improves code readability and clarity regarding error handling.
I recommend proceeding with this change.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-55: x/logic/keeper/interpreter.go#L55
Added line #L55 was not covered by testsx/logic/keeper/grpc_query_ask.go (3)
17-17: Standardized error naming for nil request checks.The change from
types.InvalidArgumenttotypes.ErrInvalidArgumentfollows Go's error naming best practices, making the code more consistent and readable.
25-26: Proper error naming convention in panic recovery.The error type for out-of-gas scenarios now correctly uses the
Errprefix, improving code clarity while maintaining the same error semantics.
52-52: Consistent error type usage for limit validation.The change from
types.LimitExceededtotypes.ErrLimitExceededensures uniform error naming throughout the application, enhancing maintainability.x/logic/util/prolog.go (5)
38-38: Standardized error type for query execution errors.Error variable naming now follows Go's convention with the
Errprefix, making the code more consistent with standard practices.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-38: x/logic/util/prolog.go#L38
Added line #L38 was not covered by tests
55-55: Error type standardization for result processing errors.The error type has been updated to use the
Errprefix, maintaining consistency with the error naming pattern applied throughout the codebase.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-55: x/logic/util/prolog.go#L55
Added line #L55 was not covered by tests
61-63: Updated error checking for limit exceeded conditions.The
errors.Is()check now correctly references the renamed error constanttypes.ErrLimitExceeded, ensuring error type checking remains functional after the renaming.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-61: x/logic/util/prolog.go#L61
Added line #L61 was not covered by tests
68-69: Consistent error type usage for panic errors.The error wrapping now uses the standardized
types.ErrLimitExceedederror type, maintaining consistency with the naming convention used throughout the codebase.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 68-68: x/logic/util/prolog.go#L68
Added line #L68 was not covered by tests
79-80: Standardized error handling for out-of-gas scenarios.The error wrapping now correctly uses the renamed
types.ErrLimitExceedederror type, ensuring consistent error naming across all gas limit handling code.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-79: x/logic/util/prolog.go#L79
Added line #L79 was not covered by testsx/logic/types/errors.go (3)
10-10: Improved error variable naming to follow standard conventions.Renaming
InvalidArgumenttoErrInvalidArgumentfollows Go's common convention of prefixing error variables withErr, making it immediately clear that this variable represents an error.
12-12: Standardized error naming for limit exceeded errors.The variable has been renamed from
LimitExceededtoErrLimitExceededto follow Go's error naming convention, improving code readability.
14-14: Consistent error variable naming for internal errors.Renaming
InternaltoErrInternalfollows the same pattern applied to other error variables, ensuring consistency throughout the codebase.x/mint/module_test.go (1)
29-29: Method call updated for consistencyThe change from
app.BaseApp.NewContext(false)toapp.NewContext(false)improves code consistency by directly using methods on the App instance rather than accessing them through BaseApp.app/export.go (2)
55-55: Direct method invocation aligns with app architectureThe change from
app.BaseApp.GetConsensusParams(ctx)toapp.GetConsensusParams(ctx)is part of a broader refactoring that favors direct method invocation on the App instance rather than through BaseApp.This line is not covered by tests according to static analysis. Consider adding test coverage to ensure the behavior is maintained after this refactoring.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-55: app/export.go#L55
Added line #L55 was not covered by tests
67-67: Simplified boolean expressionThe code has been refactored to directly derive the boolean value from the condition rather than using a multi-line if statement, which improves readability and reduces line count.
This line is not covered by tests according to static analysis. Consider adding test coverage to ensure the behavior is maintained after this refactoring.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-67: app/export.go#L67
Added line #L67 was not covered by testsapp/app.go (4)
444-444: Direct method invocation patternUsing
app.SetCircuitBreakerinstead ofapp.BaseApp.SetCircuitBreakermaintains consistency with the refactoring pattern across the codebase.This line is not covered by tests according to static analysis. Consider adding test coverage for this critical circuit breaker functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 444-444: app/app.go#L444
Added line #L444 was not covered by tests
907-907: Consistent context creationThe change from
app.BaseApp.NewUncachedContexttoapp.NewUncachedContextfollows the same refactoring pattern of removing BaseApp intermediate calls.This line is not covered by tests according to static analysis. Consider adding test coverage for application initialization with uncached contexts.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 907-907: app/app.go#L907
Added line #L907 was not covered by tests
1139-1139: Simplified service registrationUsing
app.GRPCQueryRouter()directly rather thanapp.BaseApp.GRPCQueryRouter()simplifies the code and makes it more maintainable.This line is not covered by tests according to static analysis. Consider adding tests for service registration functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1139-1139: app/app.go#L1139
Added line #L1139 was not covered by tests
1147-1147: Consistent router access patternUsing
app.GRPCQueryRouter()directly instead ofapp.BaseApp.GRPCQueryRouter()maintains consistency with the refactoring approach throughout the codebase.This line is not covered by tests according to static analysis. Consider adding test coverage for Tendermint service registration.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1147-1147: app/app.go#L1147
Added line #L1147 was not covered by testsx/vesting/types/vesting_account.go (5)
237-237: Direct method access through embeddingChanged from accessing
LockedCoinsFromVestingthrough theBaseVestingAccountto using it directly through Go's embedding mechanism. This improves code readability and eliminates an unnecessary method call indirection.
346-346: Direct method access through embeddingChanged from accessing
LockedCoinsFromVestingthrough theBaseVestingAccountto using it directly through Go's embedding mechanism, consistent with the pattern applied to other vesting account types.
449-449: Direct method access through embeddingChanged from accessing
LockedCoinsFromVestingthrough theBaseVestingAccountto using it directly through Go's embedding mechanism, maintaining consistency with other account type implementations.
505-505: Direct method access through embeddingChanged from accessing
LockedCoinsFromVestingthrough theBaseVestingAccountto using it directly through Go's embedding mechanism, following the same pattern as other vesting account implementations.
607-607: Direct method access through embeddingChanged from accessing
LockedCoinsFromVestingthrough theBaseVestingAccountto using it directly through Go's embedding mechanism, completing the consistent pattern across all vesting account implementations.x/vesting/msg_server.go (15)
55-55: Refactor to direct method call.The change replaces
s.BankKeeper.IsSendEnabledCoinswith a direct call tos.IsSendEnabledCoins. This simplifies the code and reduces the dependency on the BankKeeper intermediary component, aligning with modern Go practices for interface implementation.
59-59: Simplified address verification.Changed from
s.BankKeeper.BlockedAddr(to)to a direct call tos.BlockedAddr(to). This refactoring maintains functionality while making the code more concise and reducing the reference path depth.
63-63: Direct account retrieval.Modified from
s.AccountKeeper.GetAccount(ctx, to)tos.GetAccount(ctx, to). This change is part of a consistent pattern of interface simplification throughout the file, making the code more maintainable.
68-68: Direct account creation.Changed from using
s.AccountKeeper.NewAccountto the embedded implementation vias.NewAccount. This modification maintains the type assertion to*authtypes.BaseAccountwhile simplifying the method call.
81-81: Simplified account storage.The refactoring from
s.AccountKeeper.SetAccounttos.SetAccountfollows the pattern of direct method invocation, reducing unnecessary verbosity in the codebase.
97-97: Direct coin transfer.Modified from
s.BankKeeper.SendCoinstos.SendCoins. This change completes the pattern of direct method invocation for this particular account creation flow, simplifying the code.
122-122: Consistent call pattern in second method.The same refactoring pattern is applied consistently in the
CreatePermanentLockedAccountmethod, changing froms.BankKeeper.IsSendEnabledCoinstos.IsSendEnabledCoins. This ensures a uniform approach throughout the codebase.
126-126: Blocked address check simplified.Consistent with previous changes, the code now uses
s.BlockedAddr(to)instead ofs.BankKeeper.BlockedAddr(to)in theCreatePermanentLockedAccountmethod.
130-130: Direct account existence check.Refactored from
s.AccountKeeper.GetAccounttos.GetAccountin theCreatePermanentLockedAccountmethod, maintaining consistency with the pattern established in the first method.
135-135: Consistent account creation.Changed from
s.AccountKeeper.NewAccounttos.NewAccountin theCreatePermanentLockedAccountmethod, ensuring consistent method call patterns across different account creation workflows.
141-141: Simplified account storage in second method.The pattern of using
s.SetAccountinstead ofs.AccountKeeper.SetAccountis consistently applied in theCreatePermanentLockedAccountmethod.
157-157: Direct coin transfer in second method.Modified from
s.BankKeeper.SendCoinstos.SendCoinsin theCreatePermanentLockedAccountmethod, completing the consistent application of the refactoring pattern.
196-196: Consistent refactoring in periodic vesting.All method calls in the
CreatePeriodicVestingAccountfunction have been refactored to use direct invocation rather than through keepers. This maintains the established pattern and improves code consistency.Also applies to: 201-201, 205-205, 210-210, 216-216, 232-232
265-265: Complete refactoring in cliff vesting.The
CreateCliffVestingAccountmethod follows the same pattern of refactoring, with all keeper method calls replaced by direct invocations. This ensures a uniform approach across all vesting account creation methods.Also applies to: 269-269, 273-273, 278-278, 286-286, 302-302
19-22: Improved struct composition.The
msgServerstruct correctly embeds bothkeeper.AccountKeeperandtypes.BankKeeper, enabling the direct method calls seen throughout the file. This is a clean use of Go's structural typing that reduces code verbosity while maintaining functionality..github/workflows/lint.yml (1)
109-110: Update of golangci-lint Action Version to v2.0
This change correctly updates the linting action to use version v2.0. Please ensure that all new linter rules in the updated version are aligned with the project's expectations as defined in the associated configuration files.Makefile (1)
14-14: Update Docker Image for GolangCI Lint
The Docker image for golangci-lint has been updated togolangci/golangci-lint:v2.0, which aligns with the updated CI configuration in the workflow file. Confirm that this image version is available and that its linting behavior meets project requirements..golangci.yml (4)
1-3: Set Configuration Version and Default Linters
The configuration now explicitly setsversion: "2"and definesdefault: nonein the linters section. This ensures that only the explicitly enabled linters will run under golangci-lint v2.0, providing more precise control over linting rules.
48-62: Refined Linter Settings
The settings for linters such ascyclop,funlen,godot,lll, andtagliatellehave been updated with clear thresholds (e.g., max-complexity of 20 and funlen statements set to 65). This adjustment will help enforce consistent code quality standards across the project.
63-97: Enhanced Exclusion Rules
A comprehensive exclusions section has been introduced. It includes presets, custom rule definitions, and explicit path filters (e.g., ignoringthird_party,builtin, andexamples). These rules are designed to reduce false positives from generated or irrelevant files, thus focusing the linting process on meaningful source code.
102-125: New Formatters Configuration for Import Organization
The formatters section now enables bothgciandgoimportswith specific settings. The custom order for import sections and the defined exclusion paths (e.g., forthird_party,builtin, andexamples) aim to standardize code formatting effectively.
Adopt golangci-lint v2.0 - because it’s cute, strict, and just wants the best for our code.
Applied config migration and updated codebase to comply with the new linter rules.
Summary by CodeRabbit
Chores
Refactor