feat(x/minfee): extract go module #4003
Conversation
| DefaultNetworkMinGasPriceDec, err := sdk.NewDecFromStr(fmt.Sprintf("%f", appconsts.DefaultNetworkMinGasPrice)) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| DefaultNetworkMinGasPrice = DefaultNetworkMinGasPriceDec |
There was a problem hiding this comment.
this doesn't need to happen inside an init() so I refactored it
rootulp
added
the
backport:v2.x
rootulp
requested review from
rach-id,
evan-forbes,
cmwaters and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request primarily updates the import paths for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
x/minfee/constants_test.go (1)
10-16: Consider enhancing test coverage and documentationWhile the test correctly validates the default gas price, consider these improvements:
- Add a comment explaining why "0.000001" is the expected value
- Consider using a table-driven test approach for future extensibility
- Add test cases for edge cases or error conditions if applicable
Example enhancement:
+// TestDefaultNetworkMinGasPriceAsDec verifies that the default network minimum +// gas price is set to 0.000001 (1 utia), which is the minimum denomination +// accepted by the network for transaction fees. func TestDefaultNetworkMinGasPriceAsDec(t *testing.T) { - want, err := sdk.NewDecFromStr("0.000001") - assert.NoError(t, err) - - got := DefaultNetworkMinGasPriceAsDec() - assert.Equal(t, want, got) + testCases := []struct { + name string + expected string + }{ + { + name: "default minimum gas price", + expected: "0.000001", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + want, err := sdk.NewDecFromStr(tc.expected) + assert.NoError(t, err) + + got := DefaultNetworkMinGasPriceAsDec() + assert.Equal(t, want, got) + }) + } }x/minfee/constants.go (2)
12-14: LGTM with minor formatting suggestionThe use of
sdk.NewDecWithPrecensures precise decimal representation, which is crucial for financial calculations.Apply this formatting fix to align with
gofumpt:-var ( - defaultNetworkMinGasPriceAsDec = sdk.NewDecWithPrec(1, 6) -) +var defaultNetworkMinGasPriceAsDec = sdk.NewDecWithPrec(1, 6)🧰 Tools
🪛 golangci-lint
12-12: File is not
gofumpt-ed(gofumpt)
16-18: Add godoc for the exported functionWhile the function is straightforward, it should include godoc documentation as it's an exported function that other packages will use.
Add documentation like this:
+// DefaultNetworkMinGasPriceAsDec returns the network minimum gas price as a SDK decimal. +// This value is used to enforce minimum gas price requirements for transactions. func DefaultNetworkMinGasPriceAsDec() sdk.Dec { return defaultNetworkMinGasPriceAsDec }x/minfee/genesis.go (1)
Line range hint
1-41: Module structure looks ready for extraction.The genesis implementation is well-structured for module extraction with:
- Self-contained parameter handling
- Proper validation logic
- Clean subspace management
- Consistent decimal type usage
As you proceed with extracting other modules like
x/blobandx/signal, consider using this implementation as a template for consistent genesis handling across modules.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumx/minfee/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
app/ante/fee_checker.go(1 hunks)app/ante/min_fee_test.go(3 hunks)app/app.go(1 hunks)app/app_test.go(1 hunks)app/modules.go(1 hunks)app/test/query_test.go(2 hunks)app/test/std_sdk_test.go(2 hunks)app/test/upgrade_test.go(2 hunks)go.mod(5 hunks)pkg/appconsts/initial_consts.go(1 hunks)pkg/user/tx_client.go(1 hunks)test/tokenfilter/setup.go(1 hunks)x/minfee/constants.go(1 hunks)x/minfee/constants_test.go(1 hunks)x/minfee/genesis.go(1 hunks)x/minfee/go.mod(1 hunks)x/minfee/module_test.go(1 hunks)x/minfee/params.go(1 hunks)x/paramfilter/test/gov_params_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- app/ante/fee_checker.go
- app/app.go
- app/app_test.go
- pkg/user/tx_client.go
- test/tokenfilter/setup.go
- x/minfee/go.mod
- x/minfee/module_test.go
- x/paramfilter/test/gov_params_test.go
🧰 Additional context used
🪛 golangci-lint
x/minfee/constants.go
12-12: File is not gofumpt-ed
(gofumpt)
🔇 Additional comments (23)
x/minfee/constants_test.go (1)
1-8: LGTM: Package and imports are well-structured
The package name and imports follow Go conventions and include only the necessary dependencies.
x/minfee/constants.go (2)
1-3: LGTM: Package declaration and imports are clean
The package name and import are appropriate for the module's purpose.
5-10: Verify the minimum gas price value across the system
The constant value of 0.000001 utia needs verification to ensure it aligns with:
- Existing gas price requirements in the system
- Economic parameters of the network
- Version 2.x compatibility as mentioned in the comment
✅ Verification successful
Based on the search results, I can now provide a final response since I have gathered sufficient information about the minimum gas price constant and its usage across the codebase:
DefaultNetworkMinGasPrice value of 0.000001 utia is correctly set
The value is appropriate because:
- It serves as a network-wide minimum threshold for app version >= 2, while a higher local node minimum (DefaultMinGasPrice = 0.002 utia) acts as the first filter in CheckTx
- The small value allows flexibility for validators to set their own minimum gas prices above this network floor
- The value is properly validated through:
- Genesis validation preventing zero/negative values
- Parameter store type safety checks
- Governance parameter change proposals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other gas price constants and their usage
echo "Searching for gas price related constants and their usage..."
rg -A 2 "MinGasPrice|GasPrice" --type go
# Search for any existing version checks related to gas pricing
echo "Searching for version-related gas price checks..."
rg -A 2 "version.*2" --type go
Length of output: 62180
app/test/query_test.go (2)
Line range hint 1-12: LGTM! Package and import changes align with module extraction.
The package name change to app_test correctly reflects the file's location, and the updated import path for the minfee package aligns with the goal of extracting it as a separate module.
Line range hint 14-27: Verify constant value consistency after module extraction.
The test changes look good, appropriately using minfee.DefaultNetworkMinGasPriceAsDec() instead of the previous appconsts value.
Let's verify that the constant values remain consistent:
✅ Verification successful
Constant values are consistent across both locations
The verification confirms that DefaultNetworkMinGasPrice maintains the same value of 0.000001 in both locations:
x/minfee/constants.gopkg/appconsts/initial_consts.go
However, let's verify if there are any references to the old location to ensure complete migration:
Constants migration is complete and values are consistent
The verification confirms:
- The constant value
0.000001is identical in both locations - No remaining references to
appconsts.DefaultNetworkMinGasPriceexist in the codebase, indicating successful migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the DefaultNetworkMinGasPrice values between old and new locations
# to ensure consistency during the module extraction.
# Search for the old constant definition
echo "Old constant definition:"
rg "DefaultNetworkMinGasPrice.*=.*" --type go
# Search for the new constant definition
echo -e "\nNew constant definition in minfee module:"
rg "DefaultNetworkMinGasPrice.*=.*" --type go ./x/minfee
Length of output: 480
Script:
#!/bin/bash
# Search for any remaining references to appconsts.DefaultNetworkMinGasPrice
rg "appconsts\.DefaultNetworkMinGasPrice" --type go
Length of output: 53
x/minfee/params.go (2)
Line range hint 1-53: Implementation follows Cosmos SDK best practices
The parameter handling implementation is clean and follows Cosmos SDK conventions:
- Proper interface implementation
- Clean parameter registration
- Type-safe validation
🧰 Tools
🪛 golangci-lint
14-14: File is not gofumpt-ed
(gofumpt)
15-15: Verify the default value initialization location
The parameter key is properly defined, but we need to ensure the default value initialization (previously in init()) has been moved to an appropriate location.
✅ Verification successful
Let me gather more information about the genesis initialization.
Default value initialization is properly handled
The default value initialization has been moved from init() to a more appropriate location. The value is now:
- Defined as a constant in
constants.go - Properly initialized using
DefaultNetworkMinGasPriceAsDec() - Used in
DefaultGenesis()to set the initial genesis state
This refactoring follows better practices by avoiding init() and providing a clear initialization path through genesis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find where the default network min gas price is now initialized
# Expected: Find new initialization location in app initialization or genesis
# Search for potential default value initialization
rg -A 5 "NetworkMinGasPrice.*=.*" --type go
# Search for genesis-related initialization
ast-grep --pattern 'func DefaultGenesis() $_ {
$$$
NetworkMinGasPrice
$$$
}'
Length of output: 4481
Script:
#!/bin/bash
# Search for DefaultGenesis implementation in minfee module
rg -A 10 "func DefaultGenesis" x/minfee/
# Search for where defaultNetworkMinGasPriceAsDec is used
rg -A 5 "defaultNetworkMinGasPriceAsDec" x/minfee/
Length of output: 1079
pkg/appconsts/initial_consts.go (2)
33-35: Clear deprecation notice with migration path.
The deprecation notice and TODO comment effectively communicate the migration path to developers, aligning with the goal of extracting the x/minfee module.
33-35: Verify all usages of this constant.
Since this constant will be removed in v4, we should ensure all existing code is updated to import from x/minfee.
✅ Verification successful
All usages of DefaultNetworkMinGasPrice are correctly importing from x/minfee
Based on the verification results:
- The constant is properly defined in
x/minfee/constants.go - All files using this constant (
app/ante/min_fee_test.go,app/test/query_test.go) are correctly importing it from thex/minfeepackage - No files are importing this constant from the deprecated location in
pkg/appconsts
The deprecation notice is working as intended, and all existing code is already using the correct import path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of DefaultNetworkMinGasPrice to ensure they're updated
# to import from x/minfee instead of pkg/appconsts
# Search for direct usage of the constant
echo "Checking for direct usage of DefaultNetworkMinGasPrice:"
rg -l "DefaultNetworkMinGasPrice" --type go
# Search for imports of pkg/appconsts to identify potential indirect usage
echo -e "\nChecking for imports of pkg/appconsts that might use this constant:"
rg -l "\"github.com/celestiaorg/celestia-app/pkg/appconsts\"" --type go
Length of output: 586
Script:
#!/bin/bash
# Let's examine the actual usage context of DefaultNetworkMinGasPrice in these files
echo "Checking DefaultNetworkMinGasPrice usage in x/minfee/constants.go:"
rg -A 5 -B 5 "DefaultNetworkMinGasPrice" x/minfee/constants.go
echo -e "\nChecking DefaultNetworkMinGasPrice usage in app/test files:"
rg -A 5 -B 5 "DefaultNetworkMinGasPrice" "app/test/*.go"
echo -e "\nChecking DefaultNetworkMinGasPrice usage in app/ante/min_fee_test.go:"
rg -A 5 -B 5 "DefaultNetworkMinGasPrice" app/ante/min_fee_test.go
echo -e "\nChecking imports in these files to see if they're using the correct package:"
for file in x/minfee/genesis.go x/minfee/constants_test.go x/minfee/constants.go app/test/upgrade_test.go app/test/std_sdk_test.go app/test/query_test.go app/ante/min_fee_test.go; do
echo -e "\nImports in $file:"
rg "^import \(" -A 10 "$file"
done
Length of output: 7092
app/ante/min_fee_test.go (3)
13-13: LGTM: Import path aligns with module extraction
The updated import path correctly reflects the extraction of minfee as a separate module.
60-60: Verify constant value preservation
The gas limit calculations now use minfee.DefaultNetworkMinGasPrice instead of the previous constant from appconsts. While the change is correct, we should verify the value remains consistent.
#!/bin/bash
# Description: Compare the DefaultNetworkMinGasPrice values
# Search for the old constant definition
echo "Old constant definition:"
rg "DefaultNetworkMinGasPrice.*=.*" --type go
# Search for the new constant definition
echo -e "\nNew constant definition:"
rg "DefaultNetworkMinGasPrice.*=.*" --type go --glob "x/minfee/**"Also applies to: 68-68, 76-76, 84-84
147-147: Verify the new Dec conversion method
The parameter subspace now uses minfee.DefaultNetworkMinGasPriceAsDec(). Let's verify this method's implementation to ensure correct decimal conversion.
✅ Verification successful
Let me gather more information about the implementation details and test coverage.
Decimal conversion implementation verified successfully
The DefaultNetworkMinGasPriceAsDec() method is correctly implemented:
- Returns a properly initialized
sdk.Decwith value 0.000001 (1e-6) - Implementation is backed by comprehensive unit tests verifying the exact decimal value
- The constant is safely defined as an immutable package-level variable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of DefaultNetworkMinGasPriceAsDec
# Search for the method definition and its tests
echo "Method implementation:"
ast-grep --pattern 'func DefaultNetworkMinGasPriceAsDec() sdk.Dec'
echo -e "\nRelated tests:"
rg "DefaultNetworkMinGasPriceAsDec" --type go --glob "x/minfee/**/*_test.go"
Length of output: 560
Script:
#!/bin/bash
# Check the complete implementation including the constant definition
echo "Constants and implementation:"
rg -B 5 -A 2 "DefaultNetworkMinGasPriceAsDec" x/minfee/constants.go
echo -e "\nTest implementation:"
cat x/minfee/constants_test.go
Length of output: 700
app/test/upgrade_test.go (2)
19-19: LGTM: Import path change aligns with module extraction
The change from versioned to non-versioned import path for the minfee package is correct and consistent with the PR's objective of extracting it as a separate Go module.
172-172: LGTM: Improved parameter verification approach
The change to use minfee.DefaultNetworkMinGasPriceAsDec().String() is an improvement as it:
- Provides type safety
- Eliminates potential floating-point precision issues
- Ensures consistency with the module's canonical default values
app/modules.go (5)
Line range hint 61-90: LGTM: Module registration is correct
The minfee module is properly registered in ModuleBasics along with other modules, maintaining consistency with the Cosmos SDK module system.
Line range hint 165-168: LGTM: Version range is appropriate
The minfee module is correctly configured with version range v2 to v3, which aligns with its introduction in v2 of the application.
Line range hint 301-320: Confirm: No store key needed for minfee
The minfee module correctly doesn't have its own store key as it utilizes the params store. This is consistent with its design as a parameter-based module.
Line range hint 271-274: Critical: Maintain module initialization order
The comment correctly notes that the minfee module must be initialized before genutil to ensure proper fee checking during transaction delivery. This ordering is crucial for maintaining system integrity.
#!/bin/bash
# Description: Verify module initialization order in other files
# Look for any other InitGenesis orderings that might conflict
rg --type go "SetOrderInitGenesis|InitGenesis.*minfee" -A 5 -B 516-16: Verify import path compatibility
The minfee module now uses an unversioned import path while other modules still use v3. This change aligns with the goal of extracting the module, but we should verify that this doesn't cause version compatibility issues.
✅ Verification successful
Import path change is correct and safe
The change to use an unversioned import path for the minfee module is correct and safe because:
- The
go.modfile shows thatgithub.com/celestiaorg/celestia-app/x/minfeeis explicitly replaced with a local module (./x/minfee), indicating this is an intentional extraction. - All other imports in the codebase consistently use versioned paths (
v3) for internal modules that remain part of the main app. - The change aligns with the PR's goal of extracting the
minfeemodule into a separate Go module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential version conflicts in imports
# Look for any other unversioned imports that might indicate a pattern
rg --type go "github.com/celestiaorg/celestia-app/[^v][^0-9]" -l
# Check if minfee is imported elsewhere with versioned path
rg --type go "github.com/celestiaorg/celestia-app/v[0-9]/x/minfee"
Length of output: 4108
Script:
#!/bin/bash
# Let's check for all Go module imports in the codebase to understand the versioning pattern
rg --type go "github.com/celestiaorg/celestia-app" -B 1 -A 1
# Also check go.mod to understand module versioning
cat go.mod
Length of output: 103798
go.mod (2)
258-260: LGTM: Temporary replace directive is well documented
The replace directive for x/minfee is well documented with clear comments explaining its temporary nature and the plan for creating an official release.
36-36: Verify if genproto updates are necessary
The updates to google.golang.org/genproto packages seem unrelated to the main PR objective of extracting the minfee module. Please verify if these updates are necessary for this PR.
Also applies to: 241-241
✅ Verification successful
The genproto dependency is required for gRPC API annotations
The google.golang.org/genproto/googleapis/api dependency is necessary as it's used in the generated protobuf code (query.pb.go) for gRPC API annotations. This is a common requirement for gRPC services in Cosmos SDK modules and is not an unnecessary update.
- Found in
x/minfee/query.pb.go:_ "google.golang.org/genproto/googleapis/api/annotations"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the genproto updates are required by the minfee module
# Search for imports of genproto packages in minfee module
echo "Checking genproto usage in minfee module:"
rg -l "google.golang.org/genproto" x/minfee/
# Check if these versions are required by other dependencies
echo "Checking version requirements from other dependencies:"
go mod graph | rg "google.golang.org/genproto"
Length of output: 427
Script:
#!/bin/bash
# Let's check the actual usage of genproto in the minfee module files
# Check content of files that mention genproto
echo "Content of x/minfee/go.mod:"
cat x/minfee/go.mod
echo -e "\nChecking actual imports in query.pb.go:"
rg -A 5 "google.golang.org/genproto" x/minfee/query.pb.go
# Check if there are any direct references to genproto APIs
echo -e "\nChecking for actual usage of genproto APIs:"
rg "genproto" x/minfee/ -g '!{go.mod,go.sum}'
Length of output: 7396
app/test/std_sdk_test.go (2)
352-352: Verify constant availability in new module
The test now references minfee.DefaultNetworkMinGasPrice instead of appconsts.DefaultNetworkMinGasPrice. Let's ensure this constant is properly exported in the new module.
#!/bin/bash
# Description: Verify the DefaultNetworkMinGasPrice constant is exported in minfee package
# Expected: Find the constant declaration
ast-grep --pattern 'const DefaultNetworkMinGasPrice = $_' 17-17: Verify import path consistency across codebase
The import path change from versioned to non-versioned aligns with extracting minfee as a separate Go module.
| @@ -10,7 +10,7 @@ import ( | |||
| // DefaultGenesis returns the default genesis state. | |||
| func DefaultGenesis() *GenesisState { | |||
| return &GenesisState{ | |||
| NetworkMinGasPrice: DefaultNetworkMinGasPrice, | |||
| NetworkMinGasPrice: DefaultNetworkMinGasPriceAsDec(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Duplicate constant definition and inconsistent type usage needs attention
The verification reveals several issues that need to be addressed:
- The
DefaultNetworkMinGasPriceconstant is duplicated in bothx/minfee/constants.goandpkg/appconsts/initial_consts.go - The codebase mixes usage between the float constant and the decimal function:
- Test files (
min_fee_test.go,std_sdk_test.go) use the float constant for gas calculations - Genesis and query code uses the decimal function
- Test files (
- The decimal version is actually stored as a package variable
defaultNetworkMinGasPriceAsDecinitialized withsdk.NewDecWithPrec(1, 6)
Recommendations:
- Remove the duplicate constant from
pkg/appconsts/initial_consts.go - Update test files to use
DefaultNetworkMinGasPriceAsDec()for consistency - Consider deprecating the float constant in favor of the decimal version
🔗 Analysis chain
LGTM! Improved type safety in genesis state initialization.
The change to use DefaultNetworkMinGasPriceAsDec() ensures consistent decimal type handling and better type safety.
Let's verify the consistent usage of this new function across the codebase:
Inconsistent usage of DefaultNetworkMinGasPrice detected across the codebase
The verification reveals mixed usage of both the old constant DefaultNetworkMinGasPrice and the new function DefaultNetworkMinGasPriceAsDec(). Let's verify the implementation details to understand if this is intentional:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of the old DefaultNetworkMinGasPrice
# and verify the new function is used consistently
# Search for any remaining direct usage of DefaultNetworkMinGasPrice
rg "DefaultNetworkMinGasPrice[^A]"
# Search for usage of the new function
rg "DefaultNetworkMinGasPriceAsDec"
Length of output: 1550
Script:
#!/bin/bash
# Check the implementation and relationship between both versions
ast-grep --pattern 'func DefaultNetworkMinGasPriceAsDec'
# Check the constant declaration
rg "DefaultNetworkMinGasPrice = " -A 2
Length of output: 362
Script:
#!/bin/bash
# Let's check the implementation of DefaultNetworkMinGasPriceAsDec
cat x/minfee/constants.go
# Check if there are any imports of the old constant from pkg/appconsts
rg -p "appconsts.*DefaultNetworkMinGasPrice"
Length of output: 544
| @@ -6,6 +6,7 @@ require ( | |||
| cosmossdk.io/errors v1.0.1 | |||
| cosmossdk.io/math v1.3.0 | |||
| github.com/celestiaorg/blobstream-contracts/v3 v3.1.0 | |||
| github.com/celestiaorg/celestia-app/x/minfee v0.0.0 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a valid semantic version for x/minfee
The dependency version v0.0.0 might cause issues with Go module resolution. Consider using a proper semantic version (e.g., v0.1.0) for the initial release.
- github.com/celestiaorg/celestia-app/x/minfee v0.0.0
+ github.com/celestiaorg/celestia-app/x/minfee v0.1.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| github.com/celestiaorg/celestia-app/x/minfee v0.0.0 | |
| github.com/celestiaorg/celestia-app/x/minfee v0.1.0 |
Motivation
As part of the refactor to a multiplexed app, we want a mechanism to avoid importing state machine modules (e.g.
x/minfee) once per state machine version. We want this because each time a state machine module is imported by the multiplexer, it registers proto types in the proto registry and interfaces in the Cosmos SDK interface registry. When we import the same state machine module more than once, we hit duplicate proto registry errors (same for Cosmos SDK interface registry).This PR is the first of many that spin out a distinct Go module per state machine module. Expect subsequent PRs for
x/blob,x/signal, etc.Note: if we didn't do this for the multiplexed app, it is likely that we would do this as part of an update to new Cosmos SDK versions. For example Cosmos SDK v0.52.x spun out Go modules for all the state machine modules.
Note: the diff of
x/minfeefrom v2.x to main is minimal (seegit diff v2.x..main -- x/minfee/) so I choose to do this on main. We'll need to backport this to v2.x as well.Testing
I did this on my fork and verified that we can:
Ref: