fix: stuck escrow fees #194
Conversation
WalkthroughThe pull request introduces significant modifications to the application, primarily through the implementation of a new Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
x/xfeemarket/types/keys.go (1)
20-21: LGTM! Consider adding a comment for the new constant.The addition of
EventTypeBurnStuckEscrowedFeealigns well with the PR objective of fixing stuck escrow fees. The naming convention is consistent with existing patterns.Consider adding a brief comment explaining the purpose of the new
EventTypeBurnStuckEscrowedFeeconstant, similar to how other constants in this file are documented. This would improve code readability and maintainability.app/post_handler.go (1)
12-12: LGTM. Consider updating documentation.The addition of
AccountKeepertoPostHandlerOptionsis appropriate. It introduces a new dependency that will be used in fee market operations.Consider updating the struct's documentation to explain the purpose and requirements of the
AccountKeeperfield.x/xfeemarket/post/expected_keepers.go (1)
11-17: LGTM: Well-structuredAccountKeeperinterface.The new
AccountKeeperinterface is well-designed and follows best practices:
- It uses
context.Contextfor proper context propagation.- It includes a
go:generatedirective for easy mock generation.- The interface is well-documented, explaining its purpose and abstraction reason.
Consider adding a brief description of the
GetModuleAccountmethod in the interface documentation. For example:// AccountKeeper defines the contract needed for AccountKeeper related APIs. // Interface provides support to use non-sdk AccountKeeper for AnteHandler's decorators. // // GetModuleAccount retrieves a module account by name using the provided context. // //go:generate mockery --name AccountKeeper --filename mock_account_keeper.go type AccountKeeper interface { GetModuleAccount(ctx context.Context, name string) sdk.ModuleAccountI }x/xfeemarket/post/mocks/mock_account_keeper.go (2)
13-36: LGTM: AccountKeeper mock implementation is well-structured.The AccountKeeper struct and GetModuleAccount method are implemented correctly following the testify mock patterns. The method properly handles different return value scenarios and includes appropriate panic handling.
Consider adding a comment explaining the purpose of the type assertion in the else block:
} else { + // Attempt to cast the return value to the expected type if ret.Get(0) != nil { r0 = ret.Get(0).(types.ModuleAccountI) } }This would improve code readability and explain the purpose of this block to future maintainers.
38-50: LGTM: NewAccountKeeper function is well-implemented.The NewAccountKeeper function is correctly implemented as a constructor for the AccountKeeper mock. It properly sets up the mock and registers a cleanup function to assert expectations after the test.
For consistency with Go naming conventions, consider renaming the return variable:
-func NewAccountKeeper(t interface { +func NewAccountKeeper(testingT interface { mock.TestingT Cleanup(func()) }) *AccountKeeper { - mock := &AccountKeeper{} - mock.Mock.Test(t) + mock := &AccountKeeper{} + mock.Mock.Test(testingT) - t.Cleanup(func() { mock.AssertExpectations(t) }) + testingT.Cleanup(func() { mock.AssertExpectations(testingT) }) return mock }This change makes the parameter name more descriptive and consistent with its purpose.
x/xfeemarket/post/suite/suite.go (1)
137-137: LGTM! Consider applying consistent naming convention.The introduction of the
accountkeepervariable improves code consistency. However, for even better consistency, consider using camelCase for all local variables.Apply this minor change:
-accountkeeper := s.AccountKeeper +accountKeeper := s.AccountKeeperThis change would make the naming convention consistent with other local variables like
bankKeeperandfeeGrantKeeper.Also applies to: 169-169
x/xfeemarket/post/fee.go (1)
188-193: Add unit tests for new stuck escrow fee logicThe added logic for handling and burning stuck escrowed fees introduces new pathways that should be tested. Please add unit tests to cover:
- Scenarios where there are stuck escrowed fees in the default fee denomination.
- Transactions with fees in denominations different from the default.
- Verification that the correct events are emitted when stuck fees are burned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- app/app.go (1 hunks)
- app/post_handler.go (2 hunks)
- x/xfeemarket/post/expected_keepers.go (1 hunks)
- x/xfeemarket/post/fee.go (2 hunks)
- x/xfeemarket/post/fee_test.go (7 hunks)
- x/xfeemarket/post/mocks/mock_account_keeper.go (1 hunks)
- x/xfeemarket/post/suite/suite.go (2 hunks)
- x/xfeemarket/types/keys.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
x/xfeemarket/types/keys.go (1)
21-21: Verify the usage of the new event type across the module.The addition of
EventTypeBurnStuckEscrowedFeesuggests a new event type for burning stuck escrowed fees. To ensure proper implementation:
- Verify that this new event type is emitted in the appropriate location within the module.
- Ensure that any event handling or processing logic in the module is updated to handle this new event type.
- Update relevant documentation or comments in other files to reflect this new event type.
To help verify the usage of the new event type, you can run the following script:
app/post_handler.go (3)
19-21: LGTM. Proper validation added.The addition of the
AccountKeepervalidation check is appropriate and consistent with the existing validation pattern for other keepers. This ensures that all required dependencies are provided before constructing the post handler.
33-33: LGTM. VerifyNewFeeMarketDeductDecoratorsignature.The addition of
AccountKeeperas an argument toNewFeeMarketDeductDecoratoris consistent with the changes made toPostHandlerOptions.Please ensure that the
NewFeeMarketDeductDecoratorfunction in thexfeemarket/postpackage has been updated to accept theAccountKeeperparameter. Run the following script to verify:#!/bin/bash # Description: Verify the signature of NewFeeMarketDeductDecorator function # Test: Search for the NewFeeMarketDeductDecorator function definition ast-grep --lang go --pattern 'func NewFeeMarketDeductDecorator($_AccountKeeper, $_, $_) $_'
Line range hint
1-40: Overall LGTM. Consider reviewing related files.The changes in this file consistently introduce the
AccountKeeperdependency to the post handler setup. This appears to be part of a larger refactoring to include account-related operations in fee market handling.To ensure consistency across the codebase, please review:
- The implementation of the
AccountKeeperinterface in thexfeemarket/postpackage.- Any other files that might need updates due to this new dependency.
Run the following script to identify potentially affected files:
x/xfeemarket/post/expected_keepers.go (1)
4-5: LGTM: Import changes are appropriate.The addition of the
contextimport is necessary for the newAccountKeeperinterface. The empty line improves code readability by separating standard library imports from third-party imports.x/xfeemarket/post/mocks/mock_account_keeper.go (1)
1-11: LGTM: File header and imports are appropriate.The file header correctly indicates that this is generated code, which is important for maintaining the mock. The imports are appropriate for the mock implementation, including the necessary packages from the testify framework and cosmos-sdk.
x/xfeemarket/post/fee.go (4)
24-26: Ensure proper usage of the addedaccountKeeperYou've added
accountKeeperto theFeeMarketDeductDecoratorstruct. Please verify that this addition is consistently utilized throughout the codebase where necessary.
181-186: Ensure events are emitted for all fee denominationsThe event
feemarkettypes.EventTypeFeePayis emitted only whenfee.Denom == defaultFeeDenom. If fees can be paid in multiple denominations, consider whether you should emit this event for fees in other denominations as well to maintain consistent event logging.
167-195:⚠️ Potential issueVerify handling of stuck escrowed fees for non-default denominations
Currently, the logic for burning stuck escrowed fees only executes when
fee.Denom == defaultFeeDenom. If there are stuck escrowed fees in the default fee denomination but the transaction fee is in a different denomination, these fees may not be burned. Please verify whether it is necessary to burn stuck escrowed fees regardless of the transaction's fee denomination.Consider modifying the condition to always check and burn any stuck escrowed fees in the default fee denomination, even when the transaction fee is in a different denomination.
29-34: Update all instantiations ofNewFeeMarketDeductDecoratorThe constructor
NewFeeMarketDeductDecoratornow includes anAccountKeeperparameter (ak). Ensure that all calls to this constructor across the codebase are updated to include this new parameter to prevent compilation errors.You can run the following script to identify any outdated constructor calls:
✅ Verification successful
All instantiations of
NewFeeMarketDeductDecoratorare up to date.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find calls to `NewFeeMarketDeductDecorator` missing the new `AccountKeeper` parameter. # Test: Search for instances of `NewFeeMarketDeductDecorator` with less than three arguments. # Expect: No instances found. rg --type go 'NewFeeMarketDeductDecorator\([^,]*,[^,]*\)' --files-with-matchesLength of output: 80
x/xfeemarket/post/fee_test.go (5)
137-137: Update of Expected Consumed GasThe
expectedConsumedGasvalue has been updated to25250, reflecting the new gas consumption after recent changes. This ensures that the test accurately checks against the current gas usage.
142-142: Declaration ofzeroIntThe variable
zeroIntis declared usingmath.ZeroInt()for use in mock functions. This provides a clear and consistent representation of zero values in the tests.
212-212: Verify Mocked Balance in SimulationIn the test case
"0 gas given should pass - simulate", theGetBalancemock returns a zero balance:s.MockBankKeeper.On("GetBalance", mock.Anything, mock.Anything, mock.Anything).Return(sdk.NewCoin(baseDenom, zeroInt)).Once()Please verify that returning a zero balance is appropriate for this test scenario and does not affect the validity of the test, especially since the signer is expected to have enough funds.
435-437: Update of Expected Consumed Gas ConstantsThe
expectedConsumedGasandexpectedConsumedGasResolvevalues have been updated to reflect the new gas consumption metrics:expectedConsumedGas = 61632 expectedConsumedGasResolve = 27870 // slight difference due to denom resolverUpdating these constants ensures that the tests accurately validate against the current gas usage.
543-543: Update ofExpectConsumedGasin Test CaseIn the test case
"signer has enough funds, should pass, no tip", theExpectConsumedGasvalue has been updated to37325to reflect the new gas consumption after recent changes:ExpectConsumedGas: 37325,This ensures that the test expectations align with the current behavior.
app/app.go (1)
Line range hint
1039-1045: LGTMThe
setPostHandlerfunction correctly initializesPostHandlerOptionsand sets the post handler usingNewPostHandler. The implementation aligns with the application's initialization pattern.
|
|
Summary by CodeRabbit
New Features
PostHandlerfor enhanced transaction processing.AccountKeeperinterface for better account management.Bug Fixes
AccountKeeperin transaction processing.Tests
Chores
AccountKeeperto facilitate testing.