feat: reduce gas for first vote by validators #2163
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a feature that reduces gas costs for validators' first votes in the Nibiru EVM project. The changes encompass multiple files, including the changelog, ante handler, and oracle module. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 64.86% 65.02% +0.16%
==========================================
Files 277 278 +1
Lines 22367 22421 +54
==========================================
+ Hits 14508 14580 +72
+ Misses 6869 6847 -22
- Partials 990 994 +4
|
|
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) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/app.go (1)
237-238: Consider adding nil checks for new keepers.Since these keepers are critical for the vote fee discount functionality, consider adding nil checks during initialization to fail fast if they're not properly set up.
+ if app.StakingKeeper == nil { + panic("StakingKeeper is nil") + } + if app.OracleKeeper == nil { + panic("OracleKeeper is nil") + } StakingKeeper: *app.StakingKeeper, OracleKeeper: app.OracleKeeper,🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 237-237: app.StakingKeeper undefined (type *NibiruApp has no field or method StakingKeeper)
(typecheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)app/app.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app/app.go
[error] 224-224: app.AccountKeeper undefined (type *NibiruApp has no field or method AccountKeeper)
(typecheck)
[error] 225-225: app.BankKeeper undefined (type *NibiruApp has no field or method BankKeeper)
(typecheck)
[error] 226-226: app.FeeGrantKeeper undefined (type *NibiruApp has no field or method FeeGrantKeeper)
(typecheck)
[error] 237-237: app.StakingKeeper undefined (type *NibiruApp has no field or method StakingKeeper)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
app/app.go (2)
224-226: LGTM! Clean code formatting.The keeper assignments are well-organized and properly indented, improving code readability.
🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 224-224: app.AccountKeeper undefined (type *NibiruApp has no field or method AccountKeeper)
(typecheck)
[error] 225-225: app.BankKeeper undefined (type *NibiruApp has no field or method BankKeeper)
(typecheck)
[error] 226-226: app.FeeGrantKeeper undefined (type *NibiruApp has no field or method FeeGrantKeeper)
(typecheck)
237-238: Verify keeper initialization order.The addition of
StakingKeeperandOracleKeeperaligns with the PR's objective to implement validator fee discounts. However, let's verify the initialization order of these keepers to prevent potential dependency issues.✅ Verification successful
Keeper initialization order is correct ✅
The initialization sequence is properly structured:
StakingKeeperis initialized with its required dependencies (AccountKeeper, BankKeeper)OracleKeeperis initialized afterward, receivingStakingKeeperas one of its dependencies🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the keeper initialization order in InitKeepers # to ensure StakingKeeper and OracleKeeper are initialized before being used. # Search for keeper initialization order rg -A 10 "func.*InitKeepers" app/ # Search for any circular dependencies between keepers ast-grep --pattern 'type $keeper struct { $$$ stakingKeeper $type $$$ oracleKeeper $type $$$ }'Length of output: 557
Script:
#!/bin/bash # Search for StakingKeeper and OracleKeeper initialization rg -B 5 -A 15 "app\.StakingKeeper\s*=|app\.OracleKeeper\s*=" app/ # Search for any dependencies between these keepers rg -B 2 -A 2 "OracleKeeper.*Staking|StakingKeeper.*Oracle" app/ # Look for keeper parameters to understand dependencies ast-grep --pattern 'NewKeeper($$$)'Length of output: 2790
🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 237-237: app.StakingKeeper undefined (type *NibiruApp has no field or method StakingKeeper)
(typecheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/common/testutil/testapp/testapp.go (2)
162-171: Add parameter validation for better error handling.While the implementation is solid, consider adding validation for the input parameters to fail fast with clear error messages:
- Validate that numAddrs is positive
- Validate that nibiruApp and ctx are not nil
func AddTestAddrsIncremental( nibiruApp *app.NibiruApp, ctx sdk.Context, numAddrs int, coins sdk.Coins, ) []sdk.ValAddress { + if nibiruApp == nil { + panic("nibiruApp cannot be nil") + } + if ctx == (sdk.Context{}) { + panic("context cannot be empty") + } + if numAddrs <= 0 { + panic("numAddrs must be positive") + } var valAddrs []sdk.ValAddress
174-194: Consider batch operations for better performance with large numAddrs.The current implementation processes accounts one by one. For better performance when generating many addresses, consider:
- Pre-allocating the slice capacity
- Using batch operations for account creation and funding
- var valAddrs []sdk.ValAddress + valAddrs := make([]sdk.ValAddress, 0, numAddrs) + // If funding is needed, prepare a batch of coins + var batchCoins sdk.Coins + if !coins.IsZero() { + batchCoins = sdk.NewCoins() + for i := 0; i < numAddrs; i++ { + batchCoins = batchCoins.Add(coins...) + } + if err := nibiruApp.BankKeeper.MintCoins(ctx, inflationtypes.ModuleName, batchCoins); err != nil { + panic(err) + } + } for i := 0; i < numAddrs; i++ { // 1. Generate a new private key pk := secp256k1.GenPrivKey() // 2. Derive an account address from the pubkey accAddr := sdk.AccAddress(pk.PubKey().Address()) // 3. Create a new base account object acc := nibiruApp.AccountKeeper.NewAccountWithAddress(ctx, accAddr) nibiruApp.AccountKeeper.SetAccount(ctx, acc) // 4. If the function call passed a non-empty `coins`, fund the new account if !coins.IsZero() { - if err := FundAccount(nibiruApp.BankKeeper, ctx, accAddr, coins); err != nil { + if err := nibiruApp.BankKeeper.SendCoinsFromModuleToAccount( + ctx, inflationtypes.ModuleName, accAddr, coins, + ); err != nil { panic(err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/common/testutil/testapp/testapp.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build
- GitHub Check: lint
- GitHub Check: e2e-evm
- GitHub Check: integration-tests
- GitHub Check: unit-tests
🔇 Additional comments (1)
x/common/testutil/testapp/testapp.go (1)
13-13: LGTM: Import added in the correct section.The secp256k1 import is appropriately placed with other Cosmos SDK imports and is necessary for private key generation in the new test utility function.
|
Note to merge after V2 |
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.