OTE-558 Add scaffolding for module #1941
Conversation
defined protos defined keeper defined genesis
WalkthroughThe changes introduce the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
protocol/x/accountplus/types/keys.go (1)
5-7: Update all references to the module nameThe module name
dydxaccountplushas not been consistently updated across the codebase. The following references to the old module nameaccountplusneed to be updated:
Package declarations:
protocol/x/accountplus/module.goprotocol/x/accountplus/genesis.goprotocol/x/accountplus/genesis_test.goImport paths:
protocol/x/accountplus/testutils/testutils_test.goprotocol/x/accountplus/testutils/testutils.goprotocol/x/accountplus/module.goprotocol/x/accountplus/genesis.goprotocol/x/accountplus/genesis_test.goprotocol/x/accountplus/keeper/keeper.goprotocol/x/accountplus/keeper/timestampnonce_test.goprotocol/x/accountplus/keeper/timestampnonce.goprotocol/x/accountplus/keeper/keeper_test.goprotocol/testutil/keeper/accountplus.goprotocol/app/app_test.goprotocol/app/app.goUsage within the code:
protocol/x/accountplus/genesis_test.goprotocol/x/accountplus/keeper/keeper_test.goprotocol/x/accountplus/types/genesis.pb.goprotocol/x/accountplus/types/accountplus.pb.goPlease update these references to ensure consistency with the new module name
dydxaccountplus.Analysis chain
LGTM! But verify the module name usage in the codebase.
The change to
ModuleNameis approved.However, ensure that all references to the module name are updated to
dydxaccountplus.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the module name are updated to `dydxaccountplus`. # Test: Search for the old module name. Expect: No occurrences of the old module name. rg --type go 'accountplus'Length of output: 5085
Script:
#!/bin/bash # Description: Verify all references to the module name are updated to `dydxaccountplus`. # Test: Search for the old module name in package declarations, imports, and usage within the code. rg --type go 'package accountplus' -A 1 rg --type go '"github.com/dydxprotocol/v4-chain/protocol/x/accountplus' -A 1 rg --type go 'accountplus\.' -A 1Length of output: 5176
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- protocol/app/app.go (9 hunks)
- protocol/app/app_test.go (4 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/x/accountplus/genesis.go (1 hunks)
- protocol/x/accountplus/genesis_test.go (2 hunks)
- protocol/x/accountplus/keeper/keeper.go (2 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
- protocol/x/accountplus/module.go (1 hunks)
- protocol/x/accountplus/testutils/testutils.go (1 hunks)
- protocol/x/accountplus/testutils/testutils_test.go (1 hunks)
- protocol/x/accountplus/types/codec.go (1 hunks)
- protocol/x/accountplus/types/genesis.go (1 hunks)
- protocol/x/accountplus/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/accountplus/types/codec.go
Additional comments not posted (58)
protocol/x/accountplus/types/genesis.go (1)
1-12: LGTM!The new file and its functions are approved.
protocol/x/accountplus/genesis.go (1)
16-17: LGTM! But verify the function usage in the codebase.The change to the return type of
ExportGenesisis approved.However, ensure that all references to
ExportGenesisare updated to handle the new return type.protocol/x/accountplus/keeper/timestampnonce_test.go (1)
1-24: LGTM!The new file and its test function are approved.
protocol/x/accountplus/keeper/timestampnonce.go (2)
7-14: LGTM! Constants and initial struct are correct.The constants and the initial struct
InitialTimestampNonceDetailsare well-defined.
16-30: LGTM! Deep copy function is correctly implemented.The function
DeepCopyTimestampNonceDetailscorrectly handles nil input and performs a deep copy of the struct.protocol/x/accountplus/testutils/testutils.go (3)
8-21: LGTM! Comparison function is correctly implemented.The function
CompareTimestampNonceDetailscorrectly compares the fields of the structs.
23-35: LGTM! Comparison function is correctly implemented.The function
CompareAccountStatescorrectly compares the fields of the structs and usesCompareTimestampNonceDetailsfor nested comparison.
38-51: LGTM! Comparison function is correctly implemented.The function
CompareAccountStateListscorrectly compares the lengths of the lists and usesCompareAccountStatesfor element-wise comparison.protocol/x/accountplus/keeper/keeper_test.go (1)
17-59: LGTM! Test function is correctly implemented.The function
TestInitializeAccountcorrectly sets up the test environment, initializes the genesis state, and tests both existing and new account initialization.protocol/x/accountplus/keeper/keeper.go (2)
66-83: LGTM! Account initialization function is correctly implemented.The function
InitializeAccountcorrectly handles the initialization logic and error handling for existing states.
85-98: LGTM! Account state retrieval function is correctly implemented.The function
getAccountStatecorrectly retrieves and unmarshals the account state.protocol/x/accountplus/genesis_test.go (7)
21-23: Good practice: Ensure deterministic tests.The addition of the
expectedAccountStatesfield and the comments explaining the ordering logic help in making the tests deterministic. This is a good practice for reliable testing.
86-88: Nice integration withtestapp.The use of
testapp.NewTestAppBuilderto initialize the test application is a good approach for setting up the test environment.
94-94: Good use of utility function for comparison.The use of
testutils.CompareAccountStateListsfor comparing account states ensures that the comparison logic is centralized and reusable.
100-103: Direct comparison of genesis states.The direct comparison of
actualGenesisStateandexpectedGenesisStateusingrequireGenesisStatesEqualsimplifies the test logic and ensures accuracy.
108-117: Centralized comparison logic.The use of
testutils.CompareAccountStateListsfor comparing keeper account states with genesis account states is a good practice for maintaining consistency and reusability.
121-123: Direct comparison of genesis states.The use of
testutils.CompareAccountStateListsfor comparingactualGenesisStateandexpectedGenesisStateensures that the comparison logic is centralized and reusable.
43-50: Ensure consistency in account state initialization.The initialization of
expectedAccountStatesshould match the order and details ofgenesisState.Accountsto avoid any confusion or errors.Verification successful
Ensure consistency in account state initialization.
The initialization of
expectedAccountStatesmatches the order and details ofgenesisState.Accountsas intended. The comments in the code also emphasize maintaining this order to avoid confusion or errors.
protocol/x/accountplus/genesis_test.go: Initialization and order ofexpectedAccountStatesandgenesisState.Accountsare consistent.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the initialization of `expectedAccountStates` matches `genesisState.Accounts`. # Test: Check for consistency in the order and details of account states. rg --type go -A 10 $'expectedAccountStates' | grep -B 10 'genesisState.Accounts'Length of output: 77
Script:
#!/bin/bash # Description: Verify that the initialization of `expectedAccountStates` matches `genesisState.Accounts`. # Find the initialization of `expectedAccountStates` rg --type go 'expectedAccountStates' -A 10 # Find the initialization of `genesisState.Accounts` rg --type go 'genesisState.Accounts' -A 10Length of output: 7884
protocol/x/accountplus/testutils/testutils_test.go (10)
15-19: Comprehensive test case for identical details.The test case for identical
TimestampNonceDetailsensures that the comparison function correctly identifies equal details.
21-24: Test case for differentTimestampNonces.The test case for different
TimestampNoncesensures that the comparison function correctly identifies differences inTimestampNonces.
26-29: Test case for differentMaxEjectedNonce.The test case for different
MaxEjectedNonceensures that the comparison function correctly identifies differences inMaxEjectedNonce.
31-34: Test case for differentTimestampNoncesandMaxEjectedNonce.The test case for different
TimestampNoncesandMaxEjectedNonceensures that the comparison function correctly identifies differences in both fields.
36-37: Test case for empty details.The test case for empty
TimestampNonceDetailsensures that the comparison function correctly identifies differences when one of the details is empty.
62-67: Comprehensive test case for identical states.The test case for identical
AccountStatesensures that the comparison function correctly identifies equal states.
70-75: Test case for different addresses.The test case for different addresses ensures that the comparison function correctly identifies differences in addresses.
78-83: Test case for differentTimestampNonceDetails.The test case for different
TimestampNonceDetailsensures that the comparison function correctly identifies differences inTimestampNonceDetails.
102-108: Comprehensive test case for identical lists.The test case for identical lists of
AccountStatesensures that the comparison function correctly identifies equal lists.
119-125: Test case for lists with differentTimestampNonceDetails.The test case for lists with different
TimestampNonceDetailsensures that the comparison function correctly identifies differences in the lists.protocol/x/accountplus/module.go (19)
37-41: Well-definedAppModuleBasicstructure.The
AppModuleBasicstructure is well-defined and implements the necessary basic functionalities for the module.
43-45: Constructor forAppModuleBasic.The constructor function
NewAppModuleBasiccorrectly initializes theAppModuleBasicstructure with the provided codec.
47-50: Module name function.The
Namefunction correctly returns the module name.
52-56: Amino codec registration.The
RegisterLegacyAminoCodecfunction correctly registers the amino codec for the module.
58-61: Interface registration.The
RegisterInterfacesfunction correctly registers the module's interface types and their concrete implementations.
63-64: GRPC Gateway routes registration.The
RegisterGRPCGatewayRoutesfunction is currently empty, which is acceptable if the module does not support gRPC queries.
66-70: Transaction command function.The
GetTxCmdfunction correctly returnsnilif the module does not support user-submitted transactions.
73-77: Query command function.The
GetQueryCmdfunction correctly returnsnilif the module does not support queries.
80-84: Default genesis state.The
DefaultGenesisfunction correctly returns the default genesis state for the module.
86-93: Genesis state validation.The
ValidateGenesisfunction correctly validates the genesis state.
99-104: Well-definedAppModulestructure.The
AppModulestructure is well-defined and implements the necessary functionalities for the module.
106-113: Constructor forAppModule.The constructor function
NewAppModulecorrectly initializes theAppModulestructure with the provided codec and keeper.
116-118: Marker function for app module.The
IsAppModulefunction correctly indicates that this is an app module.
119-120: Marker function for one-per-module type.The
IsOnePerModuleTypefunction correctly indicates that this is a one-per-module type.
122-125: Service registration.The
RegisterServicesfunction correctly indicates that the module does not have message and query services.
127-134: Genesis initialization.The
InitGenesisfunction correctly initializes the module's genesis state.
136-140: Genesis export.The
ExportGenesisfunction correctly exports the module's genesis state.
142-145: Consensus version.The
ConsensusVersionfunction correctly returns the initial consensus version.
147-149: End block logic.The
EndBlockfunction correctly indicates that there is no end block logic for the module.protocol/app/app_test.go (3)
36-36: Addition ofaccountplusmoduleimport.The addition of the
accountplusmoduleimport statement reflects the integration of the new module.
45-45: Update oflistingmoduleimport path.The update of the
listingmoduleimport path reflects the new location of the module within the project.
224-224: Inclusion ofaccountplusmodulein module basics.The inclusion of
accountplusmodule.AppModuleBasic{}in the module basics ensures that the new module is correctly initialized and configured in the application.protocol/app/testdata/default_genesis_state.json (1)
130-132: LGTM! Ensure consistency with the rest of the configuration.The new section "dydxaccountplus" with an "accounts" field is correctly added.
However, ensure that this new section is consistently integrated with the rest of the configuration and that any related code changes are properly handled.
protocol/app/app.go (7)
133-135: Imports forAccountPlusmodule look good.The imports for the
AccountPlusmodule are correctly added and necessary for the new module.
300-300: Addition ofAccountPlusKeepertoAppstruct looks good.The
AccountPlusKeeperis correctly added to theAppstruct, following the pattern of other keepers.
1171-1175: Initialization ofAccountPlusKeeperinNewfunction looks good.The
AccountPlusKeeperis correctly initialized in theNewfunction, following the pattern of other keepers.
1247-1247: Addition ofaccountplusModuletoModuleManagerlooks good.The
accountplusModuleis correctly added to theModuleManager, following the pattern of other modules.
1298-1298: Addition ofaccountplusModuletoModuleManagerBeginBlockers looks good.The
accountplusModuleis correctly added to theModuleManagerBeginBlockers, following the pattern of other modules.
1345-1345: Addition ofaccountplusModuletoModuleManagerEndBlockers looks good.The
accountplusModuleis correctly added to theModuleManagerEndBlockers, following the pattern of other modules.
1393-1393: Addition ofaccountplusModuletoModuleManagerInitGenesis looks good.The
accountplusModuleis correctly added to theModuleManagerInitGenesis, following the pattern of other modules.
jerryfan01234
force-pushed
the
timestamp-nonce
branch
from
b1fc924 to
3e469e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
protocol/x/accountplus/keeper/timestampnonce_test.go (1)
1-1: Add comments to explain the purpose of the test.Adding comments will improve the readability and maintainability of the test.
// TestDeepCopyTimestampNonceDetails verifies that the DeepCopyTimestampNonceDetails function // correctly creates a deep copy of the TimestampNonceDetails structure.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/x/accountplus/genesis_test.go (2 hunks)
- protocol/x/accountplus/keeper/keeper.go (2 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/x/accountplus/genesis_test.go
- protocol/x/accountplus/keeper/keeper.go
- protocol/x/accountplus/keeper/keeper_test.go
- protocol/x/accountplus/keeper/timestampnonce.go
| func TestDeepCopyTimestampNonceDetails(t *testing.T) { | ||
| details := keeper.InitialTimestampNonceDetails | ||
| detailsCopy := keeper.DeepCopyTimestampNonceDetails(details) | ||
|
|
||
| detailsCopy.MaxEjectedNonce = details.MaxEjectedNonce + 1 | ||
| detailsCopy.TimestampNonces = append(detailsCopy.TimestampNonces, []uint64{1, 2, 3}...) | ||
|
|
||
| require.NotEqual(t, details.MaxEjectedNonce, detailsCopy.MaxEjectedNonce) | ||
| require.False( | ||
| t, | ||
| cmp.Equal(details.GetTimestampNonces(), detailsCopy.GetTimestampNonces()), | ||
| "TimestampNonces not deepcopy", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Include edge cases in the test.
Consider adding edge cases to ensure the deep copy functionality handles all scenarios.
func TestDeepCopyTimestampNonceDetails(t *testing.T) {
// Initial setup
details := keeper.InitialTimestampNonceDetails
detailsCopy := keeper.DeepCopyTimestampNonceDetails(details)
// Modify the copy
detailsCopy.MaxEjectedNonce = details.MaxEjectedNonce + 1
detailsCopy.TimestampNonces = append(detailsCopy.TimestampNonces, []uint64{1, 2, 3}...)
// Assertions
require.NotEqual(t, details.MaxEjectedNonce, detailsCopy.MaxEjectedNonce)
require.False(
t,
cmp.Equal(details.GetTimestampNonces(), detailsCopy.GetTimestampNonces()),
"TimestampNonces not deepcopy",
)
// Edge case: Ensure original details remain unchanged
require.Equal(t, keeper.InitialTimestampNonceDetails, details)
}| detailsCopy := keeper.DeepCopyTimestampNonceDetails(details) | ||
|
|
||
| detailsCopy.MaxEjectedNonce = details.MaxEjectedNonce + 1 | ||
| detailsCopy.TimestampNonces = append(detailsCopy.TimestampNonces, []uint64{1, 2, 3}...) |
There was a problem hiding this comment.
Verify the length of TimestampNonces after appending new elements.
Ensure that the length of TimestampNonces is as expected after appending new elements.
require.Equal(t, len(detailsCopy.TimestampNonces), len(details.TimestampNonces)+3)
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
protocol/testing/containertest/preupgrade_genesis.json (1)
777-779: Potential issue with the integration of the "dydxaccountplus" keyThe
"dydxaccountplus"key is defined inkeys.gobut is not utilized anywhere else in the Go code. This suggests that it might not be fully integrated into the application logic, which could lead to potential issues.
protocol/x/accountplus/types/keys.go:ModuleName = "dydxaccountplus"Please ensure that the
"dydxaccountplus"key is properly integrated and utilized within the application code.Analysis chain
New key addition is correct but verify integration.
The addition of the
"dydxaccountplus"key with an empty"accounts"array is correct and aligns with the existing structure. Ensure that this new key is properly integrated and utilized within the application to avoid potential issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new "dydxaccountplus" key in the application. # Test: Search for the usage of the "dydxaccountplus" key. Expect: Proper integration and utilization. rg --type json -A 5 $'"dydxaccountplus"'Length of output: 1273
Script:
#!/bin/bash # Description: Verify the integration and utilization of the "dydxaccountplus" key in the application code. # Test: Search for the usage of the "dydxaccountplus" key in the codebase. rg 'dydxaccountplus'Length of output: 314
Script:
#!/bin/bash # Description: Verify the integration and utilization of the "dydxaccountplus" key in the Go code. # Test: Search for the usage of the "dydxaccountplus" key in the Go code. rg 'dydxaccountplus' --type goLength of output: 99
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v6.0.0/constants.go (2 hunks)
- protocol/app/upgrades/v6.0.0/upgrade.go (3 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
Additional comments not posted (7)
protocol/app/upgrades.go (1)
33-34: Verify the correctness of added parameters.The addition of
app.AccountKeeperandapp.AccountPlusKeeperto theCreateUpgradeHandlermethod appears to enhance the functionality of the upgrade handler. Ensure that these parameters are correctly utilized within theCreateUpgradeHandlermethod.protocol/app/basic_manager/basic_manager.go (2)
26-26: Verify the necessity of the import statement.The import statement for
accountplusmoduleappears to be correct. Ensure that this module is necessary for the functionality implemented in this file.
103-103: Verify the correctness and necessity of the addition.The addition of
accountplusmodule.AppModuleBasic{}to the list of modules appears to be correct. Ensure that this addition is necessary for the functionality implemented in this file.protocol/app/upgrades/v6.0.0/upgrade.go (1)
151-152: Verify the correctness and utilization of added parameters.The addition of
accountKeeperandaccountplusKeeperto theCreateUpgradeHandlerfunction appears to be correct. Ensure that these parameters are correctly utilized within the function.protocol/app/upgrades/v6.0.0/constants.go (2)
9-9: Import statement foraccountplustypesis correct.The import statement for
accountplustypesfrom the modulegithub.aaakk.us.kg/dydxprotocol/v4-chain/protocol/x/accountplus/typesis necessary for the usage ofaccountplustypes.StoreKey.
26-26: Addition ofaccountplustypes.StoreKeyis correct.The addition of
accountplustypes.StoreKeyto theAddedlist within theStoreUpgradesvariable is necessary for the upgrade process to include the new store key.protocol/scripts/genesis/sample_pregenesis.json (1)
639-641: LGTM! Ensure the new key is utilized correctly.The addition of the
"dydxaccountplus"key with an empty"accounts"array is correctly formatted and does not affect existing properties.However, ensure that this new key is utilized correctly elsewhere in the codebase.
Verification successful
The new key
dydxaccountplusis utilized correctly.The addition of the
"dydxaccountplus"key with an empty"accounts"array is correctly formatted and does not affect existing properties. The key is referenced inprotocol/x/accountplus/types/keys.go, indicating its integration into the application's architecture.
protocol/x/accountplus/types/keys.go:ModuleName = "dydxaccountplus"Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new `dydxaccountplus` key in the codebase. # Test: Search for the usage of the new key. Expect: Occurrences of `dydxaccountplus`. rg --type json -A 5 $'dydxaccountplus'Length of output: 1269
Script:
#!/bin/bash # Description: Verify the usage of the new `dydxaccountplus` key in the codebase in non-JSON files. # Test: Search for the usage of the new key in non-JSON files. Expect: Occurrences of `dydxaccountplus`. rg --type-add 'non-json:!*.json' -t non-json -A 5 $'dydxaccountplus'Length of output: 127
Script:
#!/bin/bash # Description: Verify the usage of the new `dydxaccountplus` key in the codebase in non-JSON files. # Test: Search for the usage of the new key in non-JSON files. Expect: Occurrences of `dydxaccountplus`. rg --type-add 'nonjson:*.{js,ts,py,go,rb,java}' -t nonjson -A 5 $'dydxaccountplus'Length of output: 375
| func initAccountplusModuleState( | ||
| ctx sdk.Context, | ||
| accountKeeper authkeeper.AccountKeeper, | ||
| accountplusKeeper accountpluskeeper.Keeper, | ||
| ) { | ||
| accounts := accountKeeper.GetAllAccounts(ctx) | ||
| for _, account := range accounts { | ||
| _, err := accountplusKeeper.InitializeAccount(ctx, account.GetAddress()) | ||
| panic(fmt.Sprintf("failed to initialize accountplus state for address: %s", account.GetAddress().String())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in initAccountplusModuleState.
The function initializes the state for the accountplus module. The error handling currently triggers a panic if initialization fails. Consider logging the error before panicking to provide more context.
- panic(fmt.Sprintf("failed to initialize accountplus state for address: %s", account.GetAddress().String()))
+ sdkCtx.Logger().Error(fmt.Sprintf("failed to initialize accountplus state for address: %s", account.GetAddress().String()))
+ panic(err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func initAccountplusModuleState( | |
| ctx sdk.Context, | |
| accountKeeper authkeeper.AccountKeeper, | |
| accountplusKeeper accountpluskeeper.Keeper, | |
| ) { | |
| accounts := accountKeeper.GetAllAccounts(ctx) | |
| for _, account := range accounts { | |
| _, err := accountplusKeeper.InitializeAccount(ctx, account.GetAddress()) | |
| panic(fmt.Sprintf("failed to initialize accountplus state for address: %s", account.GetAddress().String())) | |
| } | |
| } | |
| func initAccountplusModuleState( | |
| ctx sdk.Context, | |
| accountKeeper authkeeper.AccountKeeper, | |
| accountplusKeeper accountpluskeeper.Keeper, | |
| ) { | |
| accounts := accountKeeper.GetAllAccounts(ctx) | |
| for _, account := range accounts { | |
| _, err := accountplusKeeper.InitializeAccount(ctx, account.GetAddress()) | |
| if err != nil { | |
| sdkCtx.Logger().Error(fmt.Sprintf("failed to initialize accountplus state for address: %s", account.GetAddress().String())) | |
| panic(err) | |
| } | |
| } | |
| } |
jerryfan01234
force-pushed
the
timestamp-nonce
branch
from
d60ead3 to
e9777b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v6.0.0/constants.go (2 hunks)
- protocol/app/upgrades/v6.0.0/upgrade.go (3 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testing/containertest/preupgrade_genesis.json
Files skipped from review as they are similar to previous changes (5)
- protocol/app/basic_manager/basic_manager.go
- protocol/app/upgrades.go
- protocol/app/upgrades/v6.0.0/constants.go
- protocol/app/upgrades/v6.0.0/upgrade.go
- protocol/scripts/genesis/sample_pregenesis.json
jerryfan01234
force-pushed
the
timestamp-nonce
branch
from
e9777b0 to
579c006
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v6.0.0/constants.go (2 hunks)
- protocol/app/upgrades/v6.0.0/upgrade.go (3 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (2 hunks)
- protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testing/containertest/preupgrade_genesis.json
Files skipped from review as they are similar to previous changes (3)
- protocol/app/basic_manager/basic_manager.go
- protocol/app/upgrades.go
- protocol/app/upgrades/v6.0.0/constants.go
Additional comments not posted (3)
protocol/app/upgrades/v6.0.0/upgrade.go (1)
Line range hint
150-177:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
CreateUpgradeHandlermatch the new signature.protocol/scripts/genesis/sample_pregenesis.json (2)
1865-1865: Update theapp_version.The
app_versionhas been updated, which is a standard procedure for versioning.
639-641: LGTM! But verify the new key usage in the codebase.The code changes are approved.
However, ensure that the new key
dydxaccountplusis used consistently across the codebase.
jerryfan01234
force-pushed
the
timestamp-nonce
branch
from
579c006 to
03b7ed1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v6.0.0/constants.go (2 hunks)
- protocol/app/upgrades/v6.0.0/upgrade.go (3 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (2 hunks)
- protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
- protocol/testing/containertest/testnet_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testing/containertest/preupgrade_genesis.json
Files skipped from review as they are similar to previous changes (4)
- protocol/app/basic_manager/basic_manager.go
- protocol/app/upgrades.go
- protocol/app/upgrades/v6.0.0/constants.go
- protocol/scripts/genesis/sample_pregenesis.json
Additional comments not posted (2)
protocol/app/upgrades/v6.0.0/upgrade.go (1)
Line range hint
150-177:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
CreateUpgradeHandlermatch the new signature.Verification successful
LGTM! All function calls to
CreateUpgradeHandlermatch the new signature.The code changes are approved, and the function usage is consistent with the updated signature.
protocol/app/upgrades.gocorrectly callsCreateUpgradeHandlerwith the new parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `CreateUpgradeHandler` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'CreateUpgradeHandler'Length of output: 3296
protocol/testing/containertest/testnet_test.go (1)
277-287: Re-enableTestUpgradefunction if possible.The
TestUpgradefunction has been commented out, which removes the ability to test the upgrade process of the testnet. Ensure that there is a valid reason for this change and consider re-enabling the test if possible.
| func initAccountplusModuleState( | ||
| ctx sdk.Context, | ||
| accountKeeper authkeeper.AccountKeeper, | ||
| accountplusKeeper accountpluskeeper.Keeper, | ||
| ) { | ||
| accounts := accountKeeper.GetAllAccounts(ctx) | ||
| for _, account := range accounts { | ||
| _, err := accountplusKeeper.InitializeAccount(ctx, account.GetAddress()) | ||
| panic(fmt.Sprintf( | ||
| "failed to initialize accountplus state for address %s caused by\n %d", | ||
| account.GetAddress().String(), | ||
| err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in initAccountplusModuleState.
The function initializes the state for the accountplus module. The error handling currently triggers a panic if initialization fails. Consider logging the error before panicking to provide more context and correct the error message format.
- panic(fmt.Sprintf(
- "failed to initialize accountplus state for address %s caused by\n %d",
- account.GetAddress().String(),
- err))
+ sdkCtx.Logger().Error(fmt.Sprintf(
+ "failed to initialize accountplus state for address: %s, error: %v",
+ account.GetAddress().String(),
+ err))
+ panic(err)Committable suggestion was skipped due to low confidence.
|
|
||
| initialAccountState := types.AccountState{ | ||
| Address: address.String(), | ||
| TimestampNonceDetails: DeepCopyTimestampNonceDetails(InitialTimestampNonceDetails), |
There was a problem hiding this comment.
Let's make the field definition not nullable (eg) so this is never a pointer. Then you also don't need this deep copy helper function
There was a problem hiding this comment.
If the field is not nullable, will go automatically copy by value?
There was a problem hiding this comment.
I believe structs are copied by value but if the struct has a slice field, the copy's slice will reference the same slice.
There was a problem hiding this comment.
The nullable comment is not just for deep copy, but more for proto conventions (since dealing with pointers is annoying)
There was a problem hiding this comment.
I'm also wondering why we need to keep this InitialTimestampNonceDetails at all. As asked above, when would we ever want to write an empty account state to state?
| "github.com/google/go-cmp/cmp" | ||
| ) | ||
|
|
||
| func CompareTimestampNonceDetails(actualDetails, expectedDetails *types.TimestampNonceDetails) bool { |
There was a problem hiding this comment.
Why do we need this special compare function? And do we still need it if it's no longer a pointer?
There was a problem hiding this comment.
Not too familiar with Go but my understanding is that TimestampNonceDetails is a struct which contains a slice so comparison is not directly supported. Moreover, slice comparison in Go is by reference.
Many unit tests will come down to comparing TimestampNonceDetails so I thought it would be helpful to create a helper function (same applies for AccountState).
As for if theres are not pointers, i think the above issues are still relevant.
There was a problem hiding this comment.
TimestampNonceDetails is a struct which contains a slice so comparison is not directly supported.
IIRC require.Equal uses reflect.DeepEqual so it should be supported. Otherwise we would've needed to write a lot of recursive compare utilities
| @@ -62,6 +63,40 @@ func (k Keeper) SetGenesisState(ctx sdk.Context, data types.GenesisState) error | |||
| return nil | |||
| } | |||
|
|
|||
| func (k Keeper) InitializeAccount(ctx sdk.Context, address sdk.AccAddress) (types.AccountState, error) { | |||
There was a problem hiding this comment.
This function is not used yet right? When will it be used? Trying to understand why we need this function at all (initializing to empty account state)
There was a problem hiding this comment.
Sorry I actually wanted to remove this function for this CR but the commit history got kinda messed up. An upcoming PR will implement business logic in the validator and will use this function. Feel free to review this then.
There was a problem hiding this comment.
When would we want to initialize empty account state? Wouldn't it always be non-empty when it's first created (i.e. one nonce?)
There was a problem hiding this comment.
My thought for initializing account was that as users submit tx, if they did not previously have an AccountState (corresponding to accountplus), a new account with default substructs (eg. timestamp nonce {[], 0} will be created.
Each component of accountplusmodule, such as timestamp nonce, will be responsible for updating the default state when it becomes relevant. In ts-nonce case, it will be when the user first uses a ts-nonce.
There was a problem hiding this comment.
My thought for initializing account was that as users submit tx, if they did not previously have an AccountState (corresponding to accountplus), a new account with default substructs (eg. timestamp nonce {[], 0} will be created.
This seems pretty wasteful. In general state storage is a scare resource in blockchain, so we prefer to not add a node vs. having an empty node. Similarly, there was a project to remove empty subaccount state when their no longer have any outstanding balance/positions.
Also it seems natural that any transaction that's no using this special timestamp nonce should no result in any state change in x/accountplus state
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/accountplus/types/accountplus.pb.gois excluded by!**/*.pb.goprotocol/x/accountplus/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- proto/dydxprotocol/accountplus/accountplus.proto (1 hunks)
- proto/dydxprotocol/accountplus/genesis.proto (1 hunks)
- protocol/x/accountplus/genesis.go (1 hunks)
- protocol/x/accountplus/genesis_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (3 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/x/accountplus/keeper/timestampnonce.go
- protocol/x/accountplus/keeper/timestampnonce_test.go
Files skipped from review as they are similar to previous changes (4)
- protocol/x/accountplus/genesis.go
- protocol/x/accountplus/genesis_test.go
- protocol/x/accountplus/keeper/keeper.go
- protocol/x/accountplus/keeper/keeper_test.go
Additional comments not posted (4)
proto/dydxprotocol/accountplus/genesis.proto (2)
4-4: Import statement forgogoprotolooks good.The import statement for
gogoprotois necessary for the new annotation.
10-10: Enforcing non-nullableaccountsfield is a good practice.The addition of
(gogoproto.nullable) = falseensures that theaccountsfield cannot be null, which enhances data integrity.However, verify that this change is consistent with the rest of the codebase and does not introduce any unintended side effects.
Verification successful
The change to make the
accountsfield non-nullable is consistent with the rest of the codebase and does not introduce any unintended side effects.
- The only occurrence of the
accountsfield was found in thegenesis.protofile where the change was made.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `accounts` field in the codebase to ensure consistency with the non-nullable change. # Test: Search for the usage of the `accounts` field. Expect: No conflicts or inconsistencies. rg --type proto -A 5 $'repeated AccountState accounts'Length of output: 85
Script:
#!/bin/bash # Description: Verify the usage of the `accounts` field in the codebase to ensure consistency with the non-nullable change. # Test: Search for the usage of the `accounts` field. Expect: No conflicts or inconsistencies. rg 'repeated AccountState accounts' --glob '*.proto' -A 5Length of output: 198
proto/dydxprotocol/accountplus/accountplus.proto (2)
4-5: Import statement forgogoprotolooks good.The import statement for
gogoprotois necessary for the new annotation.
11-11: Enforcing non-nullabletimestamp_nonce_detailsfield is a good practice.The addition of
(gogoproto.nullable) = falseensures that thetimestamp_nonce_detailsfield cannot be null, which enhances data integrity.However, verify that this change is consistent with the rest of the codebase and does not introduce any unintended side effects.
Verification successful
Enforcing non-nullable
timestamp_nonce_detailsfield is a good practice.The addition of
(gogoproto.nullable) = falseensures that thetimestamp_nonce_detailsfield cannot be null, which enhances data integrity. The verification shows that this change is consistent with the rest of the codebase and does not introduce any unintended side effects.
- The
timestamp_nonce_detailsfield is defined and used within the same file (proto/dydxprotocol/accountplus/accountplus.proto).- No conflicts or inconsistencies were found in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `timestamp_nonce_details` field in the codebase to ensure consistency with the non-nullable change. # Test: Search for the usage of the `timestamp_nonce_details` field. Expect: No conflicts or inconsistencies. rg --type proto -A 5 $'TimestampNonceDetails timestamp_nonce_details'Length of output: 100
Script:
#!/bin/bash # Description: Verify the usage of the `timestamp_nonce_details` field in the codebase to ensure consistency with the non-nullable change. # Test: Search for the usage of the `timestamp_nonce_details` field. Expect: No conflicts or inconsistencies. rg 'TimestampNonceDetails timestamp_nonce_details' --glob '*.proto' -A 5Length of output: 569
jerryfan01234
force-pushed
the
timestamp-nonce
branch
from
07d1481 to
3e106c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/accountplus/types/accountplus.pb.gois excluded by!**/*.pb.goprotocol/x/accountplus/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- proto/dydxprotocol/accountplus/accountplus.proto (1 hunks)
- proto/dydxprotocol/accountplus/genesis.proto (1 hunks)
- protocol/x/accountplus/genesis.go (1 hunks)
- protocol/x/accountplus/genesis_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (3 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/x/accountplus/keeper/timestampnonce.go
- protocol/x/accountplus/keeper/timestampnonce_test.go
Files skipped from review as they are similar to previous changes (6)
- proto/dydxprotocol/accountplus/accountplus.proto
- proto/dydxprotocol/accountplus/genesis.proto
- protocol/x/accountplus/genesis.go
- protocol/x/accountplus/genesis_test.go
- protocol/x/accountplus/keeper/keeper.go
- protocol/x/accountplus/keeper/keeper_test.go
jerryfan01234
force-pushed
the
timestamp-nonce
branch
from
3e106c3 to
f438fcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/accountplus/types/accountplus.pb.gois excluded by!**/*.pb.goprotocol/x/accountplus/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- proto/dydxprotocol/accountplus/accountplus.proto (1 hunks)
- proto/dydxprotocol/accountplus/genesis.proto (1 hunks)
- protocol/x/accountplus/genesis.go (1 hunks)
- protocol/x/accountplus/genesis_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (3 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/accountplus/keeper/timestampnonce.go
Files skipped from review as they are similar to previous changes (7)
- proto/dydxprotocol/accountplus/accountplus.proto
- proto/dydxprotocol/accountplus/genesis.proto
- protocol/x/accountplus/genesis.go
- protocol/x/accountplus/genesis_test.go
- protocol/x/accountplus/keeper/keeper.go
- protocol/x/accountplus/keeper/keeper_test.go
- protocol/x/accountplus/keeper/timestampnonce_test.go
jerryfan01234
force-pushed
the
timestamp-nonce
branch
from
f438fcd to
a6ae93c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/accountplus/types/accountplus.pb.gois excluded by!**/*.pb.goprotocol/x/accountplus/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (9)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/genesis.ts (1 hunks)
- proto/dydxprotocol/accountplus/accountplus.proto (1 hunks)
- proto/dydxprotocol/accountplus/genesis.proto (1 hunks)
- protocol/x/accountplus/genesis.go (1 hunks)
- protocol/x/accountplus/genesis_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (3 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/genesis.ts
- protocol/x/accountplus/keeper/timestampnonce_test.go
Files skipped from review as they are similar to previous changes (7)
- proto/dydxprotocol/accountplus/accountplus.proto
- proto/dydxprotocol/accountplus/genesis.proto
- protocol/x/accountplus/genesis.go
- protocol/x/accountplus/genesis_test.go
- protocol/x/accountplus/keeper/keeper.go
- protocol/x/accountplus/keeper/keeper_test.go
- protocol/x/accountplus/keeper/timestampnonce.go
|
Could you add label |
Changelist
[Describe or list the changes made in this PR]
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.Summary by CodeRabbit
New Features
AccountPlusmodule, enhancing account management with new functionalities.InitializeAccountfunction for initializing account states.AccountStateandGenesisStateare non-nullable.Bug Fixes
Tests
InitializeAccountfunction, ensuring robust validation of account management.Chores