refactor(evm): clean up AnteHandler setup #1966
Conversation
|
Warning Rate limit exceeded@onikonychev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 23 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates to the Changes
Poem
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 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: 0
Outside diff range, codebase verification and nitpick comments (2)
eth/codec.go (1)
Inconsistent removal of transaction extensions.
The
ExtensionOptionDynamicFeeTxis still present in the codebase, specifically ineth/dynamic_fee.pb.go. However,ExtensionOptionsWeb3Txhas been successfully removed. Please ensure thatExtensionOptionDynamicFeeTxis also removed if it is no longer required.
- File:
eth/dynamic_fee.pb.go- Occurrences: Multiple lines within the file.
Analysis chain
Line range hint
18-24:
Confirm removal of transaction extensions and verify impact.The
ExtensionOptionsWeb3TxandExtensionOptionDynamicFeeTxhave been removed. Ensure that these extensions are no longer used elsewhere in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ExtensionOptionsWeb3Tx` and `ExtensionOptionDynamicFeeTx` are no longer used in the codebase. # Test: Search for the removed transaction extensions. Expect: No occurrences. rg --type go 'ExtensionOptionsWeb3Tx|ExtensionOptionDynamicFeeTx'Length of output: 2317
eth/codec_test.go (1)
ExtensionOptionDynamicFeeTxis still present in the codebase.The
ExtensionOptionDynamicFeeTxextension is still defined and used in the codebase, specifically in the following files:
eth/dynamic_fee.pb.goproto/eth/types/v1/dynamic_fee.protoThe
ExtensionOptionsWeb3Txextension has been successfully removed.
Analysis chain
Line range hint
32-34:
Confirm removal of transaction extensions in tests and verify impact.The
ExtensionOptionsWeb3TxandExtensionOptionDynamicFeeTxhave been removed from the test. Ensure that these extensions are no longer tested or required elsewhere in the test suite.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ExtensionOptionsWeb3Tx` and `ExtensionOptionDynamicFeeTx` are no longer tested in the codebase. # Test: Search for the removed transaction extensions in test files. Expect: No occurrences. rg --type go --glob '*_test.go' 'ExtensionOptionsWeb3Tx|ExtensionOptionDynamicFeeTx'Length of output: 86
Script:
#!/bin/bash # Description: Verify that `ExtensionOptionsWeb3Tx` and `ExtensionOptionDynamicFeeTx` are no longer present in the codebase. # Test: Search for the removed transaction extensions in the entire codebase. Expect: No occurrences. rg 'ExtensionOptionsWeb3Tx|ExtensionOptionDynamicFeeTx'Length of output: 2519
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
eth/web3.pb.gois excluded by!**/*.pb.go
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- app/ante.go (3 hunks)
- eth/codec.go (3 hunks)
- eth/codec_test.go (1 hunks)
Additional comments not posted (3)
app/ante.go (2)
Line range hint
48-84:
Confirm removal of transaction extensions and verify impact.The
ExtensionOptionsWeb3TxandExtensionOptionDynamicFeeTxhave been removed. Ensure that these extensions are no longer used elsewhere in the codebase.Verification successful
Confirm removal of transaction extensions and verify impact.
The
ExtensionOptionsWeb3Txhas been fully removed from the codebase. TheExtensionOptionDynamicFeeTxis still present ineth/dynamic_fee.pb.gobut is not actively used elsewhere in the codebase.
ExtensionOptionsWeb3Tx: Fully removed.ExtensionOptionDynamicFeeTx: Present but not actively used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ExtensionOptionsWeb3Tx` and `ExtensionOptionDynamicFeeTx` are no longer used in the codebase. # Test: Search for the removed transaction extensions. Expect: No occurrences. rg --type go 'ExtensionOptionsWeb3Tx|ExtensionOptionDynamicFeeTx'Length of output: 2317
Script:
#!/bin/bash # Description: Verify if `ExtensionOptionDynamicFeeTx` is still in use in the codebase. # Test: Search for the usage of `ExtensionOptionDynamicFeeTx`. Expect: No active usage. rg --type go 'ExtensionOptionDynamicFeeTx'Length of output: 2292
21-45: Confirm simplification of AnteHandler switching and verify impact.The AnteHandler switching has been simplified by detecting the
ExtensionOptionsEthereumTxtransaction extension. Ensure that this change does not affect the handling of other transaction types.Verification successful
Confirm simplification of AnteHandler switching and verify impact.
The AnteHandler switching has been simplified by detecting the
ExtensionOptionsEthereumTxtransaction extension. Ensure that this change does not affect the handling of other transaction types.
Confirm simplification of AnteHandler switching and verify impact.
The AnteHandler switching has been simplified by detecting the
ExtensionOptionsEthereumTxtransaction extension. Ensure that this change does not affect the handling of other transaction types.
Confirm simplification of AnteHandler switching and verify impact.
The AnteHandler switching has been simplified by detecting the
ExtensionOptionsEthereumTxtransaction extension. TheNewAnteHandlerEVMfunction correctly handles Ethereum transactions by chaining the necessary AnteDecorators. This change does not negatively impact the handling of other transaction types.
app/evmante_handler.go: Implementation ofNewAnteHandlerEVM.app/ante.go: Usage ofNewAnteHandlerEVM.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the simplification of AnteHandler switching. # Test: Search for the usage of `ExtensionOptionsEthereumTx` in the codebase. Expect: Proper handling of transaction types. rg --type go 'ExtensionOptionsEthereumTx'Length of output: 2783
Script:
#!/bin/bash # Description: Verify the definition and usage of the `NewAnteHandlerEVM` function. # Search for the definition of `NewAnteHandlerEVM`. rg --type go 'func NewAnteHandlerEVM' # Search for the usage of `NewAnteHandlerEVM` in the codebase. rg --type go 'NewAnteHandlerEVM'Length of output: 408
Script:
#!/bin/bash # Description: Extract the implementation of `NewAnteHandlerEVM` from `app/evmante_handler.go`. # Extract the implementation of `NewAnteHandlerEVM`. rg --type go -A 20 'func NewAnteHandlerEVM' app/evmante_handler.goLength of output: 745
CHANGELOG.md (1)
88-88: Entry looks good!The entry correctly documents the refactor of the AnteHandler setup. Ensure that the link to the pull request is accurate and functional.
eth/codec.go
Outdated
There was a problem hiding this comment.
Why does removing dynamic fee tx necessitate removing Web3 tx?
There was a problem hiding this comment.
They're unrelated to each other, but both are unnecessary for our EVM implementation.
Ethermint had ExtensionOptionsWeb3Tx to support legacy EIP-712 signature extensions, but those were deprecated and we can just validate the signature on the Cosmos tx payload.
ExtensionOptionDynamicFeeTx was to add baseFee and priorityFee to Cosmos native module txs (i.e. non-EVM txs), but we don't have that in scope right now.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1966 +/- ##
==========================================
+ Coverage 65.71% 65.75% +0.03%
==========================================
Files 258 258
Lines 16408 16390 -18
==========================================
- Hits 10783 10777 -6
+ Misses 4815 4804 -11
+ Partials 810 809 -1
|
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
app/ante.go (1)
21-21: Inconsistent parameter renaming.The parameter
optshas been renamed tooptionsin some places but not consistently throughout theapp/ante.gofile. Please update all occurrences ofoptstooptionsto ensure uniformity.
- Lines with
opts:
opts := txWithExtensions.GetExtensionOptions()if len(opts) > 0 {opts ante.AnteHandlerOptions,wasmkeeper.NewLimitSimulationGasDecorator(opts.WasmConfig.SimulationGasLimit),wasmkeeper.NewCountTXDecorator(opts.TxCounterStoreKey),authante.NewExtensionOptionsDecorator(opts.ExtensionOptionChecker),authante.NewValidateMemoDecorator(opts.AccountKeeper),authante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper),authante.NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, opts.TxFeeChecker),devgasante.NewDevGasPayoutDecorator(opts.DevGasBankKeeper, opts.DevGasKeeper),authante.NewSetPubKeyDecorator(opts.AccountKeeper),authante.NewValidateSigCountDecorator(opts.AccountKeeper),authante.NewSigGasConsumeDecorator(opts.AccountKeeper, opts.SigGasConsumer),authante.NewSigVerificationDecorator(opts.AccountKeeper, opts.SignModeHandler),authante.NewIncrementSequenceDecorator(opts.AccountKeeper),ibcante.NewRedundantRelayDecorator(opts.IBCKeeper),Analysis chain
Clarify parameter renaming.
The parameter
optshas been renamed tooptions. Ensure that this change is consistent throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the parameter renaming from `opts` to `options` is consistent throughout the codebase. # Test: Search for occurrences of `NewAnteHandler` with `opts`. Expect: No occurrences. rg --type go -A 5 $'NewAnteHandler(keepers AppKeepers, opts'Length of output: 166
Script:
#!/bin/bash # Description: Verify that the parameter renaming from `opts` to `options` is consistent throughout the codebase. # Test: Search for occurrences of `NewAnteHandler` with `opts`. Expect: No occurrences. rg --type go -A 5 $'NewAnteHandler\\(keepers AppKeepers, opts'Length of output: 62
Script:
#!/bin/bash # Description: Verify that the parameter renaming from `opts` to `options` is consistent throughout the codebase. # Test: Search for occurrences of `opts` within the `app/ante.go` file. Expect: No occurrences. rg --type go 'opts' app/ante.goLength of output: 1102
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/ante.go (3 hunks)
- x/evm/codec.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/evm/codec.go
Additional context used
GitHub Check: codecov/patch
app/ante.go
[warning] 35-36: app/ante.go#L35-L36
Added lines #L35 - L36 were not covered by tests
[warning] 38-41: app/ante.go#L38-L41
Added lines #L38 - L41 were not covered by tests
[warning] 44-44: app/ante.go#L44
Added line #L44 was not covered by tests
Additional comments not posted (3)
app/ante.go (3)
26-26: Ensure proper validation and cleanup.The
ValidateAndCleanmethod is called onoptions. Verify that this method is correctly implemented and handles all necessary validation and cleanup.
80-80: Ensure proper fee deduction.The
NewDeductFeeDecoratoris used for fee deduction. Verify that the fee deduction logic is correctly implemented and handles all necessary cases.
82-82: Ensure proper dev gas payout.The
NewDevGasPayoutDecoratoris used for dev gas payout. Verify that the dev gas payout logic is correctly implemented and handles all necessary cases.
app/ante.go
Outdated
| txWithExtensions, ok := tx.(authante.HasExtensionOptionsTx) | ||
| if ok { | ||
| opts := txWithExtensions.GetExtensionOptions() | ||
| if len(opts) > 0 { | ||
| switch typeURL := opts[0].GetTypeUrl(); typeURL { | ||
| case "/eth.evm.v1.ExtensionOptionsEthereumTx": | ||
| // handle as *evmtypes.MsgEthereumTx | ||
| anteHandler = NewAnteHandlerEVM(keepers, options) | ||
| default: | ||
| return ctx, fmt.Errorf( | ||
| "rejecting tx with unsupported extension option: %s", typeURL) | ||
| } | ||
|
|
||
| return anteHandler(ctx, tx, sim) | ||
| } |
There was a problem hiding this comment.
Add test coverage for transaction extensions.
The logic for handling transactions with extension options is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 35-36: app/ante.go#L35-L36
Added lines #L35 - L36 were not covered by tests
[warning] 38-41: app/ante.go#L38-L41
Added lines #L38 - L41 were not covered by tests
[warning] 44-44: app/ante.go#L44
Added line #L44 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- eth/codec.go (2 hunks)
- eth/codec_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- eth/codec.go
Additional comments not posted (1)
eth/codec_test.go (1)
Line range hint
34-45:
Confirm removal ofTYPE_URL_DYNAMIC_FEE_TX.The removal of
TYPE_URL_DYNAMIC_FEE_TXfrom theWantImplsslice indicates that the dynamic fee transaction type is no longer relevant or required for the current test scenario. Ensure that this change aligns with the overall refactoring objectives and that no other parts of the codebase depend on this transaction type.Verification successful
Confirm removal of
TYPE_URL_DYNAMIC_FEE_TX.The identifier
TYPE_URL_DYNAMIC_FEE_TXis not used elsewhere in the codebase, confirming that its removal from theWantImplsslice in the test file is appropriate and aligns with the refactoring objectives.
- No further occurrences of
TYPE_URL_DYNAMIC_FEE_TXfound in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `TYPE_URL_DYNAMIC_FEE_TX` is not used elsewhere in the codebase. # Test: Search for the usage of `TYPE_URL_DYNAMIC_FEE_TX`. Expect: No occurrences. rg --type go 'TYPE_URL_DYNAMIC_FEE_TX'Length of output: 38
Script:
#!/bin/bash # Description: Verify that `TYPE_URL_DYNAMIC_FEE_TX` is not used elsewhere in the codebase. # Test: Search for the usage of `TYPE_URL_DYNAMIC_FEE_TX`. Expect: No occurrences. rg 'TYPE_URL_DYNAMIC_FEE_TX'Length of output: 28
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- app/ante.go (3 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional context used
GitHub Check: codecov/patch
app/ante.go
[warning] 36-37: app/ante.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 39-42: app/ante.go#L39-L42
Added lines #L39 - L42 were not covered by tests
[warning] 45-45: app/ante.go#L45
Added line #L45 was not covered by tests
Additional comments not posted (2)
app/ante.go (2)
81-83: LGTM!The refactoring of the
NewAnteHandlerNonEVMfunction improves readability and maintainability.
22-22: Verify the function usage in the codebase.The parameter name has been changed from
optstooptions. Ensure that all function calls toNewAnteHandlermatch the new parameter name.Verification successful
Verified the function usage in the codebase.
The parameter name has been correctly updated to
optionsin theNewAnteHandlerfunction and its calls throughout the codebase.
app/ante.goapp/app.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewAnteHandler` match the new parameter name. # Test: Search for the function usage. Expect: Only occurrences of the new parameter name. rg --type go -A 5 $'NewAnteHandler'Length of output: 2758
| txWithExtensions, ok := tx.(authante.HasExtensionOptionsTx) | ||
| if ok { | ||
| opts := txWithExtensions.GetExtensionOptions() | ||
| if len(opts) > 0 { | ||
| switch typeURL := opts[0].GetTypeUrl(); typeURL { | ||
| case "/eth.evm.v1.ExtensionOptionsEthereumTx": | ||
| // handle as *evmtypes.MsgEthereumTx | ||
| anteHandler = evmante.NewAnteHandlerEVM(options) | ||
| default: | ||
| return ctx, fmt.Errorf( | ||
| "rejecting tx with unsupported extension option: %s", typeURL) | ||
| } | ||
|
|
||
| return anteHandler(ctx, tx, sim) | ||
| } |
There was a problem hiding this comment.
Add test coverage for transaction extensions.
The logic for handling transactions with extension options is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 36-37: app/ante.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 39-42: app/ante.go#L39 - L42
Added lines #L39 - L42 were not covered by tests
[warning] 45-45: app/ante.go#L45
Added line #L45 was not covered by tests
Tools
GitHub Check: codecov/patch
[warning] 36-37: app/ante.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 39-42: app/ante.go#L39-L42
Added lines #L39 - L42 were not covered by tests
[warning] 45-45: app/ante.go#L45
Added line #L45 was not covered by tests
Purpose / Abstract
ExtensionOptionDynamicFeeTx. We don't need it, asMsgEthereumTxalready hasbaseFeeandpriorityFeein EVM-module txs.ExtensionOptionsEthereumTxtx extension.Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Removals
Tests