feat: export wired registries for reuse by integrators #417
Conversation
WalkthroughThe changes involve modifications across three files in the command-line application of the Cosmos SDK. In Changes
Poem
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: 3
🧹 Outside diff range and nitpick comments (2)
cmd/root.go (1)
19-24: Well-structured dependency injection implementation!
The updated function signature improves modularity by accepting dependencies as parameters instead of creating them internally. This change not only supports the PR objective of making ModuleBasicManager accessible to integrators but also enhances testability and flexibility.
Consider documenting these parameters in the function's comment block to help integrators understand what configurations they need to provide.
cmd/commands.go (1)
Line range hint 119-127: Improve error handling structure
While the enhanced error checking is good, consider consolidating the validation checks at the beginning of the function for better readability and maintainability.
Consider restructuring like this:
func appExport(
logger log.Logger,
db dbm.DB,
traceStore io.Writer,
height int64,
forZeroHeight bool,
jailAllowedAddrs []string,
appOpts servertypes.AppOptions,
modulesToExport []string,
) (servertypes.ExportedApp, error) {
+ // Validate inputs
+ homePath, ok := appOpts.Get(flags.FlagHome).(string)
+ if !ok || homePath == "" {
+ return servertypes.ExportedApp{}, errors.New("application home not set")
+ }
+
+ viperAppOpts, ok := appOpts.(*viper.Viper)
+ if !ok {
+ return servertypes.ExportedApp{}, errors.New("appOpts is not viper.Viper")
+ }
+
+ // Configure options
+ viperAppOpts.Set(server.FlagInvCheckPeriod, 1)
+ appOpts = viperAppOpts
+
var (
app *noble.App
err error
)
-
- // this check is necessary as we use the flag in x/upgrade.
- // we can exit more gracefully by checking the flag here.
- homePath, ok := appOpts.Get(flags.FlagHome).(string)
- if !ok || homePath == "" {
- return servertypes.ExportedApp{}, errors.New("application home not set")
- }
-
- viperAppOpts, ok := appOpts.(*viper.Viper)
- if !ok {
- return servertypes.ExportedApp{}, errors.New("appOpts is not viper.Viper")
- }
-
- // overwrite the FlagInvCheckPeriod
- viperAppOpts.Set(server.FlagInvCheckPeriod, 1)
- appOpts = viperAppOpts📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- cmd/commands.go (1 hunks)
- cmd/nobled/main.go (2 hunks)
- cmd/root.go (2 hunks)
🔇 Additional comments (5)
cmd/root.go (1)
62-62: Verify the expanded gas price configuration.
The minimum gas prices configuration now supports multiple denominations (uusdc, ausdy, ueure) all set to 0.
Please ensure:
- This change is intentional and documented
- The implications of zero-value gas prices are understood
✅ Verification successful
Gas price configuration change is intentional and properly integrated
The change to support multiple denominations (uusdc, ausdy, ueure) with zero minimum gas prices is verified to be intentional based on the following findings:
-
These denominations are properly defined in the genesis configuration (local.sh) with correct metadata including descriptions and exponents:
- uusdc: Circle USD Coin
- ausdy: Ondo US Dollar Yield
- ueure: Monerium EUR emoney
-
The e2e tests extensively use these denominations, particularly uusdc, for testing gas fee payments and transactions, confirming their integration into the system's fee mechanism.
-
The zero gas price configuration aligns with the testing environment setup, where the e2e tests show examples of transactions using these denominations for fees.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any documentation or configuration references to gas prices
# and verify if this change is documented
echo "Checking for gas price documentation..."
rg -i "gas.?price" --type md
rg -i "minimum.?gas" --type md
echo "Checking for related configuration files..."
fd -e toml -e yaml -x grep -l "minimum.*gas"
Length of output: 296
Script:
#!/bin/bash
# Let's try a broader search for gas-related configurations and documentation
# Search for any mentions of the specific denominations
echo "Checking for denomination references..."
rg -i "uusdc|ausdy|ueure"
# Search for gas-related configurations in Go files
echo "Checking gas configurations in Go files..."
rg -i "mingas|gas.?price" --type go -A 2 -B 2
# Look for app configuration or chain configuration files
echo "Looking for config files..."
fd "config|app" -e toml -e yaml -e json
# Check for any commit messages related to gas prices
echo "Checking recent commit messages..."
git log --grep="gas" --pretty=format:"%h %s" -n 5
Length of output: 10612
cmd/commands.go (3)
4-6: LGTM: Clean import organization
The added imports are necessary and properly utilized in the codebase.
Line range hint 128-152: LGTM: Robust app creation logic
The app creation logic properly handles both height-specific and regular initialization cases, with appropriate error handling. This maintains the necessary functionality for integrators while ensuring stability.
Line range hint 31-45: Verify ModuleBasicManager integration in command initialization
The command initialization correctly uses the basicManager parameter, which is crucial for the PR's objective of exporting ModuleBasicManager for integrators.
Let's verify the BasicManager usage across the codebase:
✅ Verification successful
ModuleBasicManager integration is correctly implemented
The verification confirms that ModuleBasicManager is properly integrated in the command initialization:
- Correctly passed to
genutilcli.InitCmd()in command initialization - Properly structured in the root command setup
- Consistently used across cmd/root.go, cmd/commands.go, and cmd/nobled/main.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BasicManager integration points
# Look for BasicManager usage in command initialization
rg -l "module\.BasicManager"
# Look specifically for how it's exported and used
ast-grep --pattern 'type $x struct {
$$$
BasicManager $_
$$$
}'
Length of output: 149
Script:
#!/bin/bash
# Let's examine the actual usage of BasicManager in these files
rg -A 5 "module\.BasicManager" cmd/
# Check how BasicManager is initialized and passed
ast-grep --pattern 'basicManager := module.NewBasicManager($$$)'
# Look for genutilcli.InitCmd usage with BasicManager
rg -A 3 "genutilcli.InitCmd"
Length of output: 1171
cmd/nobled/main.go (1)
54-59: Exported ModuleBasicManager for integrators
Exporting ModuleBasicManager by declaring it as a global variable aligns with the PR objectives of making it accessible for integrators. This change facilitates reuse by external partners as intended.
johnletey
force-pushed
the
john/export-modulebasicmanager
branch
from
20fe551 to
30401ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
cmd/nobled/main.go (1)
Line range hint 13-18: LGTM! Clean and robust implementation.
The main function implementation is now more focused and robust:
- Properly delegates initialization to the root command
- Handles errors appropriately with user-friendly error output
- Follows the principle of separation of concerns by moving configuration to init.go
This change aligns well with the transition to dependency injection and improves modularity.
The removal of direct Bech32 prefix configuration from main.go in favor of initialization in init.go is a good architectural decision as it:
- Reduces main.go's responsibilities
- Makes the configuration more maintainable
- Better supports the dependency injection pattern
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumgo.work.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
- cmd/init.go (1 hunks)
- cmd/nobled/main.go (1 hunks)
- cmd/root.go (2 hunks)
🔇 Additional comments (4)
cmd/nobled/main.go (1)
5-6: LGTM! Clean import organization.
The import organization is clean and follows standard Go conventions.
cmd/root.go (2)
64-64: Verify ModuleBasicManager export configuration.
The change correctly uses externally provided ModuleBasicManager, aligning with the PR objective. However, we should verify that it's properly exported for integrators.
#!/bin/bash
# Description: Verify ModuleBasicManager is properly exported
# Test: Check if ModuleBasicManager is exported and accessible
# Look for ModuleBasicManager declaration
ast-grep --pattern 'var ModuleBasicManager = $$$'
# Check if it's used in package exports
rg -A 2 'ModuleBasicManager.*exported' 18-18: Verify initialization order and side effects.
The newly added Initialize() call might have side effects that could impact the command setup. Please ensure that this is the correct point in the lifecycle for initialization.
✅ Verification successful
Initialization order is correct and safe
The Initialize() function is properly placed at the beginning of NewRootCmd. It sets up essential SDK configurations like Bech32 address prefixes, which should be done before any command setup. This is the only call to Initialize() in the codebase, avoiding any duplicate initialization concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Initialize function implementation and its usage
# Test: Look for Initialize function definition and other potential calls
ast-grep --pattern 'func Initialize() {
$$$
}'
# Check for other Initialize calls to ensure we're not duplicating initialization
rg '\bInitialize\(\)'
Length of output: 234
Script:
#!/bin/bash
# Let's try a different approach to find the Initialize function and its usage
# Search for Initialize function definition
rg -l "func Initialize\(\)"
# Search for Initialize function calls with context
rg -A 3 -B 3 "Initialize\(\)"
# Look for any files that might contain initialization logic
fd -t f "init\.go$"
Length of output: 695
cmd/init.go (1)
123-126:
Ensure clientCtx.Keyring is properly initialized before use
The call to clientCtx.Keyring.Backend() assumes that clientCtx.Keyring is initialized. If it's not set, this could lead to a nil pointer dereference. Verify that clientCtx.Keyring is properly initialized before this point.
Run the following script to check if clientCtx.Keyring is initialized before use:
johnletey
force-pushed
the
john/export-modulebasicmanager
branch
from
30401ad to
884d1e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- cmd/init.go (1 hunks)
- cmd/nobled/main.go (1 hunks)
- cmd/root.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/init.go
- cmd/nobled/main.go
🔇 Additional comments (2)
cmd/root.go (2)
57-57: Previous comment about configurable gas prices still applies
The previous suggestion about making gas prices configurable through a configuration file remains valid.
64-64: Verify ModuleBasicManager export configuration
The usage of ModuleBasicManager here aligns with the PR objective to make it available for integrators.
Let's verify the export configuration:
✅ Verification successful
Let me gather more information about the ModuleBasicManager declaration and usage.
ModuleBasicManager is properly exported and initialized
The verification shows that ModuleBasicManager is:
- Correctly declared as an exported variable (uppercase first letter)
- Properly initialized with module registrations
- Used consistently in the codebase for module management
- Available for integrators as intended by the PR objective
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ModuleBasicManager is properly exported
# Expected: ModuleBasicManager should be exported from the package
# Check if ModuleBasicManager is exported (starts with uppercase)
rg -l "^var ModuleBasicManager" cmd/
# Check for any references to ModuleBasicManager in go.mod files that might use this export
rg "noble-assets/noble" -g "go.mod"
Length of output: 171
Script:
#!/bin/bash
# Check the actual declaration and usage of ModuleBasicManager
rg "ModuleBasicManager" cmd/ -A 3 -B 3
# Check if it's exported in any package documentation
rg "ModuleBasicManager" -g "doc.go"
# Look for any exported package variables in cmd package
ast-grep --pattern 'var ModuleBasicManager = $_'
Length of output: 1317
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cmd/root.go (2)
27-28: Consider avoiding global ClientCtx variable
Using a global ClientCtx variable could lead to concurrency issues and makes testing more difficult. Consider passing the context as a parameter to maintain dependency injection principles.
Example approach:
-ClientCtx = ClientCtx.WithCmdContext(cmd.Context())
-clientCtx, err := client.ReadPersistentCommandFlags(ClientCtx, cmd.Flags())
+func NewRootCmd(clientCtx client.Context) *cobra.Command {
+ // ...
+ clientCtx = clientCtx.WithCmdContext(cmd.Context())
+ updatedCtx, err := client.ReadPersistentCommandFlags(clientCtx, cmd.Flags())64-64: Consider consistent dependency injection for ModuleBasicManager
While exporting ModuleBasicManager aligns with the PR objectives, accessing it as a global variable could lead to similar issues as with ClientCtx. Consider passing it as a parameter to maintain consistent dependency injection throughout the codebase.
Example approach:
-initRootCmd(rootCmd, ClientCtx.TxConfig, ModuleBasicManager)
+func NewRootCmd(clientCtx client.Context, moduleBasicManager *module.BasicManager) *cobra.Command {
+ // ...
+ initRootCmd(rootCmd, clientCtx.TxConfig, moduleBasicManager)📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- cmd/init.go (1 hunks)
- cmd/root.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/init.go
🔇 Additional comments (2)
cmd/root.go (2)
18-18: Error handling needed for Initialize()
A previous review has already identified this issue. The Initialize() call still lacks error handling for potential failures during setup.
57-57: Gas prices should be configurable
A previous review has already suggested making gas prices configurable instead of hardcoding them. This becomes even more important now with multiple currencies supported.
johnletey
changed the title
ModuleBasicManager for reuse by integrators
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cmd/root.go(3 hunks)
🔇 Additional comments (2)
cmd/root.go (2)
32-32: Error handling still needed for Initialize()
The previous review comment about adding error handling for Initialize() is still valid and unaddressed.
71-71: Gas prices should be configurable
The previous review comment about making gas prices configurable is still valid and unaddressed.
Prior to Noble v8, the
ModuleBasicManagerwas exported directly from Noble's application. This allowed integrators like Coinbase to easily register interfaces against a local interface registry in order to decode transactions successfully!Since the Noble application now utilizes dependency injection, we need to export this value from the
/cmdpackage.This PR does exactly that!