refactor(x/auth/ante): don't use simulate bool in ante handler #19586
Conversation
WalkthroughWalkthroughThe update involves a significant refactoring of the Changes
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.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (6)
- x/auth/ante/basic.go (8 hunks)
- x/auth/ante/ext.go (1 hunks)
- x/auth/ante/fee.go (2 hunks)
- x/auth/ante/setup.go (2 hunks)
- x/auth/ante/sigverify.go (4 hunks)
- x/auth/ante/unordered.go (2 hunks)
Additional comments: 10
x/auth/ante/ext.go (1)
- 46-52: The update to use
ctx.ExecMode() == sdk.ExecModeSimulateinstead of a booleansimulateparameter in theAnteHandlemethod is consistent with the PR's objective to simplify the codebase. This change should make the code cleaner and less prone to errors related to the misuse of thesimulateparameter. Ensure to thoroughly test this change in different execution contexts to confirm that it behaves as expected in both simulation and non-simulation scenarios.x/auth/ante/unordered.go (1)
- 37-48: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [40-73]
The modification to use
ctx.ExecMode() == sdk.ExecModeSimulatefor determining simulation mode in theAnteHandlemethod of theUnorderedTxDecoratorstruct aligns with the PR's goal of simplifying the codebase. This change should enhance code readability and maintainability by relying on the execution context. It's important to conduct comprehensive testing to ensure that unordered transactions are processed correctly in both simulation and non-simulation modes.x/auth/ante/setup.go (1)
- 27-33: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [30-70]
The update in the
AnteHandlemethod of theSetUpContextDecoratorto usectx.ExecMode() == sdk.ExecModeSimulatefor determining simulation mode is in line with the PR's objective. This approach should simplify the code and reduce potential confusion. Ensure comprehensive testing is conducted, especially focusing on gas metering and transaction processing in different execution contexts.x/auth/ante/fee.go (1)
- 39-45: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [42-70]
The modification in the
AnteHandlemethod of theDeductFeeDecoratorto usectx.ExecMode() == sdk.ExecModeSimulatefor determining simulation mode aligns with the PR's goal of simplifying the codebase. This change should make the code more readable and less prone to errors. It's critical to thoroughly test fee deduction in both simulation and non-simulation modes to ensure that transactions are processed correctly and fees are deducted as expected.x/auth/ante/basic.go (4)
- 24-33: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-39]
The update in the
AnteHandlemethod of theValidateBasicDecoratorto usectx.ExecMode() == sdk.ExecModeSimulatefor determining simulation mode is consistent with the PR's objective. This change should simplify the code and reduce potential confusion. Ensure to test this change thoroughly to confirm that basic validation behaves as expected in both simulation and non-simulation modes.
- 52-58: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [55-72]
The modification in the
AnteHandlemethod of theValidateMemoDecoratorto usectx.ExecMode() == sdk.ExecModeSimulatefor determining simulation mode aligns with the PR's goal. This should enhance code readability and maintainability. Comprehensive testing is important to ensure that memo validation works correctly in different execution contexts.
- 91-97: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [94-146]
The change in the
AnteHandlemethod of theConsumeTxSizeGasDecoratorto usectx.ExecMode() == sdk.ExecModeSimulatefor determining simulation mode supports the PR's objective. This approach should make the code more readable and less prone to errors. It's crucial to thoroughly test gas consumption for tx size in both simulation and non-simulation modes to ensure accurate gas metering.
- 193-199: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [196-209]
The update in the
AnteHandlemethod of theTxTimeoutHeightDecoratorto usectx.ExecMode() == sdk.ExecModeSimulatefor determining simulation mode is in line with the PR's goal. This change should simplify the code and reduce potential confusion. Ensure comprehensive testing is conducted, especially focusing on tx timeout height checks in different execution contexts.x/auth/ante/sigverify.go (2)
- 149-155: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [152-218]
The modification to use
ctx.ExecMode() == sdk.ExecModeSimulateinstead of thesimulateboolean parameter in theAnteHandlemethod ofSigVerificationDecoratoraligns with the PR's objective to simplify the codebase. This change enhances readability and reduces redundancy by relying on the execution context to determine the simulation mode. However, ensure that all calls to this method across the codebase are updated to reflect the removal of thesimulateparameter. Additionally, verify that theExecModemethod reliably reflects the intended execution mode in all scenarios whereAnteHandleis invoked.
- 459-465: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [462-482]
The update in the
AnteHandlemethod ofValidateSigCountDecoratorto usectx.ExecMode() == sdk.ExecModeSimulatefor determining the execution mode is consistent with the refactor's goal. This change simplifies the method signature by removing the redundantsimulateparameter. As with the previous comment, ensure that the execution context accurately represents the simulation mode in all relevant cases and that there are no dependencies on the removed parameter elsewhere in the codebase.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yml
Files selected for processing (1)
- types/handler.go (3 hunks)
Additional comments: 2
types/handler.go (2)
- 49-55: In the
ChainAnteDecoratorsfunction, thesimulateparameter is removed, and thectx.ExecMode() == ExecModeSimulatecheck is used instead. This change is consistent with the PR's objective. Ensure that all AnteDecorators in the chain are updated to match this new signature and logic.- 77-83: The
ChainPostDecoratorsfunction mirrors the changes made inChainAnteDecorators, removing thesimulateparameter and using thectx.ExecMode() == ExecModeSimulatecheck. Confirm that all PostDecorators in the chain are updated accordingly.
|
|
||
| // PostHandler like AnteHandler but it executes after RunMsgs. Runs on success | ||
| // or failure and enables use cases like gas refunding. | ||
| type PostHandler func(ctx Context, tx Tx, simulate, success bool) (newCtx Context, err error) | ||
| type PostHandler func(ctx Context, tx Tx, _, success bool) (newCtx Context, err error) |
There was a problem hiding this comment.
The simulate parameter has also been removed from the PostHandler function signature. Ensure that the documentation is updated to reflect this change and avoid mentioning the removed simulate parameter.
- type PostHandler func(ctx Context, tx Tx, _, success bool) (newCtx Context, err error)
+ type PostHandler func(ctx Context, tx Tx, success bool) (newCtx Context, err error)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.
| type PostHandler func(ctx Context, tx Tx, _, success bool) (newCtx Context, err error) | |
| type PostHandler func(ctx Context, tx Tx, success bool) (newCtx Context, err error) |
|
|
||
| // AnteDecorator wraps the next AnteHandler to perform custom pre-processing. | ||
| type AnteDecorator interface { | ||
| AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (newCtx Context, err error) | ||
| AnteHandle(ctx Context, tx Tx, _ bool, next AnteHandler) (newCtx Context, err error) |
There was a problem hiding this comment.
The simulate parameter is removed from the AnteDecorator interface's AnteHandle method. The documentation should be updated to remove references to the simulate parameter.
- AnteHandle(ctx Context, tx Tx, _ bool, next AnteHandler) (newCtx Context, err error)
+ AnteHandle(ctx Context, tx Tx, next AnteHandler) (newCtx Context, err error)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.
| AnteHandle(ctx Context, tx Tx, _ bool, next AnteHandler) (newCtx Context, err error) | |
| AnteHandle(ctx Context, tx Tx, next AnteHandler) (newCtx Context, err error) |
| } | ||
|
|
||
| // PostDecorator wraps the next PostHandler to perform custom post-processing. | ||
| type PostDecorator interface { | ||
| PostHandle(ctx Context, tx Tx, simulate, success bool, next PostHandler) (newCtx Context, err error) | ||
| PostHandle(ctx Context, tx Tx, _, success bool, next PostHandler) (newCtx Context, err error) |
There was a problem hiding this comment.
Similarly, the simulate parameter is removed from the PostDecorator interface's PostHandle method. Update the documentation to reflect the removal of the simulate parameter.
- PostHandle(ctx Context, tx Tx, _, success bool, next PostHandler) (newCtx Context, err error)
+ PostHandle(ctx Context, tx Tx, success bool, next PostHandler) (newCtx Context, err error)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.
| PostHandle(ctx Context, tx Tx, _, success bool, next PostHandler) (newCtx Context, err error) | |
| PostHandle(ctx Context, tx Tx, success bool, next PostHandler) (newCtx Context, err error) |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
seems there is a test failing for the antedecorators |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- types/handler_test.go (2 hunks)
Additional comments: 2
types/handler_test.go (2)
- 17-17: The addition of
WithExecMode(sdk.ExecModeSimulate)inTestChainAnteDecoratorscorrectly aligns with the PR's objective to use the execution context for determining the execution mode instead of a separatesimulateboolean parameter. This change ensures that the test context is explicitly set to simulation mode, which is necessary for testing the behavior of ante decorators under simulation conditions.- 42-42: Similarly, the update in
TestChainPostDecoratorsto initializectxwithWithExecMode(sdk.ExecModeSimulate)is consistent with the refactor's goal. By setting the execution mode in the context, the tests accurately reflect the intended usage of the execution context to control behavior, specifically in simulation mode. This change is crucial for ensuring that post decorators are tested under the correct execution conditions.
Description
The
simulate booland exec context are always in sync:cosmos-sdk/baseapp/baseapp.go
Lines 897 to 900 in 8585d53
But if you don't know that, it reads weird to use the exec mode within the ante handler, and pass to simulate bool through.
ref: #19161.
Author 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.
I have...
Summary by CodeRabbit