feat: add software-upgrade and recover-client helper
#4
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily enhancing the command-line interface for software upgrades and IBC client recovery. Significant modifications include updates to the Changes
Possibly related PRs
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
boojamya
changed the title
software-upgrade and recover-client wrapper cmd
boojamya
changed the title
software-upgrade and recover-client wrapper cmdsoftware-upgrade and recover-client helper
![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 (21)
client/cli/utils.go (2)
28-37: Consider adding error handling for edge cases.While the implementation is correct, consider adding error handling for the following edge cases:
- When
os.Argsis empty- When the executable path is malformed
Here's a suggested improvement:
func getDefaultDaemonName() string { // DAEMON_NAME is specifically used here to correspond with the Cosmovisor setup env vars. name := os.Getenv("DAEMON_NAME") if len(name) == 0 { + if len(os.Args) == 0 { + return "unknown" + } _, name = filepath.Split(os.Args[0]) + if name == "" { + return "unknown" + } } return name }
1-37: Consider moving utility functions to a dedicated package.Since these are utility functions copied from the SDK, consider moving them to a dedicated
internal/utilspackage. This would:
- Better organize the codebase
- Make it clear these are internal utilities
- Prevent potential naming conflicts if the SDK exports these functions in the future
e2e/ownership_test.go (1)
22-22: Consider utilizing the returned logger for enhanced debugging.Since the
Suitefunction now returns a logger, consider capturing and using it instead of ignoring it. This could be valuable for debugging test failures by logging key state transitions during the ownership transfer process.-ctx, _, _, _ := Suite(t, &wrapper, false) +ctx, logger, _, _ := Suite(t, &wrapper, false)You could then add debug logs at key points:
logger.Debug("Starting ownership transfer test") // ... after first transaction logger.Debug("Ownership transfer initiated", zap.String("pending_owner", wrapper.pendingOwner.FormattedAddress())) // ... after second transaction logger.Debug("Ownership transfer completed", zap.String("new_owner", wrapper.pendingOwner.FormattedAddress()))e2e/fee_test.go (1)
Line range hint
13-13: Consider improving the COLLECTOR constant definition.The
COLLECTORconstant could benefit from:
- Documentation explaining its purpose and significance
- Making it unexported since it's only used within this package
- Consider making it configurable rather than hardcoding the address
-var COLLECTOR = "noble17xpfvakm2amg962yls6f84z3kell8c5lc6kgnn" +// collector represents the address that receives collected fees +const collector = "noble17xpfvakm2amg962yls6f84z3kell8c5lc6kgnn"e2e/upgrade_test.go (2)
26-27: Consider documenting the funding amount rationale.While the test user setup is good for negative testing, consider adding a comment explaining why 100000 was chosen as the funding amount. Is this amount significant for the test case?
+ // Fund the unauthorized user with sufficient tokens to cover transaction fees notAuthorized := interchaintest.GetAndFundTestUsers(t, ctx, "wallet", math.NewInt(100000), wrapper.chain)[0]
30-32: Reconsider using --no-validate flag and document upgrade height.
- The
--no-validateflag might skip important validations. Consider documenting why validation is skipped or remove the flag if not necessary.- The upgrade height "50" appears to be a magic number. Consider making it a named constant with documentation.
+ const testUpgradeHeight = 50 // Sufficient blocks ahead to ensure upgrade proposal can be processed cmd := []string{"authority", "software-upgrade", "v2", "--upgrade-height", "50", - "--chain-id", wrapper.chain.Config().ChainID, "--no-validate"} + "--chain-id", wrapper.chain.Config().ChainID}Makefile (1)
70-70: Consider documenting the build configuration.Since the build now uses an explicit configuration file, it would be helpful to document the expected structure and required fields of
chains.yamlin the repository's documentation.Would you like me to help create a documentation template for the chains.yaml configuration?
e2e/clients_test.go (5)
36-38: Ensure trusting period is sufficient for test stabilitySetting
tp := 20 * time.Secondestablishes a very short trusting period. While this is acceptable for testing purposes, consider potential test flakiness due to timing sensitivities, especially in slower or loaded environments. Ensure that the trusting period is sufficient to consistently reproduce the desired client expiration behavior.
59-64: Usetime.Afterfor cleaner timer implementationInstead of creating a timer and waiting on its channel, you can simplify the code using
time.After. This makes the code more concise and idiomatic.Apply this change:
-timer := time.NewTimer(tp + 2*time.Second) -logger.Info("waiting for client to expire...") -<-timer.C +logger.Info("waiting for client to expire...") +<-time.After(tp + 2*time.Second)
82-83: Clarify the purpose of the unauthorized userThe variable name
notAuthorizedmight be more descriptive asunauthorizedUserto clearly indicate that this user is intended to test unauthorized access.Apply this renaming:
-notAuthorized := interchaintest.GetAndFundTestUsers(t, ctx, "wallet", math.NewInt(100000), wrapper.chain)[0] +unauthorizedUser := interchaintest.GetAndFundTestUsers(t, ctx, "wallet", math.NewInt(100000), wrapper.chain)[0]And update usages accordingly.
86-92: Improve error assertion for unauthorized accessCurrently, the test checks that the error message contains "signer is not authority". For more robust testing, consider checking the specific error code or using a constant if available.
Example:
require.ErrorContains(t, err, "signer is not authority") +// Alternatively, if there's an error variable or code: +// require.True(t, errors.Is(err, ErrUnauthorized))
Line range hint
101-106: Confirm successful IBC transfer after client recoveryThe test sends an IBC transfer after recovering the client. To enhance test robustness, consider verifying the balance changes on the recipient's account to ensure the transfer was successful.
Add code to check the recipient's balance:
// After require.NoError(t, err) balances, err := users[1].QueryBalances(ctx, wrapper.chain) require.NoError(t, err) require.True(t, balances.AmountOf("uusdc").Int64() >= 1_000_000, "Recipient balance not updated")client/cli/tx.go (9)
72-72: Fix grammatical error in function commentThe comment should read "schedules" instead of "schedule" to match the function's third-person singular form.
Apply this diff to correct the grammatical error:
-// NewCmdSubmitUpgrade schedule an on-chain software upgrade. +// NewCmdSubmitUpgrade schedules an on-chain software upgrade.
95-119: Handle errors more succinctly usingclientCtx.CodecIn the error-handling blocks, consider reducing repetitive error checks to improve readability. One way to do this is by using helper functions or structuring the code to reduce redundancy.
For instance, you can create a helper function to handle the repetitive pattern:
func getBoolFlag(cmd *cobra.Command, flagName string) (bool, error) { value, err := cmd.Flags().GetBool(flagName) if err != nil { return false, err } return value, nil }Then replace the repetitive error checks with calls to this helper function:
noValidate, err := getBoolFlag(cmd, FlagNoValidate) if err != nil { return err }This approach enhances code maintainability by centralizing flag retrieval logic.
106-110: Check for potential nil pointer dereferenceEnsure that
noChecksumis properly initialized before it's used. Although the current logic seems correct, adding a safety check could prevent potential nil pointer dereferences in future modifications.Consider initializing
noChecksumwith a default value or adding checks to ensure it's not nil.
112-114: Improve error message clarity when parsing plan info failsCurrently, if
plan.ParseInfofails, it returns a generic error. Enhancing the error message can provide more context to the user about what went wrong.Modify the error return to include additional context:
if planInfo, err = plan.ParseInfo(p.Info, plan.ParseOptionEnforceChecksum(!noChecksum)); err != nil { - return err + return fmt.Errorf("failed to parse upgrade plan info: %w", err) }
116-118: Enhance error message when validation failsSimilarly, when
planInfo.ValidateFullfails, attaching more context to the error message can aid in debugging.Modify the error return:
if err = planInfo.ValidateFull(daemonName); err != nil { - return err + return fmt.Errorf("upgrade plan validation failed: %w", err) }
145-147: Fix capitalization in function commentAdjust the comment to start with a capital letter for consistency and readability.
Apply this diff to correct the comment:
-// newSubmitRecoverClientCmd defines the command to recover an IBC light client. +// NewSubmitRecoverClientCmd defines the command to recover an IBC light client.
149-153: Improve command usage documentationThe
UseandShortfields can be enhanced for better clarity and consistency with CLI standards.Consider updating the command definition:
cmd := &cobra.Command{ - Use: "recover-client [subject-client-id] [substitute-client-id] [flags]", + Use: "recover-client <subject-client-id> <substitute-client-id> [flags]", - Short: "recover an IBC client", + Short: "Recover an IBC client",
153-155: Enhance the long description for therecover-clientcommandThe
Longdescription repeats the short description and may lack sufficient detail. Providing more context or examples can help users understand how to use the command effectively.Consider expanding the
Longdescription:Long: `Recover an IBC client by specifying the subject client ID to be recovered and the substitute client ID. This command facilitates the recovery of a broken or frozen client by replacing it with a valid substitute.`,
166-170: Validate error handling forMsgRecoverClientThe error handling for
msg.ValidateBasic()is good. However, consider including the error message directly for clearer logging.Modify the error return to include the error message:
if err = msg.ValidateBasic(); err != nil { - return fmt.Errorf("error validating %T: %w", clienttypes.MsgRecoverClient{}, err) + return fmt.Errorf("error validating MsgRecoverClient: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
Makefile(1 hunks)client/cli/tx.go(2 hunks)client/cli/utils.go(1 hunks)e2e/clients.json(0 hunks)e2e/clients_test.go(5 hunks)e2e/fee_test.go(1 hunks)e2e/go.mod(1 hunks)e2e/ownership_test.go(1 hunks)e2e/params_test.go(1 hunks)e2e/upgrade.json(0 hunks)e2e/upgrade_test.go(1 hunks)e2e/utils.go(2 hunks)go.mod(5 hunks)
💤 Files with no reviewable changes (2)
- e2e/clients.json
- e2e/upgrade.json
✅ Files skipped from review due to trivial changes (1)
- e2e/go.mod
🔇 Additional comments (23)
e2e/params_test.go (2)
21-21: LGTM! Clear documentation of test methodology.
The added comment effectively documents that the test uses the "execute" command for broadcasting messages, which aligns well with the PR's objective of simplifying message broadcasting.
26-26: LGTM! Correctly updated Suite function call signature.
The update properly handles the additional return value from the Suite function, maintaining consistency with other test files.
Let's verify the Suite function signature is consistent across all test files:
✅ Verification successful
All Suite function calls are consistent with 4 return values
The verification confirms that all test files consistently use the Suite function with 4 return values (ctx, logger, reporter, rly). The pattern ctx, _, _, _ := Suite(t, &wrapper, false) or ctx, logger, reporter, rly := Suite(t, &wrapper, true) is uniformly used across all test files:
- e2e/upgrade_test.go
- e2e/params_test.go
- e2e/clients_test.go
- e2e/ownership_test.go
- e2e/fee_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Suite function usage consistency across test files
# Expected: All Suite function calls should have 4 return values
# Search for Suite function calls in test files
rg -g '**/*_test.go' 'Suite\(.*\)' -A 1
Length of output: 685
client/cli/utils.go (2)
1-10: LGTM! Clean package structure and imports.
The imports are well-organized and include only the necessary packages for the functionality.
12-26: Implementation looks good, verify flag constants.
The function is well-implemented with proper error handling and clear return values.
Let's verify the flag constants are properly defined:
✅ Verification successful
Flag constants are properly defined and used correctly
The flag constants FlagUpgradeHeight and FlagUpgradeInfo are correctly defined in client/cli/tx.go with the expected string values "upgrade-height" and "upgrade-info" respectively. These match the flags being used in the parsePlan function, confirming proper implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if FlagUpgradeHeight and FlagUpgradeInfo are defined in the codebase
# Search for flag definitions
rg -A 2 'FlagUpgradeHeight.*=|FlagUpgradeInfo.*='
Length of output: 292
e2e/ownership_test.go (1)
22-22: LGTM! Signature update correctly handles the new logger return value.
The change correctly accommodates the updated Suite function signature by adding another ignored return value.
e2e/fee_test.go (1)
26-26: LGTM! Proper handling of Suite return values.
The update correctly handles all return values from the Suite function, following Go's best practices for explicit value handling.
e2e/upgrade_test.go (3)
13-14: LGTM: Import changes align with new test requirements.
The new imports support the enhanced test functionality for handling unauthorized users and chain interactions.
33-45: LGTM: Good test coverage for authorization checks.
The test properly validates both unauthorized and authorized scenarios, ensuring proper access control for software upgrades. The error assertion is specific and meaningful.
23-23: Verify consistent Suite function usage across test files.
The Suite function now returns 4 values instead of 3. Let's verify this change is consistently applied across all test files.
✅ Verification successful
Let me verify the implementation of the Suite function to confirm the return values.
All Suite function calls are correctly updated to handle 4 return values
Based on the verification results:
- The Suite function in
e2e/utils.gois defined to return 4 values:(ctx context.Context, logger *zap.Logger, execReporter *testreporter.RelayerExecReporter, relayer *rly.CosmosRelayer) - All test files (
upgrade_test.go,params_test.go,ownership_test.go,fee_test.go,clients_test.go) consistently handle the 4 return values, either by using all values or discarding unused ones with underscores.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all Suite function calls in test files
rg -g '**/*_test.go' 'Suite\(.*\)' -A 1
Length of output: 685
Script:
#!/bin/bash
# Search for Suite function definition
ast-grep --pattern 'func Suite($$$) ($$$) {
$$$
}'
Length of output: 5274
Makefile (1)
70-70: Verify the existence of chains.yaml configuration file.
The build command now explicitly references ./e2e/chains.yaml. This is a good change as it makes the build configuration explicit and version-controlled.
Let's verify the existence and content of the chains.yaml file:
✅ Verification successful
Configuration file verified and properly structured
The ./e2e/chains.yaml file exists and contains the correct configuration for the noble-authority-simd chain, including:
- Build configuration using the cosmos dockerfile
- Build directory and target specifications
- Binary path definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and basic structure of chains.yaml
# Test 1: Check if the file exists
if [ -f "./e2e/chains.yaml" ]; then
echo "✅ chains.yaml exists"
# Test 2: Verify it's a valid YAML file with chain configuration
cat "./e2e/chains.yaml"
else
echo "❌ chains.yaml not found at ./e2e/chains.yaml"
fi
Length of output: 238
e2e/utils.go (3)
26-27: LGTM! Well-structured logging enhancement.
The addition of zap logging and the modified Suite function signature improve test observability while following Go testing best practices.
Also applies to: 38-38
40-40: LGTM! Proper logger initialization.
The logger is correctly initialized using zaptest.NewLogger(t), which is the recommended approach for test logging in Go.
38-40: Verify all Suite function callers are updated.
The Suite function signature change affects multiple test files. Let's verify all callers have been updated.
✅ Verification successful
All Suite function callers are properly updated
All callers of the Suite function are correctly handling the return values:
- Most test files use
ctx, _, _, _ := Suite(...)to only capture the context clients_test.gouses all return values:ctx, logger, reporter, rly := Suite(...)
No issues found with the function signature changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all Suite function calls to ensure they handle the new logger return value
rg -A 2 "Suite\(" e2e/
Length of output: 1051
go.mod (3)
16-17: LGTM: Direct dependencies align with PR objectives.
The addition of github.com/cosmos/ibc-go/v8 and github.com/spf13/pflag aligns well with the PR objectives of adding helper commands for software upgrades and client recovery.
Also applies to: 21-22
310-310: Verify compatibility with updated dependencies.
Several core dependencies have been updated to newer versions. Please ensure these updates don't introduce breaking changes:
- golang.org/x/exp
- golang.org/x/mod
- golang.org/x/tools
- google.golang.org/api
- google.golang.org/genproto
#!/bin/bash
# Check for any deprecated API usage in the updated dependencies
echo "Checking for potential deprecated API usage..."
rg -l "deprecated|DEPRECATED"
# Look for any TODO or FIXME comments that might indicate compatibility issues
echo "Checking for compatibility-related TODOs..."
rg -l "TODO.*compatibility|FIXME.*compatibility"Also applies to: 312-312, 320-322
33-35: Verify security implications of new auth-related dependencies.
The addition of Google Cloud auth-related dependencies (cloud.google.com/go/auth and cloud.google.com/go/auth/oauth2adapt) should be reviewed to ensure they are actually required and don't introduce unnecessary security exposure.
e2e/clients_test.go (2)
27-27: Correctly updated function signature to include logger
The inclusion of logger in the Suite function call aligns with the updated function signature and enhances logging capabilities within the test environment.
94-100: Handle potential errors when executing authorized transaction
Ensure that any unexpected errors are properly handled or logged when broadcasting from the authorized account.
Run the following script to check for any error logs related to ExecTx:
client/cli/tx.go (5)
172-174: Ensure wrapping message in MsgExecute is appropriate
Wrapping MsgRecoverClient inside MsgExecute should be confirmed to align with your application's message processing logic.
Consult the application's design to ensure that messages wrapped in MsgExecute are handled correctly. If necessary, update the message construction accordingly.
138-138: Verify the accessibility of getDefaultDaemonName function
The function getDefaultDaemonName() is used here but is not defined within this file. Ensure that it is properly imported or accessible in the current context to avoid undefined function errors.
Run the following script to check if getDefaultDaemonName is defined in the codebase:
#!/bin/bash
# Description: Check if getDefaultDaemonName function is defined and accessible.
# Search for the definition of getDefaultDaemonName. Expect to find its implementation.
rg -t go '^func getDefaultDaemonName\('164-165: Confirm correct usage of authority address
Ensure that types.ModuleAddress.String() provides the correct authority address required for NewMsgRecoverClient. This is crucial for the message to be accepted and processed correctly by the chain.
Run the following script to verify the definition and value of ModuleAddress:
#!/bin/bash
# Description: Verify that ModuleAddress is correctly defined.
# Search for the definition of ModuleAddress in the codebase.
rg -t go 'ModuleAddress'90-90: Verify the existence of parsePlan function
The function parsePlan is called here but is not defined within this file. Ensure that parsePlan is properly imported or defined in the codebase so that this call does not result in an undefined function error.
Run the following script to check if parsePlan is defined elsewhere in the codebase:
✅ Verification successful
Let me try a different search pattern to locate the parsePlan function.
Function parsePlan is properly defined in the codebase
The parsePlan function is defined in client/cli/utils.go and is accessible from tx.go since both files are in the same package (cli). The function was copied from the Cosmos SDK as noted in the comment above its definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the definition of parsePlan function in Go files.
# Search for the definition of parsePlan. Expect to find its implementation.
rg -t go '^func parsePlan\('
Length of output: 1064
Script:
#!/bin/bash
# Search for parsePlan function with broader context
rg -t go "parsePlan" -A 5
# Also search in the same directory as tx.go
fd -e go . client/cli/ --exec cat {}
Length of output: 7506
121-128: Ensure authority address is correctly set
The Authority field is set using types.ModuleAddress.String(). Verify that types.ModuleAddress correctly represents the authority's address in your application context.
Run the following script to confirm that types.ModuleAddress is properly defined:
✅ Verification successful
Let me gather more context about the ModuleAddress definition and its usage.
The authority address is correctly set and verified
The types.ModuleAddress is properly defined in types/keys.go using authtypes.NewModuleAddress("authority"). This is the standard way to create module addresses in Cosmos SDK, and it's being used consistently throughout the codebase for authority-related operations as seen in the message server and CLI commands.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that types.ModuleAddress is defined and represents the correct authority address.
# Search for the definition of ModuleAddress in the codebase.
rg -t go 'ModuleAddress'
Length of output: 1064
Script:
#!/bin/bash
# Check the definition and initialization of ModuleAddress in types/keys.go
cat types/keys.go
# Check the ModuleName constant that's used to create ModuleAddress
rg -t go 'ModuleName.*='
Length of output: 517
| nobleClients, err = rly.GetClients(ctx, reporter, nobleChainID) | ||
| require.NoError(t, err) | ||
| require.Len(t, nobleClients, 3) // ignore 09-localhost client | ||
|
|
||
| newNobleClient := nobleClients[1] | ||
|
|
There was a problem hiding this comment.
Ensure selection of the new client is reliable
The test assumes nobleClients[1] is the new client. Since the ordering of clients might not be consistent, it's safer to select the new client based on a distinguishing attribute.
Refactor to select the new client by excluding the expired client and 09-localhost client:
newNobleClient := nobleClients[1]
+for _, client := range nobleClients {
+ if client.ClientID != nobleClientToExpire.ClientID && client.ClientID != "09-localhost" {
+ newNobleClient = client
+ break
+ }
+}
+require.NotNil(t, newNobleClient, "New client not found")Committable suggestion skipped: line range outside the PR's diff.
| nobleClients, err := rly.GetClients(ctx, reporter, nobleChainID) | ||
| require.NoError(t, err) | ||
| require.Len(t, nobleClients, 2) // ignore 09-localhost client | ||
|
|
||
| nobleClientToExpire := nobleClients[0] | ||
|
|
There was a problem hiding this comment.
Verify correct client selection to prevent test flakiness
The code assumes that nobleClients[0] is the client that will expire. However, the order of clients returned by GetClients may not be guaranteed. To ensure the test consistently selects the correct client, consider identifying the client by its ID or creation time.
Apply this refactor to select the client based on its trusting period:
nobleClients, err := rly.GetClients(ctx, reporter, nobleChainID)
require.NoError(t, err)
-require.Len(t, nobleClients, 2) // ignore 09-localhost client
-var nobleClientToExpire = nobleClients[0]
+var nobleClientToExpire ibc.ClientState
+for _, client := range nobleClients {
+ if client.TrustingPeriod == tp.String() {
+ nobleClientToExpire = client
+ break
+ }
+}
+require.NotNil(t, nobleClientToExpire, "Expired client not found")Committable suggestion skipped: line range outside the PR's diff.
This PR adds helper commands to the client for broadcasting
MsgSoftwareUpgradeandMsgRecoverClientmessages.This was previously pain point due to these messages being wrapped in a government proposal. This implementation instead wraps them in a
MsgExecutemessage.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Tests