[CLOB-930, CORE-29] Extend the e2e test framework to start the cosmos app using cmd. #703
Conversation
|
CLOB-930 Better testing framework for non-determinism, orderbook discrepancies, etc. CORE-29 Gracefully shutdown daemons
Currently the liquidations daemon and prices daemon do not gracefully shutdown when a validator is stopped. This is an issue in the integration tests because when we shutdown the in-process validators, this can cause the daemon goroutines to panic and the test can fail. However, not panicing is a security risk because it's possible that the validator is running while the daemon (liquidations or prices) is not running. The application assumes that the daemons are always running, and this assumption could lead to subaccounts not getting liquidated / prices not being updated. As a longer term solution, we should do the following:
|
WalkthroughThe changes primarily focus on improving the initialization and setup of the application for testing, enhancing type safety, and allowing customization of server and app configurations through interceptors. Additionally, the changes also address fee handling in transaction-related tests and remove unnecessary dependencies. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/go.mod
- protocol/go.sum
Files selected for processing (8)
- protocol/app/ante/gas_test.go (2 hunks)
- protocol/app/app_test.go (1 hunks)
- protocol/app/prepare/full_node_prepare_proposal_test.go (1 hunks)
- protocol/cmd/dydxprotocold/cmd/config.go (2 hunks)
- protocol/cmd/dydxprotocold/cmd/root.go (5 hunks)
- protocol/testutil/app/app.go (5 hunks)
- protocol/x/clob/e2e/order_removal_test.go (2 hunks)
- protocol/x/sending/app_test.go (5 hunks)
Files skipped from review due to trivial changes (5)
- protocol/app/ante/gas_test.go
- protocol/app/app_test.go
- protocol/app/prepare/full_node_prepare_proposal_test.go
- protocol/x/clob/e2e/order_removal_test.go
- protocol/x/sending/app_test.go
Additional comments: 14
protocol/cmd/dydxprotocold/cmd/config.go (2)
34-34: The return type of the
initAppConfigfunction has been changed frominterface{}to*DydxAppConfig. This is a good change as it improves type safety and makes the code easier to understand. However, please ensure that all calls to this function throughout the codebase have been updated to handle the new return type.71-71: The
appConfigis now returned as a pointer. This is a good practice as it can help to reduce memory usage and improve performance by avoiding unnecessary copying of the struct. However, please ensure that the returned pointer is always checked fornilbefore being dereferenced elsewhere in the code to avoid potential runtime panics.protocol/cmd/dydxprotocold/cmd/root.go (6)
53-68: The
NewRootCmdfunction has been updated to callNewRootCmdWithInterceptorswith default interceptors that do not modify the input. This change allows for customization of the server context, app configuration, and app instance in tests or other scenarios where customization is needed. However, it's important to ensure that these interceptors are used responsibly to avoid unintended side effects.110-110: The
appConfigInterceptorfunction is now used to potentially modify the app configuration. This change allows for customization of the app configuration in tests or other scenarios where customization is needed. However, it's important to ensure that this interceptor is used responsibly to avoid unintended side effects.122-122: The
serverCtxInterceptorfunction is now used to potentially modify the server context. This change allows for customization of the server context in tests or other scenarios where customization is needed. However, it's important to ensure that this interceptor is used responsibly to avoid unintended side effects.129-140: The
initRootCmdfunction now accepts anappInterceptorargument. This change allows for customization of the app instance in tests or other scenarios where customization is needed. However, it's important to ensure that this interceptor is used responsibly to avoid unintended side effects.163-165: The
appInterceptorfunction is now used to potentially modify the app instance. This change allows for customization of the app instance in tests or other scenarios where customization is needed. However, it's important to ensure that this interceptor is used responsibly to avoid unintended side effects.256-256: The return type of the
newAppmethod has been changed fromservertypes.Applicationto*dydxapp.App. This change improves type safety by returning a more specific type. However, it's important to ensure that all calls to this method throughout the codebase have been updated to handle the new return type.protocol/testutil/app/app.go (6)
5-19: The import of
tmcfg "github.com/cometbft/cometbft/config"andtmcli "github.com/cometbft/cometbft/libs/cli"are new and seem to be used for setting up the root command and its execution context. Ensure that these packages are compatible with the rest of the codebase and that they are maintained and secure.63-85: The addition of
alicePrivValidatorKeyJsonandaliceNodeKeyJsonconstants is new. These constants seem to be used for setting up the validator's home directory. Ensure that these keys are not sensitive information and are safe to be hardcoded in the codebase.295-300: The
baseAppOptionsfield has been removed from theTestAppBuilderstruct. Ensure that this field is not used elsewhere in the codebase, as its removal could cause issues.309-316: The
WithAppOptionsfunction no longer acceptsbaseAppOptionsas an argument. Ensure that this change does not affect the function calls elsewhere in the codebase.372-508: The initialization of the
TestApphas been significantly changed. The new code creates a temporary directory for the validator, writes the necessary files into it, and starts the Cosmos app using a root command. This is a major change and should be thoroughly tested to ensure it works as expected and does not introduce any new issues.510-513: The
DisableUpdateMonitoringForTestingfunction is now called only ifusesDefaultAppConfigis true. Ensure that this change does not affect the behavior of the tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/go.mod
- protocol/go.sum
Files selected for processing (1)
- protocol/testutil/app/app.go (5 hunks)
Additional comments: 4
protocol/testutil/app/app.go (4)
63-85: The addition of
alicePrivValidatorKeyJsonandaliceNodeKeyJsonconstants is a good practice for maintaining consistency and readability. However, it's important to ensure that these keys are only used for testing and not in production. If these keys are compromised, it could lead to security vulnerabilities.295-300: The removal of
baseAppOptionsfrom theTestAppBuilderstruct and its usage in the codebase should be verified. If there are any dependencies onbaseAppOptionsin the existing code, they should be updated or removed accordingly.309-316: The
WithAppOptionsfunction has been updated to remove thebaseAppOptionsparameter. This change should be verified across the codebase to ensure that all calls to this function have been updated to match the new signature.372-511: The changes in this hunk are significant and involve the initialization of the test application. The application is now started using the command line, which allows for flag-based testing and daemon setup on app start-up. This is a good practice for testing as it closely mimics the actual application start-up process. However, it's important to ensure that all dependencies and configurations are correctly set up for this new initialization process. The use of a temporary directory for validators and the launching of the application in a separate goroutine are good practices for improving testing efficiency. However, the use of hardcoded flags and the disabling of certain daemons should be reviewed and possibly made configurable to allow for more flexible testing.
- "start", - // Do not start tendermint. - "--grpc-only", - "true", - "--home", - validatorHomeDir, - // TODO(CORE-29): Allow the daemons to be launched and cleaned-up successfully by default. - "--price-daemon-enabled", - "false", - "--bridge-daemon-enabled", - "false", - "--liquidation-daemon-enabled", - "false", + "start", + "--grpc-only", + tApp.builder.appOptions["grpc-only"], + "--home", + validatorHomeDir, + "--price-daemon-enabled", + tApp.builder.appOptions["price-daemon-enabled"], + "--bridge-daemon-enabled", + tApp.builder.appOptions["bridge-daemon-enabled"], + "--liquidation-daemon-enabled", + tApp.builder.appOptions["liquidation-daemon-enabled"],Committable suggestion (Beta)
// Specify the start-up flags. rootCmd.SetArgs([]string{ "start", "--grpc-only", tApp.builder.appOptions["grpc-only"], "--home", validatorHomeDir, "--price-daemon-enabled", tApp.builder.appOptions["price-daemon-enabled"], "--bridge-daemon-enabled", tApp.builder.appOptions["bridge-daemon-enabled"], "--liquidation-daemon-enabled", tApp.builder.appOptions["liquidation-daemon-enabled"], })
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/go.mod
- protocol/go.sum
Files selected for processing (8)
- protocol/app/ante/gas_test.go (2 hunks)
- protocol/app/app_test.go (1 hunks)
- protocol/app/prepare/full_node_prepare_proposal_test.go (1 hunks)
- protocol/cmd/dydxprotocold/cmd/config.go (2 hunks)
- protocol/cmd/dydxprotocold/cmd/root.go (5 hunks)
- protocol/testutil/app/app.go (6 hunks)
- protocol/x/clob/e2e/order_removal_test.go (2 hunks)
- protocol/x/sending/app_test.go (5 hunks)
Files skipped from review due to trivial changes (5)
- protocol/app/ante/gas_test.go
- protocol/app/app_test.go
- protocol/app/prepare/full_node_prepare_proposal_test.go
- protocol/x/clob/e2e/order_removal_test.go
- protocol/x/sending/app_test.go
Additional comments: 15
protocol/cmd/dydxprotocold/cmd/config.go (2)
34-34: The function signature of
initAppConfighas been changed to return a pointer toDydxAppConfiginstead of aninterface{}. This change improves type safety and should help prevent runtime type assertion errors. However, please ensure that all calls to this function throughout the codebase have been updated to handle the new return type.71-71: The
appConfigis now returned as a pointer. This is a good practice as it can help to reduce memory usage by avoiding copying the entire struct when the function returns. However, please ensure that the returned pointer is not being dereferenced without a nil check in the calling code.protocol/testutil/app/app.go (6)
5-19: The new imports introduced in this hunk are necessary for the changes made in the rest of the file. They are all standard libraries or well-known packages, so there should be no issues with them.
63-85: The addition of
alicePrivValidatorKeyJsonandaliceNodeKeyJsonconstants is a good practice for maintaining hardcoded values. These constants are used later in thelaunchValidatorfunction to create the necessary configuration files for the validator.295-300: The
baseAppOptionsfield has been removed from theTestAppBuilderstruct. This change simplifies the struct and removes an unnecessary dependency onbaseapp.BaseApp. However, ensure that this does not affect the functionality of the test application.309-316: The
WithAppOptionsfunction has been updated to remove thebaseAppOptionsparameter. This change simplifies the function and removes an unnecessary dependency onbaseapp.BaseApp. However, ensure that this does not affect the functionality of the test application.372-405: The
TestAppinitialization process has been significantly modified. ThelaunchValidatorfunction is now used to start the application, which allows for more control over the application's configuration and lifecycle. This change improves the flexibility and robustness of the test framework.663-791: The
launchValidatorfunction is a new addition that starts a validator using thestartcommand with the specified genesis doc and application options. This function provides a more controlled way to launch a validator for testing purposes. It also includes proper cleanup procedures to ensure that resources are freed after the test.protocol/cmd/dydxprotocold/cmd/root.go (7)
53-68: The
NewRootCmdfunction has been updated to useNewRootCmdWithInterceptorsfunction. This change allows for server and app configuration customization through interceptors. The interceptors are functions that are passed as arguments and are called at specific points in the initialization process. This is a good practice as it allows for more flexible and customizable code. However, ensure that the interceptors are used correctly and safely throughout the codebase.70-75: The
NewRootCmdWithInterceptorsfunction has been added. This function takes in interceptors for the server context, app configuration, and app itself. These interceptors allow for customization of the server context, app configuration, and app during the initialization process. This is a good practice as it allows for more flexible and customizable code. However, ensure that the interceptors are used correctly and safely throughout the codebase.110-110: The
appConfigInterceptorfunction is now being used to get the app configuration. This allows for customization of the app configuration during the initialization process. Ensure that the interceptor is used correctly and safely throughout the codebase.122-122: The
serverCtxInterceptorfunction is now being used to customize the server context. This allows for customization of the server context during the initialization process. Ensure that the interceptor is used correctly and safely throughout the codebase.129-140: The
initRootCmdfunction now takes in anappInterceptorfunction as an argument. This allows for customization of the app during the initialization process. Ensure that the interceptor is used correctly and safely throughout the codebase.163-165: The
appInterceptorfunction is now being used to create the app. This allows for customization of the app during the initialization process. Ensure that the interceptor is used correctly and safely throughout the codebase.256-256: The return type of the
newAppmethod has been changed fromservertypes.Applicationto*dydxapp.App. This improves type safety by returning a more specific type. Ensure that all calls to this method throughout the codebase have been updated to handle the new return type.
| serverCtxPtr.Logger = logger.(log.Logger) | ||
| } | ||
| }, | ||
| // Override the addresses to use domain sockets to avoid port conflicts. |
| } | ||
|
|
||
| shutdownFn = func() error { | ||
| cancelFn() |
There was a problem hiding this comment.
Why are we maybe calling cancelFn twice? It's definitely called on line 779, right?
There was a problem hiding this comment.
Previous comment didn't make sense.
The first invocation is to cancelFn is in the case where creating the Cosmos app errored and I want to cancel the context so that any resources that Cosmos created that uses it are cleaned up.
The second invocation is inside a lambda that is used by the caller of this method to shutdown the Cosmos app and represents the case where we created the Cosmos app successfully.
There was a problem hiding this comment.
ICIC, go case statements do not fall through.
… app using cmd. This works towards enabling and passing in flags for flag based testing. This also works towards enabling daemons on e2e app start-up once the daemons can be cleaned up. This relies on dydxprotocol/cosmos-sdk#29
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- protocol/go.mod
- protocol/go.sum
Files selected for processing (8)
- protocol/app/ante/gas_test.go (2} hunks)
- protocol/app/app_test.go (1} hunks)
- protocol/app/prepare/full_node_prepare_proposal_test.go (1} hunks)
- protocol/cmd/dydxprotocold/cmd/config.go (2} hunks)
- protocol/cmd/dydxprotocold/cmd/root.go (5} hunks)
- protocol/testutil/app/app.go (6} hunks)
- protocol/x/clob/e2e/order_removal_test.go (2} hunks)
- protocol/x/sending/app_test.go (5} hunks)
Files skipped from review due to trivial changes (5)
- protocol/app/ante/gas_test.go
- protocol/app/app_test.go
- protocol/app/prepare/full_node_prepare_proposal_test.go
- protocol/x/clob/e2e/order_removal_test.go
- protocol/x/sending/app_test.go
Additional comments: 14
protocol/cmd/dydxprotocold/cmd/config.go (2)
34-34: The function signature of
initAppConfighas been updated to return a pointer to aDydxAppConfigstruct instead of aninterface{}. This change improves type safety and allows for better compile-time checks. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.71-71: The
initAppConfigfunction now returns a pointer to theappConfigstruct. This is a good practice as it avoids copying the struct and can improve performance, especially if the struct is large. However, be aware that any modifications to the returned struct will affect the originalappConfigdue to the nature of pointers.protocol/cmd/dydxprotocold/cmd/root.go (6)
53-68: The
NewRootCmdfunction has been updated to useNewRootCmdWithInterceptors. This change allows for the customization of server and app configurations through the use of interceptors. This is a good practice as it provides flexibility and makes the code more adaptable to changes in the future. However, ensure that the interceptors are used correctly throughout the codebase to avoid potential issues.110-110: The
appConfigInterceptorfunction is now used to initialize the app configuration. This change improves the flexibility of the app configuration process, but it's important to ensure that the interceptor function is implemented correctly and returns a valid app configuration.122-122: The
serverCtxInterceptorfunction is now used to modify the server context. This change improves the flexibility of the server context setup process, but it's important to ensure that the interceptor function is implemented correctly and modifies the server context as intended.129-140: The
initRootCmdfunction now takes an additionalappInterceptorargument. This change allows for the customization of the app during the initialization of the root command. This is a good practice as it provides flexibility and makes the code more adaptable to changes in the future. However, ensure that the interceptor is used correctly throughout the codebase to avoid potential issues.163-165: The
server.AddCommandsfunction now uses theappInterceptorfunction to create a new app. This change improves the flexibility of the app creation process, but it's important to ensure that the interceptor function is implemented correctly and returns a valid app.256-256: The return type of the
newAppfunction has been changed fromservertypes.Applicationto*dydxapp.App. This change improves type safety and makes the code more readable. However, ensure that all calls to this function throughout the codebase have been updated to match the new return type.protocol/testutil/app/app.go (6)
5-22: The new hunk introduces several new imports that are used in the updated code. These imports are necessary for the new functionality being added, such as starting the Cosmos app using the command line and setting up the necessary environment for testing.
63-85: The new hunk introduces two constants
alicePrivValidatorKeyJsonandaliceNodeKeyJsonwhich are used to set up the validator's home directory in thelaunchValidatorfunction. This is a part of the new functionality to start the Cosmos app using the command line for testing.300-305: The
baseAppOptionsfield has been removed from theTestAppBuilderstruct. This change is consistent with the removal of thebaseAppOptionsparameter from theWithAppOptionsfunction. ThebaseAppOptionswere previously used to customize theBaseAppinstance, but it seems that this customization is no longer necessary or has been moved elsewhere.314-321: The
WithAppOptionsfunction has been updated to remove thebaseAppOptionsparameter. This change is consistent with the removal of thebaseAppOptionsfield from theTestAppBuilderstruct. ThebaseAppOptionswere previously used to customize theBaseAppinstance, but it seems that this customization is no longer necessary or has been moved elsewhere.377-410: The new hunk introduces a significant change in the way the test application is initialized. Instead of using the
DefaultTestAppfunction to create the application, thelaunchValidatorfunction is used. This function starts the Cosmos app using the command line, which allows for more flexibility in testing. ThelaunchValidatorfunction also sets up the necessary environment for testing, including creating a temporary directory for the validator's home directory and setting up the necessary configuration files.672-802: The new hunk introduces the
launchValidatorfunction, which starts the Cosmos app using the command line for testing. This function creates a temporary directory for the validator's home directory, sets up the necessary configuration files, and starts the Cosmos app using theNewRootCmdWithInterceptorsfunction. ThelaunchValidatorfunction also returns ashutdownFnfunction that can be used to stop the execution of the app. This function significantly enhances the flexibility and robustness of the testing environment.
Changelist
Extend the e2e test framework to start the cosmos app using cmd.
This works towards enabling and passing in flags for flag based testing. This also works towards enabling daemons on e2e app start-up once the daemons can be cleaned up. This relies on dydxprotocol/cosmos-sdk#29
Test Plan
Existing tests updated and pass.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.