feat: reintegrate globalfee, upgraded and improved #415
Conversation
WalkthroughThe changes introduce the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (11)app.go (5)
The new imports for the globalfee module are correctly placed and follow the existing pattern of organizing imports by module category. Also applies to: 94-94
The GlobalFeeKeeper field is correctly added to the App struct and appropriately placed in the Noble Modules section.
The GlobalFeeKeeper is correctly added to the dependency injection system, following the established pattern. Line range hint The GlobalFeeKeeper is correctly added to the upgrade handler. However, since this is a reintegration of a previously removed module:
#!/bin/bash
# Look for any existing globalfee migrations or state handling
rg -A 10 "globalfee.*migration|migration.*globalfee"
# Check for any existing globalfee state management
ast-grep --pattern 'func $_($$) {
$$$
globalfee
$$$
}'
The integration of
upgrade/upgrade.go (6)
The new imports are necessary for the GlobalFee module integration and message handling functionality. Also applies to: 29-29, 49-50, 55-56
The addition of Also applies to: 72-72
The GlobalFee module's parameter subspace is properly initialized following the same pattern as other modules.
Based on previous review comments, consider adding these additional message types: bypassMessages := []string{
sdk.MsgTypeURL(&clienttypes.MsgUpdateClient{}),
sdk.MsgTypeURL(&channeltypes.MsgRecvPacket{}),
sdk.MsgTypeURL(&channeltypes.MsgTimeout{}),
sdk.MsgTypeURL(&channeltypes.MsgAcknowledgement{}),
+ "/cosmos.upgrade.v1beta1.MsgCancelUpgrade",
+ "/cosmos.upgrade.v1beta1.MsgSoftwareUpgrade",
+ "/ibc.core.channel.v1.MsgTimeoutOnClose",
+ "/ibc.core.client.v1.MsgRecoverClient",
}
As suggested in a previous review, consider using module name constants instead of hardcoded strings: -bypassMessages = append(bypassMessages, GetModuleMessages(registry, "circle")...)
-bypassMessages = append(bypassMessages, GetModuleMessages(registry, "aura")...)
-bypassMessages = append(bypassMessages, GetModuleMessages(registry, "halo")...)
-bypassMessages = append(bypassMessages, GetModuleMessages(registry, "florin")...)
+bypassMessages = append(bypassMessages, GetModuleMessages(registry, circletypes.ModuleName)...)
+bypassMessages = append(bypassMessages, GetModuleMessages(registry, auratypes.ModuleName)...)
+bypassMessages = append(bypassMessages, GetModuleMessages(registry, halotypes.ModuleName)...)
+bypassMessages = append(bypassMessages, GetModuleMessages(registry, florintypes.ModuleName)...)
The utility function is well-implemented with proper filtering and sorting of message types. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
upgrade/upgrade.go (1)
139-158: Consider adding transaction boundary.The bypass message migration involves multiple state changes. Consider wrapping these operations in a transaction to ensure atomicity.
Would you like me to provide an implementation using the Cosmos SDK transaction management?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
app.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumgo.work.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
app.go(7 hunks)upgrade/store.go(2 hunks)upgrade/upgrade.go(5 hunks)
🔇 Additional comments (11)
upgrade/store.go (2)
10-10: LGTM: Clean import addition
The import follows the existing pattern of organizing imports by their source (Cosmos SDK vs Noble modules).
21-21: Verify upgrade handler implementation for globalfee module
The addition of globalfeetypes.ModuleName to store upgrades is correct. However, ensure that the corresponding upgrade handler properly handles the module's state initialization.
Let's verify the upgrade handler implementation:
✅ Verification successful
Let me try a different search strategy to verify the upgrade handler implementation.
Let me try one final search strategy focusing on the upgrade package and store upgrades.
Upgrade handler implementation for globalfee module is properly configured
The verification shows that:
- The globalfee module is correctly added to store upgrades
- The upgrade handler properly initializes the globalfee module's parameters through its key table setup in
upgrade/upgrade.go - The upgrade handler is properly registered in the app initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check upgrade handler implementation for globalfee module
# Test 1: Look for upgrade handler that handles globalfee initialization
echo "Checking upgrade handlers for globalfee initialization..."
rg -l "globalfee.*[Ii]nit|[Ii]nit.*globalfee" --type go
# Test 2: Check for any migration functions
echo "Checking for migration functions..."
rg "func.*[Mm]igrate.*globalfee|globalfee.*[Mm]igrate" --type go
# Test 3: Verify proper keeper initialization
echo "Checking keeper initialization..."
ast-grep --pattern 'func $_(app *App) $_ {
$$$
globalfee$_
$$$
}'
Length of output: 481
Script:
#!/bin/bash
# Search for upgrade handler registration and implementation
echo "Checking upgrade handler registration..."
rg -A 5 "RegisterUpgradeHandler.*v17" --type go
echo "Checking app initialization of globalfee..."
rg -A 10 "NewApp.*func" --type go
echo "Checking upgrade store keys..."
rg -A 5 -B 5 "StoreUpgrades.*globalfee" --type go
# Look for any globalfee related code in upgrade handlers
echo "Checking globalfee in upgrade handlers..."
rg "globalfee" app/upgrades/ || echo "No matches in upgrades directory"
Length of output: 647
Script:
#!/bin/bash
# Search for upgrade-related files and globalfee references
echo "Searching for upgrade-related files..."
fd -e go upgrade
echo "Checking store upgrades implementation..."
rg -A 10 "StoreUpgrades" --type go
echo "Checking globalfee module initialization..."
rg -A 10 "globalfeetypes.ModuleName" --type go
echo "Looking for upgrade handler in any directory..."
rg -A 5 "RegisterUpgradeHandler" --type go
Length of output: 2134
upgrade/upgrade.go (4)
13-13: LGTM: Required imports for GlobalFee integration.
The new imports are correctly added to support the GlobalFee module integration:
codectypesfor interface registryglobalfeekeeperandglobalfeetypesfor the GlobalFee module
Also applies to: 37-38
45-45: LGTM: Handler signature updated with required dependencies.
The CreateUpgradeHandler signature is properly updated to include:
registry codectypes.InterfaceRegistryfor message type resolutionglobalFeeKeeper *globalfeekeeper.Keeperfor GlobalFee operations
Also applies to: 54-54
78-79: LGTM: GlobalFee param table initialization.
Correctly initializes the GlobalFee parameter table during migration, maintaining consistency with other module parameter migrations.
139-158: Verify error handling in bypass message migration.
The GlobalFee bypass message migration logic looks sound, but let's ensure robust error handling:
Additionally, consider these improvements:
- Add logging for skipped messages that couldn't be resolved
- Consider batching the bypass message operations to improve performance
// Remove unsupported messages from GlobalFee bypass list.
oldBypassMessages, err := globalFeeKeeper.GetBypassMessages(ctx)
if err != nil {
return vm, err
}
err = globalFeeKeeper.BypassMessages.Clear(ctx, nil)
if err != nil {
return vm, err
}
+logger.Info("Migrating GlobalFee bypass messages")
+skippedMessages := 0
for _, bypassMessage := range oldBypassMessages {
resolved, err := registry.Resolve(bypassMessage)
if err != nil || resolved == nil {
+ skippedMessages++
+ logger.Debug("Skipping unresolvable bypass message", "type", bypassMessage)
continue
}
err = globalFeeKeeper.BypassMessages.Set(ctx, bypassMessage)
if err != nil {
return vm, err
}
}
+logger.Info("Completed GlobalFee bypass message migration",
+ "total", len(oldBypassMessages),
+ "skipped", skippedMessages)✅ Verification successful
The search results show consistent error handling patterns throughout the upgrade module, where errors are properly propagated using return vm, err. The GlobalFee bypass message migration follows the same pattern as other sections of the code, with proper error handling for all operations including getting messages, clearing, and setting new values.
The error handling in bypass message migration is consistent and robust
The suggested improvements in the review comment for adding logging and batching are valid optimizations but not critical issues. The core error handling implementation is sound and follows the established patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error handling patterns in other migration functions
rg -A 3 "return\s+vm,\s*err" upgrade/
Length of output: 1003
app.go (5)
43-43: LGTM: Import statements follow the established pattern.
The globalfee module and its keeper are properly imported, maintaining consistency with the existing import structure.
Also applies to: 80-80
131-131: LGTM: App struct modification follows established patterns.
The GlobalFeeKeeper field is correctly added to the App struct, maintaining consistency with other Noble module keepers.
207-207: LGTM: Dependency injection properly configured.
The GlobalFeeKeeper is correctly integrated into the dependency injection system, following the established module organization pattern.
Line range hint 282-291: LGTM: Upgrade handler properly configured with GlobalFeeKeeper.
The GlobalFeeKeeper is correctly added to the upgrade handler parameters, maintaining alphabetical ordering and ensuring proper state migration capabilities during upgrades.
224-224: Verify the TxFeeChecker configuration.
The TxFeeChecker is properly configured using the globalfee module's checker function. This setup will enable the intended fee bypass functionality for specific messages.
Let's verify the fee checker implementation:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
upgrade/upgrade.go(7 hunks)
🔇 Additional comments (1)
upgrade/upgrade.go (1)
244-254: Verify module name prefixes in message type URLs
In the GetModuleMessages function, using strings.HasPrefix may unintentionally include messages from other modules if their message type URLs share similar prefixes. Ensure that the module names provided accurately correspond to the message type URL prefixes to prevent unintended messages from being added to the bypass list.
To confirm that only the intended messages are included, you can run the following script:
✅ Verification successful
Based on the results, I can now generate a final response since we have clear evidence of how GetModuleMessages is used in the codebase.
Module name prefixes are correctly used in message filtering
The usage of strings.HasPrefix in GetModuleMessages is appropriate for this codebase. The function is specifically used to collect messages from well-defined modules ("circle", "aura", "halo", "florin") with clear namespace boundaries. These module names are used as direct prefixes for their respective message type URLs, which follows the standard Cosmos SDK message naming convention of /<module_name>/....
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all registered message type URLs and check their prefixes.
# List all message implementations registered in the codec
ast-grep --pattern $'func (*$Type) XXX_MessageName() string {\n return "$msgTypeURL"\n}' --lang go | \
jq -r '.nodes[].captures[]?.["$msgTypeURL"]' | \
sort | uniq | while read -r msgTypeURL; do
echo "$msgTypeURL"
done | grep "^/$MODULE_NAME\." | sort
# Replace $MODULE_NAME with the actual module name (e.g., "circle", "aura")
Length of output: 407
Script:
#!/bin/bash
# Let's try a different approach to find message type URLs in the codebase
# Search for message type URL patterns in proto files and Go files
echo "Searching in proto files for message definitions:"
fd '\.proto$' --exec rg -l 'message\s+\w+\s*{' {} | while read -r file; do
echo "=== $file ==="
rg 'message\s+\w+\s*{' "$file" -A 1
done
echo -e "\nSearching in Go files for message registrations:"
rg -g '*.go' 'RegisterImplementations.*sdk\.Msg' -A 2 -B 2
echo -e "\nSearching for message type URL constants or string literals:"
rg -g '*.go' '(const\s+.*TypeURL|MsgType.*=.*"/[^"]+")'
Length of output: 546
Script:
#!/bin/bash
# Let's examine the usage context of GetModuleMessages and analyze the module names
# This will help us understand how the function is used and what module names are passed
# First, find where GetModuleMessages is called
echo "=== GetModuleMessages usage ==="
rg "GetModuleMessages\(" -B 3 -A 3
# Look for module name constants or variables that might be used
echo -e "\n=== Module name definitions ==="
rg "ModuleName\s*=\s*[\"'].*[\"']"
# Check for any message registration patterns that might show module prefixes
echo -e "\n=== Message registration patterns ==="
rg "RegisterInterfaces|RegisterInterface|RegisterImplementation" -B 2 -A 2
Length of output: 2119
For the initial testnet upgrade of v8 Helium, our team removed the GlobalFee module in favor of validators configuring the accepted gas prices themselves. Obviously, this is not ideal as the module also allowed certain messages to bypass the gas prices (like CCTP transactions), allowing easier usage of the Noble chain.
This PR reintegrates a Noble owned version of the module, that has been upgraded to Cosmos SDK v0.50!