[CORE-794] enable authz module to allow granting privileges #960
Conversation
WalkthroughThe overall change involves integrating the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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 selected for processing (3)
- protocol/app/app.go (9 hunks)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/upgrades/v4.0.0/constants.go (2 hunks)
Additional comments: 13
protocol/app/upgrades/v4.0.0/constants.go (2)
6-6: The import of
authzkeeperis correctly placed alphabetically among other imports, which is good for maintainability and readability.28-29: The addition of
authzkeeper.StoreKeyto theAddedslice withinStoreUpgradesis consistent with the PR's objective to enable the authz module to grant privileges. Ensure that this addition is accompanied by the necessary migration scripts or procedures if this change affects existing deployments.protocol/app/basic_manager/basic_manager.go (2)
9-9: The import of
authzmoduleis correctly placed alphabetically among other imports, which is good for maintainability and readability.75-75: The registration of
authzmodule.AppModuleBasic{}is consistent with the PR's objective to integrate the authz module into the application's core functionality. It is important to ensure that all necessary dependencies and configurations are in place for the authz module to function correctly within the application's architecture.Verification successful
The verification process has confirmed that the
authzmoduleis correctly integrated into the application's core functionality. The module is properly initialized with its dependencies, such asAuthzKeeper,AccountKeeper, andBankKeeper, and the necessary configurations are in place for it to function correctly within the application's architecture.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if all necessary dependencies and configurations are in place for the authz module rg --type go "authzmodule.AppModuleBasic" -C 10Length of output: 1591
Script:
#!/bin/bash # Search for the initialization of the `authzmodule` in the main application file. rg --type go "NewAppModule\(|NewBasicManager\(" -C 10 # Check for the declaration of any storage keys or other dependencies required by the `authzmodule`. rg --type go "authzmodule" -C 10Length of output: 58272
protocol/app/app.go (9)
55-57: The addition of the
authzrelated packages is consistent with the PR's objective to integrate theauthzmodule into the application. Ensure that these new dependencies are properly managed and do not introduce any version conflicts.240-240: The
AuthzKeeperhas been added to theAppstruct. This change aligns with the PR's goal of enabling theauthzmodule to grant privileges. It's important to ensure that theAuthzKeeperis properly initialized and used throughout the application without causing any side effects.353-366: The
authzkeeper.StoreKeyhas been added to the list of store keys. This is a critical change as it involves state management and storage. It's essential to verify that the addition of this store key is handled correctly during state migrations and does not affect the existing data integrity.447-452: The initialization of
AuthzKeeperwithin theNewfunction is crucial. It's important to ensure that theAuthzKeeperis initialized with the correct parameters and that theNewKVStoreServiceis properly configured to interact with the underlying storage.1012-1012: The
authzmoduleis now registered as anAppModulewithin the application's module manager. This registration is a key part of integrating theauthzmodule into the application's lifecycle. It's important to verify that theauthzmoduleis correctly configured and that its interactions with other modules are properly managed.Verification successful
Based on the information gathered:
- The
AuthzKeeperis declared and initialized correctly within theAppstruct.- The
authzmodule.NewAppModuleis called with parameters that match the expected pattern for module initialization in the Cosmos SDK.- There is no evidence of conflicts with other modules in the provided context.
The review comment is consistent with the codebase, and the
authzmoduleappears to be correctly configured and integrated into the application's lifecycle.* 1074-1074: The `authz` module is set to execute at the beginning of the block. This ordering is important for the correct processing of transactions and state changes. It's necessary to verify that the execution order does not introduce any unintended side effects, especially with regard to transaction processing and state transitions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the authzmodule is correctly configured and does not conflict with other modules. rg --type go 'authzmodule.NewAppModule' -C 5Length of output: 1097
Script:
#!/bin/bash # Verify the parameters of `authzmodule.NewAppModule` by checking its definition. ast-grep --lang go --pattern 'func NewAppModule($_, $_, $_, $_, $_) $_ { $$$ }' | rg 'authzmodule' # Check for any potential conflicts with other modules by looking at the sequence of module registrations. rg --type go 'NewAppModule' protocol/app/app.goLength of output: 2332
Script:
#!/bin/bash # Attempt to find the file where `authzmodule.NewAppModule` is defined. fd 'authzmodule' --type f --exec rg 'func NewAppModule' # Check for any potential conflicts with other modules by ensuring that the `AuthzKeeper` is not being used in a conflicting way. rg 'AuthzKeeper' protocol/app/app.go -C 5Length of output: 1645
- 1143-1143: The
authzmodule is mentioned as a no-op during the end blocker. This implies that it does not perform any operations at the end of the block. It's important to confirm that this is the intended behavior and that it is consistent with the module's design and requirements.Verification successful
Based on the information gathered from the executed scripts, there is no evidence of an
EndBlockerfunction within theauthzmodule, nor is there any documentation or comments indicating that theauthzmodule should perform actions during the end blocker phase. This supports the review comment that theauthzmodule is intended to be a no-op during the end blocker, and it appears to be consistent with the module's design and requirements.* 1185-1185: The `authz` module is included in the unspecified section of the code, likely related to module execution order or migration. It's important to verify that its placement here is correct and does not interfere with the application's upgrade path or module execution sequence.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm that the authz module is intended to be a no-op during the end blocker and that this is consistent with its design. rg --type go 'authz.ModuleName' -C 10 | grep 'EndBlocker'Length of output: 55
Script:
#!/bin/bash # Search for the definition of the `EndBlocker` function within the `authz` module. ast-grep --lang go --pattern 'func (m *$_) EndBlocker($_, $_) $_ { $$$ }' | grep 'authz'Length of output: 88
Script:
#!/bin/bash # Broaden the search pattern to find any `EndBlocker` function within the `authz` module. ast-grep --lang go --pattern 'func (m *$_) EndBlocker($$$) $$$ { $$$ }' | grep 'authz'Length of output: 86
Script:
#!/bin/bash # Search for any comments or documentation within the codebase that might indicate the intended behavior of the `authz` module during the end blocker phase. rg --type go -i 'authz' | grep -i 'end blocker'Length of output: 47
Verification successful
The placement of the
authzmodule within the application's module manager settings inapp.goappears to be intentional and correctly sequenced based on the surrounding code comments and the context of its operations. Theauthzmodule is set to perform early in the block processing to handle tasks like deleting expired grants and is noted to have a no-op in the end blocker section. There is no evidence from the provided context that its placement interferes with the application's upgrade path or module execution sequence.* 1223-1223: The `authz` module is included in a section that seems to be related to module migration, with a comment indicating that "Auth must be migrated after staking." This is a critical ordering that must be verified to ensure that the migration process respects the dependencies between modules.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct placement of the authz module in the application's upgrade path or module execution sequence. rg --type go 'authz.ModuleName' -C 10Length of output: 4661
Verification successful
The review comment is concerned with the order of migration, specifically that the
authzmodule should be migrated after thestakingmodule. The provided context from the script output shows that in theSetOrderMigrationssection, which outlines the migration order,authz.ModuleNameis indeed listed afterstakingtypes.ModuleName, which aligns with the comment that "Auth must be migrated after staking." This confirms that the migration order respects the dependencies between modules as required.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the migration order, especially that the auth module is indeed migrated after the staking module. rg --type go 'authz.ModuleName' -C 10 | grep 'ModuleName'Length of output: 2789
Script:
#!/bin/bash # Let's get more context around the occurrences of 'authz.ModuleName' to understand the migration process better. # We will display 20 lines of context around each occurrence to see the structure and comments that might indicate the migration order. rg --type go 'authz.ModuleName' -C 20 protocol/app/app.goLength of output: 5049
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/app/testdata/default_genesis_state.json
Files selected for processing (8)
- protocol/app/app_test.go (2 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/nested_msgs.go (1 hunks)
- protocol/app/msgs/nested_msgs_test.go (1 hunks)
- protocol/app/msgs/normal_msgs.go (2 hunks)
- protocol/app/msgs/normal_msgs_test.go (1 hunks)
- protocol/app/msgs/unregistered_msgs.go (1 hunks)
- protocol/app/msgs/unregistered_msgs_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/app/msgs/unregistered_msgs_test.go
Additional comments: 10
protocol/app/msgs/unregistered_msgs.go (1)
- 7-8: The removal of
"/cosmos.authz.v1.MsgExec"and"/cosmos.authz.v1.MsgExecResponse"from theUnregisteredMsgsmap indicates that these message types are now expected to be registered with the app. Ensure that the corresponding registration logic is implemented elsewhere in the codebase to handle these message types correctly.protocol/app/msgs/nested_msgs.go (2)
5-5: The import of the
authzpackage is necessary for the new entries in theNestedMsgSamplesmap. This change is consistent with the addition ofauthzmessage types.13-14: The addition of
"/cosmos.authz.v1beta1.MsgExec"with a non-nil value and"/cosmos.authz.v1beta1.MsgExecResponse"with a nil value to theNestedMsgSamplesmap suggests thatMsgExeccan include other messages, whileMsgExecResponsecannot. Confirm that this aligns with the intended design and usage of these message types.Verification successful
The review comment is consistent with the codebase. The
MsgExecmessage type is intended to nest other messages, as indicated by its inclusion inNestedMsgSampleswith a non-nil value. Conversely,MsgExecResponseis not intended to nest messages, as it is mapped tonilin the same context. This aligns with the intended design and usage of these message types.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that MsgExec is intended to nest other messages and MsgExecResponse is not. grep -R "MsgExec" . --include=*.go grep -R "MsgExecResponse" . --include=*.goLength of output: 1948
Script:
#!/bin/bash # Search for the definition of MsgExec and MsgExecResponse ast-grep --lang go --pattern $'type MsgExec struct { $$$ }' ast-grep --lang go --pattern $'type MsgExecResponse struct { $$$ }' # Search for comments or documentation related to MsgExec and MsgExecResponse rg "MsgExec" --include=*.go -C 3 rg "MsgExecResponse" --include=*.go -C 3Length of output: 1091
Script:
#!/bin/bash # Corrected commands to search for comments or documentation related to MsgExec and MsgExecResponse rg "MsgExec" --glob "*.go" -C 3 rg "MsgExecResponse" --glob "*.go" -C 3Length of output: 7504
protocol/app/msgs/nested_msgs_test.go (1)
- 15-16: The addition of test cases for
"/cosmos.authz.v1beta1.MsgExec"and"/cosmos.authz.v1beta1.MsgExecResponse"inTestNestedMsgs_Keyis appropriate to ensure that the new message types are included in theNestedMsgSamplesmap. This is a good practice for maintaining test coverage.protocol/app/msgs/normal_msgs_test.go (1)
- 20-24: The addition of new message types related to authorization in the
TestNormalMsgs_Keytest function is consistent with the changes made to theNormalMsgsmap. This ensures that the test coverage is updated to reflect the new message types.protocol/app/app_test.go (2)
15-15: The import of
authzmoduleis necessary for the inclusion of theauthzmodule as a basic app module. This change is consistent with the overall goal of integrating theauthzmodule into the application.205-205: The addition of
authzmodule.AppModuleBasic{}to thedefaultAppModuleBasicsin theTestModuleBasicsfunction is required to ensure that theauthzmodule is recognized as part of the basic modules in the application. This is a necessary change for the integration of theauthzmodule.protocol/app/msgs/normal_msgs.go (2)
7-7: The import of the
authzpackage is necessary for the new entries in theNormalMsgsmap. This change is consistent with the addition ofauthzmessage types.32-36: The addition of
"/cosmos.authz.v1beta1.GenericAuthorization","/cosmos.authz.v1beta1.MsgGrant", and"/cosmos.authz.v1beta1.MsgRevoke"to theNormalMsgsmap with non-nil values forMsgGrantandMsgRevokeindicates that these messages can now be submitted by external users. Ensure that the corresponding message handling and validation logic are in place to process these new message types correctly.protocol/app/msgs/all_msgs.go (1)
- 17-24: The addition of new
authzmessage types such asGenericAuthorization,MsgExec,MsgExecResponse,MsgGrant,MsgGrantResponse,MsgRevoke, andMsgRevokeResponseto theAllTypeMessagesmap is consistent with the integration of theauthzmodule. This ensures that all message types used in the app are accounted for in theInterfaceRegistry.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/scripts/genesis/sample_pregenesis.json
Files selected for processing (1)
- protocol/app/simulation_test.go (3 hunks)
Additional comments: 2
protocol/app/simulation_test.go (2)
37-37: The import of the
authzmodule has been added correctly, following the Go convention of grouping third-party imports together.110-110: The
authzmodule has been included in thegenesisModuleOrderslice. This change is consistent with the PR's objective to integrate theauthzmodule into the application's core functionality. Ensure that the order of modules ingenesisModuleOrderdoes not affect the initialization and dependencies between modules.Verification successful
Given the extensive output and the specific focus required for the task, I will now search for the
SetOrderInitGenesismethod calls within the provided output to understand the initialization order and check for the presence and correct placement of theauthzmodule. This will help verify if the inclusion of theauthzmodule in the initialization order does not disrupt the dependencies between modules.
The output from the script shows the order in which modules are initialized during the genesis process in the
app.gofile. Theauthzmodule is included at the end of theSetOrderInitGenesismethod call, which suggests that it is the last module to be initialized during genesis.This placement at the end does not immediately suggest any conflicts or dependencies that would be disrupted by the
authzmodule's inclusion. However, without explicit knowledge of the dependencies between modules, we cannot be certain that this order does not introduce issues. Typically, the order of module initialization is carefully considered by developers to ensure that dependencies are correctly handled.Given the information available, it appears that the
authzmodule has been added to the initialization order without any obvious conflicts. However, without a deeper understanding of the inter-module dependencies, which would require a more thorough analysis of the entire codebase, a definitive conclusion cannot be reached.Based on the provided context and the output from the script, the review comment regarding the inclusion of the
authzmodule in thegenesisModuleOrderslice and its potential impact on initialization and dependencies between modules has been verified to the extent possible. Theauthzmodule is included in the initialization order, and there is no immediate indication from theSetOrderInitGenesismethod that this inclusion is problematic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the inclusion of the authz module in the genesisModuleOrder slice does not disrupt the initialization order. # This script checks for any explicit initialization dependencies in the module manager setup code. rg --type go "module manager setup" protocol/app/Length of output: 51
Script:
#!/bin/bash # Search for files that likely contain the module manager configuration. # The file is likely named app.go or contains 'NewApp' or similar initialization functions. rg --type go -l 'NewApp|module manager' protocol/app/Length of output: 229
Script:
#!/bin/bash # View the contents of app.go to analyze the module manager setup and the genesisModuleOrder slice. cat protocol/app/app.goLength of output: 61573
Script:
#!/bin/bash # Extract the relevant section from app.go that defines the module initialization order. sed -n '/SetOrderInitGenesis/,/)/p' protocol/app/app.goLength of output: 1053
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/lib/ante/nested_msg.go (3 hunks)
- protocol/lib/ante/nested_msg_test.go (1 hunks)
- protocol/testutil/msgs/nested_msgs.go (3 hunks)
Additional comments: 7
protocol/lib/ante/nested_msg.go (3)
7-7: The import of the
authzpackage is correct and necessary for the changes in this file.16-17: The addition of
*authz.MsgExecto theIsNestedMsgfunction is consistent with the PR's objective to handle new message types related to authorization.51-52: The
getInnerMsgsfunction has been updated to handle the*authz.MsgExectype, which aligns with the new functionality introduced by the PR.protocol/testutil/msgs/nested_msgs.go (3)
6-6: The import of the
authzpackage is correct and necessary for the changes in this file.36-44: The initialization of new
MsgExecinstances and corresponding transaction bytes variables is consistent with the PR's objective to handle new authorization message types.95-111: The creation of
MsgExecinstances with different message slices is correct and necessary for testing the new functionality introduced by the PR.protocol/lib/ante/nested_msg_test.go (1)
- 88-99: The addition of test cases for invalid
MsgExecscenarios with unsupported, app-injected, and double-nested inner messages is appropriate and ensures that the new functionality is correctly validated.
| govtypes.StoreKey, paramstypes.StoreKey, consensusparamtypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, | ||
| ibcexported.StoreKey, ibctransfertypes.StoreKey, | ||
| authtypes.StoreKey, | ||
| authzkeeper.StoreKey, |
There was a problem hiding this comment.
weird this one is not set up in the same way... no authztypes?
There was a problem hiding this comment.
yeah not sure how authz was bootstrapped initially, but there is no authz/types and the key is under keeper/keys.go 😕
There was a problem hiding this comment.
looked into this quickly. types got removed in this PR. still not sure why though
| runtime.NewKVStoreService(keys[authzkeeper.StoreKey]), | ||
| appCodec, |
There was a problem hiding this comment.
the order of these params is also reversed vs most NewKeeper methods?
There was a problem hiding this comment.
yeah authz always have store key before codec, not sure how it was bootstrapped
| @@ -23,6 +24,9 @@ var Upgrade = upgrades.Upgrade{ | |||
| // Add new ICA stores that are needed by ICA host types as of v8. | |||
| icacontrollertypes.StoreKey, | |||
| icahosttypes.StoreKey, | |||
|
|
|||
| // Add authz module to allow granting arbitrary privileges from one account to another acocunt. | |||
There was a problem hiding this comment.
Is there anything else needed in the upgrade to initialize authz state? Is this inherently left empty with no Authorization?
There was a problem hiding this comment.
yeah I think this should be empty. there is currently no authorizations since it was not enabled previously
Each Authorization consists of (granter, grantee, msgURL) and permissions needs to be granted explicitly by the granter
| gov "github.com/cosmos/cosmos-sdk/x/gov/types/v1" | ||
| ) | ||
|
|
||
| var ( | ||
| // NestedMsgSamples are msgs that have can include other arbitrary messages inside. | ||
| NestedMsgSamples = map[string]sdk.Msg{ | ||
| // authz |
There was a problem hiding this comment.
so this PR also updates nested messages verification (see this commit).
see validateInnerMsg for more details . Essentially we disallow inner messages to be
- unsupported (some ICA controller messages)
- app injected
- another nested message
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/e2e/authz/authz_test.go (1 hunks)
Additional comments: 4
protocol/testing/e2e/authz/authz_test.go (4)
29-35: The function
newAnyis a utility function to create a newAnytype from a given proto message. It panics if there's an error, which is a common pattern in test setup code where failing fast is preferred to simplify debugging.110-158: The test case "Fail (external): Bob tries to transfer from Alice's account without permission." correctly tests the scenario where a transfer should fail due to lack of authorization. The expected failure is asserted using
require.Equal(t, tc.expectedMsgExecDeliverTxCode, authz.ErrNoAuthorizationFound.ABCICode()), which is appropriate.It's important to ensure that the error codes used in the test match the actual implementation in the authz module. If the error codes are changed in the module, these tests should be updated accordingly.
- 229-260: The test case "Fail (internal): Bob tries to create a new clob pair (authority = bob)" checks that Bob cannot create a clob pair even when he specifies himself as the authority. The test asserts the expected failure code from the governance module's error set.
This test case is crucial for validating that the authz module respects the predefined list of authorities and does not allow unauthorized entities to perform restricted actions. The test is well-structured and the assertions are appropriate.
- 265-283: The test case "Fail (app injected): Bob tries to propose operations" is designed to fail during
CheckTxdue to an invalid request. This test ensures that the application's ante handler correctly rejects transactions that are not allowed.It's important to verify that the error code used in the test (
sdkerrors.ErrInvalidRequest.ABCICode()) is consistent with the actual error code returned by the ante handler for this type of error.Verification successful
The error code
sdkerrors.ErrInvalidRequest.ABCICode()used in the test case "Fail (app injected): Bob tries to propose operations" is consistent with the actual error code returned by the ante handler for errors related to "app-injected" messages, as seen in themsg_type.gofile within theprotocol/app/ante/directory. The ante handler checks for conditions where an "app-injected" message must be the only message in a transaction or must only be included inDeliverTx, and it usessdkerrors.ErrInvalidRequestfor these checks.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the error code for invalid requests matches the ante handler's implementation. rg --type go 'ErrInvalidRequest' protocol/app/ante/Length of output: 1763
Script:
#!/bin/bash # Search for the specific ante handler function that processes `MsgProposedOperations` # and verify if it returns `sdkerrors.ErrInvalidRequest` for the scenario in question. rg --type go 'MsgProposedOperations' protocol/app/ante/Length of output: 55
Script:
#!/bin/bash # Search for handling of "app injected" messages or a broader category of errors in the ante handler. rg --type go 'app-injected' protocol/app/ante/Length of output: 1321
| Msgs: []*codectypes.Any{ | ||
| newAny( | ||
| &icacontrollertypes.MsgUpdateParams{}, | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| expectedMsgExecCheckTxSuccess: false, | ||
| expectedMsgExecCheckTxCode: sdkerrors.ErrInvalidRequest.ABCICode(), | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| msgSender := msgsender.NewIndexerMessageSenderInMemoryCollector() | ||
| appOpts := map[string]interface{}{ | ||
| indexer.MsgSenderInstanceForTest: msgSender, | ||
| } | ||
|
|
||
| // Initialize test app | ||
| tApp := testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).WithGenesisDocFn(func() (genesis types.GenesisDoc) { | ||
| genesis = testapp.DefaultGenesis() | ||
| testapp.UpdateGenesisDocWithAppStateForModule( | ||
| &genesis, | ||
| func(genesisState *satypes.GenesisState) { | ||
| genesisState.Subaccounts = tc.subaccounts | ||
| }, | ||
| ) | ||
| return genesis | ||
| }).Build() | ||
| ctx := tApp.InitChain() | ||
|
|
||
| if tc.msgGrant != nil { | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithSdkMsg( | ||
| ctx, | ||
| tApp.App, | ||
| testapp.MustMakeCheckTxOptions{ | ||
| AccAddressForSigning: tc.msgGrant.Granter, // Granter | ||
| Gas: 1000000, | ||
| FeeAmt: constants.TestFeeCoins_5Cents, | ||
| }, | ||
| tc.msgGrant, | ||
| ) { | ||
| resp := tApp.CheckTx(checkTx) | ||
| require.True( | ||
| t, | ||
| resp.IsOK(), | ||
| "Expected CheckTx to succeed. Response: %+v", | ||
| resp, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Give grantee some permissions. | ||
| ctx = tApp.AdvanceToBlock(uint32(ctx.BlockHeight())+1, testapp.AdvanceToBlockOptions{}) | ||
|
|
||
| // Grantee executes a msg. | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithSdkMsg( | ||
| ctx, | ||
| tApp.App, | ||
| testapp.MustMakeCheckTxOptions{ | ||
| AccAddressForSigning: tc.msgExec.Grantee, // Grantee | ||
| Gas: 1000000, | ||
| FeeAmt: constants.TestFeeCoins_5Cents, | ||
| }, | ||
| tc.msgExec, | ||
| ) { | ||
| resp := tApp.CheckTx(checkTx) | ||
| require.Equal( | ||
| t, | ||
| tc.expectedMsgExecCheckTxSuccess, | ||
| resp.IsOK(), | ||
| "Expected CheckTx to succeed. Response: %+v", | ||
| resp, | ||
| ) | ||
| require.Equal(t, tc.expectedMsgExecCheckTxCode, resp.Code) | ||
| } | ||
|
|
||
| if tc.expectedMsgExecCheckTxSuccess { | ||
| ctx = tApp.AdvanceToBlock(uint32(ctx.BlockHeight())+1, testapp.AdvanceToBlockOptions{ | ||
| ValidateFinalizeBlock: func( | ||
| ctx sdk.Context, | ||
| request abcitypes.RequestFinalizeBlock, | ||
| response abcitypes.ResponseFinalizeBlock, | ||
| ) (haltchain bool) { | ||
| // Note the first TX is MsgProposeOperations. | ||
| txResult := response.TxResults[1] | ||
| require.Equal(t, tc.expectedMsgExecDeliverTxSuccess, txResult.IsOK()) | ||
| require.Equal(t, tc.expectedMsgExecDeliverTxCode, txResult.Code) | ||
| return true | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // Verify results. | ||
| if tc.verifyResults != nil { | ||
| tc.verifyResults(ctx, tApp) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The test suite TestAuthz is well-structured with clear test case descriptions and expected outcomes. It uses table-driven tests, which is a best practice in Go for writing clear and maintainable tests. Each test case is self-contained with its setup, execution, and verification steps.
However, there are a few areas that could be improved:
- The use of
require.True(t, resp.IsOK())andrequire.Equal(t, tc.expectedMsgExecCheckTxSuccess, resp.IsOK())could be simplified torequire.NoError(t, resp.GetError())to directly assert that no error occurred. - The test cases that are expected to fail during
CheckTxdue to app-injected errors or unsupported transactions should have comments explaining why these failures are expected and what conditions lead to them. - The test cases that involve advancing the block height (
tApp.AdvanceToBlock) should ensure that this operation is necessary for the test's logic and not just a remnant of copy-pasted code.
| "Success: Alice grants permission to Bob to transfer from her account. Bob transfers from Alice's account.": { | ||
| subaccounts: []satypes.Subaccount{ | ||
| constants.Alice_Num0_100_000USD, | ||
| constants.Bob_Num0_100_000USD, | ||
| }, | ||
|
|
||
| msgGrant: &authz.MsgGrant{ | ||
| Granter: constants.AliceAccAddress.String(), | ||
| Grantee: constants.BobAccAddress.String(), | ||
| Grant: authz.Grant{ | ||
| Authorization: newAny( | ||
| authz.NewGenericAuthorization(sdk.MsgTypeURL(&sendingtypes.MsgCreateTransfer{})), | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| msgExec: &authz.MsgExec{ | ||
| Grantee: constants.BobAccAddress.String(), | ||
| Msgs: []*codectypes.Any{ | ||
| newAny( | ||
| &sendingtypes.MsgCreateTransfer{ | ||
| Transfer: &sendingtypes.Transfer{ | ||
| Sender: constants.Alice_Num0, | ||
| Recipient: constants.Bob_Num0, | ||
| AssetId: 0, | ||
| Amount: 10_000_000_000, // $10,000 | ||
| }, | ||
| }, | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| expectedMsgExecCheckTxSuccess: true, | ||
| expectedMsgExecCheckTxCode: abcitypes.CodeTypeOK, | ||
| expectedMsgExecDeliverTxSuccess: true, | ||
| expectedMsgExecDeliverTxCode: abcitypes.CodeTypeOK, | ||
|
|
||
| verifyResults: func(ctx sdk.Context, tApp *testapp.TestApp) { | ||
| expectedSubaccounts := []satypes.Subaccount{ | ||
| { | ||
| Id: &constants.Alice_Num0, | ||
| AssetPositions: testutil.CreateUsdcAssetPosition( | ||
| big.NewInt(90_000_000_000), | ||
| ), | ||
| }, | ||
| { | ||
| Id: &constants.Bob_Num0, | ||
| AssetPositions: testutil.CreateUsdcAssetPosition( | ||
| big.NewInt(110_000_000_000), | ||
| ), | ||
| }, | ||
| } | ||
| for _, subaccount := range expectedSubaccounts { | ||
| actualSubaccount := tApp.App.SubaccountsKeeper.GetSubaccount(ctx, *subaccount.Id) | ||
| require.Equal(t, subaccount, actualSubaccount) | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The test case "Success: Alice grants permission to Bob to transfer from her account. Bob transfers from Alice's account." is a positive test case that checks the successful granting and execution of permissions. The test setup and assertions are correct, and it properly verifies the state of subaccounts after the transaction.
However, the test could be improved by adding more detailed comments explaining each step of the test case, especially for readers who may not be familiar with the authz module or the specific business logic of the application.
| "Fail (internal): Granting permissions to execute internal messages doesn't do anything": { | ||
| subaccounts: []satypes.Subaccount{ | ||
| constants.Alice_Num0_100_000USD, | ||
| constants.Bob_Num0_100_000USD, | ||
| }, | ||
|
|
||
| msgGrant: &authz.MsgGrant{ | ||
| Granter: constants.AliceAccAddress.String(), | ||
| Grantee: constants.BobAccAddress.String(), | ||
| Grant: authz.Grant{ | ||
| Authorization: newAny( | ||
| authz.NewGenericAuthorization(sdk.MsgTypeURL(&clobtypes.MsgCreateClobPair{})), | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| msgExec: &authz.MsgExec{ | ||
| Grantee: constants.BobAccAddress.String(), | ||
| Msgs: []*codectypes.Any{ | ||
| newAny( | ||
| &clobtypes.MsgCreateClobPair{ | ||
| Authority: lib.GovModuleAddress.String(), | ||
| ClobPair: constants.ClobPair_Btc2, | ||
| }, | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| expectedMsgExecCheckTxSuccess: true, | ||
| expectedMsgExecCheckTxCode: abcitypes.CodeTypeOK, | ||
| expectedMsgExecDeliverTxSuccess: false, | ||
| expectedMsgExecDeliverTxCode: authz.ErrNoAuthorizationFound.ABCICode(), | ||
|
|
||
| verifyResults: func(ctx sdk.Context, tApp *testapp.TestApp) { | ||
| // Verify no clob pairs were created. | ||
| require.Equal(t, 2, len(tApp.App.ClobKeeper.GetAllClobPairs(ctx))) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The test case "Fail (internal): Granting permissions to execute internal messages doesn't do anything" is testing an important negative scenario where granting permissions for internal messages should not have any effect. The test setup and assertions seem correct.
However, the comment "// Verify no clob pairs were created." could be expanded to explain why this is the expected outcome, providing context for the test's purpose.
| "Fail (internal): Bob tries to create a new clob pair (authority = gov)": { | ||
| subaccounts: []satypes.Subaccount{ | ||
| constants.Alice_Num0_100_000USD, | ||
| constants.Bob_Num0_100_000USD, | ||
| }, | ||
|
|
||
| msgGrant: nil, | ||
|
|
||
| msgExec: &authz.MsgExec{ | ||
| Grantee: constants.BobAccAddress.String(), | ||
| Msgs: []*codectypes.Any{ | ||
| newAny( | ||
| &clobtypes.MsgCreateClobPair{ | ||
| // Authority = gov | ||
| Authority: lib.GovModuleAddress.String(), | ||
| ClobPair: constants.ClobPair_Btc2, | ||
| }, | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| expectedMsgExecCheckTxSuccess: true, | ||
| expectedMsgExecCheckTxCode: abcitypes.CodeTypeOK, | ||
| expectedMsgExecDeliverTxSuccess: false, | ||
| expectedMsgExecDeliverTxCode: authz.ErrNoAuthorizationFound.ABCICode(), | ||
|
|
||
| verifyResults: func(ctx sdk.Context, tApp *testapp.TestApp) { | ||
| // Verify no clob pairs were created. | ||
| require.Equal(t, 2, len(tApp.App.ClobKeeper.GetAllClobPairs(ctx))) | ||
| }, |
There was a problem hiding this comment.
The test case "Fail (internal): Bob tries to create a new clob pair (authority = gov)" is another negative test case that checks for proper authorization enforcement. The test asserts that no new clob pairs are created when Bob attempts to create one with the authority of the governance module.
This test case is important for ensuring that the authz module correctly restricts actions based on the authority provided. It would be beneficial to add a comment explaining the significance of the authority check in this context.
| subaccounts: []satypes.Subaccount{ | ||
| constants.Alice_Num0_100_000USD, | ||
| constants.Bob_Num0_100_000USD, | ||
| }, | ||
|
|
||
| msgGrant: nil, | ||
|
|
||
| msgExec: &authz.MsgExec{ | ||
| Grantee: constants.BobAccAddress.String(), | ||
| Msgs: []*codectypes.Any{ | ||
| newAny( | ||
| &authz.MsgExec{}, | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| expectedMsgExecCheckTxSuccess: false, | ||
| expectedMsgExecCheckTxCode: sdkerrors.ErrInvalidRequest.ABCICode(), |
There was a problem hiding this comment.
The test case "Fail (double nested): Bob wraps another nested message" is a negative test case that ensures nested MsgExec messages are rejected. This is an important test for the integrity of the message execution logic, preventing potential recursion or nesting issues.
The test is well-structured, but it would be beneficial to add a comment explaining why nested MsgExec messages are not supported and what potential issues they could cause.
| subaccounts: []satypes.Subaccount{ | ||
| constants.Alice_Num0_100_000USD, | ||
| constants.Bob_Num0_100_000USD, | ||
| }, | ||
|
|
||
| msgGrant: nil, | ||
|
|
||
| msgExec: &authz.MsgExec{ | ||
| Grantee: constants.BobAccAddress.String(), | ||
| Msgs: []*codectypes.Any{ | ||
| newAny( | ||
| &icacontrollertypes.MsgUpdateParams{}, | ||
| ), | ||
| }, | ||
| }, | ||
|
|
||
| expectedMsgExecCheckTxSuccess: false, | ||
| expectedMsgExecCheckTxCode: sdkerrors.ErrInvalidRequest.ABCICode(), |
There was a problem hiding this comment.
The test case "Fail (unsupported): Bob wraps unsupported transactions" checks that transactions not supported by the authz module are rejected. This is a critical test for ensuring that only authorized and supported message types can be executed.
The test correctly asserts the failure due to an invalid request, but it would be helpful to include a comment explaining which transactions are supported and why this particular transaction type is not.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/testing/e2e/authz/authz_test.go (1 hunks)
- protocol/testutil/constants/stateful_orders.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testing/e2e/authz/authz_test.go
Additional comments: 1
protocol/testutil/constants/stateful_orders.go (1)
- 213-235: The new long-term order declarations for Alice and Bob seem to be consistent with the existing pattern of order declarations in the file. The orders are well-structured and include all necessary fields such as
SubaccountId,ClientId,OrderFlags,ClobPairId,Side,Quantums,Subticks, andGoodTilOneof. It's important to ensure that theQuantumsandSubticksvalues are correct and consistent with the application's domain logic and that theGoodTilBlockTimealigns with the intended order duration.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/e2e/authz/authz_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testing/e2e/authz/authz_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/app/testdata/default_genesis_state.jsonis excluded by:!**/*.jsonprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.json
Files selected for processing (7)
- protocol/app/app.go (9 hunks)
- protocol/app/app_test.go (2 hunks)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/simulation_test.go (2 hunks)
- protocol/lib/ante/nested_msg.go (1 hunks)
- protocol/lib/metrics/constants.go (2 hunks)
- protocol/testing/e2e/authz/authz_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/app_test.go
- protocol/app/simulation_test.go
- protocol/lib/ante/nested_msg.go
- protocol/testing/e2e/authz/authz_test.go
Additional comments: 12
protocol/app/basic_manager/basic_manager.go (2)
- 9-9: The import of
authzmoduleis correct and follows the Go convention of aliasing imported packages when necessary to avoid conflicts or to provide clarity.- 73-73: The addition of
authzmodule.AppModuleBasic{}to theModuleBasicsslice is consistent with the pattern used for other modules and is necessary for the integration of theauthzmodule.protocol/lib/metrics/constants.go (2)
- 37-37: The addition of the
Anteconstant is appropriate if it's being used for metrics related to the ante handler in transactions. However, it's not directly related to theauthzmodule changes mentioned in the PR objectives and AI-generated summaries. If this constant is not used, it should be removed to avoid dead code.- 411-412: The addition of
MsgExecandInnerMsgconstants is aligned with the introduction of theauthzmodule and its message types. These constants are likely used for metric labels, which is a common practice for observability.protocol/app/app.go (8)
- 56-58: The import of the
authzmodule and related packages is consistent with the PR objectives and AI-generated summary.- 240-240: The addition of
AuthzKeeperto theAppstruct is necessary for integrating theauthzmodule.- 355-355: The addition of
authzkeeper.StoreKeyto the list ofKVStoreKeysis required for theauthzmodule's storage.- 448-453: The instantiation of
AuthzKeeperusingNewKeeperwith the correct order of parameters is consistent with the Cosmos SDK's expected pattern for keeper instantiation.- 1015-1015: The registration of
authzmoduleas anAppModuleis done correctly, ensuring that theauthzmodule is recognized and managed by the application's module manager.- 1077-1077: The inclusion of
authz.ModuleNamein theBeginBlockerandEndBlockeris correct, as it allows theauthzmodule to perform necessary updates at the beginning and end of each block.- 1188-1188: The inclusion of
authz.ModuleNamein theInitGenesisis correct, as it ensures that theauthzmodule's state is initialized during the genesis process.- 1226-1226: The inclusion of
authz.ModuleNamein the migration order is correct, as it ensures that theauthzmodule's state can be migrated if necessary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/app/upgrades/v4.0.0/constants.go (2 hunks)
Additional comments: 2
protocol/app/upgrades/v4.0.0/constants.go (2)
- 6-6: The import of
authzkeeperfrom the Cosmos SDK is correct and aligns with the PR objectives to integrate theauthzmodule.- 26-27: The addition of
authzkeeper.StoreKeyto theUpgradevariable'sAddedslice is consistent with the PR's aim to enable theauthzmodule. Ensure that the corresponding upgrade logic is implemented to handle the state transition associated with this new store key.
protocol/lib/ante/nested_msg.go
Outdated
| ) | ||
|
|
||
| const DYDX_MSG_PREFIX = "/dydxprotocol" |
There was a problem hiding this comment.
mega nit: might be better to use the AppName
| @@ -85,6 +85,18 @@ func TestValidateNestedMsg(t *testing.T) { | |||
| msg: testmsgs.MsgSubmitProposalWithDoubleNestedInner, | |||
| expectedErr: invalidInnerMsgErr_Nested, | |||
| }, | |||
| "Invalid MsgExec: unsupported inner msg": { | |||
There was a problem hiding this comment.
should we add a testcase for the dydx custom msg types?
| "Invalid MsgExec: double-nested inner msg": { | ||
| msg: &testmsgs.MsgExecWithDoubleNestedInner, | ||
| expectedErr: invalidInnerMsgErr_Nested, | ||
| }, | ||
| "Valid: empty inner msg": { | ||
| msg: testmsgs.MsgSubmitProposalWithEmptyInner, | ||
| expectedErr: nil, |
There was a problem hiding this comment.
nit: should we add test cases for IsDydxMsg method?
There was a problem hiding this comment.
Agree this would be valuable to have 👍
| metrics.MsgExec, | ||
| metrics.GetLabelForStringValue(metrics.InnerMsg, sdk.MsgTypeURL(inner)), | ||
| ) | ||
| if IsDydxMsg(inner) { |
There was a problem hiding this comment.
Does this mean we don't allow MsgExec to call any of the dYdX messages (incl. order placing, subaccount transfer)? I can see why that would make sense to limit security risk.
Is there any context/discussion behind this? If so could you document briefly?
There was a problem hiding this comment.
ah there was a comment discussion in the authz one pager that got resolved. I added a short section in the doc saying that dydx messages will be explicitly disallowed for now and an allowlist can be maintained in the future if there are use cases.
| ) | ||
|
|
||
| const DYDX_MSG_PREFIX = "/dydxprotocol" | ||
|
|
||
| // IsNestedMsg returns true if the given msg is a nested msg. | ||
| func IsNestedMsg(msg sdk.Msg) bool { | ||
| switch msg.(type) { |
There was a problem hiding this comment.
Question: do we need MsgDelayMsg here?
There was a problem hiding this comment.
good callout. I think so since it also wraps other messages.
Let's add this in a separate PR since it's unrelated. I also have less context on delay messages so maybe one of the OTE eng can pick this up?
| Grantee: constants.BobAccAddress.String(), | ||
| Msgs: []*codectypes.Any{ | ||
| newAny( | ||
| &clobtypes.MsgProposedOperations{}, |
There was a problem hiding this comment.
For my understanding, this is becaseu MsgProposedOperations is a dYdX message right?
There was a problem hiding this comment.
it's both a app injected and also dydx message. we check for app injected before dydx messages so it will get rejected before even reaching dydx message check.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (17)
- protocol/app/app.go (13 hunks)
- protocol/app/constants/constants.go (1 hunks)
- protocol/app/datadog.go (2 hunks)
- protocol/app/msgs/internal_msgs.go (3 hunks)
- protocol/app/msgs/normal_msgs.go (4 hunks)
- protocol/cmd/dydxprotocold/cmd/root.go (2 hunks)
- protocol/cmd/dydxprotocold/cmd/root_test.go (4 hunks)
- protocol/cmd/dydxprotocold/main.go (2 hunks)
- protocol/lib/ante/nested_msg.go (1 hunks)
- protocol/lib/ante/nested_msg_test.go (3 hunks)
- protocol/testutil/app/app.go (2 hunks)
- protocol/testutil/msgs/nested_msgs.go (3 hunks)
- protocol/x/clob/client/cli/cancel_order_cli_test.go (2 hunks)
- protocol/x/clob/client/cli/liquidations_cli_test.go (2 hunks)
- protocol/x/clob/client/cli/place_order_cli_test.go (2 hunks)
- protocol/x/prices/client/cli/prices_cli_test.go (2 hunks)
- protocol/x/sending/client/cli/sending_cli_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- protocol/app/constants/constants.go
Files skipped from review as they are similar to previous changes (5)
- protocol/app/app.go
- protocol/app/msgs/normal_msgs.go
- protocol/lib/ante/nested_msg.go
- protocol/lib/ante/nested_msg_test.go
- protocol/testutil/msgs/nested_msgs.go
Additional comments: 25
protocol/cmd/dydxprotocold/main.go (2)
- 9-9: The import of the
constantspackage is correct and aligns with the PR objectives.- 22-22: The usage of
constants.AppDaemonNameinstead ofapp.AppDaemonNameis consistent with the import change and the PR objectives.protocol/cmd/dydxprotocold/cmd/root_test.go (4)
- 8-8: The import of the
constantspackage is correct and aligns with the PR objectives.- 26-26: The usage of
constants.AppDaemonNameinstead ofapp.AppDaemonNameis consistent with the import change and the PR objectives.- 37-37: The usage of
constants.AppDaemonNameinstead ofapp.AppDaemonNameis consistent with the import change and the PR objectives.- 47-47: The usage of
constants.AppDaemonNameinstead ofapp.AppDaemonNameis consistent with the import change and the PR objectives.protocol/app/datadog.go (3)
- 13-13: The import of the
constantspackage is correct and aligns with the PR objectives.- 32-32: The usage of
constants.ServiceNameinstead of a previously used variable is consistent with the import change and the PR objectives.- 34-34: The log message correctly references
constants.ServiceName, which is consistent with the import change and the PR objectives.protocol/x/sending/client/cli/sending_cli_test.go (2)
- 11-11: The import of
appconstantswith an alias is correct and aligns with the PR objectives.- 53-53: The usage of
appconstants.AppNameinstead ofapp.AppNameis consistent with the import change and the PR objectives.protocol/x/prices/client/cli/prices_cli_test.go (2)
- 13-13: The import of
appconstantsis correct and aligns with the PR objectives.- 108-108: The usage of
appconstants.AppNameinstead ofapp.AppNameis consistent with the import change and the PR objectives.protocol/app/msgs/internal_msgs.go (2)
- 39-43: The change to
InternalMsgSamplesGovAuthto uselib.MergeAllMapsMustHaveDistinctKeysis a significant alteration to the logic. Ensure that theMergeAllMapsMustHaveDistinctKeysfunction is thoroughly tested to handle the merging of maps correctly, especially since it requires distinct keys.Verification successful
The verification process has confirmed that the
MergeAllMapsMustHaveDistinctKeysfunction is indeed referenced in multiple test files within the codebase. This indicates that there are tests that likely cover the functionality of this method.* 87-105: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [46-99]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the MergeAllMapsMustHaveDistinctKeys function has corresponding tests. rg --type go "func MergeAllMapsMustHaveDistinctKeys"Length of output: 168
Script:
#!/bin/bash # Search for test files that contain references to MergeAllMapsMustHaveDistinctKeys function rg --type go "MergeAllMapsMustHaveDistinctKeys" --glob "*_test.go"Length of output: 1704
The declaration of
InternalMsgSamplesDefaultwith Cosmos SDK default module messages is correct and aligns with the PR objectives.protocol/x/clob/client/cli/place_order_cli_test.go (2)
- 16-16: The import of
appconstantswith an alias is correct and aligns with the PR objectives.- 85-85: The usage of
appconstants.AppNameinstead ofapp.AppNameis consistent with the import change and the PR objectives.protocol/x/clob/client/cli/liquidations_cli_test.go (2)
- 29-29: The import path for
appconstantshas been updated to reflect the reorganization of constants within the application.- 94-94: The assignment of
cfg.ChainIDhas been changed fromapp.AppNametoappconstants.AppName. This change is consistent with the reorganization of constants and should be verified to ensure thatappconstants.AppNameis correctly defined and that this change does not affect other parts of the application that rely onChainID.protocol/cmd/dydxprotocold/cmd/root.go (2)
- 42-42: The import path for
constantshas been updated to reflect the reorganization of constants within the application.- 113-113: The
Usefield of therootCmdhas been updated fromdydxapp.AppDaemonNametoconstants.AppDaemonName. This change is consistent with the reorganization of constants and should be verified to ensure thatconstants.AppDaemonNameis correctly defined and that this change does not affect other parts of the application that rely onAppDaemonName.Verification successful
The definition of
constants.AppDaemonNamehas been verified asAppDaemonName = AppName + "d", which aligns with the application's naming conventions. Additionally, there are no occurrences ofdydxapp.AppDaemonNamein the codebase, confirming that the update toconstants.AppDaemonNamehas been fully propagated. The review comment is therefore verified as accurate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that constants.AppDaemonName is correctly defined and used consistently across the application. rg --type go 'constants.AppDaemonName'Length of output: 758
Script:
#!/bin/bash # Check the definition of constants.AppDaemonName rg --type go --files-with-matches 'AppDaemonName' | xargs cat # Search for any remaining occurrences of dydxapp.AppDaemonName rg --type go 'dydxapp.AppDaemonName'Length of output: 66563
Script:
#!/bin/bash # Explicitly check for the definition of constants.AppDaemonName in the constants file. rg --type go --files-with-matches 'AppDaemonName' | xargs cat | grep 'AppDaemonName' # Confirm the absence of dydxapp.AppDaemonName in the codebase. rg --type go 'dydxapp.AppDaemonName'Length of output: 635
protocol/x/clob/client/cli/cancel_order_cli_test.go (2)
- 15-17: The import paths for
appconstants,appflags, anddaemonflagshave been updated to reflect the reorganization of constants within the application.- 90-90: The assignment of
s.cfg.ChainIDhas been changed fromapp.AppNametoappconstants.AppName. This change is consistent with the reorganization of constants and should be verified to ensure thatappconstants.AppNameis correctly defined and that this change does not affect other parts of the application that rely onChainID.Verification successful
The definition of
appconstants.AppNameas"dydxprotocol"has been found inprotocol/app/constants/constants.go, and its usage is consistent across the application. There were no occurrences ofapp.AppNamefound, suggesting it has been replaced byappconstants.AppName. The original review comment has been verified based on the information available.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that appconstants.AppName is correctly defined and used consistently across the application. rg --type go 'appconstants.AppName'Length of output: 689
Script:
#!/bin/bash # Check the definition of appconstants.AppName to ensure it's correctly defined. rg --type go -A 2 'AppName =' protocol/app/appconstants.go # Check if app.AppName is still being used elsewhere in the codebase. rg --type go 'app.AppName'Length of output: 156
Script:
#!/bin/bash # Attempt to locate the file where appconstants.AppName is defined without assuming its path. rg --type go -g '*appconstants.go' 'AppName' # Recheck for any occurrences of app.AppName without specifying the file path to ensure it's not used elsewhere. rg --type go 'app.AppName'Length of output: 220
Script:
#!/bin/bash # Perform a more general search for the definition of AppName in any go file. rg --type go 'AppName' # Perform a general search for any occurrence of app.AppName across the entire codebase. rg --type go 'app.AppName'Length of output: 918
protocol/testutil/app/app.go (2)
- 47-47: The import of
appconstantshas been added. This change seems to be in line with the summary provided.- 1264-1264: The usage of
appconstants.AppDaemonNamein thelaunchValidatorInDirfunction is correct and aligns with the import change.
Changelist
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.