refactor(x/feegrant): set environment in context #20529
Conversation
WalkthroughWalkthroughThe recent updates focus on modifying context handling and header information retrieval across various files in the Changes
Sequence Diagram(s) (Beta)Not applicable for this set of changes. Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- x/feegrant/basic_fee.go (2 hunks)
- x/feegrant/filtered_fee.go (3 hunks)
- x/feegrant/keeper/keeper.go (2 hunks)
- x/feegrant/periodic_fee.go (2 hunks)
Additional context used
Path-based instructions (4)
x/feegrant/basic_fee.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/feegrant/filtered_fee.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/feegrant/periodic_fee.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/feegrant/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (7)
x/feegrant/basic_fee.go (3)
9-10: The changes to the imports and the context handling in theAcceptmethod align with the PR's goal to avoid usingsdk.UnwrapSDKContextand instead use the newcorecontext.EnvironmentContextKey. This is a good application of the proposed solution to use module-specific context keys.Also applies to: 28-33
30-33: The use oferrorsmod.Wrapfor error handling in theAcceptmethod is appropriate and enhances the clarity and maintainability of error management. The error messages are specific and informative, aiding in debugging and user feedback.
Line range hint
43-62: TheValidateBasicmethod's implementation is robust, ensuring that theSpendLimitandExpirationare valid. The use of detailed error messages witherrorsmod.Wrapanderrorsmod.Wrapfenhances error clarity, which is crucial for debugging and user feedback.
[APROVED]x/feegrant/filtered_fee.go (2)
11-12: The changes to the imports and the context handling in theallowedMsgsToMapandallMsgTypesAllowedmethods align with the PR's goal to avoid usingsdk.UnwrapSDKContextand instead use the newcorecontext.EnvironmentContextKey. The integration of the gas meter through the environment is a good application of the proposed solution to use module-specific context keys.Also applies to: 95-101, 110-116
Line range hint
72-89: The use oferrorsmod.Wrapfor error handling in theAcceptmethod is appropriate and enhances the clarity and maintainability of error management. The error messages are specific and informative, aiding in debugging and user feedback.x/feegrant/periodic_fee.go (1)
9-10: The changes to the imports and the context handling in theAcceptmethod align with the PR's goal to avoid usingsdk.UnwrapSDKContextand instead use the newcorecontext.EnvironmentContextKey. This is a good application of the proposed solution to use module-specific context keys.Also applies to: 28-32
x/feegrant/keeper/keeper.go (1)
236-236: The change to the context handling in theUseGrantedFeesmethod aligns with the PR's goal to avoid usingsdk.UnwrapSDKContextand instead use the newcorecontext.EnvironmentContextKey. This is a good application of the proposed solution to use module-specific context keys.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
x/feegrant/basic_fee_test.go
Outdated
| storetypes "cosmossdk.io/store/types" | ||
| "cosmossdk.io/x/feegrant" | ||
|
|
||
| corecontext "cosmossdk.io/core/context" | ||
| "github.com/cosmos/cosmos-sdk/runtime" |
There was a problem hiding this comment.
we are trying to avoid runtime/v2 imports in modules. IMHO we should do the same for runtime v0
There was a problem hiding this comment.
Can we just add mocks service instead? And when #20487 is merged, we can add those mocks there and use them here instead.
There was a problem hiding this comment.
got it, makes sense to avoid dependency import
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
x/feegrant/basic_fee.go (1)
Line range hint
16-16: Resolve undefined references toBasicAllowanceand interfaceFeeAllowanceI.Ensure that
BasicAllowanceandFeeAllowanceIare properly defined and imported in this file, as they are referenced but not defined within the visible scope.Also applies to: 28-28, 52-52, 70-70, 75-75
x/feegrant/CHANGELOG.md (1)
Line range hint
66-66: Correct the typographical error in the changelog.- ValidateBasic` is treated as a no op now with with acceptance of RFC001 + ValidateBasic` is treated as a no op now with acceptance of RFC001Remove the repeated word "with" to improve the readability of the changelog entry.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
x/feegrant/go.modis excluded by!**/*.mod
Files selected for processing (6)
- x/feegrant/CHANGELOG.md (1 hunks)
- x/feegrant/basic_fee.go (2 hunks)
- x/feegrant/basic_fee_test.go (2 hunks)
- x/feegrant/filtered_fee.go (3 hunks)
- x/feegrant/filtered_fee_test.go (3 hunks)
- x/feegrant/periodic_fee_test.go (2 hunks)
Additional context used
Path-based instructions (6)
x/feegrant/basic_fee.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/feegrant/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/feegrant/basic_fee_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/feegrant/filtered_fee.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/feegrant/filtered_fee_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/feegrant/periodic_fee_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
golangci-lint
x/feegrant/basic_fee.go
16-16: undefined: FeeAllowanceI
28-28: undefined: BasicAllowance
52-52: undefined: BasicAllowance
70-70: undefined: BasicAllowance
75-75: undefined: BasicAllowance
35-35: undefined: ErrFeeLimitExpired
41-41: undefined: ErrFeeLimitExceeded
63-63: undefined: ErrInvalidDuration
x/feegrant/filtered_fee.go
26-26: undefined: FeeAllowanceI
27-27: undefined: AllowedMsgAllowance
31-31: undefined: AllowedMsgAllowance
37-37: undefined: FeeAllowanceI
54-54: undefined: AllowedMsgAllowance
64-64: undefined: AllowedMsgAllowance
75-75: undefined: AllowedMsgAllowance
98-98: undefined: AllowedMsgAllowance
115-115: undefined: AllowedMsgAllowance
138-138: undefined: AllowedMsgAllowance
155-155: undefined: AllowedMsgAllowance
164-164: undefined: AllowedMsgAllowance
32-32: undefined: FeeAllowanceI
47-47: undefined: AllowedMsgAllowance
57-57: undefined: ErrNoAllowance
81-81: undefined: ErrMessageNotAllowed
140-140: undefined: ErrNoAllowance
143-143: undefined: ErrNoMessages
8-8: "github.com/cosmos/gogoproto/proto" imported and not used
LanguageTool
x/feegrant/CHANGELOG.md
[duplication] ~66-~66: Possible typo: you repeated a word
Context: ...alidateBasic` is treated as a no op now with with acceptance of RFC001 * [#17869](https:/...
Additional comments not posted (5)
x/feegrant/CHANGELOG.md (1)
34-34: Ensure the changelog entry accurately reflects the changes made.The changelog correctly notes the requirement for the
authz environmentin thecontext.Contextfor theAcceptmethod on theFeeAllowanceIinterface. This aligns with the changes made in the PR.x/feegrant/basic_fee_test.go (1)
144-146: Ensure the environment is correctly set up in the test context.The test correctly sets up the environment using
runtime.NewEnvironmentand passes it into the context. This is crucial for ensuring that the unit tests accurately reflect the new context handling introduced in the PR.x/feegrant/filtered_fee.go (1)
76-80: Properly handle errors when messages are not allowed.The code correctly handles the scenario where a message is not allowed by returning an appropriate error. This ensures that the function behaves as expected in cases where the conditions for message allowance are not met.
x/feegrant/filtered_fee_test.go (1)
Line range hint
152-164: Ensure the environment is correctly set up in the test context.The test correctly sets up the environment using
runtime.NewEnvironmentand passes it into the context. This is crucial for ensuring that the unit tests accurately reflect the new context handling introduced in the PR.x/feegrant/periodic_fee_test.go (1)
225-228: Ensure the environment is correctly set up in the test context.The test correctly sets up the environment using
runtime.NewEnvironmentand passes it into the context. This is crucial for ensuring that the unit tests accurately reflect the new context handling introduced in the PR.
| "cosmossdk.io/core/appmodule" | ||
| corecontext "cosmossdk.io/core/context" |
There was a problem hiding this comment.
Ensure proper error handling when the environment is not set in the context.
- if !ok {
- return false, fmt.Errorf("environment not set")
+ if !ok {
+ return false, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "environment not set in context")This change aligns error handling with Cosmos SDK's standard practices by using sdkerrors.Wrap for consistency and clarity.
Also applies to: 29-34
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/core/appmodule" | |
| corecontext "cosmossdk.io/core/context" | |
| "cosmossdk.io/core/appmodule" | |
| corecontext "cosmossdk.io/core/context" | |
| // ... other code ... | |
| if !ok { | |
| return false, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "environment not set in context") | |
| } |
| return false, fmt.Errorf("environment not set") | ||
| } | ||
| headerInfo := environment.HeaderService.HeaderInfo(ctx) | ||
| if a.Expiration != nil && a.Expiration.Before(headerInfo.Time) { | ||
| return true, errorsmod.Wrap(ErrFeeLimitExpired, "basic allowance") |
There was a problem hiding this comment.
Correct the undefined error constants used in the file.
- errorsmod.Wrap(ErrFeeLimitExpired, "basic allowance")
+ errorsmod.Wrap(sdkerrors.ErrFeeLimitExpired, "basic allowance")
- errorsmod.Wrap(ErrFeeLimitExceeded, "basic allowance")
+ errorsmod.Wrap(sdkerrors.ErrFeeLimitExceeded, "basic allowance")
- errorsmod.Wrap(ErrInvalidDuration, "expiration time cannot be negative")
+ errorsmod.Wrap(sdkerrors.ErrInvalidDuration, "expiration time cannot be negative")Replace the undefined error constants with the correct ones from the sdkerrors package to ensure that the code compiles and the errors are correctly categorized.
Also applies to: 41-41, 63-63
Committable suggestion was skipped due low confidence.
Tools
golangci-lint
35-35: undefined: ErrFeeLimitExpired
Tip
Codebase Verification
The search results show numerous instances where HeaderInfo().Time is accessed, but there are no explicit nil checks for HeaderInfo in the surrounding code. This indicates a potential risk of nil pointer dereference.
To address this, it is crucial to ensure that HeaderInfo is always non-nil before accessing its Time property. Here are the steps to mitigate this risk:
- Add nil checks: Ensure that
HeaderInfois checked for nil before accessing itsTimeproperty in all relevant parts of the code. - Refactor code: Refactor the code to handle cases where
HeaderInfomight be nil, providing appropriate fallback mechanisms or error handling.
Locations to Address:
x/feegrant/basic_fee.goline 35- Any other instances where
HeaderInfo().Timeis accessed without a nil check.
Suggested Code Change:
Before accessing HeaderInfo().Time, add a nil check like this:
headerInfo := ctx.HeaderInfo()
if headerInfo == nil {
return false, errors.New("headerInfo is nil")
}
return true, errorsmod.Wrap(ErrFeeLimitExpired, "basic allowance")Action Required:
- Review and update the code to include nil checks for
HeaderInfobefore accessing itsTimeproperty. - Test the changes to ensure that they handle nil cases gracefully and do not introduce new issues.
Analysis chain
Handle potential nil pointer dereference when accessing headerInfo.Time.
Ensure that HeaderInfo is always properly checked for nil before accessing its Time property to prevent runtime panics.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `HeaderInfo` is always non-nil when accessed.
# Test: Search for potential nil checks around `HeaderInfo` usage.
rg --type go $'HeaderInfo\\(\\)\\.Time' -C 3
Length of output: 56167
Script:
#!/bin/bash
# Description: Verify that `HeaderInfo` is always non-nil when accessed.
# Test: Search for all instances where `HeaderInfo` is accessed and check for nil checks in the surrounding code.
rg --type go 'HeaderInfo\(\)' -C 5
Length of output: 139685
Tools
golangci-lint
35-35: undefined: ErrFeeLimitExpired
| environment, ok := ctx.Value(corecontext.EnvironmentContextKey).(appmodule.Environment) | ||
| if !ok { | ||
| return nil, fmt.Errorf("environment not set") | ||
| } | ||
| gasMeter := environment.GasService.GasMeter(ctx) | ||
| for _, msg := range a.AllowedMessages { | ||
| sdkCtx.GasMeter().ConsumeGas(gasCostPerIteration, "check msg") | ||
| if err := gasMeter.Consume(gasCostPerIteration, "check msg"); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Handle potential issues when the environment is not set in the context.
- if !ok {
- return nil, fmt.Errorf("environment not set")
+ if !ok {
+ return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "environment not set in context")This change aligns error handling with Cosmos SDK's standard practices by using sdkerrors.Wrap for consistency and clarity.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| environment, ok := ctx.Value(corecontext.EnvironmentContextKey).(appmodule.Environment) | |
| if !ok { | |
| return nil, fmt.Errorf("environment not set") | |
| } | |
| gasMeter := environment.GasService.GasMeter(ctx) | |
| for _, msg := range a.AllowedMessages { | |
| sdkCtx.GasMeter().ConsumeGas(gasCostPerIteration, "check msg") | |
| if err := gasMeter.Consume(gasCostPerIteration, "check msg"); err != nil { | |
| return nil, err | |
| } | |
| environment, ok := ctx.Value(corecontext.EnvironmentContextKey).(appmodule.Environment) | |
| if !ok { | |
| return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "environment not set in context") | |
| } | |
| gasMeter := environment.GasService.GasMeter(ctx) | |
| for _, msg := range a.AllowedMessages { | |
| if err := gasMeter.Consume(gasCostPerIteration, "check msg"); err != nil { | |
| return nil, err | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (9)
- x/feegrant/CHANGELOG.md (1 hunks)
- x/feegrant/basic_fee.go (2 hunks)
- x/feegrant/basic_fee_test.go (2 hunks)
- x/feegrant/filtered_fee.go (3 hunks)
- x/feegrant/filtered_fee_test.go (2 hunks)
- x/feegrant/keeper/keeper.go (2 hunks)
- x/feegrant/mock_test.go (1 hunks)
- x/feegrant/periodic_fee.go (2 hunks)
- x/feegrant/periodic_fee_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (7)
- x/feegrant/basic_fee.go
- x/feegrant/basic_fee_test.go
- x/feegrant/filtered_fee.go
- x/feegrant/filtered_fee_test.go
- x/feegrant/keeper/keeper.go
- x/feegrant/periodic_fee.go
- x/feegrant/periodic_fee_test.go
Additional context used
Path-based instructions (2)
x/feegrant/mock_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/feegrant/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/feegrant/CHANGELOG.md
[uncategorized] ~30-~30: Possible missing comma found.
Context: ... is extracted to have a separate go.mod file which allows it to be a standalone modu...
[duplication] ~66-~66: Possible typo: you repeated a word
Context: ...alidateBasic` is treated as a no op now with with acceptance of RFC001 * [#17869](https:/...
Additional comments not posted (3)
x/feegrant/mock_test.go (2)
22-24: LGTM! Correct use of the new environment context key.
30-32: LGTM! Simple and effective mock implementation.x/feegrant/CHANGELOG.md (1)
34-34: LGTM! Clear and informative changelog entry.
| func (h mockHeaderService) HeaderInfo(ctx context.Context) coreheader.Info { | ||
| return sdk.UnwrapSDKContext(ctx).HeaderInfo() | ||
| } |
There was a problem hiding this comment.
Replace sdk.UnwrapSDKContext to align with PR goals.
- return sdk.UnwrapSDKContext(ctx).HeaderInfo()
+ // Replace with direct use of environment's header service if available
+ return environment.HeaderService.HeaderInfo(ctx)Committable suggestion was skipped due low confidence.
* main: refactor(x/feegrant): set environment in context (#20529) refactor(x/accounts)!: accounts and auth module use the same account number tracking (#20405) chore: remove sonar from simapp (#20528) docs: add docs on permissions (#20526) refactor(x/gov): set environment in context for legacy proposals (#20521) docs: migrate diagrams to mermaidjs (#20503) refactor(tools/hubl): don't use nil panic (#20515) refactor(x/authz): set environment in context (#20502) build(deps): Bump github.com/spf13/viper from 1.18.2 to 1.19.0 (#20519) feat(x/bank): Placing SendRestriction before Deduction of Coins in SendCoins (#20517) chore: sonar ignore directories with their own go.mods (#20509) ci: run action in merge queue (#20508) refactor(server/v2/cometbft): update function comments (#20506)
Description
Closes: #19640
AcceptcallsAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Chores