eip1559 fee checker #372
eip1559 fee checker #372
Conversation
📝 WalkthroughWalkthroughThe update deprecates the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TxService
participant MsgServer
participant Keeper
Client->>TxService: Submit MsgUpdateParams
TxService->>MsgServer: Forward update request
MsgServer->>Keeper: Validate and call SetParams
Keeper-->>MsgServer: Confirm parameter update
MsgServer-->>TxService: Return update response
TxService-->>Client: Confirm success
sequenceDiagram
participant TxHandler
participant BlockGasDecorator
participant BlockGasMeter
TxHandler->>BlockGasDecorator: Process transaction
BlockGasDecorator->>BlockGasMeter: Accumulate used gas
BlockGasMeter-->>BlockGasDecorator: Acknowledge accumulation
BlockGasDecorator-->>TxHandler: Continue tx processing
Poem
🪧 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![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
🧹 Nitpick comments (7)
x/move/ante/fee.go (2)
64-64: Return an error instead of a default priority value.Changed to properly propagate the error when fetching the price fails instead of continuing with a default priority value. This improves error handling by ensuring invalid transactions aren't processed with default values.
Also consider adding context to the error with
errors.Wrapfto provide more information about which coin's price could not be fetched.
31-120: Consider refactoring the complex fee checking logic.The
CheckTxFeeWithMinGasPricesmethod has become quite complex with multiple error return points and nested conditions. The EIP-1559 integration adds additional complexity.Consider breaking down this method into smaller, more focused helper methods to improve readability and maintainability. For example:
- Extract base fee validation into a separate method
- Extract min gas price validation into a separate method
- Consider using early returns to reduce nesting levels
x/move/types/eip1559_params.go (1)
34-64: Add validation for MinBaseFee vs MaxBaseFee relationship.The validation function thoroughly checks for non-negative values and appropriate ranges. However, it doesn't explicitly validate that MinBaseFee < MaxBaseFee, which could allow an invalid configuration where MinBaseFee > MaxBaseFee.
Consider adding this validation:
func (p EIP1559FeeParams) Validate() error { // existing validations... + if p.MinBaseFee > p.MaxBaseFee { + return fmt.Errorf("min base fee must be less than or equal to max base fee") + } return nil }x/move/keeper/msg_server_test.go (1)
239-260: Enhance test coverage for EIP1559 fee parameter updates.The test successfully verifies that valid parameters can be updated and retrieved. However, it could be improved to test more scenarios:
- It doesn't test the error case when the authority is invalid
- It doesn't test parameter validation failures
- The
TargetGasparameter is missing in the test parametersConsider expanding the test with these additional test cases:
func Test_UpdateEIP1559FeeParams(t *testing.T) { ctx, input := createDefaultTestInput(t) ms := keeper.NewMsgServerImpl(&input.MoveKeeper) // Test successful update msg := &types.MsgUpdateEIP1559FeeParams{ Authority: input.MoveKeeper.GetAuthority(), Eip1559Feeparams: types.EIP1559FeeParams{ BaseFee: 100, MaxBaseFee: 200, MinBaseFee: 10, + TargetGas: 500_000, MaxChangeRate: math.LegacyNewDecWithPrec(10, 2), }, } _, err := ms.UpdateEIP1559FeeParams(ctx, msg) require.NoError(t, err) eip1559FeeKeeper := keeper.NewEIP1559FeeKeeper(&input.MoveKeeper) params, err := eip1559FeeKeeper.GetParams(ctx) require.NoError(t, err) require.Equal(t, msg.Eip1559Feeparams, params) + // Test invalid authority + msgInvalidAuth := &types.MsgUpdateEIP1559FeeParams{ + Authority: "invalid_authority", + Eip1559Feeparams: types.EIP1559FeeParams{ + BaseFee: 100, + MaxBaseFee: 200, + MinBaseFee: 10, + TargetGas: 500_000, + MaxChangeRate: math.LegacyNewDecWithPrec(10, 2), + }, + } + _, err = ms.UpdateEIP1559FeeParams(ctx, msgInvalidAuth) + require.Error(t, err) + + // Test invalid parameters (BaseFee > MaxBaseFee) + msgInvalidParams := &types.MsgUpdateEIP1559FeeParams{ + Authority: input.MoveKeeper.GetAuthority(), + Eip1559Feeparams: types.EIP1559FeeParams{ + BaseFee: 300, + MaxBaseFee: 200, + MinBaseFee: 10, + TargetGas: 500_000, + MaxChangeRate: math.LegacyNewDecWithPrec(10, 2), + }, + } + _, err = ms.UpdateEIP1559FeeParams(ctx, msgInvalidParams) + require.Error(t, err) }x/move/keeper/genesis.go (1)
154-237: Consider optimizing EIP1559FeeKeeper instantiation.The code creates new instances of
EIP1559FeeKeeperin both theInitGenesisandExportGenesismethods. This approach works but may be less efficient than storing a single instance as a field in theKeeperstruct.Consider refactoring to store the
EIP1559FeeKeeperas a field in theKeeperstruct:type Keeper struct { // existing fields + eip1559FeeKeeper *EIP1559FeeKeeper } func NewKeeper(...) Keeper { k := Keeper{ // initialize existing fields } + k.eip1559FeeKeeper = NewEIP1559FeeKeeper(&k) return k } // Then in InitGenesis and ExportGenesis: - eip1559FeeKeeper := NewEIP1559FeeKeeper(&k) + // Use k.eip1559FeeKeeper directlyThis would avoid creating multiple instances and make the code more maintainable.
x/move/keeper/eip1559.go (2)
39-60: Fee updating mechanism implements EIP-1559 algorithm correctly.The fee adjustment follows the formula:
(gasUsed - targetGas) / targetGas * maxChangeRate + 1, which is consistent with EIP-1559 design. The implementation also properly:
- Retrieves current parameters with error handling
- Gets block gas consumption from context
- Calculates the multiplier based on usage vs target
- Applies multiplier to current base fee
- Constrains the result within min/max bounds
- Updates parameters with error handling
Consider adding a mechanism to handle edge cases like very first block execution or chain restarts where historical gas usage might not be available or relevant.
49-49: Base fee multiplier calculation is complex and could benefit from clarification.The calculation is mathematically correct but combines multiple operations in a single line, making it harder to understand and maintain.
Consider breaking down the calculation into smaller, more readable steps:
- baseFeeMultiplier := math.LegacyNewDec(int64(gasUsed) - params.TargetGas).QuoInt64(params.TargetGas).Mul(params.MaxChangeRate).Add(math.OneInt().ToLegacyDec()) + // Calculate how much gas was used relative to target (can be negative) + gasUsedDelta := int64(gasUsed) - params.TargetGas + // Calculate the percentage of deviation from target + gasUsedRatio := math.LegacyNewDec(gasUsedDelta).QuoInt64(params.TargetGas) + // Apply the maximum change rate to the ratio + adjustmentFactor := gasUsedRatio.Mul(params.MaxChangeRate) + // Base multiplier is 1 + adjustment factor (can be less than 1 if gas used < target) + baseFeeMultiplier := math.OneInt().ToLegacyDec().Add(adjustmentFactor)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
api/initia/move/v1/query_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapi/initia/move/v1/tx_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapi/initia/mstaking/v1/query_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/move/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (32)
api/initia/move/v1/genesis.pulsar.go(18 hunks)api/initia/move/v1/query.pulsar.go(5 hunks)api/initia/move/v1/tx.pulsar.go(5 hunks)api/initia/move/v1/types.pulsar.go(6 hunks)api/initia/mstaking/v1/query.pulsar.go(25 hunks)app/blocksdk.go(1 hunks)app/modules.go(1 hunks)proto/initia/move/v1/genesis.proto(1 hunks)proto/initia/move/v1/query.proto(2 hunks)proto/initia/move/v1/tx.proto(2 hunks)proto/initia/move/v1/types.proto(1 hunks)x/move/abci.go(2 hunks)x/move/abci_test.go(3 hunks)x/move/ante/fee.go(4 hunks)x/move/ante/fee_test.go(3 hunks)x/move/common_test.go(1 hunks)x/move/keeper/ante.go(1 hunks)x/move/keeper/common_test.go(1 hunks)x/move/keeper/dex.go(0 hunks)x/move/keeper/eip1559.go(1 hunks)x/move/keeper/eip1559_test.go(1 hunks)x/move/keeper/genesis.go(2 hunks)x/move/keeper/keeper.go(5 hunks)x/move/keeper/msg_server.go(1 hunks)x/move/keeper/msg_server_test.go(2 hunks)x/move/keeper/querier.go(1 hunks)x/move/keeper/querier_test.go(1 hunks)x/move/module.go(1 hunks)x/move/types/eip1559_params.go(1 hunks)x/move/types/exported.go(1 hunks)x/move/types/genesis.go(1 hunks)x/move/types/keys.go(1 hunks)
💤 Files with no reviewable changes (1)
- x/move/keeper/dex.go
🧰 Additional context used
🧬 Code Definitions (14)
x/move/module.go (1)
x/move/abci.go (1) (1)
EndBlocker(106-111)
x/move/keeper/querier.go (2)
api/initia/move/v1/types.pulsar.go (3) (3)
EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)api/initia/move/v1/query.pulsar.go (6) (6)
QueryEIP1559FeeParamsRequest(19104-19108)QueryEIP1559FeeParamsRequest(19123-19123)QueryEIP1559FeeParamsRequest(19126-19128)QueryEIP1559FeeParamsResponse(19131-19137)QueryEIP1559FeeParamsResponse(19152-19152)QueryEIP1559FeeParamsResponse(19155-19157)
x/move/types/genesis.go (1)
x/move/types/eip1559_params.go (1) (1)
DefaultEIP1559Params(24-32)
app/blocksdk.go (3)
x/move/keeper/ante.go (1) (1)
NewAnteKeeper(16-21)x/move/keeper/dex.go (1) (1)
NewDexKeeper(24-26)x/move/keeper/eip1559.go (1) (1)
NewEIP1559FeeKeeper(16-20)
x/move/keeper/eip1559_test.go (3)
x/move/keeper/common_test.go (1) (1)
createDefaultTestInput(244-246)x/move/keeper/eip1559.go (1) (1)
NewEIP1559FeeKeeper(16-20)api/initia/move/v1/types.pulsar.go (3) (3)
EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)
x/move/keeper/common_test.go (2)
x/move/keeper/eip1559.go (1) (1)
NewEIP1559FeeKeeper(16-20)api/initia/move/v1/types.pulsar.go (3) (3)
EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)
x/move/keeper/ante.go (3)
x/move/keeper/dex.go (12) (12)
DexKeeper(19-21)k(30-45)k(49-55)k(58-68)k(71-76)k(80-90)k(94-99)k(102-119)k(122-132)k(136-146)k(150-160)k(162-168)x/move/keeper/eip1559.go (1) (1)
EIP1559FeeKeeper(12-14)x/move/types/exported.go (1) (1)
AnteKeeper(9-15)
x/move/keeper/msg_server_test.go (3)
x/move/keeper/msg_server.go (15) (15)
ms(29-57)ms(60-97)ms(100-137)ms(140-176)ms(179-209)ms(212-245)ms(248-289)ms(292-333)ms(336-370)ms(373-407)ms(409-426)ms(428-445)ms(447-463)ms(465-481)_(21-21)api/initia/move/v1/tx.pulsar.go (3) (3)
MsgUpdateEIP1559FeeParams(16210-16220)MsgUpdateEIP1559FeeParams(16235-16235)MsgUpdateEIP1559FeeParams(16238-16240)api/initia/move/v1/types.pulsar.go (18) (18)
EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)_(17-17)_(82-82)_(103-103)_(760-760)_(781-781)_(1364-1364)_(1385-1385)_(2022-2022)_(2043-2043)_(2574-2574)_(2595-2595)_(3186-3186)_(3207-3207)_(3738-3738)_(3759-3759)
api/initia/move/v1/tx.pulsar.go (2)
api/initia/move/v1/types.pulsar.go (79) (79)
init(72-80)init(751-758)init(1354-1362)init(2014-2020)init(2565-2572)init(3178-3184)init(3728-3736)init(4404-4408)init(4809-4814)init(5340-5346)init(5904-5912)init(7404-7404)x(23-28)x(30-32)x(34-38)x(40-44)x(46-48)x(50-52)x(54-57)x(59-61)x(86-88)x(90-100)x(107-109)x(110-112)x(113-115)x(119-121)x(126-128)x(131-133)EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)_(17-17)_(82-82)_(103-103)_(760-760)_(781-781)_(1364-1364)_(1385-1385)_(2022-2022)_(2043-2043)_(2574-2574)_(2595-2595)_(3186-3186)_(3207-3207)_(3738-3738)_(3759-3759)_(4410-4410)l(419-419)l(465-465)l(1068-1068)l(1108-1108)l(1691-1691)l(1735-1735)l(2311-2311)l(2348-2348)l(2882-2882)l(2923-2923)l(3475-3475)l(3512-3512)l(4065-4065)l(4110-4110)l(4661-4661)l(4689-4689)n(418-418)n(1067-1067)n(1690-1690)n(2310-2310)n(2881-2881)n(3474-3474)n(4064-4064)n(4660-4660)n(5085-5085)n(5646-5646)n(6240-6240)Params(6575-6589)Params(6604-6604)Params(6607-6609)v(660-660)v(1294-1294)api/initia/move/v1/genesis.pulsar.go (1) (1)
v(55-55)
x/move/types/eip1559_params.go (1)
api/initia/move/v1/types.pulsar.go (3) (3)
EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)
x/move/abci_test.go (2)
api/initia/move/v1/types.pulsar.go (3) (3)
EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)x/move/types/eip1559_params.go (1) (1)
DefaultBaseFee(11-11)
api/initia/move/v1/genesis.pulsar.go (1)
api/initia/move/v1/types.pulsar.go (67) (67)
x(23-28)x(30-32)x(34-38)x(40-44)x(46-48)x(50-52)x(54-57)x(59-61)x(86-88)x(90-100)x(107-109)x(110-112)x(113-115)x(119-121)x(126-128)x(131-133)EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)l(419-419)l(465-465)l(1068-1068)l(1108-1108)l(1691-1691)l(1735-1735)l(2311-2311)l(2348-2348)l(2882-2882)l(2923-2923)l(3475-3475)l(3512-3512)l(4065-4065)l(4110-4110)l(4661-4661)l(4689-4689)n(418-418)n(1067-1067)n(1690-1690)n(2310-2310)n(2881-2881)n(3474-3474)n(4064-4064)n(4660-4660)n(5085-5085)n(5646-5646)n(6240-6240)Params(6575-6589)Params(6604-6604)Params(6607-6609)Module(6709-6719)Module(6734-6734)Module(6737-6739)Checksum(6777-6785)Checksum(6800-6800)Checksum(6803-6805)Resource(6829-6838)Resource(6853-6853)Resource(6856-6858)TableInfo(6889-6897)TableInfo(6912-6912)TableInfo(6915-6917)TableEntry(6941-6951)TableEntry(6966-6966)TableEntry(6969-6971)DexPair(7046-7053)DexPair(7068-7068)DexPair(7071-7073)
api/initia/move/v1/query.pulsar.go (1)
api/initia/move/v1/types.pulsar.go (8) (8)
EIP1559FeeParams(7144-7154)EIP1559FeeParams(7169-7169)EIP1559FeeParams(7172-7174)Params(6575-6589)Params(6604-6604)Params(6607-6609)v(660-660)v(1294-1294)
api/initia/move/v1/types.pulsar.go (2)
api/initia/move/v1/genesis.pulsar.go (33) (33)
init(383-396)init(1771-1771)x(23-28)x(30-32)x(34-38)x(40-44)x(46-48)x(50-52)x(54-57)x(59-61)x(69-74)x(76-78)x(80-84)x(86-90)x(92-96)x(98-103)x(105-108)x(110-112)_(17-17)_(63-63)_(114-114)_(165-165)_(216-216)_(267-267)_(318-318)_(398-398)_(419-419)_(1539-1539)_(1541-1541)l(898-898)l(976-976)n(897-897)v(55-55)api/initia/move/v1/tx.pulsar.go (53) (53)
init(72-78)init(615-618)init(1069-1078)init(1831-1834)init(2285-2294)init(3047-3050)init(3499-3506)init(4137-4140)init(4589-4596)init(5227-5230)init(5633-5640)init(6239-6242)init(6694-6704)_(19-19)_(80-80)_(101-101)_(620-620)_(641-641)l(379-379)l(417-417)l(852-852)l(877-877)l(1446-1446)l(1499-1499)l(2068-2068)l(2093-2093)l(2662-2662)l(2715-2715)l(3284-3284)l(3309-3309)l(3836-3836)l(3881-3881)n(378-378)n(851-851)n(1445-1445)n(2067-2067)n(2661-2661)n(3283-3283)n(3835-3835)n(4373-4373)n(4925-4925)n(5463-5463)n(5959-5959)n(6475-6475)n(7090-7090)n(7755-7755)n(8370-8370)v(57-57)v(1051-1051)v(3483-3483)v(5617-5617)v(6675-6675)v(9235-9235)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
🔇 Additional comments (89)
api/initia/move/v1/types.pulsar.go (4)
5895-6209: Well-structured implementation of EIP-1559 fee parametersThe addition of
EIP1559FeeParamsmessage type provides a comprehensive implementation of Ethereum's EIP-1559 fee market mechanism. This structure properly includes all necessary parameters:
- Base fee for transactions
- Minimum and maximum bounds for the base fee
- Target gas usage to determine fee adjustments
- Maximum change rate to limit volatility
The implementation uses appropriate data types for each field and includes proper annotations for decimal handling in the
max_change_ratefield.
7144-7209: Good implementation of getter methods for EIP1559FeeParamsThe getter methods follow the standard Go protobuf pattern and provide appropriate default values when the fields are not set.
7530-7541: Consistent message exporter implementationThe exporter function for
EIP1559FeeParamsproperly handles the state, sizeCache, and unknownFields as required by the protobuf implementation.
7379-7379: Proper message type registrationThe modification of the message types count (from 10 to 11) and updating the NumMessages value ensures the new EIP1559FeeParams message is properly registered with the protobuf system.
Also applies to: 7549-7549
x/move/types/keys.go (1)
65-66: Key addition for EIP-1559 fee parameters looks correct.The addition of
EIP1559FeeParamsKeywith value0x32follows the existing pattern of defining byte keys for the move store. This is a necessary change to support storing and retrieving EIP-1559 fee parameters in the state.x/move/types/exported.go (1)
14-14: Interface extension for EIP-1559 support is correctly implemented.Adding the
GetBaseFeemethod to theAnteKeeperinterface is necessary for the ante handler to access the current base fee when validating transaction fees under the EIP-1559 model. This enables proper fee validation during transaction processing.proto/initia/move/v1/genesis.proto (1)
48-52: Genesis state properly extended with EIP-1559 fee parameters.The addition of the
eip1559_feeparamsfield to theGenesisStatemessage ensures that EIP-1559 fee parameters can be initialized at genesis. The field is correctly configured with appropriate serialization options.app/modules.go (1)
220-220:✅ Verification successful
EndBlocker execution order includes move module for fee updates.
Adding
movetypes.ModuleNameto theorderEndBlockersfunction is necessary to ensure the base fee is updated at the end of each block based on network congestion, which is a core component of the EIP-1559 fee model.Verify the implementation with:
🏁 Script executed:
#!/bin/bash # Check that the EndBlocker in the move module updates the base fee rg -A 10 "EndBlocker" x/move/Length of output: 1813
Verified: EndBlocker in the move module correctly updates the base fee as intended.
The grep output confirms that the EndBlocker implementation inx/move/abci.goproperly callsk.EIP1559FeeKeeper().UpdateBaseFee(sdk.UnwrapSDKContext(ctx)). Furthermore, the test case inx/move/abci_test.govalidates that the move module’s EndBlocker is functioning as required. Includingmovetypes.ModuleNamein the execution order is correctly triggering the base fee update in line with the EIP-1559 fee model.x/move/keeper/querier_test.go (1)
469-481: Well-structured test for EIP1559FeeParams querierThe test correctly validates that the querier method properly fetches and returns EIP1559 fee parameters. It follows the same pattern as other tests in the file by setting up the context, invoking the method, and verifying the results against direct keeper access.
x/move/types/genesis.go (1)
33-33: Good addition to genesis state initializationThe EIP1559 fee parameters are correctly initialized in the genesis state using the default parameters. This ensures that the blockchain will start with properly configured fee parameters.
x/move/keeper/querier.go (1)
460-470: Clean implementation of the EIP1559FeeParams query methodThe method follows the established pattern in the codebase for query methods. It correctly retrieves the parameters from the fee keeper and handles errors appropriately.
app/blocksdk.go (1)
94-94: Properly integrates EIP1559FeeKeeper into the ante handlerThe modified code correctly initializes an AnteKeeper with both the DexKeeper and EIP1559FeeKeeper, enabling EIP1559 fee validation during transaction processing. This is essential for the EIP1559 implementation to take effect in transaction validation.
x/move/keeper/common_test.go (1)
422-429: Well implemented EIP1559FeeKeeper initialization in test setup.The code correctly initializes the EIP1559FeeKeeper with appropriate test parameters, including min/max base fees, target gas, and change rate. This setup is essential for testing the EIP-1559 fee mechanism in isolation.
proto/initia/move/v1/types.proto (1)
139-150: Good implementation of EIP1559FeeParams message.The EIP1559FeeParams message structure is well-defined with appropriate field types and annotations. The use of
cosmossdk.io/math.LegacyDecfor max_change_rate allows for precise decimal values needed for fee calculations.x/move/module.go (1)
157-160: Properly implemented EndBlock hook for fee updates.The EndBlock method correctly calls the EndBlocker function, which is responsible for updating the base fee at the end of each block based on gas usage. This implementation follows the standard pattern for Cosmos SDK modules.
Note that this implementation completes the integration of the EIP-1559 fee mechanism by ensuring base fees are recalculated at the end of each block.
x/move/common_test.go (1)
114-119: Useful gas info extraction for fee testing.This helper function extends the existing executeMsgs function to also return gas information, which is essential for verifying the correct behavior of the EIP-1559 fee mechanism in tests.
x/move/keeper/keeper.go (5)
43-43: New field added for EIP1559 fee management.The
eip1559FeeKeeperfield is correctly added to theKeeperstruct, following the existing pattern of including specific keepers for different functionalities.
61-61: New storage item for EIP1559 fee parameters.The addition of
EIP1559FeeParamscollection item is consistent with other storage-related fields in theKeeperstruct. This field will store the fee parameters needed for the EIP-1559 mechanism.
134-134: Storage initialization for EIP1559 fee parameters.The
EIP1559FeeParamsfield is correctly initialized with the proper key, name, and codec. This follows the pattern of other collection initializations in theNewKeeperfunction.
150-150: Instantiation of EIP1559FeeKeeper.The
eip1559FeeKeeperis properly initialized by passing the keeper instance toNewEIP1559FeeKeeper. This follows the pattern used for other internal keepers.
191-194: Accessor method for EIP1559FeeKeeper.The
EIP1559FeeKeeper()method follows the established pattern of providing access to internal keepers. This ensures consistent access to the EIP1559 fee management functionality.x/move/keeper/ante.go (4)
9-12: New AnteKeeper struct for transaction validation.The
AnteKeeperstruct is designed to handle transaction validation, embeddingDexKeeperand including theeip1559FeeKeeper. This approach provides access to both DEX and EIP1559 fee functionality for transaction validation.
14-14: Interface validation for AnteKeeper.The variable declaration ensures that
AnteKeeperimplements thetypes.AnteKeeperinterface at compile time. This is a good practice to catch interface implementation issues early.
16-21: Constructor function for AnteKeeper.The
NewAnteKeeperfunction properly initializes anAnteKeeperinstance with the required dependencies. This follows the standard Go pattern for constructors.
23-25: Method to retrieve the base fee.The
GetBaseFeemethod delegates to theeip1559FeeKeeper's implementation, following the principle of delegation to specialized components. This method is required to fulfill thetypes.AnteKeeperinterface.x/move/abci.go (2)
13-14: New SDK import for context unwrapping.Adding the SDK import is necessary for unwrapping the SDK context in the new
EndBlockerfunction. This is a standard pattern in Cosmos SDK modules.
106-111: New EndBlocker function for updating the base fee.The
EndBlockerfunction correctly implements the EIP-1559 base fee update at the end of each block. It follows the standard pattern with telemetry measurement and delegates the actual update to theEIP1559FeeKeeper. This is critical for the EIP-1559 fee mechanism to adjust fees based on network demand.x/move/ante/fee.go (2)
75-88: Added base fee validation for EIP-1559.The code correctly implements EIP-1559 base fee validation by checking if the total fee in base amount meets the required base fee. This is essential for the EIP-1559 fee mechanism to work properly, ensuring that transactions include at least the minimum required base fee.
111-111: Consistent use of totalFeeBaseAmount in error message.Updated the error message to use
totalFeeBaseAmountconsistently, which improves code clarity and ensures the error message accurately reflects the actual values being compared.x/move/keeper/msg_server.go (1)
464-481: New governance message handler for EIP1559 fee parameter updatesThe implementation follows the standard pattern for governance parameter updates, with proper authority validation, parameter validation, and telemetry tracking.
x/move/ante/fee_test.go (3)
25-25: Added baseFee field to TestAnteKeeper to support EIP1559 fee testsThis field addition to the TestAnteKeeper struct enables testing of the EIP1559 fee validation functionality.
64-66: Added GetBaseFee method to TestAnteKeeperThis method properly implements the interface method required for EIP1559 fee checking in tests.
160-235: Test coverage for EIP1559 fee validationThe test case thoroughly validates the EIP1559 fee checking implementation with multiple scenarios:
- When transaction fees equal the baseFee (pass case)
- When transaction fees are less than baseFee (fail case)
- When the baseFee check passes but min gas price check fails (fail case)
I note that in line 177, the dex pool weights are set to [0.5, 0.5] which gives equal weights to both tokens. This is different from the weights [0.2, 0.8] used in the previous test at line 79-82. This is intentional as it sets up different price calculations for a more comprehensive test.
x/move/keeper/eip1559_test.go (1)
13-58: Test coverage for EIP1559 base fee update mechanismThis test thoroughly validates the dynamic fee adjustment mechanism of EIP1559:
- The fee stays constant when gas usage matches the target
- The fee increases when gas usage exceeds the target (by 10% as specified by MaxChangeRate)
- The fee decreases when gas usage is below the target
The test correctly validates the behavior of the
UpdateBaseFeemethod, which maintains the EIP1559 fee parameters based on network congestion (gas usage compared to target).proto/initia/move/v1/query.proto (2)
117-121: Added EIP1559FeeParams query RPC endpointThis RPC method will allow clients to query the current EIP1559 fee parameters, which is essential for estimating transaction fees under the new mechanism.
427-433: Added request and response messages for EIP1559FeeParams queryThe request message is appropriately empty since no parameters are needed for this query. The response includes the full EIP1559FeeParams structure with proper protobuf annotation.
x/move/types/eip1559_params.go (3)
1-8: Clean package structure with appropriate imports.The file is organized well with standard Go package structure and imports only what's needed for the EIP1559 fee parameter functionality.
9-22: Clear default values with good documentation.The default values are well-documented with comments indicating their denominations (e.g., 0.01 INIT). The constants are appropriately named with the
Defaultprefix indicating their purpose.
24-32: Well-structured default parameters function.The
DefaultEIP1559Paramsfunction provides a clean way to initialize the parameters with default values, promoting consistency throughout the codebase.x/move/keeper/msg_server_test.go (1)
6-6: Clean import addition.The
cosmossdk.io/mathpackage is appropriately imported to support the fee parameter testing.x/move/keeper/genesis.go (2)
154-158: EIP1559 fee parameters properly integrated into genesis initialization.The code correctly retrieves EIP1559 fee parameters from the genesis state and sets them in the keeper with proper error handling.
233-237: EIP1559 fee parameters properly included in genesis export.The code correctly retrieves the current EIP1559 fee parameters and includes them in the exported genesis state.
x/move/abci_test.go (3)
6-6: Appropriate import for math operations.The import of
cosmossdk.io/mathis correctly added to support decimal operations for EIP-1559 fee parameters.
19-19: Bank types import properly added.The
banktypesimport is correctly added to support the MsgSend transaction creation.
116-191: Comprehensive end-blocker test for EIP-1559 fee dynamics.The test effectively verifies the key behavior of EIP-1559 fee adjustments by:
- Setting initial fee parameters
- Executing a minimal transaction to establish a baseline fee
- Generating load with multiple transactions
- Verifying the base fee increases in response to network activity
This provides good coverage of the dynamic fee adjustment mechanism. The test is well-structured with appropriate assertions and error handling.
proto/initia/move/v1/tx.proto (3)
51-55: New RPC Method Addition: UpdateEIP1559FeeParamsThe new RPC method
UpdateEIP1559FeeParamsis clearly documented and follows the naming conventions and structure of the other RPC methods. Ensure that the corresponding service handler implementations (in the keeper and querier files) correctly handle the new fee parameter update logic.
359-372: New Message Type: MsgUpdateEIP1559FeeParamsThe message
MsgUpdateEIP1559FeeParamsis implemented consistently with similar messages in the file. It correctly sets the signer option to"authority"and defines theeip1559_feeparamsfield as required (non-nullable withdont_omitempty). This provides a clear schema for fee parameters updates. If any special validation is needed during processing, consider mentioning that in a comment or handling it in the service layer.
374-376: New Message Response: MsgUpdateEIP1559FeeParamsResponseThe empty response message
MsgUpdateEIP1559FeeParamsResponseis consistent with similar response types in this proto file. Ensure that the service implementation returns the correct response object after processing fee parameter updates.api/initia/move/v1/tx.pulsar.go (5)
14058-14913: Validate 'authority' in keeper logic.
This reflection code forMsgUpdateEIP1559FeeParamsis standard auto-generated protobuf. Ensure that the module keeper or governance logic properly checks that only a valid authority can invoke this update.
16209-16284: New message definitions look good.
These proto definitions forMsgUpdateEIP1559FeeParamsand its response appear correct and consistent with the rest of the code.
16508-16601: No issues with the wire format changes.
This hunk simply updates the protobuf wire format for the new message fields. Everything appears consistent.
16627-16701: Proper registration of new message types.
The increment in message count and indexing for the new fee param messages in the arrays is handled correctly.
17020-17044: Exporter logic is properly extended.
The exporter functions forMsgUpdateEIP1559FeeParamsand its response follow the usual proto pattern with no apparent issues.api/initia/move/v1/genesis.pulsar.go (15)
380-380: Field descriptor added correctly for EIP1559 fee params.The field descriptor is correctly declared to match the new field added to the GenesisState structure.
395-395: Field descriptor initialized properly in init function.The initialization follows the same pattern as other field descriptors, maintaining consistency in the codebase.
517-522: Range method implementation handles EIP1559 fee params correctly.The implementation properly checks if the field is populated before adding it to the range iteration, following the same pattern as other fields in the struct.
556-557: Has method correctly checks for presence of EIP1559 fee params.The null check implementation is consistent with how other message fields are checked.
592-593: Clear method properly handles EIP1559 fee params.The implementation sets the field to nil when cleared, consistent with other message fields.
658-660: Get method correctly retrieves EIP1559 fee params.The value retrieval logic follows the same pattern as other message fields in the struct.
713-714: Set method properly handles setting EIP1559 fee params.The method correctly assigns the value to the field, ensuring proper type conversion.
782-786: Mutable method handles EIP1559 fee params initialization.The implementation ensures a new instance is created if the field is nil, following the pattern used for other message fields.
828-830: NewField method creates EIP1559 fee params correctly.The implementation instantiates a new EIP1559FeeParams object, consistent with how other message types are handled.
949-952: Size calculation for EIP1559 fee params is implemented correctly.The size calculation logic follows the same pattern as other message fields, ensuring proper serialization.
982-995: Marshal implementation for EIP1559 fee params is correct.The marshaling logic properly encodes the field when present, following the same pattern as other message fields.
1460-1495: Unmarshal implementation for EIP1559 fee params handles all edge cases.The deserialization logic properly handles type checking, length verification, and error conditions, consistent with how other fields are unmarshaled.
1559-1559: EIP1559 fee params field added to GenesisState struct definition.The field is properly defined with the correct type and JSON tags, maintaining consistency with other fields.
1645-1650: Getter method for EIP1559 fee params implemented correctly.The getter follows the same pattern as other getters in the file, including proper nil checking.
1709-1728: Protocol buffer metadata updated correctly for the new field.The protobuf metadata changes include all necessary adjustments for the new field, ensuring proper serialization and deserialization.
x/move/keeper/eip1559.go (5)
12-14: EIP1559FeeKeeper structure follows good design patterns.The structure embeds the base Keeper, which is a good design choice as it follows the composition pattern and gives access to all the base keeper's methods.
16-20: Constructor follows standard Go patterns for initializing keeper.The constructor function properly initializes the EIP1559FeeKeeper with the provided move keeper.
22-28: Parameter getter and setter methods properly implemented.The methods correctly delegate to the underlying storage mechanisms for parameter management.
30-37: GetBaseFee method provides convenient access to the current base fee.The method properly extracts just the base fee from the full parameter set, with appropriate error handling.
51-53: Base fee constraint logic ensures valid fee range.The implementation correctly ensures that the new base fee stays within the configured minimum and maximum values, preventing extreme fluctuations.
api/initia/move/v1/query.pulsar.go (6)
16830-17620: Implementation of QueryEIP1559FeeParamsRequest looks goodThe implementation of the reflection methods for QueryEIP1559FeeParamsRequest follows the standard pattern for protocol buffer message types. It's an empty message with no fields, which is appropriate for a simple parameter query that doesn't need input parameters.
17186-17619: Implementation of QueryEIP1559FeeParamsResponse looks goodThe QueryEIP1559FeeParamsResponse implementation correctly includes a field for the EIP1559FeeParams and implements all necessary protobuf reflection methods. The field is properly handled in all operations including marshaling, unmarshaling, and reflection.
19103-19164: Message type definitions for EIP1559 fee parameters are well-structuredThe struct definitions for QueryEIP1559FeeParamsRequest and QueryEIP1559FeeParamsResponse are properly implemented. The response type correctly includes the Eip1559Feeparams field with appropriate JSON tagging and a getter method.
19388-19560: API endpoint definition for EIP1559FeeParams is properly configuredThe protocol buffer descriptor is correctly updated to include the new message types and RPC method. The endpoint path
/initia/move/v1/eip1559_feeparamsfollows the existing API naming conventions.
19575-19683: Type registry and dependency indices are correctly updatedThe message type registry and dependency indices are properly updated to include the new message types. The Go types slice has been extended to include QueryEIP1559FeeParamsRequest and QueryEIP1559FeeParamsResponse, and the dependency indices are adjusted to properly reference the EIP1559FeeParams type.
20088-20119: Exporter functions and message count are properly updatedThe exporter functions for the new message types are correctly implemented, and the message count is properly updated from 33 to 35 to reflect the addition of the two new message types.
api/initia/mstaking/v1/query.pulsar.go (10)
6-6: New import for Coin typeThe import of
v1beta11from the Cosmos SDK is added to support theCointype used in the newQueryDelegatorTotalDelegationBalanceResponsestructure.
11263-12175: Well-structured implementation of QueryDelegatorTotalDelegationBalance request handlerThe added code implements the complete reflection and serialization infrastructure for the new
QueryDelegatorTotalDelegationBalanceRequestmessage type. This follows the standard pattern for protocol buffer message implementations in Go.
12195-12195: Consistent index updates throughout the fileThe message type indexes have been consistently updated throughout the file to accommodate the two new message types, ensuring proper protobuf descriptor referencing.
Also applies to: 12553-12553, 12986-12986, 13344-13344, 14855-14855, 14885-14885, 14899-14899, 14919-14919, 14933-14933, 14949-14949, 14963-14963
14769-14843: New Query message type definitions for delegator total balanceThe implementation adds two new message types:
QueryDelegatorTotalDelegationBalanceRequestto query for a delegator's total balanceQueryDelegatorTotalDelegationBalanceResponseto return the total delegation balance as a list of coinsThese new types enhance the API by allowing users to query the sum of all delegation balances for a specific delegator, which is useful for EIP-1559 fee parameter management.
14982-14983: Added coin.proto import to support balance fieldThe raw descriptor data includes the import path for
cosmos/base/v1beta1/coin.protowhich is necessary for the new balance field in the response message.
15203-15219: Binary descriptor data for the new message typesThe raw binary descriptor data has been properly updated to include the definitions of the new message types.
15369-15385: New RPC endpoint definition for querying total delegation balanceA new REST endpoint is defined for querying a delegator's total delegation balance at
/initia/mstaking/v1/delegators/{delegator_addr}/total_delegation_balance. This endpoint follows RESTful API design principles and is consistent with other endpoints in the module.
15429-15530: Updated type and dependency indicesAll Go type mappings and dependency indices have been correctly updated to include the new message types and their relationships. This ensures proper code generation and type safety throughout the codebase.
15805-15805: Updated exporter functions for the new message typesThe exporter functions are correctly implemented for the new message types to support the protocol buffer reflection system.
Also applies to: 15817-15817
15883-15883: Updated message countThe total number of messages has been correctly updated from 26 to 28 to account for the two new message types.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
+ Coverage 39.83% 39.86% +0.02%
==========================================
Files 277 291 +14
Lines 26923 27205 +282
==========================================
+ Hits 10726 10846 +120
- Misses 14539 14685 +146
- Partials 1658 1674 +16
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (19)
x/dynamic-fee/client/cli/flags.go (1)
36-42: Incorrect function comment for FlagSetJSONArgsThe function comment should be updated to reflect the correct function name.
-// FlagSetArgs Returns the FlagSet for args related operations. +// FlagSetJSONArgs Returns the FlagSet for JSON args related operations. func FlagSetJSONArgs() *flag.FlagSet {x/dynamic-fee/keeper/common_test.go (4)
185-193: Consider concurrency if used in parallel tests
TestFaucetdoes not appear concurrency-safe if used among multiple goroutines simultaneously. If your test environment is strictly single-threaded, it's fine. Otherwise, consider using synchronization mechanisms (e.g., mutexes) or ensuring a single-threaded execution path to avoid race conditions.
273-510: Too large function_createTestInput
This function initializes multiple keepers and sets module parameters in one go, making it lengthy. Splitting it into smaller helper functions (e.g., separate keepers’ setup) would improve clarity and maintainability.@@ -273,10 +273,10 @@ func _createTestInput( t testing.TB, isCheckTx bool, moveConfig moveconfig.MoveConfig, db dbm.DB, ) (sdk.Context, TestKeepers) { - // Large block of keeper setups + // Consider extracting each keeper setup into dedicated helper functions for readability ... }
533-541: Panic on file-read error
WithinReadMoveFile, callingpanic(err)stops all remaining tests if the file is missing or unreadable. While suitable in some test scenarios, returning an error or marking the test as failed allows more graceful error handling.
686-693: Panicking function for converting string to type tag
MustConvertStringToTypeTagpanics on error. This is fine for test scenarios, but if reused in production, consider a version that gracefully returns an error instead of panicking.proto/initia/dynamicfee/module/v1/module.proto (1)
7-14: Proto module configuration
TheModulemessage is concise. If you anticipate scenarios whereauthoritymight be unset, consider a default or add additional documentation clarifying fallback behavior.x/dynamic-fee/autocli.go (1)
8-21: AutoCLI implementation for query commandsThe AutoCLIOptions method correctly implements the CLI registration for querying dynamic fee parameters. The method returns a well-structured ModuleOptions that properly configures the "params" query command.
Consider adding command options for any transactions this module supports, such as parameter updates. If parameter updates are intended to go through governance or another mechanism, this is fine as is.
x/move/keeper/whitelist.go (1)
222-237: Function implementation looks good, consider edge casesThe implementation of
GetWhitelistedTokenseffectively retrieves denominations from key metadata in the DexPairs map. The logic looks correct and handles errors appropriately by stopping iteration upon encountering any error.Consider handling potential edge cases:
- Are duplicate tokens possible in the output list?
- Is an empty result expected in some scenarios?
func (k Keeper) GetWhitelistedTokens(ctx context.Context) ([]string, error) { whitelistedTokens := []string{} err := k.DexPairs.Walk(ctx, nil, func(key, value []byte) (stop bool, err error) { metadataQuote, err := vmtypes.NewAccountAddressFromBytes(key) if err != nil { return true, err } denomQuote, err := types.DenomFromMetadataAddress(ctx, k.MoveBankKeeper(), metadataQuote) if err != nil { return true, err } + // Optional: Consider using a map to deduplicate tokens if duplicates are possible whitelistedTokens = append(whitelistedTokens, denomQuote) return false, nil }) return whitelistedTokens, err }x/dynamic-fee/keeper/querier_test.go (1)
12-22: Test looks good but could be more comprehensiveThe test correctly validates that the querier returns the same parameters as the keeper. This is a good basic test.
To make it more comprehensive, consider adding:
- Verification of specific parameter fields
- Testing with custom parameter values
- Testing error scenarios
x/dynamic-fee/keeper/msg_server_test.go (1)
13-33: Test covers basic functionality but could be more robustThe test creates appropriate test parameters and verifies that they are correctly set after calling
UpdateParams. This validates the basic happy path functionality.Consider enhancing the test with:
- Testing invalid parameter values (e.g., negative values or values exceeding limits)
- Testing with an invalid authority (permission checks)
- Verifying error scenarios
Example enhancement for testing with invalid authority:
// Test with invalid authority msgInvalidAuth := &types.MsgUpdateParams{ Authority: "invalid_authority", Params: msg.Params, } _, err = ms.UpdateParams(ctx, msgInvalidAuth) require.Error(t, err) require.Contains(t, err.Error(), "invalid authority")x/dynamic-fee/types/genesis.go (1)
17-21: Fix comment to reference "dynamic-fee" instead of "move".The function comment refers to "move genesis data" but should refer to "dynamic-fee genesis data" instead, as this is for the dynamic fee module.
-// ValidateGenesis performs basic validation of move genesis data returning an +// ValidateGenesis performs basic validation of dynamic-fee genesis data returning anx/dynamic-fee/keeper/genesis.go (1)
10-16: InitGenesis implementation looks good with one unused parameterThe implementation correctly initializes the genesis state by setting parameters. However, there's an unused parameter
moduleNames []stringthat's not utilized in the function body.If the
moduleNamesparameter is not needed, consider removing it to simplify the function signature:- func (k Keeper) InitGenesis(ctx context.Context, moduleNames []string, genState types.GenesisState) error { + func (k Keeper) InitGenesis(ctx context.Context, genState types.GenesisState) error {x/dynamic-fee/keeper/keeper_test.go (1)
13-57: Test cases properly verify the base fee update mechanismThe test thoroughly validates the dynamic fee keeper's functionality for updating base gas prices under different scenarios. It shows how the base price adjusts based on gas consumption relative to the target gas value.
Consider adding comments explaining the expected behavior in each test case and why the values change as they do. For example, explain the formula for calculating the updated base gas price when gas consumption is above or below the target value.
// update base fee err = input.DynamicFeeKeeper.UpdateBaseGasPrice(ctx) require.NoError(t, err) +// Base gas price should remain the same when gas used equals target gas baseGasPrice, err = input.DynamicFeeKeeper.BaseGasPrice(ctx) require.Equal(t, math.LegacyNewDecWithPrec(1, 2), baseGasPrice) ctx = ctx.WithBlockGasMeter(storetypes.NewInfiniteGasMeter()) ctx.BlockGasMeter().ConsumeGas(200000, "test") // update base fee err = input.DynamicFeeKeeper.UpdateBaseGasPrice(ctx) require.NoError(t, err) +// Base gas price should increase when gas used is greater than target gas +// New price: 0.01 + (0.01 * 0.1 * (200000/100000 - 1)) = 0.011 baseGasPrice, err = input.DynamicFeeKeeper.BaseGasPrice(ctx) require.Equal(t, math.LegacyNewDecWithPrec(11, 3), baseGasPrice) ctx = ctx.WithBlockGasMeter(storetypes.NewInfiniteGasMeter()) ctx.BlockGasMeter().ConsumeGas(0, "test") // update base fee err = input.DynamicFeeKeeper.UpdateBaseGasPrice(ctx) require.NoError(t, err) +// Base gas price should decrease when gas used is less than target gas +// New price: 0.011 - (0.011 * 0.1 * (1 - 0/100000)) = 0.0099 baseGasPrice, err = input.DynamicFeeKeeper.BaseGasPrice(ctx) require.Equal(t, math.LegacyNewDecWithPrec(99, 4), baseGasPrice)x/dynamic-fee/abci_test.go (1)
20-73: Comprehensive test for dynamic fee adjustment based on block gas usageThe test implements a good flow for validating the dynamic fee mechanism, checking both the decrease in base gas price when utilization is low and the increase when transactions are processed.
Consider adding more test scenarios to verify edge cases:
- Test with transaction volume right at the target gas value
- Test with extremely high gas usage to verify max price constraints
- Test with low gas usage to verify min price constraints
Also, add comments explaining the expected outcomes and the reason for using 100 transactions:
+// Create 100 transactions to ensure significant gas consumption to trigger +// base gas price adjustment. The gas used will exceed the target of 1,000,000 +// set in the parameters. msgs := []sdk.Msg{} for i := 0; i < 100; i++ { msgs = append(msgs, &banktypes.MsgSend{x/dynamic-fee/keeper/gas_price.go (1)
50-70: GasPrice method follows similar pattern with appropriate error handlingThe
GasPricemethod correctly retrieves the gas price for a specific denomination with proper error handling and zero-value checks.Consider adding logging for debug purposes, especially when errors occur:
if baseSpotPrice.IsZero() { - return sdk.DecCoin{}, fmt.Errorf("baseSpotPrice is zero: %s", denom) + err := fmt.Errorf("baseSpotPrice is zero: %s", denom) + // Add some logging here for debugging purposes + return sdk.DecCoin{}, err }x/dynamic-fee/ante/fee_test.go (1)
131-131: Comment references to old field name.
Line 131 references the old concept “base_min_gas_price.” Consider updating the comment to reflect the new naming to avoid confusion.- // set high base_min_gas_price to test should be failed + // set high baseGasPrice to test insufficient feesapi/initia/dynamicfee/v1/types.pulsar.go (1)
695-699: Consider using numeric proto types or adding rigorous validation.The fields
BaseGasPrice,MinBaseGasPrice,MaxBaseGasPrice,MaxChangeRateare stored as strings. If these are intended for numeric calculations, consider either a decimal-friendly proto type or strong input validation to avoid parse errors and invalid data.x/dynamic-fee/module.go (2)
34-36: Update leftover references to “move module”.Multiple comments and documentation lines refer to this as the “move module.” This is likely a copy-paste remnant. Rename these to “dynamic fee module” or similar for consistency and clarity.
Here is an example diff to fix a few spots:
-// AppModuleBasic defines the basic application module used by the move module. +// AppModuleBasic defines the basic application module used by the dynamic fee module. -// InitGenesis performs genesis initialization for the move module. +// InitGenesis performs genesis initialization for the dynamic fee module. -// EndBlock returns the end blocker for the move module. +// EndBlock returns the end blocker for the dynamic fee module.Also applies to: 50-53, 55-60, 61-69, 78-79, 115-117, 118-127, 129-134, 136-139
82-84: Unused fieldmoduleNames[]string?In the
AppModulestruct,moduleNamesgets passed toInitGenesisbut appears unused within the keeper logic, potentially leftover or planned for future use. Confirm whether it’s actually needed; consider removing if it remains unused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (8)
api/initia/dynamicfee/v1/query_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapi/initia/dynamicfee/v1/tx_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/dynamic-fee/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/dynamic-fee/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/dynamic-fee/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/dynamic-fee/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/dynamic-fee/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/move/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (53)
api/initia/dynamicfee/module/v1/module.pulsar.go(1 hunks)api/initia/dynamicfee/v1/genesis.pulsar.go(1 hunks)api/initia/dynamicfee/v1/query.pulsar.go(1 hunks)api/initia/dynamicfee/v1/tx.pulsar.go(1 hunks)api/initia/dynamicfee/v1/types.pulsar.go(1 hunks)api/initia/move/v1/types.pulsar.go(15 hunks)app/ante/ante.go(3 hunks)app/app.go(1 hunks)app/blocksdk.go(2 hunks)app/keepers/keepers.go(3 hunks)app/keepers/keys.go(2 hunks)app/modules.go(4 hunks)proto/initia/dynamicfee/module/v1/module.proto(1 hunks)proto/initia/dynamicfee/v1/genesis.proto(1 hunks)proto/initia/dynamicfee/v1/query.proto(1 hunks)proto/initia/dynamicfee/v1/tx.proto(1 hunks)proto/initia/dynamicfee/v1/types.proto(1 hunks)proto/initia/move/v1/types.proto(2 hunks)x/dynamic-fee/abci.go(1 hunks)x/dynamic-fee/abci_test.go(1 hunks)x/dynamic-fee/ante/fee.go(5 hunks)x/dynamic-fee/ante/fee_test.go(5 hunks)x/dynamic-fee/ante/gas_prices_test.go(1 hunks)x/dynamic-fee/autocli.go(1 hunks)x/dynamic-fee/client/cli/flags.go(1 hunks)x/dynamic-fee/common_test.go(1 hunks)x/dynamic-fee/keeper/ante.go(1 hunks)x/dynamic-fee/keeper/common_test.go(1 hunks)x/dynamic-fee/keeper/gas_price.go(1 hunks)x/dynamic-fee/keeper/genesis.go(1 hunks)x/dynamic-fee/keeper/keeper.go(1 hunks)x/dynamic-fee/keeper/keeper_test.go(1 hunks)x/dynamic-fee/keeper/msg_server.go(1 hunks)x/dynamic-fee/keeper/msg_server_test.go(1 hunks)x/dynamic-fee/keeper/querier.go(1 hunks)x/dynamic-fee/keeper/querier_test.go(1 hunks)x/dynamic-fee/module.go(1 hunks)x/dynamic-fee/types/ante.go(1 hunks)x/dynamic-fee/types/codec.go(1 hunks)x/dynamic-fee/types/expected_keeper.go(1 hunks)x/dynamic-fee/types/genesis.go(1 hunks)x/dynamic-fee/types/keys.go(1 hunks)x/dynamic-fee/types/params.go(1 hunks)x/dynamic-fee/types/tx.go(1 hunks)x/move/keeper/dex.go(0 hunks)x/move/keeper/dex_test.go(0 hunks)x/move/keeper/genesis.go(0 hunks)x/move/keeper/handler.go(1 hunks)x/move/keeper/handler_test.go(1 hunks)x/move/keeper/params.go(0 hunks)x/move/keeper/whitelist.go(1 hunks)x/move/types/params.go(0 hunks)x/move/types/params_test.go(1 hunks)
💤 Files with no reviewable changes (5)
- x/move/keeper/genesis.go
- x/move/types/params.go
- x/move/keeper/params.go
- x/move/keeper/dex_test.go
- x/move/keeper/dex.go
✅ Files skipped from review due to trivial changes (4)
- x/move/keeper/handler.go
- x/move/keeper/handler_test.go
- x/dynamic-fee/ante/gas_prices_test.go
- x/dynamic-fee/types/keys.go
🚧 Files skipped from review as they are similar to previous changes (3)
- app/blocksdk.go
- app/modules.go
- proto/initia/move/v1/types.proto
🧰 Additional context used
🧬 Code Definitions (25)
x/dynamic-fee/keeper/gas_price.go (3)
x/dynamic-fee/keeper/keeper.go (9) (9)
Keeper(16-30)k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)k(97-117)x/dynamic-fee/keeper/ante.go (2) (2)
k(20-22)k(24-32)x/dynamic-fee/keeper/genesis.go (3) (3)
k(10-16)k(19-28)err(22-22)
x/dynamic-fee/keeper/msg_server_test.go (3)
x/dynamic-fee/keeper/common_test.go (1) (1)
createDefaultTestInput(248-250)x/dynamic-fee/keeper/msg_server.go (1) (1)
ms(27-43)x/dynamic-fee/types/tx.go (1) (1)
msg(17-23)
x/dynamic-fee/abci.go (5)
x/dynamic-fee/keeper/ante.go (2) (2)
k(20-22)k(24-32)x/dynamic-fee/keeper/gas_price.go (2) (2)
k(15-48)k(51-70)x/dynamic-fee/keeper/genesis.go (2) (2)
k(10-16)k(19-28)x/dynamic-fee/keeper/keeper.go (8) (8)
k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)Keeper(16-30)x/dynamic-fee/types/keys.go (1) (1)
ModuleName(5-5)
x/dynamic-fee/abci_test.go (2)
x/dynamic-fee/common_test.go (6) (6)
secondBondDenom(31-31)addr2(37-37)secondBondCoin(45-45)addr1(35-35)executeMsgsWithGasInfo(114-119)priv2(36-36)x/dynamic-fee/types/params.go (1) (1)
DefaultBaseGasPrice(10-10)
x/dynamic-fee/client/cli/flags.go (1)
api/initia/move/v1/types.pulsar.go (3) (3)
UpgradePolicy_name(5795-5799)UpgradePolicy_COMPATIBLE(5788-5788)UpgradePolicy_IMMUTABLE(5790-5790)
x/dynamic-fee/keeper/keeper_test.go (2)
x/dynamic-fee/keeper/common_test.go (1) (1)
createDefaultTestInput(248-250)api/initia/dynamicfee/v1/types.pulsar.go (3) (3)
Params(690-700)Params(715-715)Params(718-720)
x/dynamic-fee/types/tx.go (2)
api/initia/dynamicfee/v1/tx.pulsar.go (9) (9)
MsgUpdateParams(888-900)MsgUpdateParams(915-915)MsgUpdateParams(918-920)_(32-32)_(53-53)_(527-527)_(548-548)_(882-882)_(884-884)api/initia/dynamicfee/v1/types.pulsar.go (3) (3)
Params(690-700)Params(715-715)Params(718-720)
x/move/keeper/whitelist.go (6)
x/move/keeper/dex.go (2) (2)
k(29-44)k(48-54)x/move/keeper/keeper.go (1) (1)
Keeper(23-65)x/move/types/connector.go (1) (1)
err(591-591)x/move/types/denom.go (1) (1)
DenomFromMetadataAddress(75-88)x/move/keeper/bank.go (1) (1)
MoveBankKeeper(24-26)x/bank/types/expected_keeper.go (1) (1)
MoveBankKeeper(13-34)
x/dynamic-fee/types/genesis.go (4)
x/move/types/genesis.go (1) (1)
ValidateGenesis(44-46)api/initia/dynamicfee/v1/types.pulsar.go (3) (3)
Params(690-700)Params(715-715)Params(718-720)api/initia/dynamicfee/v1/genesis.pulsar.go (3) (3)
GenesisState(466-472)GenesisState(487-487)GenesisState(490-492)x/dynamic-fee/types/params.go (1) (1)
DefaultParams(21-29)
app/keepers/keys.go (1)
x/dynamic-fee/types/keys.go (1) (1)
StoreKey(8-8)
api/initia/dynamicfee/module/v1/module.pulsar.go (6)
api/initia/dynamicfee/v1/genesis.pulsar.go (25) (25)
init(22-26)init(560-560)x(32-34)x(36-46)x(53-55)x(56-58)x(59-61)x(65-67)x(72-74)x(77-79)x(83-85)x(92-99)x(112-122)x(130-140)x(148-159)x(171-181)x(193-206)x(211-222)_(28-28)_(49-49)_(460-460)_(462-462)l(283-283)l(312-312)n(282-282)api/initia/dynamicfee/v1/tx.pulsar.go (15) (15)
init(25-30)init(522-525)init(1045-1045)_(32-32)_(53-53)_(527-527)_(548-548)_(882-882)_(884-884)l(306-306)l(339-339)l(759-759)l(784-784)n(305-305)n(758-758)api/initia/dynamicfee/v1/types.pulsar.go (5) (5)
init(26-34)init(838-838)l(363-363)l(407-407)n(362-362)api/initia/move/v1/types.pulsar.go (17) (17)
init(71-78)Module(5953-5963)Module(5978-5978)Module(5981-5983)l(398-398)l(440-440)n(397-397)n(982-982)n(1562-1562)n(2182-2182)n(2753-2753)n(3346-3346)n(3936-3936)n(4532-4532)n(4957-4957)v(596-596)v(1166-1166)x/dynamic-fee/keeper/msg_server.go (1) (1)
ms(27-43)x/move/keeper/msg_server.go (13) (13)
ms(29-57)ms(60-97)ms(100-137)ms(140-176)ms(179-209)ms(212-245)ms(248-289)ms(292-333)ms(336-370)ms(373-407)ms(409-426)ms(428-445)ms(447-463)
x/dynamic-fee/keeper/genesis.go (3)
x/dynamic-fee/keeper/keeper.go (9) (9)
k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)k(97-117)Keeper(16-30)api/initia/dynamicfee/v1/genesis.pulsar.go (3) (3)
GenesisState(466-472)GenesisState(487-487)GenesisState(490-492)api/initia/dynamicfee/v1/types.pulsar.go (3) (3)
Params(690-700)Params(715-715)Params(718-720)
x/dynamic-fee/ante/fee_test.go (3)
x/dynamic-fee/keeper/gas_price.go (2) (2)
k(15-48)k(51-70)x/dynamic-fee/keeper/genesis.go (3) (3)
k(10-16)k(19-28)err(22-22)x/dynamic-fee/keeper/keeper.go (8) (8)
k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)k(97-117)
x/dynamic-fee/keeper/common_test.go (5)
x/dynamic-fee/keeper/keeper.go (1) (1)
Keeper(16-30)x/dynamic-fee/keeper/genesis.go (1) (1)
err(22-22)x/dynamic-fee/keeper/msg_server.go (1) (1)
ms(27-43)x/dynamic-fee/types/params.go (1) (1)
DefaultParams(21-29)x/dynamic-fee/keeper/querier.go (1) (1)
NewQuerier(16-18)
x/dynamic-fee/keeper/querier.go (2)
api/initia/dynamicfee/v1/types.pulsar.go (3) (3)
Params(690-700)Params(715-715)Params(718-720)api/initia/dynamicfee/v1/query.pulsar.go (6) (6)
QueryParamsRequest(824-828)QueryParamsRequest(843-843)QueryParamsRequest(846-848)QueryParamsResponse(851-858)QueryParamsResponse(873-873)QueryParamsResponse(876-878)
app/ante/ante.go (5)
x/dynamic-fee/keeper/ante.go (1) (1)
AnteKeeper(10-12)x/dynamic-fee/types/ante.go (1) (1)
AnteKeeper(9-13)x/dynamic-fee/keeper/keeper.go (1) (1)
Keeper(16-30)x/dynamic-fee/ante/fee.go (1) (1)
NewMempoolFeeChecker(23-29)x/dynamic-fee/ante/gas_prices.go (1) (1)
NewGasPricesDecorator(15-17)
api/initia/dynamicfee/v1/genesis.pulsar.go (2)
api/initia/dynamicfee/v1/types.pulsar.go (9) (9)
init(26-34)init(838-838)Params(690-700)Params(715-715)Params(718-720)l(363-363)l(407-407)n(362-362)file_initia_dynamicfee_v1_types_proto_init(839-875)api/initia/move/v1/types.pulsar.go (17) (17)
init(71-78)Params(5835-5848)Params(5863-5863)Params(5866-5868)l(398-398)l(440-440)n(397-397)n(982-982)n(1562-1562)n(2182-2182)n(2753-2753)n(3346-3346)n(3936-3936)n(4532-4532)n(4957-4957)v(596-596)v(1166-1166)
x/dynamic-fee/keeper/querier_test.go (3)
x/dynamic-fee/keeper/common_test.go (1) (1)
createDefaultTestInput(248-250)x/dynamic-fee/keeper/querier.go (1) (1)
NewQuerier(16-18)api/initia/dynamicfee/v1/query.pulsar.go (3) (3)
QueryParamsRequest(824-828)QueryParamsRequest(843-843)QueryParamsRequest(846-848)
x/dynamic-fee/common_test.go (3)
x/dynamic-fee/keeper/common_test.go (2) (2)
bondDenom(106-106)MakeEncodingConfig(150-174)x/move/keeper/common_test.go (2) (2)
bondDenom(103-103)MakeEncodingConfig(147-171)x/move/keeper/dex_test.go (2) (2)
createDexPool(31-90)decToVmArgument(17-29)
x/dynamic-fee/keeper/ante.go (4)
x/dynamic-fee/keeper/keeper.go (9) (9)
Keeper(16-30)k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)k(97-117)x/dynamic-fee/types/ante.go (1) (1)
AnteKeeper(9-13)x/dynamic-fee/keeper/gas_price.go (2) (2)
k(15-48)k(51-70)x/dynamic-fee/keeper/genesis.go (3) (3)
k(10-16)k(19-28)err(22-22)
api/initia/dynamicfee/v1/tx.pulsar.go (3)
api/initia/dynamicfee/v1/genesis.pulsar.go (9) (9)
init(22-26)init(560-560)_(28-28)_(49-49)_(460-460)_(462-462)l(283-283)l(312-312)n(282-282)api/initia/dynamicfee/v1/types.pulsar.go (10) (10)
init(26-34)init(838-838)Params(690-700)Params(715-715)Params(718-720)_(36-36)_(57-57)l(363-363)l(407-407)n(362-362)x/dynamic-fee/keeper/msg_server.go (1) (1)
ms(27-43)
x/dynamic-fee/keeper/msg_server.go (5)
x/dynamic-fee/keeper/keeper.go (8) (8)
Keeper(16-30)k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)x/dynamic-fee/keeper/ante.go (2) (2)
k(20-22)k(24-32)x/dynamic-fee/keeper/gas_price.go (2) (2)
k(15-48)k(51-70)x/dynamic-fee/keeper/genesis.go (3) (3)
k(10-16)k(19-28)err(22-22)api/initia/move/v1/tx.pulsar.go (6) (6)
MsgUpdateParams(15278-15290)MsgUpdateParams(15305-15305)MsgUpdateParams(15308-15310)MsgUpdateParamsResponse(15328-15332)MsgUpdateParamsResponse(15347-15347)MsgUpdateParamsResponse(15350-15352)
x/dynamic-fee/module.go (7)
x/dynamic-fee/keeper/genesis.go (4) (4)
err(22-22)genState(20-20)k(10-16)k(19-28)x/dynamic-fee/types/keys.go (1) (1)
ModuleName(5-5)x/dynamic-fee/types/genesis.go (2) (2)
DefaultGenesisState(11-15)ValidateGenesis(19-21)x/dynamic-fee/keeper/keeper.go (8) (8)
Keeper(16-30)k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)x/dynamic-fee/keeper/msg_server.go (2) (2)
MsgServer(16-18)NewMsgServerImpl(23-25)x/dynamic-fee/keeper/querier.go (1) (1)
NewQuerier(16-18)x/dynamic-fee/abci.go (1) (1)
EndBlocker(15-20)
api/initia/dynamicfee/v1/types.pulsar.go (3)
api/initia/dynamicfee/v1/genesis.pulsar.go (9) (9)
init(22-26)init(560-560)_(28-28)_(49-49)_(460-460)_(462-462)l(283-283)l(312-312)n(282-282)api/initia/dynamicfee/v1/tx.pulsar.go (15) (15)
init(25-30)init(522-525)init(1045-1045)_(32-32)_(53-53)_(527-527)_(548-548)_(882-882)_(884-884)l(306-306)l(339-339)l(759-759)l(784-784)n(305-305)n(758-758)api/initia/dynamicfee/v1/query.pulsar.go (11) (11)
init(23-26)init(380-384)init(965-965)_(28-28)_(49-49)l(260-260)l(285-285)l(641-641)l(670-670)n(259-259)n(640-640)
api/initia/dynamicfee/v1/query.pulsar.go (3)
api/initia/dynamicfee/v1/genesis.pulsar.go (9) (9)
init(22-26)init(560-560)_(28-28)_(49-49)_(460-460)_(462-462)l(283-283)l(312-312)n(282-282)api/initia/dynamicfee/v1/tx.pulsar.go (15) (15)
init(25-30)init(522-525)init(1045-1045)_(32-32)_(53-53)_(527-527)_(548-548)_(882-882)_(884-884)l(306-306)l(339-339)l(759-759)l(784-784)n(305-305)n(758-758)api/initia/dynamicfee/v1/types.pulsar.go (11) (11)
init(26-34)init(838-838)_(36-36)_(57-57)l(363-363)l(407-407)n(362-362)Params(690-700)Params(715-715)Params(718-720)file_initia_dynamicfee_v1_types_proto_init(839-875)
🔇 Additional comments (94)
api/initia/move/v1/types.pulsar.go (12)
17-61: Confirmed field removal in auto-generated struct helpersThis part of the generated code shows the updated list handling for
Params.AllowedPublishers, now using_Params_4_list(previously would have been_Params_5_list), indicating successful removal of thebase_min_gas_pricefield that was previously in the struct.
63-78: Field descriptor update reflects removal of base_min_gas_priceThe field descriptors for
Paramsnow only include four fields (base_denom, contract_shared_revenue_ratio, script_enabled, allowed_publishers), with no descriptor for the removed base_min_gas_price field.
144-168: Range method updated to handle field removalThe
Rangemethod forParamshas been updated to properly handle the remaining fields after the removal ofbase_min_gas_price. The field indices have been adjusted accordingly to maintain serialization consistency.
182-197: Has method updated to reflect field removalThe
Hasmethod forParamsnow only checks for the existence of the four remaining fields, correctly omitting checks for the removedbase_min_gas_pricefield.
397-416: Size calculation updated for Params structThe size calculation logic has been adjusted to only account for the four remaining fields in the
Paramsstructure, ensuring proper marshaling after the removal ofbase_min_gas_price.
446-464: Marshal method updated with corrected field indicesThe marshaling code for
Paramshas been updated with the proper field indices after the removal ofbase_min_gas_price. TheAllowedPublishersfield is now handled with field identifier 0x22 instead of what would have previously been a higher index.
527-643: Unmarshal field handling corrected for ParamsThe field numbers in the unmarshal switch statement have been updated to reflect the removal of
base_min_gas_price. The conditional branches now properly correspond to the remaining fields with indices 1-4.
679-691: RawParams field descriptors updatedThe field descriptors for
RawParamshave been correctly updated to only include the three remaining fields after removal ofbase_min_gas_price.
835-848: Clean Params struct definition with base_min_gas_price removedThe go struct definition for
Paramshas been updated to remove thebase_min_gas_pricefield. The struct now correctly contains only the following fields:
- BaseDenom
- ContractSharedRevenueRatio
- ScriptEnabled
- AllowedPublishers
899-908: Clean RawParams struct definition without base_min_gas_priceThe
RawParamsstruct definition has been updated to remove thebase_min_gas_pricefield, now containing only:
- BaseDenom
- ContractSharedRevenueRatio
- ScriptEnabled
6396-6417: Proto definition updated in marshaling codeThe raw proto definition embedded in the generated file has been updated to remove the
base_min_gas_pricefield from theParamsmessage, maintaining the correct field numbering for proper wire compatibility.
6417-6433: Proto definition updated for RawParamsThe raw proto definition for
RawParamshas also been updated to remove thebase_min_gas_pricefield, ensuring consistency across the entire API.Can you verify that all consumers of this API have been updated to handle the removal of the
base_min_gas_pricefield? This change might affect any code that reads or writes this field from the module parameters.x/move/types/params_test.go (1)
26-26: Modification aligns with the removal of BaseMinGasPriceThe test has been modified to validate
AllowedPublishersinstead of the removedBaseMinGasPricefield. This change is consistent with the PR objective of removing thebase_min_gas_pricefield from parameter definitions.x/dynamic-fee/types/params.go (3)
9-19: Default parameter values are well-defined with appropriate precisionThe default values for gas prices and rates are defined with clear decimal precision using
math.LegacyNewDecWithPrec. The comments help clarify the actual decimal values.
21-29: Well-structured DefaultParams functionThe function provides a clean implementation that initializes all dynamic fee parameters with their default values.
31-61: Comprehensive parameter validationThe validation logic thoroughly checks all necessary conditions:
- Non-negative values for all parameters
- Base gas price stays within min/max bounds
- Clear, specific error messages for each validation failure
This validation ensures the dynamic fee system will operate with valid parameters.
x/dynamic-fee/common_test.go (6)
29-46: Well-defined test constants and variablesTest variables are properly initialized with appropriate values for testing dynamic fee functionality. The bond denominations and test addresses are set up to enable comprehensive testing scenarios.
48-51: Useful helper function for balance verificationThe
checkBalancefunction provides a clean way to verify account balances during tests, improving test readability.
53-105: Comprehensive app initialization for testingThe
createAppfunction effectively sets up a test environment with:
- Genesis accounts with appropriate balances
- DEX pool creation
- Validator setup
- Distribution parameters configuration
This provides a complete testing environment for the dynamic fee module.
107-119: Utility functions for message execution in testsBoth
executeMsgsandexecuteMsgsWithGasInfoprovide clean abstractions for executing transactions in tests, with the latter capturing gas information for fee-related assertions.
121-133: VM argument conversion functionThe
decToVmArgumentfunction correctly handles the conversion from Go's decimal type to VM-compatible byte representation, including proper endianness handling.
135-191: DEX pool creation for testingThe
createDexPoolfunction provides the necessary setup for liquidity pool tests, which is important for comprehensive testing of fees in various transaction scenarios.x/dynamic-fee/client/cli/flags.go (4)
11-15: Clear flag constantsThe flag constants are well-named and provide clear references for CLI commands.
17-26: Well-implemented upgrade policy flagThe flag set provides clear options for upgrade policies with helpful descriptions.
28-34: Well-documented BCS arguments flagThe flag includes clear examples of the expected format for BCS arguments.
44-50: Well-documented type arguments flagThe flag provides clear examples of the expected format for type arguments.
x/dynamic-fee/keeper/common_test.go (2)
194-202: Faucet initialization structure
NewTestFaucetstraightforwardly enforces a non-empty initial supply and properly callsMinton creation. Good job on ensuring validity checks withrequire.NotEmpty.
551-684: Comprehensive test message router
TestMsgRoutercovers various message types and thoroughly emits corresponding events. This approach is well-structured and beneficial for the dynamic fee module’s message-handling scenarios.x/dynamic-fee/abci.go (1)
15-20: Error handling inEndBlocker
EndBlockerreturns an error fromk.UpdateBaseGasPrice, but there’s no logging or fallback handling. Confirm that callers buffer potential errors (e.g., logging, retry logic) to avoid silent failures.x/dynamic-fee/types/tx.go (2)
1-12: Implementation of MsgUpdateParams looks goodThis code correctly defines that MsgUpdateParams implements the sdk.Msg interface, which is necessary for message routing in the Cosmos SDK framework.
14-23: Well-implemented validation methodThe Validate method appropriately checks the authority address validity using the provided address codec and delegates parameter validation to the Params.Validate() method. This follows best practices for Cosmos SDK message validation by:
- Validating the authority address format
- Using proper error wrapping with context
- Delegating to parameter-specific validation
proto/initia/dynamicfee/v1/types.proto (2)
1-10: Proper protobuf configurationThe file has the correct package declaration, imports, and Go package options which align with Cosmos SDK standards.
11-41: Well-structured Params message definitionThe Params message is properly defined with appropriate types and options for each field:
- All decimal fields use
cosmossdk.io/math.LegacyDeccustom type- Fields are marked as non-nullable with
(gogoproto.nullable) = false- The
dont_omitemptyamino option ensures fields are always included in serialization- YAML tags are provided for configuration file compatibility
This follows best practices for defining parameters in Cosmos SDK modules.
x/dynamic-fee/autocli.go (1)
1-6: Proper imports for AutoCLI implementationThe imports correctly include the necessary packages for implementing AutoCLI functionality.
app/keepers/keys.go (2)
47-47: Import for dynamic fee moduleThe import for the dynamic fee module types is correctly added.
64-64: Integration of dynamic fee store keyThe dynamic fee module's store key is properly added to the application's KV store keys, which is essential for the module to function correctly within the application.
x/dynamic-fee/types/codec.go (2)
18-24: Interface registration follows standard patternThe interface registration correctly registers the message implementation and message service descriptor, following the standard Cosmos SDK pattern.
11-16:✅ Verification successful
Ensure proper Amino codec registration
The
RegisterLegacyAminoCodecfunction looks correct, registering both the message type and the concreteParamsstruct.Ensure that the message route "dynamicfee/MsgUpdateParams" follows the convention used throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check the message route convention in other modules rg "RegisterAminoMsg\(cdc, .*, \".*\/Msg" --type go -A 1Length of output: 189
Amino Codec Registration Verified – Confirm Message Route Convention
The function
RegisterLegacyAminoCodecinx/dynamic-fee/types/codec.gocorrectly registers both theMsgUpdateParamsmessage and the concreteParamsstruct. However, please manually verify that the message route"dynamicfee/MsgUpdateParams"follows the naming conventions used elsewhere in the codebase (typically in the<module>/Msg…format).
- Ensure that other modules also register messages with a similar route pattern.
- Confirm that the naming convention for the message route is consistent with project standards.
The above script uses a corrected regex pattern to help locate similar registrations. Please run it and confirm that the output aligns with expectations before final approval.
--- 🏁 Script executed: ```shell #!/bin/bash # Re-verify the message route convention using a corrected regex pattern. # This script searches for RegisterAminoMsg calls with a route following the expected "<module>/Msg…" pattern. rg 'RegisterAminoMsg\(cdc,.*, "[^"]+/Msg' --type go -A 1Length of output: 4077
Amino Codec Registration Verified – No Changes Required
- The function
RegisterLegacyAminoCodecinx/dynamic-fee/types/codec.goproperly registers both theMsgUpdateParamsmessage and the concreteParamsstruct.- Verification across modules confirms that the message route
"dynamicfee/MsgUpdateParams"aligns with the established convention (e.g., similar routes inreward,mstaking, and other modules).x/dynamic-fee/types/ante.go (1)
12-12:❓ Verification inconclusive
Method renamed from BaseMinGasPrice to BaseGasPrice.
This change reflects the broader refactoring to remove the
base_min_gas_pricefield across the codebase. The interface signature remains the same, still returning amath.LegacyDecand an error, but the name now better reflects its purpose for managing dynamic fees.
🏁 Script executed:
#!/bin/bash # Verify that all implementations of AnteKeeper have been updated to use BaseGasPrice instead of BaseMinGasPrice # Search for any remaining implementations that still use BaseMinGasPrice rg "func .*BaseMinGasPrice\(" --type goLength of output: 41
Confirm refactor of AnteKeeper interface: BaseMinGasPrice → BaseGasPrice
The method renaming is consistent with the intended dynamic fee refactor. The updated interface now correctly exposes:
BaseGasPrice(ctx context.Context) (math.LegacyDec, error)which maintains the expected return types. However, the initial shell script that looked for function declarations using
BaseMinGasPriceproduced no output, and this result is too minimal for a conclusive automated check. Please manually verify that no lingering references—either in function definitions, method calls, or documentation—remain in the codebase.
- Confirm that all implementations of AnteKeeper now refer exclusively to
BaseGasPrice.- Check for any stray occurrences of
BaseMinGasPriceacross the repository (e.g., in comments, tests, or usage).proto/initia/dynamicfee/v1/genesis.proto (1)
1-16: New genesis proto file for the dynamic-fee module looks good.The file correctly defines the
GenesisStatemessage with aparamsfield for the dynamic fee module. The standard Cosmos SDK proto conventions are followed, including proper imports and field annotations.app/app.go (1)
476-476:❓ Verification inconclusive
Replaced MoveKeeper.DexKeeper with DynamicFeeKeeper.
This change correctly updates the transaction query registration to use the new
DynamicFeeKeeperinstead of the previous DEX-based approach, aligning with the broader effort to transition to dynamic fee management.
🏁 Script executed:
#!/bin/bash # Verify that DynamicFeeKeeper implements the necessary interfaces expected by initiatx.RegisterTxQuery # Check the RegisterTxQuery function signature to understand what interfaces are required rg -A 10 "func RegisterTxQuery\(" --type go # Check if DynamicFeeKeeper implements the AnteKeeper interface rg -A 10 "type DynamicFeeKeeper" --type goLength of output: 603
Action Required: Verify the Implementation of DynamicFeeKeeper
The update in
app/app.goreplacingMoveKeeper.DexKeeperwithDynamicFeeKeepercorrectly reflects our transition to dynamic fee management. However, the automated search did not return any definition forDynamicFeeKeeper, which suggests that either its implementation isn’t straightforwardly defined (e.g., it might be an alias or defined via composition in another module) or further manual confirmation is needed. Please verify that:
DynamicFeeKeeperis indeed defined in the codebase (or via an alias) and implements the interface (or behaviors) expected byRegisterTxQuery(specifically conforming to theGasPriceKeeperparameter requirements).- The usage in
app/app.go, line 476, properly registersDynamicFeeKeeperwithout any unintended side effects.Once you verify these points, we can finalize the review.
x/dynamic-fee/keeper/querier.go (2)
9-18: New Querier implementation follows best practices.The Querier struct properly embeds the Keeper and the implementation ensures it satisfies the
types.QueryServerinterface. The constructor function follows standard patterns for Cosmos SDK modules.
20-29:✅ Verification successful
Params query implementation looks good.
The method correctly retrieves parameters from the keeper and wraps them in the expected response type. Error handling is appropriately implemented.
🏁 Script executed:
#!/bin/bash # Verify that the GetParams method exists in the Keeper implementation rg -A 5 "func \([\w\s\*]+\) GetParams\(" --type go -- "x/dynamic-fee/keeper"Length of output: 472
Params Query Implementation Approved
The implementation in
x/dynamic-fee/keeper/querier.gocorrectly invokesGetParamsfrom the keeper, properly handles errors, and wraps the retrieved parameters in the expected response. The verification confirms that theGetParamsmethod is implemented as expected inx/dynamic-fee/keeper/keeper.go.proto/initia/dynamicfee/v1/query.proto (3)
1-13: Well-structured protocol buffer definitions.The file properly defines the package, imports necessary dependencies, and sets appropriate Go package options, following standard Protobuf conventions.
14-20: LGTM: gRPC service correctly defined.The Query service is properly defined with a Params method that follows RESTful API conventions via the google.api.http option.
22-29: Well-defined query message types.The request and response message types are properly structured, with appropriate documentation comments and the response correctly using gogoproto.nullable = false for the params field.
x/dynamic-fee/types/genesis.go (1)
1-16: Genesis state management looks good.The functions for creating and defaulting genesis state are well-implemented, following standard Cosmos SDK patterns.
app/ante/ante.go (4)
15-21: Imports correctly updated for dynamic fee integration.The imports are appropriately updated to replace move module references with dynamic fee module references.
26-35: HandlerOptions successfully migrated to dynamic fee.The HandlerOptions struct has been properly updated to use DynamicFeeKeeper instead of MoveKeeper, maintaining all required fields for the ante handler.
58-61: Fee checker properly updated to use dynamic fee implementation.The txFeeChecker is correctly initialized using the new dynamicfeeante.NewMempoolFeeChecker with the DynamicFeeKeeper.
78-96: Ante decorator chain updated correctly.The ante decorator chain has been updated to use dynamicfeeante.NewGasPricesDecorator() instead of the move module equivalent, maintaining the correct order of decorators.
x/dynamic-fee/keeper/ante.go (3)
10-18: Well-implemented AnteKeeper struct and constructor.The AnteKeeper struct correctly embeds the Keeper and implements the types.AnteKeeper interface. The constructor function is properly implemented.
20-22: BaseDenom method correctly delegates to the base denom keeper.The method properly delegates to the underlying baseDenomKeeper to maintain separation of concerns.
24-32: GetBaseSpotPrice method handles special cases correctly.The method includes proper error handling and a special case for when the requested denom is the base denom (returning 1.0), while delegating to the tokenPriceKeeper for other denominations.
x/dynamic-fee/keeper/genesis.go (1)
19-28: Good implementation of ExportGenesis with appropriate error handlingThe
ExportGenesisfunction correctly retrieves and exports the module parameters. Usingpanicfor handling errors during export is appropriate since export functionality is expected to succeed in normal operation.app/keepers/keepers.go (3)
75-76: Good: Import statements for dynamic fee module addedThe import statements for the dynamic fee keeper and types are correctly added.
132-132: Field added to AppKeepers structThe
DynamicFeeKeeperfield is properly added to theAppKeepersstruct, allowing access to the dynamic fee functionality throughout the application.
325-335:✅ Verification successful
Clarify the usage of MoveKeeper in dynamic fee keeper initialization
The dynamic fee keeper is initialized with the MoveKeeper being passed three times as parameters. It's not immediately clear why this is needed or what role each instance plays.
Consider adding comments or checking if all three MoveKeeper references are necessary:
dynamicFeeKeeper := dynamicfeekeeper.NewKeeper( appCodec, runtime.NewKVStoreService(appKeepers.keys[dynamicfeetypes.StoreKey]), - movekeeper.NewDexKeeper(appKeepers.MoveKeeper), - appKeepers.MoveKeeper, - appKeepers.MoveKeeper, + movekeeper.NewDexKeeper(appKeepers.MoveKeeper), // For dex functionality + appKeepers.MoveKeeper, // For gas price calculations + appKeepers.MoveKeeper, // For fee collection ac, authorityAddr, )Let's verify the DynamicFeeKeeper constructor to understand why we're passing MoveKeeper multiple times:
🏁 Script executed:
#!/bin/bash # Check the DynamicFeeKeeper constructor to understand how MoveKeeper is used grep -A 20 "func NewKeeper" x/dynamic-fee/keeper/keeper.goLength of output: 694
Clarification: The three MoveKeeper parameters are intentional.
The
dynamicfeekeeper.NewKeeperconstructor expects three distinct interfaces:
- The first parameter (
movekeeper.NewDexKeeper(appKeepers.MoveKeeper)) serves as the token price keeper—providing dex (exchange) functionality.- The second parameter (
appKeepers.MoveKeeper) is used as the whitelist keeper.- The third parameter (
appKeepers.MoveKeeper) fulfills the base denomination keeper role.This design reuses the single
MoveKeeperinstance to satisfy all three required interfaces. For clarity and maintainability, please consider adding inline comments to the initialization code that explicitly document each role, as illustrated below:dynamicFeeKeeper := dynamicfeekeeper.NewKeeper( appCodec, runtime.NewKVStoreService(appKeepers.keys[dynamicfeetypes.StoreKey]), - movekeeper.NewDexKeeper(appKeepers.MoveKeeper), - appKeepers.MoveKeeper, - appKeepers.MoveKeeper, + movekeeper.NewDexKeeper(appKeepers.MoveKeeper), // Provides token price (dex functionality) + appKeepers.MoveKeeper, // Acts as the whitelist keeper + appKeepers.MoveKeeper, // Serves as the base denomination keeper ac, authorityAddr, )x/dynamic-fee/keeper/msg_server.go (4)
1-14: Imports look good and well-organizedThe imports are appropriately organized with standard library imports first, followed by external dependencies, and finally internal packages. This follows good Go practices.
16-20: MsgServer implementation follows standard patternThe
MsgServerstruct properly embeds the*Keeperpointer, which is a common pattern in Cosmos SDK message servers.
22-25: Constructor function is clean and follows standardsThe
NewMsgServerImplfunction is implemented correctly, returning a properly initializedMsgServerinstance.
27-43:❓ Verification inconclusive
Telemetry subsystem name should be updated
The telemetry measurement is using "move" as the subsystem name instead of "dynamic-fee", which doesn't align with the current module.
- defer telemetry.MeasureSince(time.Now(), "move", "msg", "update-params") + defer telemetry.MeasureSince(time.Now(), "dynamic-fee", "msg", "update-params")Otherwise, the
UpdateParamsmethod implementation is solid with proper authority checking, context unwrapping, validation, and parameter updating.
🏁 Script executed:
#!/bin/bash # Check for other telemetry usage patterns in the codebase echo "Checking telemetry usage patterns in the codebase..." rg "telemetry\.MeasureSince.*dynamic-fee" --type goLength of output: 168
Action Required: Update Telemetry Subsystem Naming in UpdateParams
It appears that the telemetry measurement call in
x/dynamic-fee/keeper/msg_server.gostill uses"move"instead of the expected"dynamic-fee". Our automated check for patterns containing"dynamic-fee"returned no instances, which suggests the change has not been applied in this file.
- File:
x/dynamic-fee/keeper/msg_server.go(withinUpdateParams)- Issue: The subsystem name in the telemetry call is
"move"instead of"dynamic-fee".- Suggested Diff:
- defer telemetry.MeasureSince(time.Now(), "move", "msg", "update-params") + defer telemetry.MeasureSince(time.Now(), "dynamic-fee", "msg", "update-params")Please manually verify that:
- The telemetry call is updated as indicated.
- No other parts of the code are inadvertently still using the outdated
"move"subsystem naming.proto/initia/dynamicfee/v1/tx.proto (4)
1-12: Proto file header and imports are well-structuredThe proto file has proper syntax declaration, package definition, and imports. The go_package option is correctly set to point to the types package.
13-20: Msg service definition follows Cosmos SDK standardsThe service definition correctly uses the cosmos.msg.v1.service option and provides a clear RPC method for updating parameters.
22-37: MsgUpdateParams message definition is properly structuredThe message definition follows Cosmos SDK conventions with:
- Proper signer annotation
- Clear documentation for fields
- Appropriate scalar annotation for the authority field
- Non-nullable params field with don't_omitempty option set
This ensures the message will be correctly processed by the Cosmos SDK framework.
39-41: Empty response message is correctly definedThe empty response message follows the standard pattern for Cosmos SDK messages that don't need to return data.
x/dynamic-fee/keeper/gas_price.go (2)
1-13: Interface implementation verification is properly setThe file correctly verifies that
Keeperimplements thetx.GasPriceKeeperinterface using a type assertion, which is a good practice to catch interface implementation errors at compile time.
14-48: GasPrices method is well-implemented with proper error handlingThe
GasPricesmethod correctly:
- Retrieves parameters and extracts the base gas price
- Gets the base denomination
- Retrieves whitelisted tokens
- Calculates gas prices for each token based on their spot price
- Includes thorough error handling
The implementation checks for division by zero by verifying that baseSpotPrice is not zero, which is a good practice.
x/dynamic-fee/types/expected_keeper.go (5)
1-7: Imports are clean and minimalisticThe imports are appropriately limited to only what's necessary for the interfaces defined in this file.
9-11: TokenPriceKeeper interface is well-definedThe
TokenPriceKeeperinterface clearly defines the method needed from token price modules to calculate gas prices in terms of different tokens.
13-15: WhitelistKeeper interface is focused and clearThe
WhitelistKeeperinterface defines a single method for retrieving whitelisted tokens, which is exactly what the dynamic fee module needs.
17-19: BaseDenomKeeper interface follows the same patternThe
BaseDenomKeeperinterface is consistent with the other interfaces, providing a clean way to access the base denomination.
1-20: The interfaces follow the Interface Segregation PrincipleAll three interfaces are narrowly focused on exactly what the dynamic-fee module needs from other modules, following the Interface Segregation Principle from SOLID design principles. This is excellent for maintaining loose coupling between modules.
api/initia/dynamicfee/v1/genesis.pulsar.go (1)
1-599: Auto-generated code confirmation.
This file appears to be generated by Protobuf tools. It neatly defines theGenesisStatemessage and corresponding reflection logic. Typically, such generated code follows standard patterns and is rarely modified manually. No immediate correctness, performance, or security concerns are evident.api/initia/dynamicfee/v1/tx.pulsar.go (1)
19-30: Protobuf message definitions.
BothMsgUpdateParamsandMsgUpdateParamsResponseappear to be standard auto-generated structures designed for dynamic fee parameter updates. The reflection logic is consistent with protobuf best practices, and no obvious issues or improvements stand out.Also applies to: 32-516
x/dynamic-fee/ante/fee_test.go (6)
10-10: Updated import path.
Switching fromx/move/antetox/dynamic-fee/antealigns well with the new dynamic fee module naming.
20-24: Renaming fields and addingbaseGasPrice.
TheTestAnteKeepernow includesbaseGasPriceinstead of a minimum gas price. This is a logical rename consistent with the new dynamic fee approach. Implementation looks clean and straightforward.
27-29: Basic check for base denom.
ReturningLegacyOneDec()for the base denom case is sensible. No concerns regarding edge cases, as any unknown denom falls back to the logic that checkspoolsandweights.
49-50:BaseGasPricefunction.
This getter function returnsbaseGasPricewithout additional checks, matching the new dynamic fee design.
69-75: Fee checker with zero base gas price.
Creating aMempoolFeeCheckerusing zero forbaseGasPriceis helpful for testing no-minimum conditions. Logical approach for negative or boundary scenario testing.
133-137: Fee checker with nonzero base gas price.
Applying a higherbaseGasPriceto simulate insufficient fees is correct. The code properly tests the failure path when fees don’t meet the threshold.api/initia/dynamicfee/module/v1/module.pulsar.go (1)
1-584: No concerns, auto-generated code.This entire file appears to be auto-generated by
protoc-gen-go. It follows the standard reflection patterns for theModuletype, including theAuthorityfield for governance or custom authorization. Auto-generated code typically does not require detailed manual review unless there are specific custom modifications. The implementation here shows no sign of errors or questionable logic.x/dynamic-fee/ante/fee.go (6)
9-9: Change of import package name.Switched from presumably
movetypestodynamicfeetypesfor the AnteKeeper. This aligns with the rest of the dynamic fee module changes.
19-20: MempoolFeeChecker keeper field switched to dynamic fee keeper.The keeper field now references
dynamicfeetypes.AnteKeeperinstead of the older keeper type. This is consistent with the refactoring to the new dynamic fee module.
24-29: Constructor updated to accept dynamic fee keeper.The constructor properly sets the
dynamicfeetypes.AnteKeeper. No issues found.
67-82: Base gas price check for insufficient fee.This newly added check (
gasPriceFromTotalFee < baseGasPrice) ensures transactions pay at least the required gas price, aligning with typical dynamic fee requirements. No issues found.
105-105: Error message referencing totalFeeBaseAmount.The updated error message now includes
totalFeeBaseAmount, which improves debugging visibility for insufficient fees. Looks good.
53-66:✅ Verification successful
Fee calculation logic with base spot prices.
New spot price lookup (
fc.keeper.GetBaseSpotPrice) is introduced to calculatetotalFeeBaseAmount. Consider verifying that the returned base price is non-negative and handles edge cases (e.g., unknown denominators, zero/negative prices).To confirm that no negative or zero base prices are silently accepted, run a quick search over the keeper code to look for any validations:
🏁 Script executed:
#!/bin/bash # This script searches for base spot price validations in the dynamic fee keeper # to ensure it guards against negative/zero price returns. rg "func.*GetBaseSpotPrice" -A 20Length of output: 7584
Ensure Base Spot Price Validation Is Documented
The fee calculation now uses
fc.keeper.GetBaseSpotPrice, which ultimately calls into the underlying implementations (e.g., inx/move/types/connector.go) that return a zero price when a zero balance is detected. This indirectly guards against division errors and unintended zero-price computations. Since coin/amount values are assumed to be non-negative by design, negative base prices should not occur. Still, it might be beneficial to document this invariant explicitly—and, if future changes might introduce external inputs, consider adding an explicit runtime check.
- Verified that when a zero balance is encountered, the connector’s
GetBaseSpotPricereturns a zero price.- No explicit guard against negative prices was found; however, coin amounts are expected to be non-negative by construction.
- Consider documenting these invariants for clarity.
api/initia/dynamicfee/v1/query.pulsar.go (2)
1-50: QueryParamsRequest structure.Standard proto-generated request type. No issues identified. The reflection logic correctly handles an empty message without fields.
380-410: QueryParamsResponse structure with Params field.Implements normal proto reflection methods for the newly introduced
Paramsreference. This is a standard approach for returning module parameters. No concerns regarding functionality or performance.x/dynamic-fee/keeper/keeper.go (4)
16-30: Struct layout appears consistent.The
Keeperstruct is well-structured, housing references to parameter collections and external keepers. Be mindful of any concurrency implications (i.e., if these keepers are ever used concurrently, synchronization may be needed), although there’s no indication of concurrent usage here.
32-61: Panic on schema build failure is acceptable in Cosmos SDK.Your constructor pattern is typical: panic if the schema build fails at startup. This approach ensures fast failure if initialization is invalid. The overall
NewKeepersetup looks good.
67-77: Getter methods look fine.The straightforward getters adhere to standard best practices. No issues noted.
79-85:✅ Verification successful
Parameter handling is straightforward.
Storing and retrieving parameters via
k.Paramsis a clean pattern. Confirm that upstream code handles any errors returned by these methods.Do you want to run a quick check to ensure all calls to
SetParamsandGetParamshandle errors properly in the codebase?
🏁 Script executed:
#!/bin/bash rg -A 5 "SetParams\(|GetParams\("Length of output: 61390
Upstream error handling for parameter functions verified.
All calls to bothSetParamsandGetParamsin the codebase—including those in production and test files—properly check and propagate errors (usingif err != nilchecks, panics where appropriate, and assertions likerequire.NoError). No changes are needed regarding error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
x/dynamic-fee/types/params.go (2)
41-71: Consider adding validation between min and max gas price.The validation logic is thorough for individual parameters, but there's no check to ensure that
MinBaseGasPriceis less than or equal toMaxBaseGasPrice.func (p Params) Validate() error { if p.BaseGasPrice.IsNegative() { return fmt.Errorf("base gas price must be non-negative") } if p.MinBaseGasPrice.IsNegative() { return fmt.Errorf("min base gas price must be non-negative") } if p.MaxBaseGasPrice.IsNegative() { return fmt.Errorf("max base gas price must be non-negative") } + if p.MinBaseGasPrice.GT(p.MaxBaseGasPrice) { + return fmt.Errorf("min base gas price must be less than or equal to max base gas price") + } if p.TargetGas < 0 { return fmt.Errorf("target gas must be non-negative") }
1-7: Consider adding godoc-style comments.Adding godoc-style comments for exported functions and variables would improve documentation and help users understand the purpose of each component better.
package types import ( "fmt" "cosmossdk.io/math" ) +// Default parameter values for the dynamic fee module var (x/dynamic-fee/abci_test.go (4)
20-71: Test_EndBlocker looks good but could be improved with more explicit assertions.The test correctly verifies the dynamic fee behavior by:
- Setting up parameters with a 10% max change rate
- Executing transactions
- Verifying the base gas price increases after transactions
Consider enhancing the test by:
- Adding a more specific assertion on the expected base gas price value or range
- Using constants instead of hardcoded values for parameters
- Adding a comment explaining the expected gas price adjustment behavior
func Test_EndBlocker(t *testing.T) { app := createApp(t) _, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) require.NoError(t, err) ctx := app.BaseApp.NewUncachedContext(false, tmproto.Header{}) + // Setting dynamic fee parameters with 10% max change rate err = app.DynamicFeeKeeper.SetParams(ctx, types.Params{ BaseGasPrice: math.LegacyNewDecWithPrec(15, 3), MinBaseGasPrice: math.LegacyNewDecWithPrec(1, 3), MaxBaseGasPrice: math.LegacyNewDec(10), TargetGas: 1_000_000, MaxChangeRate: math.LegacyNewDecWithPrec(1, 1), }) require.NoError(t, err) _, err = app.Commit() require.NoError(t, err) _, err = app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) require.NoError(t, err) // initialize staking for secondBondDenom ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) // fund addr2 app.BankKeeper.SendCoins(ctx, movetypes.StdAddr, addr2, sdk.NewCoins(secondBondCoin)) _, err = app.Commit() require.NoError(t, err) ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) lessBaseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) require.NoError(t, err) require.True(t, lessBaseGasPrice.LT(types.DefaultBaseGasPrice)) + // Create 100 transactions to simulate high network usage msgs := []sdk.Msg{} for i := 0; i < 100; i++ { msgs = append(msgs, &banktypes.MsgSend{ FromAddress: addr2.String(), ToAddress: addr1.String(), Amount: sdk.NewCoins(sdk.NewInt64Coin(secondBondDenom, 10)), }) } _, err = executeMsgsWithGasInfo(t, app, msgs, []uint64{1}, []uint64{0}, priv2) require.NoError(t, err) ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) baseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) require.NoError(t, err) require.True(t, baseGasPrice.GT(lessBaseGasPrice)) + // Optionally add a more precise assertion about the expected increase + // expectedMinIncrease := lessBaseGasPrice.Mul(math.LegacyNewDecWithPrec(1, 2)) // 1% minimum increase + // require.True(t, baseGasPrice.Sub(lessBaseGasPrice).GTE(expectedMinIncrease)) }
1-118: Consider reducing code duplication between the two test functions.Both test functions share similar structure and steps. Consider extracting common setup and verification logic into helper functions to improve maintainability.
+// testEndBlockerWithParams runs a generic test for the EndBlocker with specific parameters +func testEndBlockerWithParams(t *testing.T, params types.Params, expectPriceChange bool) { + app := createApp(t) + + _, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) + require.NoError(t, err) + + ctx := app.BaseApp.NewUncachedContext(false, tmproto.Header{}) + err = app.DynamicFeeKeeper.SetParams(ctx, params) + require.NoError(t, err) + _, err = app.Commit() + require.NoError(t, err) + + _, err = app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) + require.NoError(t, err) + + // initialize staking for secondBondDenom + ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) + + // fund addr2 + app.BankKeeper.SendCoins(ctx, movetypes.StdAddr, addr2, sdk.NewCoins(secondBondCoin)) + + _, err = app.Commit() + require.NoError(t, err) + + ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) + initialBaseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) + require.NoError(t, err) + + // Create 100 transactions to simulate high network usage + msgs := []sdk.Msg{} + for i := 0; i < 100; i++ { + msgs = append(msgs, &banktypes.MsgSend{ + FromAddress: addr2.String(), + ToAddress: addr1.String(), + Amount: sdk.NewCoins(sdk.NewInt64Coin(secondBondDenom, 10)), + }) + } + + _, err = executeMsgsWithGasInfo(t, app, msgs, []uint64{1}, []uint64{0}, priv2) + require.NoError(t, err) + + ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) + finalBaseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) + require.NoError(t, err) + + if expectPriceChange { + require.True(t, finalBaseGasPrice.GT(initialBaseGasPrice)) + } else { + require.True(t, finalBaseGasPrice.Equal(initialBaseGasPrice)) + } +} + func Test_EndBlocker(t *testing.T) { - app := createApp(t) - - _, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) - require.NoError(t, err) - - ctx := app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - err = app.DynamicFeeKeeper.SetParams(ctx, types.Params{ + params := types.Params{ BaseGasPrice: math.LegacyNewDecWithPrec(15, 3), MinBaseGasPrice: math.LegacyNewDecWithPrec(1, 3), MaxBaseGasPrice: math.LegacyNewDec(10), TargetGas: 1_000_000, MaxChangeRate: math.LegacyNewDecWithPrec(1, 1), - }) - require.NoError(t, err) - _, err = app.Commit() - require.NoError(t, err) - - _, err = app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) - require.NoError(t, err) - - // initialize staking for secondBondDenom - ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - - // fund addr2 - app.BankKeeper.SendCoins(ctx, movetypes.StdAddr, addr2, sdk.NewCoins(secondBondCoin)) - - _, err = app.Commit() - require.NoError(t, err) - - ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - lessBaseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) - require.NoError(t, err) - require.True(t, lessBaseGasPrice.LT(types.DefaultBaseGasPrice)) - - msgs := []sdk.Msg{} - for i := 0; i < 100; i++ { - msgs = append(msgs, &banktypes.MsgSend{ - FromAddress: addr2.String(), - ToAddress: addr1.String(), - Amount: sdk.NewCoins(sdk.NewInt64Coin(secondBondDenom, 10)), - }) - } - - _, err = executeMsgsWithGasInfo(t, app, msgs, []uint64{1}, []uint64{0}, priv2) - require.NoError(t, err) - - ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - baseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) - require.NoError(t, err) - require.True(t, baseGasPrice.GT(lessBaseGasPrice)) + } + testEndBlockerWithParams(t, params, true) } func Test_EndBlocker_NoBaseGasPriceChange(t *testing.T) { - app := createApp(t) - - _, err := app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) - require.NoError(t, err) - - ctx := app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - err = app.DynamicFeeKeeper.SetParams(ctx, types.NoBaseGasPriceChangeParams()) - require.NoError(t, err) - _, err = app.Commit() - require.NoError(t, err) - - _, err = app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1}) - require.NoError(t, err) - - // initialize staking for secondBondDenom - ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - - // fund addr2 - app.BankKeeper.SendCoins(ctx, movetypes.StdAddr, addr2, sdk.NewCoins(secondBondCoin)) - - _, err = app.Commit() - require.NoError(t, err) - - ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - baseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) - require.NoError(t, err) - require.True(t, baseGasPrice.Equal(types.DefaultBaseGasPrice)) - - msgs := []sdk.Msg{} - for i := 0; i < 100; i++ { - msgs = append(msgs, &banktypes.MsgSend{ - FromAddress: addr2.String(), - ToAddress: addr1.String(), - Amount: sdk.NewCoins(sdk.NewInt64Coin(secondBondDenom, 10)), - }) - } - - _, err = executeMsgsWithGasInfo(t, app, msgs, []uint64{1}, []uint64{0}, priv2) - require.NoError(t, err) - - ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) - baseGasPrice, err = app.DynamicFeeKeeper.BaseGasPrice(ctx) - require.NoError(t, err) - require.True(t, baseGasPrice.Equal(types.DefaultBaseGasPrice)) + testEndBlockerWithParams(t, types.NoBaseGasPriceChangeParams(), false) }
51-53: Consider adding a comment explaining why the base gas price is expected to be less than default.It's not immediately obvious why
lessBaseGasPrice.LT(types.DefaultBaseGasPrice)is expected to be true. A comment would help clarify the initial gas price behavior.ctx = app.BaseApp.NewUncachedContext(false, tmproto.Header{}) lessBaseGasPrice, err := app.DynamicFeeKeeper.BaseGasPrice(ctx) require.NoError(t, err) + // After initialization with minimal activity, base gas price should be less than default require.True(t, lessBaseGasPrice.LT(types.DefaultBaseGasPrice))
55-62: Consider using a constant for the number of test transactions.The hardcoded value of 100 transactions could be extracted to a constant for better maintainability.
+ // Number of transactions to simulate network congestion + const numTestTxs = 100 + msgs := []sdk.Msg{} - for i := 0; i < 100; i++ { + for i := 0; i < numTestTxs; i++ { msgs = append(msgs, &banktypes.MsgSend{ FromAddress: addr2.String(), ToAddress: addr1.String(), Amount: sdk.NewCoins(sdk.NewInt64Coin(secondBondDenom, 10)), }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (5)
app/app.go(1 hunks)x/dynamic-fee/abci_test.go(1 hunks)x/dynamic-fee/types/params.go(1 hunks)x/move/keeper/handler.go(1 hunks)x/move/types/params_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- x/move/types/params_test.go
- x/move/keeper/handler.go
- app/app.go
🧰 Additional context used
🧬 Code Definitions (1)
x/dynamic-fee/abci_test.go (2)
x/dynamic-fee/common_test.go (6)
createApp(53-105)addr2(37-37)secondBondCoin(45-45)addr1(35-35)secondBondDenom(31-31)executeMsgsWithGasInfo(114-119)x/dynamic-fee/types/params.go (2)
DefaultBaseGasPrice(10-10)NoBaseGasPriceChangeParams(31-39)
🔇 Additional comments (5)
x/dynamic-fee/types/params.go (3)
9-19: Well-defined default values with clear documentation.The default values are clearly defined with appropriate comments explaining their decimal representations. Using
math.LegacyNewDecWithPrecensures precision for blockchain fee calculations.
21-39: Clean parameter initialization functions.Both
DefaultParams()andNoBaseGasPriceChangeParams()provide clear ways to initialize the parameters with sensible defaults. The zero change rate option is particularly useful for testing or scenarios where price stability is required.
1-72:❓ Verification inconclusive
Missing struct definition for Params.
The file uses the
Paramsstruct throughout but doesn't include its definition. While it might be defined elsewhere in the package, consider either adding a reference comment or including the definition here for better code readability.
🏁 Script executed:
#!/bin/bash # Find the Params struct definition grep -r "type Params struct" --include="*.go" ./x/dynamic-fee/types/Length of output: 70
Action Required: Clarify the Source of the
ParamsDefinitionIt appears that the
Paramsstruct is used throughout the file, yet our search in thex/dynamic-fee/types/directory did not locate its definition. If the struct is defined elsewhere (e.g., in an auto-generated file or another part of the repository), please add a reference note in this file indicating its location. If it’s meant to be part of this package but is missing, consider adding its definition here for better code readability.x/dynamic-fee/abci_test.go (2)
1-18: LGTM on imports and package declaration.The imports look good, providing all the necessary packages for testing the dynamic fee module's EndBlocker functionality.
73-118: Test_EndBlocker_NoBaseGasPriceChange effectively verifies fixed gas price behavior.This test confirms that when MaxChangeRate is set to zero, the base gas price remains unchanged even after executing transactions. The test uses
NoBaseGasPriceChangeParams()which sets the MaxChangeRate to zero, preventing any changes to the base gas price.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/dynamic-fee/types/errors.go (2)
7-9: Add documentation for the error variable.The error definition is clear but could benefit from additional context about when this error occurs and why having a zero target gas is problematic. Consider adding a descriptive comment above the error declaration.
var ( + // ErrTargetGasZero is returned when the target gas parameter is set to zero, + // which would prevent proper fee calculation in the dynamic fee module. ErrTargetGasZero = errorsmod.Register(ModuleName, 2, "target gas is zero") )
1-9: Consider adding more error cases for robustness.This file only defines a single error case (target gas is zero). Consider if there are other validation errors that should be defined for the dynamic fee module, such as validation for other parameters like minimum gas prices or fee adjustment rates.
x/dynamic-fee/keeper/keeper.go (2)
32-61: Consider using pointer receivers for consistency.
All methods onKeeperuse value receivers (func (k Keeper) ...), but this constructor returns a pointer (*Keeper). Adopting pointer receivers for methods would avoid copying theKeeperstruct and maintain consistency.-func (k Keeper) GetAuthority() string { +func (k *Keeper) GetAuthority() string { return k.authority }
79-94: Params management is straightforward, but consider validating parameters.
Storing and retrieving parameters in the store is correct. However, it may be beneficial to validate parameters (e.g., non-zero TargetGas, positive MaxChangeRate) insideSetParamsor via a dedicated validation method to prevent invalid data from being persisted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
x/dynamic-fee/keeper/keeper.go(1 hunks)x/dynamic-fee/types/errors.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
x/dynamic-fee/keeper/keeper.go (3)
api/initia/dynamicfee/v1/types.pulsar.go (3)
Params(690-700)Params(715-715)Params(718-720)x/dynamic-fee/types/expected_keeper.go (3)
TokenPriceKeeper(9-11)WhitelistKeeper(13-15)BaseDenomKeeper(17-19)x/dynamic-fee/types/errors.go (1)
ErrTargetGasZero(8-8)
🔇 Additional comments (4)
x/dynamic-fee/types/errors.go (1)
8-8:✅ Verification successful
Verify the ModuleName constant is properly defined.
The code uses
ModuleNamebut it's not defined in this file. Ensure it's properly defined in another file within this package (likely inconstants.goor similar).
🏁 Script executed:
#!/bin/bash # Check if ModuleName is defined in the package grep -r "ModuleName" --include="*.go" x/dynamic-fee/types/Length of output: 525
ModuleName is properly defined.
The ModuleName constant is already defined inx/dynamic-fee/types/keys.go(with the value"dynamicfee"), so no changes are required in this context.x/dynamic-fee/keeper/keeper.go (3)
16-30: Structures and fields look good.
No concurrency or correctness issues appear in theKeeperstruct fields. Storing references to other keepers and the address codec is consistent with Cosmos SDK conventions.
63-77: Getter methods appear standard and correct.
These simple accessors for authority and other keeper references are straightforward, and no issues stand out regarding correctness, security, or performance.
96-120: Implementation of EIP-1559-like fee adjustment looks correct, but watch for negative multipliers.
The logic for dynamically adjusting the base gas price is solid, including clamping to [MinBaseGasPrice, MaxBaseGasPrice] and returning an error ifTargetGasis zero. Note that ifgasUsedis significantly less thanTargetGas, the computed multiplier can become negative before adding 1, resulting in a final multiplier that is ≤ 1. This is presumably intentional, but keep in mind it effectively decreases the gas price. Also, usingsdk.Contexthere (as opposed tocontext.Contextelsewhere) is consistent with needing block gas meter data, although it’s worth confirming that mixing both context types in one keeper is an intentional design choice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
x/dynamic-fee/keeper/gas_price_test.go (8)
16-28: Consider adding validation for edge cases indecToVmArgumentThe function correctly converts a decimal to VM argument format by reversing byte order and serializing, but it doesn't handle edge cases like zero or negative values. Consider adding validation or tests for these scenarios.
func decToVmArgument(t *testing.T, val math.LegacyDec) []byte { + // Ensure we're handling all cases properly + if val.IsZero() { + bz, err := vmtypes.SerializeBytes([]byte{0}) + require.NoError(t, err) + return bz + } + + // Handle negative values if needed + if val.IsNegative() { + // Implementation depends on how VM handles negative values + } // big-endian bytes (bytes are cloned) bz := val.BigInt().Bytes() // reverse bytes to little-endian slices.Reverse(bz) // serialize bytes bz, err := vmtypes.SerializeBytes(bz) require.NoError(t, err) return bz }
44-44: Consider more robust LP denom constructionThe current LP token denom construction is simplistic and could potentially cause conflicts if denominations contain overlapping patterns. Consider using a separator or more structured approach.
- denomLP := "ulp" + baseCoin.Denom + quoteCoin.Denom + denomLP := "ulp/" + baseCoin.Denom + "/" + quoteCoin.Denom
56-59: Extract magic numbers as constants or parametersThe swap fee (0.3%) and weights are hardcoded values. Consider extracting these as constants or making them function parameters for better flexibility and clarity.
+ const ( + DefaultSwapFeePercent = 0.3 // 0.3% + ) // In the function: - // 0.003 == 0.3% - swapFeeBz := decToVmArgument(t, math.LegacyNewDecWithPrec(3, 3)) + swapFeeBz := decToVmArgument(t, math.LegacyNewDecWithPrec(int64(DefaultSwapFeePercent*10), 3))
91-151: RefactorregisterDexPoolto reduce hardcoded valuesThe function contains multiple hardcoded values for denominations, amounts, and weights. Consider extracting these as constants or parameters to improve test maintainability and clarity.
+ type DexPoolConfig struct { + BaseDenom string + BaseAmount math.Int + QuoteDenom string + QuoteAmount math.Int + BaseWeight math.LegacyDec + QuoteWeight math.LegacyDec + } func registerDexPool( t *testing.T, ctx sdk.Context, input TestKeepers, + basePrice math.LegacyDec, + configs []DexPoolConfig, - basePrice math.LegacyDec ) ([]string, []math.LegacyDec) { // Set params // ... + denoms := make([]string, 0, len(configs)) + prices := make([]math.LegacyDec, 0, len(configs)) + + for _, config := range configs { + metadataQuote, err := movetypes.MetadataAddressFromDenom(config.QuoteDenom) + require.NoError(t, err) + + metadataLP := createDexPool( + t, ctx, input, + sdk.NewCoin(config.BaseDenom, config.BaseAmount), + sdk.NewCoin(config.QuoteDenom, config.QuoteAmount), + config.BaseWeight, config.QuoteWeight, + ) + // Store dex pair and verify price... + // ... + denoms = append(denoms, config.QuoteDenom) + prices = append(prices, quotePrice) + } + + return denoms, prices } // Then in the test: + configs := []DexPoolConfig{ + { + BaseDenom: bondDenom, + BaseAmount: math.NewInt(40), + QuoteDenom: "uusdc", + QuoteAmount: math.NewInt(10), + BaseWeight: math.LegacyNewDecWithPrec(8, 1), + QuoteWeight: math.LegacyNewDecWithPrec(2, 1), + }, + { + BaseDenom: bondDenom, + BaseAmount: math.NewInt(40), + QuoteDenom: "utia", + QuoteAmount: math.NewInt(20), + BaseWeight: math.LegacyNewDecWithPrec(5, 1), + QuoteWeight: math.LegacyNewDecWithPrec(5, 1), + }, + } + denoms, prices := registerDexPool(t, ctx, input, basePrice, configs)
153-164: Improve test assertions with descriptive error messagesThe test assertions would benefit from descriptive error messages to help identify what failed if the test doesn't pass.
- require.Equal(t, basePrice.Quo(gasPrices.AmountOf(denoms[0])), prices[0]) - require.Equal(t, basePrice.Quo(gasPrices.AmountOf(denoms[1])), prices[1]) + require.Equal(t, basePrice.Quo(gasPrices.AmountOf(denoms[0])), prices[0], + "gas price calculation incorrect for %s", denoms[0]) + require.Equal(t, basePrice.Quo(gasPrices.AmountOf(denoms[1])), prices[1], + "gas price calculation incorrect for %s", denoms[1])
156-156: Extract magic number as a named constantThe base price (0.01) is hardcoded. Consider extracting this as a named constant for better readability.
+ const testBaseGasPrice = math.LegacyNewDecWithPrec(1, 2) // 0.01 // Then in the test: - basePrice := math.LegacyNewDecWithPrec(1, 2) // 0.01 + basePrice := testBaseGasPrice
166-181: Add test coverage for error casesThe
TestGasPricefunction only tests the happy path. Consider adding test cases for error scenarios, such as non-existent denominations or edge cases.func TestGasPrice(t *testing.T) { ctx, input := createDefaultTestInput(t) basePrice := math.LegacyNewDecWithPrec(1, 2) // 0.01 denoms, prices := registerDexPool(t, ctx, input, basePrice) // Test happy path gasPrice, err := input.DynamicFeeKeeper.GasPrice(ctx, denoms[0]) require.NoError(t, err) require.Equal(t, gasPrice.Denom, denoms[0]) require.Equal(t, basePrice.Quo(gasPrice.Amount), prices[0]) gasPrice, err = input.DynamicFeeKeeper.GasPrice(ctx, denoms[1]) require.NoError(t, err) require.Equal(t, gasPrice.Denom, denoms[1]) require.Equal(t, basePrice.Quo(gasPrice.Amount), prices[1]) + // Test non-existent denomination + _, err = input.DynamicFeeKeeper.GasPrice(ctx, "nonexistent") + require.Error(t, err) + require.Contains(t, err.Error(), "not found") + + // Test with empty denomination + _, err = input.DynamicFeeKeeper.GasPrice(ctx, "") + require.Error(t, err) + + // Test with base denomination + gasPrice, err = input.DynamicFeeKeeper.GasPrice(ctx, bondDenom) + require.NoError(t, err) + require.Equal(t, gasPrice.Amount, basePrice) }
153-181: Consider adding more comprehensive test casesThe current tests only cover basic scenarios. Consider adding tests for edge cases like:
- Extreme price differences between tokens
- Handling of zero or near-zero prices
- Very large or small gas prices
- Multiple calls to the gas price updater
These would help ensure robustness in different market conditions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
x/dynamic-fee/module.go (7)
33-36: Update comment to refer to "dynamicfee module" instead of "move module"The comment refers to the "move module" but this is actually the "dynamicfee module". This suggests the code might have been copied from another module without updating the documentation.
-// AppModuleBasic defines the basic application module used by the move module. +// AppModuleBasic defines the basic application module used by the dynamicfee module.
49-52: Update module name in commentSimilar to the previous comment, update to refer to "dynamicfee module" instead of "move module".
-// Name returns the move module's name. +// Name returns the dynamicfee module's name.
54-58: Update module reference in commentThe comment should refer to the "dynamicfee module" instead of "move module".
-// DefaultGenesis returns default genesis state as raw bytes for the move +// DefaultGenesis returns default genesis state as raw bytes for the dynamicfee
60-68: Update module reference in commentThe comment should refer to the "dynamicfee module" instead of "move module".
-// ValidateGenesis performs genesis state validation for the move module. +// ValidateGenesis performs genesis state validation for the dynamicfee module.
113-114: Update module reference in commentThe comment should refer to the "dynamicfee module" instead of "move module".
-// RegisterInvariants registers the move module invariants. +// RegisterInvariants registers the dynamicfee module invariants.
127-132: Update module reference in commentThe comment should refer to the "dynamicfee module" instead of "move module".
-// ExportGenesis returns the exported genesis state as raw bytes for the move +// ExportGenesis returns the exported genesis state as raw bytes for the dynamicfee
134-137: Update module reference in commentThe comment should refer to the "dynamicfee module" instead of "move module".
-// EndBlock returns the end blocker for the move module. +// EndBlock returns the end blocker for the dynamicfee module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
x/dynamic-fee/ante/fee_utils.go(0 hunks)x/dynamic-fee/module.go(1 hunks)
💤 Files with no reviewable changes (1)
- x/dynamic-fee/ante/fee_utils.go
🧰 Additional context used
🧬 Code Definitions (1)
x/dynamic-fee/module.go (7)
x/dynamic-fee/keeper/genesis.go (2)
err(22-22)genState(20-20)x/dynamic-fee/types/keys.go (1)
ModuleName(5-5)x/dynamic-fee/types/genesis.go (2)
DefaultGenesisState(11-15)ValidateGenesis(19-21)x/dynamic-fee/keeper/keeper.go (9)
Keeper(16-30)k(63-65)k(67-69)k(71-73)k(75-77)k(79-81)k(83-85)k(87-94)k(97-120)x/dynamic-fee/keeper/msg_server.go (2)
MsgServer(16-18)NewMsgServerImpl(23-25)app/app.go (1)
genesisState(374-374)x/dynamic-fee/abci.go (1)
EndBlocker(15-20)
🔇 Additional comments (5)
x/dynamic-fee/module.go (5)
1-18: Code organization looks goodThe imports are well-organized and include all necessary dependencies for a Cosmos SDK module.
20-31: Interface implementations look correctThe code properly declares interface compliance for all required Cosmos SDK module interfaces. This helps catch incompatibilities at compile time rather than runtime.
38-47: Good error handling in RegisterGRPCGatewayRoutesProper error handling with panic on failure, which is appropriate for initialization code that should fail fast if there's an issue.
108-111: Service registration looks goodThe module correctly registers both message and query services with the Cosmos SDK configurator.
116-125:✅ Verification successful
Update module reference in comment and verify moduleNames usage
The comment refers to "move module" instead of "dynamicfee module". Also, ensure that the keeper's
InitGenesismethod can handle a potentially nil or emptymoduleNamesslice, or properly initialize it in the constructor.-// InitGenesis performs genesis initialization for the move module. It returns +// InitGenesis performs genesis initialization for the dynamicfee module. It returnsLet's verify how the
moduleNamesparameter is used in the keeper'sInitGenesismethod:
🏁 Script executed:
#!/bin/bash # Check how moduleNames is used in the keeper's InitGenesis method rg -t go "func \(.*\) InitGenesis.*moduleNames" --context 10 x/dynamic-fee/keeper/Length of output: 1234
Please update the InitGenesis comment and clarify moduleNames usage
- Updated the comment to refer to the "dynamicfee module" instead of the "move module."
- Verified via the grep output that the keeper's
InitGenesismethod does not use themoduleNamesparameter at all. As a result, passing a nil or empty slice does not affect the initialization process.- If
moduleNamesis intended for future use, consider documenting its purpose or initializing it appropriately in the constructor. Otherwise, you might consider removing the unused parameter to simplify the API.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/dynamic-fee/module.go (2)
33-33: Rename "move module" to "dynamic fee module" in doc comments.Multiple doc comments still refer to this as the "move module," which appears to be leftover from a template. Updating these comments to mention the "dynamic fee module" or simply "this module" will reduce confusion and reinforce consistency.
Apply this diff to correct the doc comments:
-// AppModuleBasic defines the basic application module used by the move module. +// AppModuleBasic defines the basic application module used by the dynamic fee module. -// Name returns the move module's name. +// Name returns the dynamic fee module's name. -// DefaultGenesis returns default genesis state as raw bytes for the move -// module. +// DefaultGenesis returns default genesis state as raw bytes for the dynamic fee module. -// ValidateGenesis performs genesis state validation for the move module. +// ValidateGenesis performs genesis state validation for the dynamic fee module. -// AppModule implements an application module for the move module. +// AppModule implements an application module for the dynamic fee module. -// RegisterInvariants registers the move module invariants. +// RegisterInvariants registers the dynamic fee module invariants. -// InitGenesis performs genesis initialization for the move module. It returns +// InitGenesis performs genesis initialization for the dynamic fee module. It returns -// ExportGenesis returns the exported genesis state as raw bytes for the move -// module. +// ExportGenesis returns the exported genesis state as raw bytes for the dynamic fee module. -// EndBlock returns the end blocker for the move module. +// EndBlock returns the end blocker for the dynamic fee module.Also applies to: 49-49, 54-54, 60-60, 77-77, 112-112, 115-115, 126-126, 133-133
42-47: Optional: Avoid panicking when registering GRPC Gateway routes.In Cosmos SDK modules, it's somewhat common to panic in case of failure. However, for better resilience, consider returning an error instead and handling it at a higher level. This is optional but can help prevent application-wide crashes on recoverable failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
x/move/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (6)
api/initia/move/v1/types.pulsar.go(5 hunks)proto/initia/move/v1/types.proto(3 hunks)x/dynamic-fee/keeper/genesis.go(1 hunks)x/dynamic-fee/module.go(1 hunks)x/move/types/params.go(1 hunks)x/move/types/params_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- x/move/types/params_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- x/move/types/params.go
- x/dynamic-fee/keeper/genesis.go
🧰 Additional context used
🧬 Code Definitions (1)
x/dynamic-fee/module.go (6)
x/dynamic-fee/types/keys.go (1)
ModuleName(5-5)x/dynamic-fee/types/genesis.go (2)
DefaultGenesisState(11-15)ValidateGenesis(19-21)x/dynamic-fee/keeper/keeper.go (1)
Keeper(16-30)x/dynamic-fee/keeper/msg_server.go (2)
MsgServer(16-18)NewMsgServerImpl(23-25)x/dynamic-fee/keeper/querier.go (1)
NewQuerier(16-18)x/dynamic-fee/abci.go (1)
EndBlocker(15-20)
🔇 Additional comments (5)
proto/initia/move/v1/types.proto (3)
22-23: Deprecation Directive Added Correctly forParams.base_min_gas_price
The addition of(amino.dont_omitempty) = trueanddeprecated = truein theParamsmessage aligns well with the previous recommendation (see past review comment by beer-1). This change ensures that the deprecated field remains available for backward compatibility during migration to the new dynamic fee system.
52-53: Consistent Deprecation Update forRawParams.base_min_gas_price
Similar to theParamsmessage, the modifications in theRawParamsmessage—adding(amino.dont_omitempty) = trueanddeprecated = true—ensure consistency across parameter messages. This facilitates a smoother in-store migration while signaling to users that the field should no longer be used.
139-139: Minor Formatting Change inExecuteAuthorizationItem
The tilde marker on line 139 suggests a minor formatting or whitespace adjustment for the closing brace. Please confirm that this change was intentional and does not affect any tooling or code generation expectations.api/initia/move/v1/types.pulsar.go (2)
5969-5970: Correctly implementing deprecation for thebase_min_gas_pricefield.The field is properly marked as deprecated using standard Go documentation comments while maintaining backward compatibility. This approach allows existing code to continue functioning while signaling to developers that they should migrate to the new dynamic-fee module.
Also applies to: 6007-6013, 6043-6044, 6078-6084
6547-6554: Good Protocol Buffers field annotation for deprecated field.The Protocol Buffers annotation
[deprecated=true](visible in the generated code asa8 e7 b0 2a 01) properly marks this field as deprecated at the proto level, ensuring tools and IDEs can provide appropriate warnings.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
x/dynamic-fee/keeper/common_test.go (1)
693-695: Error handling in type conversion utilityThe MustConvertStringToTypeTag function uses panic for error handling, which is acceptable in a test utility but should be used with caution. Consider adding a comment explaining that this function is intended for test scenarios only.
func MustConvertStringToTypeTag(str string) vmtypes.TypeTag { tt, err := vmapi.TypeTagFromString(str) if err != nil { + // Panic is acceptable in test code as test failures should be obvious panic(err) } return tt }x/dynamic-fee/types/params.go (2)
9-19: Consider documenting the default values.
Providing a brief explanation for why these particular default values were chosen can be beneficial for future maintainers.
41-71: Add a check to ensure thatMinBaseGasPriceis less than or equal toMaxBaseGasPrice.
This is a common validation practice that will help avoid inconsistent parameter ranges.Below is a suggested snippet to insert after the
MaxBaseGasPricenon-negative check (e.g. near line 52):+ // Ensure the minimum is not greater than the maximum + if p.MinBaseGasPrice.GT(p.MaxBaseGasPrice) { + return fmt.Errorf("min base gas price must be <= max base gas price") + }x/dynamic-fee/types/keys.go (2)
20-24: Consider future-proofing the comments for store keys.The comment "Keys for dynamic fee store" uses plural form, but currently only a single key (
ParamsKey) is defined. Consider either changing the comment to singular form or ensuring the comment anticipates future additions.
22-24: Consider adding documentation about key usage.While the current comment explains the purpose of the key, it would be beneficial to add more detailed documentation about how exactly this key is used within the module (e.g., what parameters it stores, format, etc.).
x/dynamic-fee/keeper/keeper_test.go (4)
12-57: Comprehensive test for base fee updates, but could benefit from more assertionsThe test correctly validates the
UpdateBaseGasPricefunctionality by testing three scenarios: equal to target gas, above target gas, and no gas consumed. However, it would be more robust if you added assertions to verify that the base gas price stays within the defined min/max boundaries when those thresholds would be exceeded.Consider adding additional test cases:
- Gas consumption that would push the price above
MaxBaseGasPrice- Gas consumption that would push the price below
MinBaseGasPrice- Test with different
MaxChangeRatevalues to ensure they affect the outcome as expected
46-46: Clarify the expected base gas price calculationThe value
0.011(represented asmath.LegacyNewDecWithPrec(11, 3)) should be documented to explain how it was derived from the initial base gas price of0.01when consuming double the target gas.Add a comment explaining the base fee multiplier calculation:
// accumulate gas input.DynamicFeeKeeper.ResetAccumulatedGas(ctx) input.DynamicFeeKeeper.AccumulateGas(ctx, 200000) + // Expected: baseGasPrice * (1 + (200000-100000)/100000 * 0.1) = 0.01 * 1.1 = 0.011 // update base fee err = input.DynamicFeeKeeper.UpdateBaseGasPrice(ctx) require.NoError(t, err)
56-56: Clarify the base gas price reduction calculationThe expected value of
0.0099(represented asmath.LegacyNewDecWithPrec(99, 4)) needs explanation to show how it was derived when no gas was consumed.Add a comment explaining the calculation:
// consume gas input.DynamicFeeKeeper.ResetAccumulatedGas(ctx) + // Expected: baseGasPrice * (1 + (0-100000)/100000 * 0.1) = 0.011 * 0.9 = 0.0099 // update base fee err = input.DynamicFeeKeeper.UpdateBaseGasPrice(ctx) require.NoError(t, err)
59-69: Test coverage for AccumulateGas could be expandedThe current test only verifies basic gas accumulation, but should also test cumulative effects and reset functionality.
Expand the test to verify:
- Multiple accumulations add up correctly
ResetAccumulatedGascorrectly resets the counter to zero- Error handling for edge cases
func Test_AccumulateGas(t *testing.T) { ctx, input := createDefaultTestInput(t) // Test initial gas accumulation gasLimit1 := uint64(100000) err := input.DynamicFeeKeeper.AccumulateGas(ctx, gasLimit1) require.NoError(t, err) accumulatedGas, err := input.DynamicFeeKeeper.GetAccumulatedGas(ctx) require.NoError(t, err) require.Equal(t, gasLimit1, accumulatedGas) // Test cumulative gas accumulation gasLimit2 := uint64(50000) err = input.DynamicFeeKeeper.AccumulateGas(ctx, gasLimit2) require.NoError(t, err) accumulatedGas, err = input.DynamicFeeKeeper.GetAccumulatedGas(ctx) require.NoError(t, err) require.Equal(t, gasLimit1+gasLimit2, accumulatedGas) // Test reset functionality err = input.DynamicFeeKeeper.ResetAccumulatedGas(ctx) require.NoError(t, err) // After reset, getting accumulated gas should return not found error _, err = input.DynamicFeeKeeper.GetAccumulatedGas(ctx) require.ErrorIs(t, err, collections.ErrNotFound) }x/dynamic-fee/keeper/keeper.go (5)
127-131: Add more detailed documentation for the base fee algorithmThe formula for calculating the base fee multiplier is a critical part of the dynamic fee mechanism but lacks detailed explanation.
Expand the comment to explain the reasoning and impact of the formula:
- // baseFeeMultiplier = (accumulatedGas - targetGas) / targetGas * maxChangeRate + 1 + // Calculate the base fee multiplier using a formula inspired by EIP-1559: + // baseFeeMultiplier = (accumulatedGas - targetGas) / targetGas * maxChangeRate + 1 + // + // This formula has the following properties: + // - When accumulatedGas == targetGas: multiplier = 1 (no change) + // - When accumulatedGas > targetGas: multiplier > 1 (fee increases) + // - When accumulatedGas < targetGas: multiplier < 1 (fee decreases) + // - The maxChangeRate parameter controls the maximum percent change
147-151: Consider unifying error handling patternsThere are different error handling patterns between various methods. For example, the
GetAccumulatedGasmethod doesn't handlecollections.ErrNotFoundinternally, whileAccumulateGasandUpdateBaseGasPricedo.Consider using consistent error handling:
- Either handle
collections.ErrNotFoundin all methods, or- Let all methods return the error and handle it at the call site.
For example, you could modify
GetAccumulatedGasto handle the error internally:func (k Keeper) GetAccumulatedGas(ctx context.Context) (uint64, error) { - return k.AccumulatedGas.Get(ctx) + accumulatedGas, err := k.AccumulatedGas.Get(ctx) + if errors.Is(err, collections.ErrNotFound) { + return 0, nil + } + return accumulatedGas, err }
45-75: Consider error handling in NewKeeper constructorThe constructor panics when schema building fails, which might be appropriate for initialization, but it's worth evaluating whether error handling would be more suitable.
For schema building errors, consider whether returning an error would be better than panicking.
This depends on your error handling philosophy - if you consider schema build failures as unrecoverable,
panic is appropriate.
36-75: Add unit tests for parameter validationThe parameter validation in
UpdateBaseGasPricechecks for target gas being zero, but there are no explicit validations when setting parameters withSetParams. Consider adding parameter validation when parameters are set.Consider adding parameter validation in
SetParams:func (k Keeper) SetParams(ctx context.Context, params types.Params) error { + // Validate parameters + if params.TargetGas == 0 { + return types.ErrTargetGasZero + } + if params.BaseGasPrice.IsNegative() { + return fmt.Errorf("base gas price cannot be negative") + } + if params.MinBaseGasPrice.IsNegative() { + return fmt.Errorf("min base gas price cannot be negative") + } + if params.MaxBaseGasPrice.LT(params.MinBaseGasPrice) { + return fmt.Errorf("max base gas price cannot be less than min base gas price") + } + if params.MaxChangeRate.IsNegative() { + return fmt.Errorf("max change rate cannot be negative") + } return k.Params.Set(ctx, params) }And add corresponding unit tests for these validations.
162-165: Consider documenting that ResetAccumulatedGas is for testing onlyThe function comment indicates this is for testing, but it might be clearer to document why this shouldn't be used in production code.
- // ResetAccumulatedGas resets the accumulated gas for testing + // ResetAccumulatedGas resets the accumulated gas. + // NOTE: This method is intended for testing purposes only and should not be + // used in production code as it can disrupt the normal gas accumulation process.app/keepers/keys.go (1)
47-47: Import placement could be improvedThe import for
dynamicfeetypesis added after the Noble forwarding keeper import section, but it belongs with the other Initia-specific imports.Consider moving the import to be grouped with other Initia imports around line 30-34:
// local dependencies ibchookstypes "github.com/initia-labs/initia/x/ibc-hooks/types" ibcnfttransfertypes "github.com/initia-labs/initia/x/ibc/nft-transfer/types" ibcpermtypes "github.com/initia-labs/initia/x/ibc/perm/types" movetypes "github.com/initia-labs/initia/x/move/types" stakingtypes "github.com/initia-labs/initia/x/mstaking/types" rewardtypes "github.com/initia-labs/initia/x/reward/types" +dynamicfeetypes "github.com/initia-labs/initia/x/dynamic-fee/types"x/dynamic-fee/ante/gas_prices.go (1)
14-17: Consider potential confusion between standard and SDK contexts.The
BlockGasMeterinterface relies on the standardcontext.Contexttype instead ofsdk.Context. Ensure that using the standard library context does not conflict with typical Cosmos SDK patterns or overshadowsdk.Context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (13)
app/ante/ante.go(3 hunks)app/keepers/keepers.go(3 hunks)app/keepers/keys.go(2 hunks)x/dynamic-fee/ante/fee_test.go(5 hunks)x/dynamic-fee/ante/gas_prices.go(1 hunks)x/dynamic-fee/ante/gas_prices_test.go(2 hunks)x/dynamic-fee/keeper/common_test.go(1 hunks)x/dynamic-fee/keeper/gas_price.go(1 hunks)x/dynamic-fee/keeper/keeper.go(1 hunks)x/dynamic-fee/keeper/keeper_test.go(1 hunks)x/dynamic-fee/types/ante.go(1 hunks)x/dynamic-fee/types/keys.go(1 hunks)x/dynamic-fee/types/params.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/dynamic-fee/keeper/gas_price.go
- x/dynamic-fee/ante/gas_prices_test.go
🧰 Additional context used
🧬 Code Definitions (5)
app/keepers/keys.go (1)
x/dynamic-fee/types/keys.go (2)
StoreKey(8-8)TStoreKey(11-11)
x/dynamic-fee/ante/gas_prices.go (1)
x/dynamic-fee/ante/context_keys.go (1)
GasPricesContextKey(7-7)
x/dynamic-fee/keeper/keeper.go (4)
api/initia/dynamicfee/v1/types.pulsar.go (3)
Params(690-700)Params(715-715)Params(718-720)x/dynamic-fee/types/expected_keeper.go (3)
TokenPriceKeeper(9-11)WhitelistKeeper(13-15)BaseDenomKeeper(17-19)x/dynamic-fee/types/keys.go (2)
ParamsKey(23-23)AccumulatedGasKey(28-28)x/dynamic-fee/types/errors.go (1)
ErrTargetGasZero(8-8)
app/ante/ante.go (4)
x/dynamic-fee/types/ante.go (1)
AnteKeeper(9-14)x/dynamic-fee/keeper/ante.go (1)
AnteKeeper(10-12)x/dynamic-fee/ante/fee.go (1)
NewMempoolFeeChecker(23-29)x/dynamic-fee/ante/gas_prices.go (1)
NewGasPricesDecorator(26-30)
x/dynamic-fee/keeper/common_test.go (5)
x/dynamic-fee/keeper/keeper.go (1)
Keeper(17-34)x/dynamic-fee/types/keys.go (3)
StoreKey(8-8)TStoreKey(11-11)ModuleName(5-5)x/dynamic-fee/types/params.go (1)
DefaultParams(21-29)x/dynamic-fee/keeper/msg_server.go (1)
NewMsgServerImpl(23-25)x/dynamic-fee/keeper/querier.go (1)
NewQuerier(16-18)
🔇 Additional comments (33)
x/dynamic-fee/keeper/common_test.go (8)
1-88: Good organization of imports and necessary dependenciesThe imports section is well-organized, grouping related packages together. The inclusion of all necessary dependencies from Cosmos SDK, CometBFT, and project-specific packages is thorough and appropriate for a comprehensive test environment.
90-103: Appropriate module setup for testingThe ModuleBasics declaration correctly includes all necessary modules that would interact with the dynamic fee module during testing.
185-231: Well-implemented TestFaucet for testingThe TestFaucet implementation provides a useful abstraction for managing test account balances. The methods for minting, funding, and creating new accounts are well-designed and include proper error handling with test assertions.
233-245: Complete TestKeepers struct with DynamicFeeKeeper integrationThe TestKeepers struct properly includes the DynamicFeeKeeper alongside other necessary keepers, ensuring that the dynamic fee module can be comprehensively tested in its interactions with other modules.
430-446: Ensure gas price parameters are appropriate for test scenariosThe DynamicFeeKeeper initialization uses test-specific parameters for gas prices. These values should be reviewed to ensure they cover a range of scenarios that might occur in production.
MinBaseGasPrice: math.LegacyNewDecWithPrec(1, 3), // 0.001 BaseGasPrice: math.LegacyNewDecWithPrec(1, 2), // 0.01 MaxBaseGasPrice: math.LegacyNewDec(200), // 200The range between min and max gas price is very wide (0.001 to 200), which might be intentional for testing edge cases.
280-285: Correct store setup for the dynamic fee moduleThe dynamic fee module's store key and transient store key are properly mounted in the test environment, which is essential for the module to function correctly.
500-513: Complete keeper initialization for test environmentAll necessary keepers, including the DynamicFeeKeeper, are properly initialized and added to the TestKeepers struct. This ensures a complete test environment where all module interactions can be tested.
434-438: Verify DynamicFeeKeeper dependenciesThe DynamicFeeKeeper is initialized with three dependencies from the MoveKeeper:
- DexKeeper for token price information
- MoveKeeper directly for whitelist functionality
- MoveKeeper again for base denom information
Ensure these are the correct dependencies for the DynamicFeeKeeper's needs. Consider adding comments to clarify the purpose of each dependency.
x/dynamic-fee/types/params.go (3)
1-2: No concerns with the package declaration.
21-29: Implementation looks good.
Returning a properly initializedParamsstruct with default dynamic fee parameters is a clear approach.
31-39: Clarity is good.
No base gas price changes are enforced with a zeroMaxChangeRate. The function name is clear about its intention.x/dynamic-fee/types/keys.go (2)
3-18: LGTM: Constants follow Cosmos SDK module conventions.The module constants are well-defined and properly documented, following the standard naming patterns used in Cosmos SDK modules.
26-29: LGTM: Transient store key for accumulated gas.The
AccumulatedGasKeyis properly defined for tracking accumulated gas in the transient store, which is appropriate for data that doesn't need to persist between blocks.x/dynamic-fee/keeper/keeper_test.go (1)
13-13:❌ Incorrect review comment
Missing test helper function
The test uses
createDefaultTestInput()which is not defined in this file.
🏁 Script executed:
#!/bin/bash # Look for the createDefaultTestInput function definition rg -A 5 "func createDefaultTestInput" --type goLength of output: 5232
Clarification: Test Helper Function Defined in common_test.go
ThecreateDefaultTestInput()helper function is not missing—it’s defined inx/dynamic-fee/keeper/common_test.goand is intentionally shared across tests in the package. Please disregard the original comment as this organization is consistent with our testing conventions.Likely an incorrect or invalid review comment.
x/dynamic-fee/keeper/keeper.go (1)
116-118: Good protection against division by zeroThe check for
params.TargetGas == 0addresses the potential division by zero issue that was mentioned in a previous review.x/dynamic-fee/types/ante.go (1)
12-13: Interface updates align with the dynamic fee module implementationThe changes to the
AnteKeeperinterface are well-aligned with the implementation in the dynamic fee module:
- Removing
HasDexPairmethod reflects the elimination of DEX gas price computations- Renaming
BaseMinGasPricetoBaseGasPricematches the dynamic fee implementation- Adding
AccumulateGasenables tracking gas usage during transaction processingapp/keepers/keys.go (1)
64-64: Properly integrated dynamic-fee module storesThe dynamic-fee module's store keys have been correctly added to both KV store keys and transient store keys, enabling proper state management for the module.
Also applies to: 68-68
app/keepers/keepers.go (2)
75-76: New import references look consistent.No immediate concerns. Good addition of the packages needed for dynamic fee functionality.
132-132: New DynamicFeeKeeper field integrated into AppKeepers.This addition appears well-structured and maintains a clean naming convention consistent with other keepers.
x/dynamic-fee/ante/gas_prices.go (3)
19-23: Struct definition is straightforward.Defining the
GasPricesDecoratorwith a single keeper reference is concise and follows standard design patterns.
25-30: Constructor’s design aligns with best practices.Clean constructor pattern returning a pointer. No concerns here.
32-64: Double-check infinite gas meter usage and context transitions.While this logic to record gas usage without imposing charges can be valid, ensure it does not inadvertently allow unbounded resource consumption. Verify proper retrieval of the stored gas price from the context to maintain consistent fee calculations.
app/ante/ante.go (4)
15-15: New imports for dynamic fee ante package.These imports replace references to the previous Move-based ante logic. The transition appears correct.
Also applies to: 21-21
28-34: New HandlerOptions fields for dynamic fees and additional keepers.Adding multiple fields to unify dynamic fee management, IBC, and auction data demonstrates a clear architectural direction. Names are coherent with other keeper references.
60-60: Switched to dynamic fee checker.Replacing the old fee checker with
dynamicfeeante.NewMempoolFeeCheckeris consistent with the new dynamic fee approach. No issues identified.
82-82: NewGasPricesDecorator integrated in the ante chain.Adopting the GasPricesDecorator from the dynamic fee module is cohesive with the rest of the changes. Verified no immediate overlaps or conflicts.
x/dynamic-fee/ante/fee_test.go (7)
10-10: Good architectural change moving fee management to its own module.Shifting from
x/move/antetox/dynamic-fee/anteclearly separates fee management into its own dedicated module, which improves code organization and modularity.
24-24: Appropriate renaming frombaseMinGasPricetobaseGasPrice.This renaming better reflects the concept that this value represents the standard base gas price rather than just a minimum threshold, which aligns with the dynamic fee management approach.
27-30: Good optimization for base denomination price calculation.Adding a specific check to directly return
math.LegacyOneDec()when the quote denomination matches the base denomination is a logical improvement. This optimization handles the 1:1 price ratio case efficiently.
49-51: Consistent method renaming.The method name change from
BaseMinGasPricetoBaseGasPricemaintains consistency with the field renaming, ensuring a cohesive API.
75-79: Constructor parameters aligned with struct field changes.The constructor parameters for
TestAnteKeeperhave been properly updated to match the renamed fields, maintaining code consistency.
137-141: Constructor parameters consistently updated in second test case.The second initialization of
TestAnteKeepercorrectly uses the renamedbaseGasPricefield, ensuring consistency throughout the test file.
53-55:✅ Verification successful
New
AccumulateGasmethod is a no-op in tests.The newly added
AccumulateGasmethod currently returnsnil, which is appropriate for a test implementation but suggests that the production code will implement gas usage tracking functionality for the dynamic fee system.
🏁 Script executed:
#!/bin/bash # Check if the production implementation of AccumulateGas does more than return nil rg "func.*AccumulateGas" --type go x/dynamic-fee/Length of output: 460
Test Implementation Acknowledged: No-op
AccumulateGasis ExpectedThe
AccumulateGasmethod inx/dynamic-fee/ante/fee_test.goreturnsnil, which is intentional for testing purposes. The production implementation—located inx/dynamic-fee/keeper/keeper.go—appears to contain the actual logic for gas tracking. This separation ensures that the test suite remains focused on behavior validation while the production code handles real gas accumulation as designed.
…corator in x/dynamicfee
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/move/ante/gas_prices_test.go (1)
18-25: Consider reusing the existing TestBlockGasMeter implementation.The
TestBlockGasMeterstruct and itsAccumulateGasmethod duplicate code that already exists inx/dynamic-fee/ante/block_gas_test.go. To maintain DRY principles and ensure consistent behavior across tests, consider importing and reusing the existing implementation rather than duplicating it.- type TestBlockGasMeter struct { - gasUsed uint64 - } - - func (t *TestBlockGasMeter) AccumulateGas(ctx context.Context, gas uint64) error { - t.gasUsed += gas - return nil - } + // Import and use TestBlockGasMeter from x/dynamic-fee/ante/block_gas_test.go
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (5)
app/ante/ante.go(3 hunks)x/dynamic-fee/ante/block_gas.go(1 hunks)x/dynamic-fee/ante/block_gas_test.go(1 hunks)x/move/ante/gas_prices.go(2 hunks)x/move/ante/gas_prices_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/move/ante/gas_prices.go
🚧 Files skipped from review as they are similar to previous changes (1)
- app/ante/ante.go
🧰 Additional context used
🧬 Code Definitions (2)
x/move/ante/gas_prices_test.go (1)
x/dynamic-fee/ante/block_gas_test.go (1)
TestBlockGasMeter(14-16)
x/dynamic-fee/ante/block_gas_test.go (3)
x/move/ante/gas_prices_test.go (1)
TestBlockGasMeter(18-20)x/move/ante/ante_test.go (1)
AnteTestSuite(35-42)x/dynamic-fee/ante/block_gas.go (1)
NewBlockGasDecorator(24-28)
🔇 Additional comments (9)
x/move/ante/gas_prices_test.go (4)
16-16: Good practice: Using a named constant for the base denomination.Using a named constant for
baseDenomrather than hardcoding the denomination throughout the test improves maintainability and readability.
46-46: Explicit context modification improves test clarity.Using
WithIsCheckTx(false)explicitly clarifies that the test is running in normal (non-check) mode, making the test conditions more transparent. This aligns with the comment on line 45: "in normal mode".
4-5: Import "context" package for test improvements.Adding the context import is necessary for the new
AccumulateGasmethod signature, which properly follows Go's standard practice of accepting a context as the first parameter.
11-13: Added imports support the new dynamic fee module integration.The imports for
initiaappare necessary to access theBondDenomconstant, supporting the transition from hardcoded values to configuration-based values.x/dynamic-fee/ante/block_gas_test.go (2)
14-21: TestBlockGasMeter implementation looks good!Clean and straightforward implementation of the block gas meter for testing purposes. The
AccumulateGasmethod correctly increments the gasUsed counter, matching the interface requirement.
23-62: Test cases verify key behaviors correctlyThe test correctly verifies the
BlockGasDecoratorbehavior in all three important execution contexts:
- Normal mode - gas is accumulated (lines 42-47)
- Check tx mode - gas is not accumulated (lines 50-54)
- Simulation mode - gas is not accumulated (lines 57-61)
This ensures the decorator will only accumulate gas during actual transaction processing, which aligns with the expected behavior.
x/dynamic-fee/ante/block_gas.go (3)
13-16: Interface definition is clean and focusedGood interface design with a single, clear responsibility: accumulating gas with context awareness.
18-28: BlockGasDecorator implementation and constructor are well structuredThe decorator properly encapsulates the block gas meter and provides a clean constructor. Good documentation explains the purpose of both.
30-52: AnteHandle implementation is correct and efficientThe implementation correctly:
- Extracts gas from fee transactions
- Only accumulates gas in production mode (not simulate/check)
- Uses an infinite gas meter to prevent charging gas for this operation
- Follows the proper ante handler chain pattern
Note that you're using a value receiver
(d BlockGasDecorator)instead of a pointer receiver(d *BlockGasDecorator). This is fine since the decorator doesn't modify its state, but differs from some other ante handlers that use pointer receivers for consistency.
No description provided.