imp(ante): Remove automatic withdrawal of staking rewards in case of insufficient funds #2719
Conversation
WalkthroughThe recent changes focus on optimizing fee deduction processes and error handling across multiple files. Key modifications include streamlining the logic, enhancing test clarity, and simplifying error messages. By removing unnecessary functions and improving the maintainability of the code, these updates ensure that fee deductions are handled more directly and intuitively, ultimately improving the user experience. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeductFeeDecorator
participant Authante
User->>DeductFeeDecorator: Request transaction with fees
DeductFeeDecorator->>Authante: Invoke DeductFees
Authante-->>DeductFeeDecorator: Return error or success
DeductFeeDecorator-->>User: Respond with fee deduction result
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 (
|
There was a problem hiding this comment.
LGTM! Great work @MalteHerrmann!
can you please add a changelog entry?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/ante/cosmos/fees.go (2 hunks)
- app/ante/cosmos/fees_test.go (2 hunks)
- app/ante/evm/09_gas_consume.go (1 hunks)
- app/ante/evm/09_gas_consume_test.go (4 hunks)
Additional comments not posted (9)
app/ante/evm/09_gas_consume.go (1)
89-89: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
deductFeeshandle the scenario where the user's balance is insufficient.Verification successful
Ensure proper error handling for insufficient funds in
deductFeescalls.The function
deductFeesis called in:
app/ante/evm/09_gas_consume.go(line 89)app/ante/cosmos/fees.goBoth instances have error handling in place. Verify that these handlers properly address scenarios where the user's balance is insufficient.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `deductFees` handle the scenario where the user's balance is insufficient. # Test: Search for the function usage. Expect: Proper error handling for insufficient funds. rg --type go -A 5 $'deductFees'Length of output: 2055
app/ante/evm/09_gas_consume_test.go (5)
94-94: Improved error handling in tests.The change to use a string for
expectedErrorinstead of an error type improves the clarity of the test cases.
99-100: Descriptive error messages enhance test clarity.Using a string for
expectedErrorallows for more descriptive error messages, improving debugging and understanding of test failures.
107-107: Descriptive error messages enhance test clarity.Using a string for
expectedErrorallows for more descriptive error messages, improving debugging and understanding of test failures.
118-118: Descriptive error messages enhance test clarity.Using a string for
expectedErrorallows for more descriptive error messages, improving debugging and understanding of test failures.
152-154: Improved error handling in tests.The change to use a string for
expectedErrorinstead of an error type improves the clarity of the test cases.app/ante/cosmos/fees.go (1)
132-133: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
deductFeehandle the scenario where the user's balance is insufficient.Verification successful
Let's verify that all calls to
deductFeeand related functions handle insufficient funds properly. We will search for instances wheredeductFeeis called and check for proper error handling.
LGTM! Proper error handling for insufficient funds is present in all instances of
deductFee.The code changes are approved.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `deductFee` handle the scenario where the user's balance is insufficient. # Test: Search for the function usage. Expect: Proper error handling for insufficient funds. rg --type go -A 5 $'deductFee'Length of output: 4974
Script:
#!/bin/bash # Description: Verify all function calls to `deductFee` handle the scenario where the user's balance is insufficient. # Test: Search for the function usage and check for error handling. Expect: Proper error handling for insufficient funds. rg --type go -A 5 $'deductFee' | grep -B 5 -A 5 'err'Length of output: 3045
app/ante/cosmos/fees_test.go (2)
66-73: LGTM! Improved test case clarity.The test case name and expected error message have been updated to focus on the insufficient funds condition and provide a clearer understanding of the failure reason.
86-93: LGTM! Consistent error message.The test case has been updated to align with the modified error message, ensuring consistency in the test logic and expected outcomes.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
CHANGELOG.md (1)
71-71: Fix line length to adhere to Markdownlint standards.The line length exceeds the recommended limit of 120 characters.
- - (ante) [#2719](https://github.com/evmos/evmos/pull/2719) Remove automatic withdrawal of staking rewards in case of insufficient funds. + - (ante) [#2719](https://github.com/evmos/evmos/pull/2719) Remove automatic withdrawal of staking rewards + in case of insufficient funds.Tools
Markdownlint
71-71: Expected: 120; Actual: 136
Line length(MD013, line-length)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional context used
Markdownlint
CHANGELOG.md
71-71: Expected: 120; Actual: 136
Line length(MD013, line-length)
|
https://github.com/Mergifyio backport Vvaradinov/ics-devnet |
✅ Backports have been created
|
…insufficient funds (#2719) * remove automatic withdrawal of staking rewards in case of insufficient funds * add changelog entry
…insufficient funds (backport #2719) (#2801) * imp(ante): Remove automatic withdrawal of staking rewards in case of insufficient funds (#2719) * remove automatic withdrawal of staking rewards in case of insufficient funds * add changelog entry (cherry picked from commit b591299) # Conflicts: # CHANGELOG.md * fix conflicts --------- Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com> Co-authored-by: tom <tomasguerraalda@hotmail.com>
This PR removes the logic associated with the automatic withdrawal of staking rewards in case a user has not enough funds to pay for the transaction fees.
This feature is not going to work with ICS and has also been a point of critique for the taxable event that is created and subsequently removed from different partner's forks.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation