feat(wardend): burn 30% of the tx fee #85
Conversation
|
This PR should be merged after warden-protocol/snapshots#1 |
| SignModeHandler: in.TxConfig.SignModeHandler(), | ||
| FeegrantKeeper: in.FeeGrantKeeper, | ||
| SigGasConsumer: authante.DefaultSigVerificationGasConsumer, | ||
| FeeBurnRatio: math.LegacyNewDecWithPrec(3, 1), // 30% |
There was a problem hiding this comment.
Do you know if we can get this number from the modules Params structure? This way it can get updated via governance proposals
There was a problem hiding this comment.
Yes, we definitely can. Good point, I'll implement it.
|
Did you consider implementing this feature as a BeginBlocker instead of an AnteHandler? Do you know if there are limitations to one or the other possible implementations? |
WalkthroughThe changes introduce significant enhancements to the Warden protocol's transaction processing capabilities, focusing on account permissions, fee management, and the ante handler's structure. Notably, new features allow for fee burning and improved transaction handling logic, while unit tests and mock implementations bolster the testing framework. Overall, these modifications enhance security, efficiency, and flexibility within the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AnteHandler
participant BankKeeper
participant FeeCollector
participant MockTest
User->>AnteHandler: Submit Transaction
AnteHandler->>BankKeeper: Validate and Deduct Fees
BankKeeper->>FeeCollector: Burn Portion of Fees
FeeCollector-->>BankKeeper: Confirm Burn
BankKeeper-->>AnteHandler: Fees Processed
AnteHandler-->>User: Transaction Complete
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: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- warden/app/app_config.go (2 hunks)
- warden/x/warden/ante/ante.go (1 hunks)
- warden/x/warden/ante/burn_fee.go (1 hunks)
- warden/x/warden/ante/burn_fee_test.go (1 hunks)
- warden/x/warden/ante/testutil_test.go (1 hunks)
- warden/x/warden/module/module.go (5 hunks)
- warden/x/warden/testutil/expected_keepers_mocks.go (1 hunks)
- warden/x/warden/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- warden/x/warden/testutil/expected_keepers_mocks.go
Additional context used
golangci-lint
warden/x/warden/ante/testutil_test.go
120-120: Error return value of
acc.SetAccountNumberis not checked(errcheck)
130-130: field
descis unused(unused)
131-131: field
malleateis unused(unused)
Additional comments not posted (24)
warden/x/warden/ante/burn_fee.go (2)
16-19: Struct definition is appropriate.The
BurnFeeDecoratorstruct is well-defined with necessary fields for fee burning functionality.
21-26: Constructor is correctly implemented.The
NewBurnFeeDecoratorfunction initializes theBurnFeeDecoratorwith the necessary dependencies.warden/x/warden/ante/burn_fee_test.go (1)
19-46: Test implementation is comprehensive.The
TestDeductFeesfunction effectively tests theBurnFeeDecoratorlogic using mocks and assertions.warden/x/warden/types/expected_keepers.go (3)
16-20: Interface enhancements are appropriate.The additions to the
AccountKeeperinterface provide necessary functionality for account and module management.
25-29: Interface enhancements are appropriate.The additions to the
BankKeeperinterface support advanced financial operations, such as coin burning and transfers.
31-33: New interface addition is valuable.The introduction of the
FeegrantKeeperinterface facilitates efficient fee management and transaction handling.warden/x/warden/ante/ante.go (3)
3-15: Imports are well-organized and necessary.The imports are correctly organized and include all necessary packages for the functionality provided in this file.
17-26:HandlerOptionsstruct is well-defined.The
HandlerOptionsstruct includes all necessary fields for configuring the AnteHandler, including the newFeeBurnRatio.
28-61:NewAnteHandlerfunction is well-structured.The function constructs an AnteHandler with appropriate decorators and error handling for missing dependencies. The inclusion of
BurnFeeDecoratoraligns with the PR's objective to burn transaction fees.warden/x/warden/module/module.go (6)
Line range hint
5-28: Imports are necessary and well-organized.The new imports are necessary for the functionalities introduced, such as transaction handling and ante handler setup.
89-92: Updated gRPC gateway handling mechanism.The change from
runtime.ServeMuxtogwruntime.ServeMuxlikely reflects an update for compatibility or improved functionality.
188-192:ModuleInputsstruct enhancement withTxConfig.The addition of
TxConfigis essential for transaction configuration, aligning with the changes in transaction handling.
198-201:ModuleOutputsstruct enhancement withBaseAppOption.The inclusion of
BaseAppOptionallows for custom ante processing logic, enhancing module flexibility.
203-234:newAnteHandlerfunction is robust and well-implemented.The function ensures essential dependencies are present before creating the ante handler, improving error handling and robustness.
Line range hint
234-267:ProvideModulefunction integrates custom ante processing logic.The inclusion of a base application option that sets the ante handler allows for a more integrated approach to handling transactions.
warden/x/warden/ante/testutil_test.go (7)
3-37: Imports are appropriate for test utilities.The imports include necessary testing packages and mock utilities, which are appropriate for the test utilities provided in this file.
39-43:TestAccountstruct is straightforward and useful.The
TestAccountstruct is useful for managing test accounts in a straightforward manner.
45-56:AnteTestSuitestruct is well-structured for testing.The struct provides all necessary components for comprehensive ante handler testing.
58-112:SetupTestSuitefunction ensures proper test environment setup.The function is well-implemented, providing a proper test environment setup with mock controllers and context.
157-170:DeliverMsgsfunction is well-implemented for testing.The function provides a clear way to construct and deliver transactions through the ante handler for testing purposes.
172-206:RunTestCasefunction is comprehensive for executing tests.The function ensures that test cases are executed with appropriate checks for expected errors and outcomes.
208-259:CreateTestTxfunction is well-implemented for creating test transactions.The function provides necessary functionality for creating test transactions, handling signatures and encoding effectively.
warden/app/app_config.go (2)
260-262: Flag potential implications of skipping the ante handler.Setting
SkipAnteHandler: trueallows transactions to bypass the ante handler, which may affect fee deduction, signature verification, and other pre-processing tasks. Ensure that this change does not compromise transaction security or integrity.
175-175: Approve the addition of burner permission to the fee collector account.The addition of
Burnerpermission to theFeeCollectorNameaccount aligns with the objective to burn transaction fees. Ensure that this change does not conflict with existing permissions or operations.Verification successful
Verify compatibility of Burner permission with existing operations.
The addition of the
Burnerpermission to theFeeCollectorNameaccount appears consistent with its usage in burning operations. Ensure that no other permissions or operations are affected by this change.
- Usage in
burn_fee.go: Confirms the intent to burn coins.- Tests in
burn_fee_test.go: Validate the burning functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the added Burner permission to the FeeCollectorName account. # Test: Search for usage of FeeCollectorName in the codebase to ensure compatibility with the new permission. rg --type go 'FeeCollectorName'Length of output: 554
| func (bfd BurnFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { | ||
| feeTx, ok := tx.(sdk.FeeTx) | ||
| if !ok { | ||
| return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") | ||
| } | ||
|
|
||
| // Assume that default sdk FeeChecker is used. It follows that actual fee is always equal to feeTx.GetFee() | ||
| fee := feeTx.GetFee() | ||
|
|
||
| burnAmount, _ := sdk.NewDecCoinsFromCoins(fee...).MulDec(bfd.burnRatio).TruncateDecimal() | ||
|
|
||
| if !burnAmount.IsZero() { | ||
| err := bfd.bankKeeper.BurnCoins(ctx, authtypes.FeeCollectorName, burnAmount) | ||
| if err != nil { | ||
| return ctx, errorsmod.Wrap(sdkerrors.ErrLogic, "Failed to burn fee") | ||
| } | ||
| } | ||
|
|
||
| return next(ctx, tx, simulate) |
There was a problem hiding this comment.
Enhance error message for BurnCoins failure.
The error message when burning coins fails could be more descriptive to aid in debugging.
- return ctx, errorsmod.Wrap(sdkerrors.ErrLogic, "Failed to burn fee")
+ return ctx, errorsmod.Wrapf(sdkerrors.ErrLogic, "Failed to burn fee: %v", err)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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (bfd BurnFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { | |
| feeTx, ok := tx.(sdk.FeeTx) | |
| if !ok { | |
| return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") | |
| } | |
| // Assume that default sdk FeeChecker is used. It follows that actual fee is always equal to feeTx.GetFee() | |
| fee := feeTx.GetFee() | |
| burnAmount, _ := sdk.NewDecCoinsFromCoins(fee...).MulDec(bfd.burnRatio).TruncateDecimal() | |
| if !burnAmount.IsZero() { | |
| err := bfd.bankKeeper.BurnCoins(ctx, authtypes.FeeCollectorName, burnAmount) | |
| if err != nil { | |
| return ctx, errorsmod.Wrap(sdkerrors.ErrLogic, "Failed to burn fee") | |
| } | |
| } | |
| return next(ctx, tx, simulate) | |
| func (bfd BurnFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { | |
| feeTx, ok := tx.(sdk.FeeTx) | |
| if !ok { | |
| return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") | |
| } | |
| // Assume that default sdk FeeChecker is used. It follows that actual fee is always equal to feeTx.GetFee() | |
| fee := feeTx.GetFee() | |
| burnAmount, _ := sdk.NewDecCoinsFromCoins(fee...).MulDec(bfd.burnRatio).TruncateDecimal() | |
| if !burnAmount.IsZero() { | |
| err := bfd.bankKeeper.BurnCoins(ctx, authtypes.FeeCollectorName, burnAmount) | |
| if err != nil { | |
| return ctx, errorsmod.Wrapf(sdkerrors.ErrLogic, "Failed to burn fee: %v", err) | |
| } | |
| } | |
| return next(ctx, tx, simulate) |
| // TestCase represents a test case used in test tables. | ||
| type TestCase struct { | ||
| desc string | ||
| malleate func(*AnteTestSuite) TestCaseArgs | ||
| simulate bool | ||
| expPass bool | ||
| expErr error | ||
| } |
There was a problem hiding this comment.
Address unused fields in TestCase struct.
The fields desc and malleate are unused. Consider utilizing them or removing them if they are unnecessary.
Tools
golangci-lint
130-130: field
descis unused(unused)
131-131: field
malleateis unused(unused)
| func (suite *AnteTestSuite) CreateTestAccounts(numAccs int) []TestAccount { | ||
| var accounts []TestAccount | ||
|
|
||
| for i := 0; i < numAccs; i++ { | ||
| priv, _, addr := testdata.KeyTestPubAddr() | ||
| acc := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr) | ||
| acc.SetAccountNumber(uint64(i + 1000)) | ||
| suite.accountKeeper.SetAccount(suite.ctx, acc) | ||
| accounts = append(accounts, TestAccount{acc, priv}) | ||
| } | ||
|
|
||
| return accounts | ||
| } |
There was a problem hiding this comment.
Check the error return value of SetAccountNumber.
The error return value of SetAccountNumber should be checked to ensure that account setup is successful.
- acc.SetAccountNumber(uint64(i + 1000))
+ err := acc.SetAccountNumber(uint64(i + 1000))
+ require.NoError(t, err)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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (suite *AnteTestSuite) CreateTestAccounts(numAccs int) []TestAccount { | |
| var accounts []TestAccount | |
| for i := 0; i < numAccs; i++ { | |
| priv, _, addr := testdata.KeyTestPubAddr() | |
| acc := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr) | |
| acc.SetAccountNumber(uint64(i + 1000)) | |
| suite.accountKeeper.SetAccount(suite.ctx, acc) | |
| accounts = append(accounts, TestAccount{acc, priv}) | |
| } | |
| return accounts | |
| } | |
| func (suite *AnteTestSuite) CreateTestAccounts(numAccs int) []TestAccount { | |
| var accounts []TestAccount | |
| for i := 0; i < numAccs; i++ { | |
| priv, _, addr := testdata.KeyTestPubAddr() | |
| acc := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr) | |
| err := acc.SetAccountNumber(uint64(i + 1000)) | |
| require.NoError(t, err) | |
| suite.accountKeeper.SetAccount(suite.ctx, acc) | |
| accounts = append(accounts, TestAccount{acc, priv}) | |
| } | |
| return accounts | |
| } |
Tools
golangci-lint
120-120: Error return value of
acc.SetAccountNumberis not checked(errcheck)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores