feat: reduce gas for first vote by validators #2163
Conversation
WalkthroughThis pull request introduces a new feature to reduce gas costs for validators' first votes in the Nibiru EVM project. The changes span multiple files, including the changelog, ante handler, and oracle module. A new Changes
Sequence DiagramsequenceDiagram
participant Tx as Transaction
participant Ante as AnteHandler
participant VoteDiscount as VoteFeeDiscountDecorator
participant Oracle as OracleKeeper
participant Staking as StakingKeeper
Tx->>Ante: Submit Transaction
Ante->>VoteDiscount: Check Transaction
VoteDiscount->>Staking: Is Validator Valid?
Staking-->>VoteDiscount: Validator Details
VoteDiscount->>Oracle: Has Voted in Current Period?
Oracle-->>VoteDiscount: Voting Status
alt First Vote
VoteDiscount->>Ante: Reduce Gas Fees
else Already Voted
VoteDiscount->>Ante: Standard Gas Fees
end
Assessment against linked issues
Possibly related PRs
Poem
Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- app/ante/handler_opts.go: Evaluated as low risk
- app/app.go: Evaluated as low risk
- CHANGELOG.md: Evaluated as low risk
- x/oracle/keeper/ballot.go: Evaluated as low risk
Comments suppressed due to low confidence (2)
x/oracle/ante/fee_discount.go:36
- [nitpick] The variable name 'vfd' is ambiguous. It should be renamed to 'voteFeeDiscountDecorator' for better readability.
vfd VoteFeeDiscountDecorator
x/oracle/ante/fee_discount.go:46
- The error message should be more descriptive. Suggest changing to 'invalid transaction type for VoteFeeDiscountDecorator: %T'.
return ctx, sdkerrors.Wrapf(sdkerrors.ErrTxDecode, "invalid tx type %T", tx)
…biru into mat/reduce-gas-oracle-txs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2163 +/- ##
==========================================
+ Coverage 65.14% 65.22% +0.07%
==========================================
Files 277 278 +1
Lines 22238 22292 +54
==========================================
+ Hits 14488 14539 +51
- Misses 6760 6762 +2
- Partials 990 991 +1
|
|
This is great. We've had this ticket open a while |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/ante/handler_opts.go (1)
Line range hint
55-57: Validate New Keepers in AnteHandlerOptionsThe
ValidateAndCleanmethod should include checks to ensure thatOracleKeeperandStakingKeeperare notnil, similar to the other keepers. This prevents potentialnilpointer dereferences during AnteHandler execution.Apply this diff to add validation:
func (opts *AnteHandlerOptions) ValidateAndClean() error { if opts.BankKeeper == nil { return AnteHandlerError("bank keeper") } + if opts.OracleKeeper == nil { + return AnteHandlerError("oracle keeper") + } + if opts.StakingKeeper == nil { + return AnteHandlerError("staking keeper") + }
🧹 Nitpick comments (9)
x/oracle/ante/fee_discount.go (4)
19-22: Consider Interface Naming Conventions for ClarityThe interfaces
OracleKeeperIandStakingKeeperIcould be renamed toOracleKeeperandStakingKeeperrespectively, removing the trailingI. This aligns with Go naming conventions, where interface names are typically not suffixed unless necessary for disambiguation.
37-42: Simplify AnteDecorator Logic for Multiple SignersCurrently, the decorator checks for exactly one signer and passes through if there are multiple signers (lines 54-57). If the discount is intended only for single-signer transactions, consider explicitly handling multi-signer transactions by returning an error or documenting the intended behavior for clarity.
59-66: Extend Message Type Checks to Support Future Oracle MessagesThe current implementation only allows
MsgAggregateExchangeRatePrevoteandMsgAggregateExchangeRateVote. To enhance flexibility, consider supporting additional oracle-related message types or refactoring the check to accommodate future message expansions.
13-18: Remove Residual Comments from Template CodeThe comments in lines 13-18 suggest that this code may be adapted from a template or example. For production code, remove or update these comments to reflect the actual logic and context of the application to avoid confusion.
x/oracle/ante/keeper_interface.go (1)
16-18: Document Interface Purpose and UsageAdd comments to the
OracleKeeperIinterface to clarify its role and how it should be implemented. This will aid in maintenance and ensure consistent implementation across different modules.app/ante/handler_opts.go (2)
13-14: Optimize Imports for ConsistencyThe imports for
stakingkeeperandoraclekeepercould be grouped with related imports for clarity and to adhere to Go import grouping conventions (standard library, third-party, local packages).
29-31: Use Interfaces Instead of Concrete Types in AnteHandlerOptionsIn the
AnteHandlerOptionsstruct, consider using interfaces (StakingKeeperIandOracleKeeperI) instead of concrete keeper types. This promotes abstraction and makes the codebase more modular and testable.Apply this diff to use interfaces:
- OracleKeeper oraclekeeper.Keeper - StakingKeeper stakingkeeper.Keeper + OracleKeeper ante.OracleKeeperI + StakingKeeper ante.StakingKeeperIx/oracle/keeper/ballot.go (1)
104-107: Add documentation and improve error handling.The method implementation is correct but could benefit from:
- Documentation explaining the return value (true if voted, false otherwise).
- Explicit error type checking instead of using a generic error check.
Apply this diff to improve the implementation:
+// HasVotedInCurrentPeriod returns true if the validator has already voted in the +// current voting period, false otherwise. func (k Keeper) HasVotedInCurrentPeriod(ctx sdk.Context, valAddr sdk.ValAddress) bool { - _, err := k.Votes.Get(ctx, valAddr) - return err == nil + _, err := k.Votes.Get(ctx, valAddr) + return !collections.IsNotFound(err) }x/oracle/ante/fee_discount_test.go (1)
62-111: Consider adding validation for empty signers array.The buildTestTx helper function is well-implemented but should validate that the signers array is not empty to prevent potential panics.
Add this validation at the start of the function:
func (s *VoteFeeDiscountTestSuite) buildTestTx(msgs []sdk.Msg, signers []sdk.AccAddress) (sdk.Tx, error) { + if len(signers) == 0 { + return nil, fmt.Errorf("at least one signer is required") + } // Rest of the implementation...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)app/ante.go(2 hunks)app/ante/handler_opts.go(2 hunks)app/app.go(2 hunks)x/common/testutil/testapp/testapp.go(2 hunks)x/oracle/ante/fee_discount.go(1 hunks)x/oracle/ante/fee_discount_test.go(1 hunks)x/oracle/ante/keeper_interface.go(1 hunks)x/oracle/keeper/ballot.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (8)
x/oracle/ante/keeper_interface.go (1)
10-13: Align Interface Methods with Required FunctionalityVerify that the
StakingKeeperIinterface includes all necessary methods required by theVoteFeeDiscountDecorator. If future methods are needed, consider embeddingstakingtypes.StakingKeeperor selectively adding methods to minimize interface exposure.app/ante.go (1)
76-76: LGTM! Appropriate placement of the VoteFeeDiscountDecorator.The decorator is correctly positioned in the ante handler chain:
- After basic validation (ValidateBasic)
- Before fee deduction
- Before memo validation and timeout checks
x/common/testutil/testapp/testapp.go (1)
160-195: LGTM! Well-structured test utility function.The implementation is clean and well-documented with:
- Clear step-by-step process
- Appropriate error handling
- Proper key generation using secp256k1
- Optional funding capability
x/oracle/ante/fee_discount_test.go (2)
23-44: LGTM! Clean mock implementations.The mock implementations for StakingKeeper and OracleKeeper are minimal yet sufficient for testing purposes.
138-342: LGTM! Comprehensive test coverage.The test scenarios are thorough and cover:
- Happy path
- Invalid transaction types
- Multiple signers
- Non-oracle messages
- Validator conditions (not found, jailed, threshold)
- Already voted case
app/app.go (2)
219-221: LGTM! Code formatting improvement.The reformatting of keeper assignments improves code readability.
232-233: LGTM! Required keepers for vote fee discount feature.The addition of
StakingKeeperandOracleKeeperto the ante handler options is necessary for implementing the validator vote fee discount feature.CHANGELOG.md (1)
265-265: LGTM! Clear and concise changelog entry.The changelog entry correctly documents the feature addition and is placed in the appropriate section since it modifies state machine behavior around gas fees.
| // 3. If validator, we check whether they've posted a vote this period | ||
| hasVoted := vfd.oracleKeeper.HasVotedInCurrentPeriod(ctx, valAddr) | ||
| if !hasVoted { | ||
| // 4. If first time, let's say we reduce gas cost. | ||
| minGasPrices := ctx.MinGasPrices() | ||
| var discounted []sdk.DecCoin | ||
| for _, mgp := range minGasPrices { | ||
| discounted = append(discounted, sdk.NewDecCoinFromDec(mgp.Denom, mgp.Amount.QuoInt64(69_420))) | ||
| } | ||
| // We'll create a new context with the updated MinGasPrices | ||
| ctx = ctx.WithMinGasPrices(discounted) | ||
| } |
There was a problem hiding this comment.
Ensure Gas Discount Logic Correctly Applies Discounts
The gas price reduction divides the MinGasPrices by 69,420 (line 94), which seems arbitrary and may reduce fees below acceptable limits, potentially affecting network security.
Consider revising the discount factor to a more appropriate value or making it configurable.
Apply this diff to adjust the discount factor:
- discounted = append(discounted, sdk.NewDecCoinFromDec(mgp.Denom, mgp.Amount.QuoInt64(69_420)))
+ discountFactor := sdk.NewDecWithPrec(5, 1) // 50% discount
+ discountedAmount := mgp.Amount.Mul(discountFactor)
+ discounted = append(discounted, sdk.NewDecCoinFromDec(mgp.Denom, discountedAmount))Committable suggestion skipped: line range outside the PR's diff.
| // needs to have at least 0.5% of the supply bonded | ||
| totalBonded := vfd.stakingKeeper.TotalBondedTokens(ctx) | ||
| currentlyBonded := validator.Tokens | ||
|
|
||
| if currentlyBonded.LT(totalBonded.Mul(math.NewInt(50)).Quo(math.NewInt(10000))) { | ||
| return next(ctx, tx, simulate) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust Bonded Tokens Threshold Calculation for Precision
The calculation for determining if a validator has at least 0.5% of the bonded tokens (line 83) uses integer arithmetic, which may lead to imprecise results due to truncation. Consider using precise decimal arithmetic with sdk.Dec to ensure accuracy in the threshold calculation.
Apply this diff to enhance precision:
- if currentlyBonded.LT(totalBonded.Mul(math.NewInt(50)).Quo(math.NewInt(10000))) {
+ threshold := sdk.NewDecFromInt(totalBonded).Mul(sdk.NewDecWithPrec(5, 3)) // 0.5%
+ if sdk.NewDecFromInt(currentlyBonded).LT(threshold) {📝 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.
| // needs to have at least 0.5% of the supply bonded | |
| totalBonded := vfd.stakingKeeper.TotalBondedTokens(ctx) | |
| currentlyBonded := validator.Tokens | |
| if currentlyBonded.LT(totalBonded.Mul(math.NewInt(50)).Quo(math.NewInt(10000))) { | |
| return next(ctx, tx, simulate) | |
| } | |
| // needs to have at least 0.5% of the supply bonded | |
| totalBonded := vfd.stakingKeeper.TotalBondedTokens(ctx) | |
| currentlyBonded := validator.Tokens | |
| threshold := sdk.NewDecFromInt(totalBonded).Mul(sdk.NewDecWithPrec(5, 3)) // 0.5% | |
| if sdk.NewDecFromInt(currentlyBonded).LT(threshold) { | |
| return next(ctx, tx, simulate) | |
| } |
Purpose / Abstract
This pull request includes several changes to the
Nibiruapplication, focusing on adding a new fee discount mechanism for validators who vote in the current period, updating theAnteHandleroptions, and enhancing test utilities. The most important changes include the addition of theVoteFeeDiscountDecorator, updates to theAnteHandlerOptionsstruct, and the implementation of new test utilities.Fee Discount Mechanism:
VoteFeeDiscountDecoratorto apply a fee discount for validators who vote in the current period (x/oracle/ante/fee_discount.go).VoteFeeDiscountDecoratorto ensure correct functionality (x/oracle/ante/fee_discount_test.go).StakingKeeperandOracleKeeperto be used in theVoteFeeDiscountDecorator(x/oracle/ante/keeper_interface.go).AnteHandler Options:
AnteHandlerOptionsstruct to includeOracleKeeperandStakingKeeper(app/ante/handler_opts.go).NewAnteHandlerNonEVMto include the newVoteFeeDiscountDecorator(app/ante.go).Test Utilities:
AddTestAddrsIncrementalfunction to generate test addresses and optionally fund them (x/common/testutil/testapp/testapp.go).Additional Changes:
oracleanteandstakingkeeperin relevant files (app/ante.go,app/ante/handler_opts.go). [1] [2]NewNibiruAppto includeOracleKeeperandStakingKeeperin the options (app/app.go).These changes enhance the functionality and testability of the
Nibiruapplication by introducing a new fee discount mechanism for validators and improving the configuration and testing capabilities of the application.Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
Release Notes
New Features
Improvements
Technical Updates
The release focuses on optimizing validator interactions and improving the testing infrastructure for the Nibiru EVM project.