[OTE-882] Add prefix to accountplus keeper #2526
Conversation
WalkthroughThe changes introduce a migration function for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
protocol/x/accountplus/types/keys.go (1)
20-23: LGTM! Consider enhancing the comment for clarity.The addition of
AccountStateKeyPrefixaligns well with the PR objective and follows the existing naming conventions. Good job on organizing it in a separate const block for clarity.Consider expanding the comment slightly for better context:
-// Prefix for account state. +// Prefix for account state keys in the KV store.protocol/app/upgrades/v8.0/upgrade_test.go (2)
13-27: LGTM: Test suite structure is well-organized.The
UpgradeTestSuiteand its setup follow best practices for Go test suites. TheSetupTestmethod properly initializes the test environment.Consider adding a
TearDownTestmethod to clean up resources after each test, even if it's not strictly necessary in this case. This promotes good testing practices:func (s *UpgradeTestSuite) TearDownTest() { // Clean up any resources if necessary }
29-71: LGTM: The test method comprehensively covers the migration process.The
TestUpgrade_MigrateAccountplusAccountStatemethod effectively tests the migration of account states from unprefixed to prefixed storage. It creates test data, performs the migration, and verifies the results using appropriate assertions.Consider the following improvements to enhance the test's robustness:
- Use a helper function to generate test data with random addresses and nonces:
func generateTestAccountStates(count int) []accountplustypes.AccountState { states := make([]accountplustypes.AccountState, count) for i := 0; i < count; i++ { states[i] = accountplustypes.AccountState{ Address: sdk.AccAddress(testutil.RandomAddress()).String(), TimestampNonceDetails: accountplustypes.TimestampNonceDetails{ TimestampNonces: []uint64{uint64(rand.Intn(1000)), uint64(rand.Intn(1000)), uint64(rand.Intn(1000))}, MaxEjectedNonce: uint64(rand.Intn(1000)), }, } } return states }
- Add error checking when marshaling account states:
- bz := s.tApp.App.AccountPlusKeeper.GetCdc().MustMarshal(&accountState) + bz, err := s.tApp.App.AccountPlusKeeper.GetCdc().Marshal(&accountState) + s.Require().NoError(err, "Failed to marshal account state")
- Test with a larger number of account states to ensure scalability:
testAccountStates := generateTestAccountStates(100)These changes will make the test more robust and closer to real-world scenarios.
protocol/x/accountplus/keeper/timestampnonce.go (1)
119-119: LGTM: Simplified return statement in isLargerThanSmallestValueThe simplification of the return statement in
isLargerThanSmallestValueis a good improvement. It reduces code complexity while maintaining the same logic.For even better readability, consider using a named return value:
func isLargerThanSmallestValue(value uint64, values []uint64) (isLarger bool, minIndex int) { // ... existing code ... return value > values[minIndex], minIndex }This makes the purpose of the return values clearer at both the function signature and the return statement.
protocol/app/upgrades/v8.0/upgrade.go (1)
47-47: Resolve TODO comments before mergingThe file contains a TODO comment referencing upgrade scaffolding. Please address this task or create a tracking issue before merging to maintain code cleanliness.
protocol/x/accountplus/keeper/keeper.go (1)
68-69: Include the key in the error message for better debuggingAdding the key that caused the unmarshalling error can aid in diagnosing issues more effectively.
Apply this diff to include the key in the error message:
if err != nil { - return nil, fmt.Errorf("failed to unmarshal account state: %w", err) + return nil, fmt.Errorf("failed to unmarshal account state for key %s: %w", string(iterator.Key()), err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- protocol/app/upgrades/v8.0/upgrade.go (1 hunks)
- protocol/app/upgrades/v8.0/upgrade_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (6 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
- protocol/x/accountplus/keeper/timestampnonce.go (3 hunks)
- protocol/x/accountplus/types/keys.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (12)
protocol/x/accountplus/types/keys.go (1)
22-22: Verify usage of the new constant across the module.The
AccountStateKeyPrefixconstant has been added correctly, but it's not used within this file. To ensure it's properly integrated:Run the following script to check its usage across the module:
✅ Verification successful
Usage of
AccountStateKeyPrefixhas been verified across the module and is properly integrated.
- Found usage in
keeper/keeper.gofiles ensuring effective integration with state management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of AccountStateKeyPrefix across the module # Test: Search for AccountStateKeyPrefix usage rg -A 5 'AccountStateKeyPrefix' protocol/x/accountplusLength of output: 1788
protocol/app/upgrades/v8.0/upgrade_test.go (1)
1-11: LGTM: Package declaration and imports are appropriate.The package name
v_8_0follows the version naming convention, and all imports are relevant to the test suite's functionality.protocol/x/accountplus/keeper/timestampnonce.go (3)
44-44: LGTM: Function name update improves clarityThe change from
GetAccountPlusStateWithTimestampNonceDetailstoAccountStateFromTimestampNonceDetailsis a good improvement. The new function name more accurately describes its purpose, which is to create an account state from timestamp nonce details rather than retrieving it.
59-62: LGTM: Improved efficiency in EjectStaleTimestampNoncesThe refactoring in the
EjectStaleTimestampNoncesfunction is a good improvement:
- Calculating
oldestAllowedTsonce at the beginning of the function avoids repeated calculations in the loop.- The simplified condition in the for loop using
oldestAllowedTsimproves readability.These changes maintain the same logic while potentially improving performance and making the code easier to understand.
Line range hint
1-121: Overall assessment: Positive improvements to account state managementThe changes in this file are part of a broader restructuring of account state management in the
accountplusmodule. The modifications enhance code clarity, efficiency, and maintainability while maintaining the core functionality of timestamp nonce processing.Key improvements:
- Better function naming in
ProcessTimestampNonce.- Improved efficiency in
EjectStaleTimestampNonces.- Simplified logic in
isLargerThanSmallestValue.These changes align well with the related modifications in
keeper.goandkeys.go, contributing to a more robust and efficient account state management system.protocol/app/upgrades/v8.0/upgrade.go (2)
41-41: Ensure correct deletion of old unprefixed keysWhen deleting the old keys, confirm that you are deleting the correct unprefixed keys. Since the migration iterates over all keys, it's important to ensure that only the intended
AccountStatekeys are deleted to prevent data loss.
38-38: Confirm that SetAccountState correctly handles key prefixingThe function
k.SetAccountState(ctx, key, accountState)is called with the original unprefixed key. Verify thatSetAccountStatecorrectly adds the new prefix to the key internally. If it does not, you may need to modify the key before passing it to ensure theAccountStateis stored under the new prefixed key.Run the following script to verify how
SetAccountStatehandles key prefixing:This script searches for the
SetAccountStatemethod to confirm whether it prefixes the key withAccountStateKeyPrefix.protocol/x/accountplus/keeper/keeper.go (5)
8-8: Import statement is appropriateThe addition of
cosmossdk.io/store/prefixis necessary for utilizing prefix stores in the subsequent code changes.
56-69: Refactoring to use prefix store enhances efficiencyThe changes in
GetAllAccountStatesto useprefix.NewStorewithtypes.AccountStateKeyPrefiximprove the specificity and efficiency of data retrieval from the store. The implementation correctly iterates over the prefixed keys and unmarshals theAccountStateobjects.
168-169: Consistent use of prefix store in data access functionsUpdating
GetAccountStateandSetAccountStateto useprefix.NewStorewithtypes.AccountStateKeyPrefixensures consistency in data access patterns across theKeeper. This enhances maintainability and aligns with the refactoring done inGetAllAccountStates.Also applies to: 191-193
Line range hint
150-155: Ensure all references to the renamed function are updatedThe function
GetAccountPlusStateWithTimestampNonceDetailshas been renamed toAccountStateFromTimestampNonceDetails. Please verify that all occurrences of the old function name have been updated throughout the codebase to prevent any potential issues.Run the following script to find any usages of the old function name:
#!/bin/bash # Description: Search for any remaining usages of the old function name. rg 'GetAccountPlusStateWithTimestampNonceDetails'
40-47: Verify the necessity and security of exposing internal fieldsThe methods
GetStoreKey()andGetCdc()expose the internal fieldsstoreKeyandcdcof theKeeperstruct. Ensure that exposing these fields is required and does not introduce any security concerns.Run the following script to verify the usage of these methods:
✅ Verification successful
Verified: The methods
GetStoreKey()andGetCdc()are used internally within the codebase as required and do not introduce any security concerns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'GetStoreKey' and 'GetCdc' methods to ensure they are necessary. rg 'GetStoreKey\(' rg 'GetCdc\('Length of output: 143
Script:
#!/bin/bash # Description: Find all usages of 'GetStoreKey' and 'GetCdc' methods to ensure they are necessary. rg 'GetStoreKey\(' rg 'GetCdc\('Length of output: 622
| @@ -144,7 +147,7 @@ func (k Keeper) GetAllAuthenticatorData(ctx sdk.Context) ([]types.AuthenticatorD | |||
| return accountAuthenticators, nil | |||
| } | |||
|
|
|||
| func GetAccountPlusStateWithTimestampNonceDetails( | |||
| func AccountStateFromTimestampNonceDetails( | |||
There was a problem hiding this comment.
Nit: this is only used for newly initialized account state
| func AccountStateFromTimestampNonceDetails( | |
| func NewAccountStateFromTimestampNonceDetails( |
| @@ -46,26 +53,22 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger { | |||
| func (k Keeper) InitializeForGenesis(ctx sdk.Context) { | |||
| } | |||
|
|
|||
| // Get all account details pairs in store | |||
| // Get all AccountStates from kvstore | |||
| func (k Keeper) GetAllAccountStates(ctx sdk.Context) ([]types.AccountState, error) { | |||
| store := ctx.KVStore(k.storeKey) | |||
There was a problem hiding this comment.
Simplify next 3 lines by:
iterator := storetypes.KVStorePrefixIterator(ctx.KVStore(k.storeKey), []byte(types.AccountStateKeyPrefix))
| // GetAllAccountStates | ||
| accountStates, err := s.tApp.App.AccountPlusKeeper.GetAllAccountStates(ctx) | ||
| s.Require().NoError(err, "Should not error when getting all account states") | ||
| s.Require().Equal(len(accountStates), 1, "Incorrect number of AccountStates retrieved") |
There was a problem hiding this comment.
Here and below let's also compare returned object(s)
| Ctx sdk.Context | ||
| } | ||
|
|
||
| func TestUpgradeTestSuite(t *testing.T) { |
There was a problem hiding this comment.
For my understanding did you write these tests based on some existing template? Have we considered using the conventional upgrade containter test (example), which is more end-to-end?
The current test more like a unit test on migrateAccountplusAccountState - no upgrade is actually being run and we are just calling migrateAccountplusAccountState with whitebox set-up and postcheck. Probably more accurate to rename this test to TestMigrateAccountplusAccountState.
There was a problem hiding this comment.
I spoke with Jay on this and we thought unit test should be sufficient for this. I looked at the container e2e tests which is the convention, but I don't think accountplus can be tested in that manner right now because query has not been implemented.
| s.Require().Nil(bzOld, "Unprefixed AccountState should be deleted for %s", addr) | ||
|
|
||
| // Check that the new prefixed key exists | ||
| bzNew := prefixStore.Get(accAddress) |
There was a problem hiding this comment.
Should be relatively easy to not only check the node keys but also the content
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
protocol/app/upgrades/v8.0/migrate_accountplus_test.go (3)
9-10: Consolidate duplicate imports for accountplus typesThere are two imports for the same package with different aliases. Consider using a single import:
- "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types" - accountplustypes "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types" + "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"And update all references to use the same package alias throughout the code.
25-28: Consider adding TearDown methodWhile the setup is correct, consider adding a
TearDownmethod to clean up resources after tests:func (s *UpgradeTestSuite) TearDownTest() { // Clean up any resources if necessary s.tApp = nil }
35-48: Consider using more realistic test dataThe test uses simple address strings. Consider using actual bech32 addresses to better reflect production scenarios:
- addresses := []string{"address1", "address2", "address3"} + addresses := []string{ + "dydx1qqgzqvzq2ps8pqys5zcvp58q7rqrz9c6h2v", + "dydx1qqqfzqvzq2ps8pqys5zcvp58q7rqrz9c6h3", + "dydx1qqpfzqvzq2ps8pqys5zcvp58q7rqrz9c6h4", + }protocol/x/accountplus/keeper/keeper.go (1)
56-74: LGTM: Improved efficiency with prefix iterator.The refactor to use
KVStorePrefixIteratoris a good improvement. The error handling is also more robust with specific error messages.Consider wrapping the iterator error in the returned error for better debugging:
if err != nil { - return nil, fmt.Errorf("failed to unmarshal account state: %w", err) + return nil, fmt.Errorf("failed to unmarshal account state at key %X: %w", iterator.Key(), err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/app/upgrades/v8.0/migrate_accountplus_test.go (1 hunks)
- protocol/x/accountplus/keeper/keeper.go (6 hunks)
- protocol/x/accountplus/keeper/keeper_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/x/accountplus/keeper/keeper_test.go
🧰 Additional context used
🔇 Additional comments (4)
protocol/app/upgrades/v8.0/migrate_accountplus_test.go (1)
14-19: LGTM! Well-structured test suiteThe test suite structure follows testing best practices with proper embedding of
suite.Suiteand necessary test dependencies.protocol/x/accountplus/keeper/keeper.go (3)
8-8: LGTM: Clean implementation of prefix store support and getter methods.The addition of the prefix store import and the implementation of getter methods align well with the PR objective of adding prefix support to the accountplus keeper. The methods follow Go conventions and provide controlled access to private fields.
Also applies to: 40-46
Line range hint
150-161: Skip comment: Function rename was already reviewed.
168-169: LGTM: Consistent implementation of prefix store.The update to use prefix store in both methods is well-implemented and consistent with the module's new design.
Let's verify that all account state accesses use the new prefix:
Also applies to: 191-193
✅ Verification successful
Let me search for any other potential access patterns to ensure completeness of the verification.
This will help us verify that:
- There are no other store operations involving AccountState that might bypass the prefix store
- The AccountStateKeyPrefix is consistently used across the codebase
Based on the search results, I can now generate the final response as we have comprehensive evidence about the AccountState storage access patterns.
Verified: Account state prefix store implementation is correct and consistent
The verification confirms that:
- All account state access in
keeper.goproperly uses the prefix store withAccountStateKeyPrefix- The only other usages of
AccountStateKeyPrefixare in:
- Key definition (
types/keys.go)- Migration tests and upgrade handlers that explicitly check for proper prefix usage
- No direct KVStore access bypassing the prefix store was found for account states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct store access to account states that might bypass the prefix rg -g '*.go' -B 2 -A 2 'KVStore.*AccountState' --no-headingLength of output: 880
Script:
#!/bin/bash # Search for any store operations related to AccountState rg -g '*.go' -B 2 -A 2 'AccountState.*(?:Get|Set|Delete)' --no-heading # Also check for any other usage of AccountStateKeyPrefix rg -g '*.go' -B 2 -A 2 'AccountStateKeyPrefix' --no-headingLength of output: 4323
| func (s *UpgradeTestSuite) TestUpgrade_MigrateAccountplusAccountState() { | ||
| ctx := s.Ctx | ||
| store := ctx.KVStore(s.tApp.App.AccountPlusKeeper.GetStoreKey()) | ||
| prefixStore := prefix.NewStore(store, []byte(accountplustypes.AccountStateKeyPrefix)) | ||
|
|
||
| // Create some AccountState with no prefixes | ||
| addresses := []string{"address1", "address2", "address3"} | ||
| for _, addr := range addresses { | ||
| accAddress := sdk.AccAddress([]byte(addr)) | ||
| accountState := accountplustypes.AccountState{ | ||
| Address: addr, | ||
| TimestampNonceDetails: accountplustypes.TimestampNonceDetails{ | ||
| TimestampNonces: []uint64{1, 2, 3}, | ||
| MaxEjectedNonce: 0, | ||
| }, | ||
| } | ||
| bz := s.tApp.App.AccountPlusKeeper.GetCdc().MustMarshal(&accountState) | ||
| store.Set(accAddress, bz) | ||
| } | ||
|
|
||
| // Verify unprefixed keys were successfully created | ||
| for _, addr := range addresses { | ||
| accAddress := sdk.AccAddress([]byte(addr)) | ||
| bz := store.Get(accAddress) | ||
| s.Require().NotNil(bz, "Unprefixed key not created for %s", addr) | ||
| } | ||
|
|
||
| // Migrate | ||
| migrateAccountplusAccountState(ctx, s.tApp.App.AccountPlusKeeper) | ||
|
|
||
| // Verify that unprefixed keys are deleted and prefixed keys exist | ||
| for _, addr := range addresses { | ||
| accAddress := sdk.AccAddress([]byte(addr)) | ||
|
|
||
| // Check that the old key no longer exists | ||
| bzOld := store.Get(accAddress) | ||
| s.Require().Nil(bzOld, "Unprefixed AccountState should be deleted for %s", addr) | ||
|
|
||
| // Check that the new prefixed key exists | ||
| bzNew := prefixStore.Get(accAddress) | ||
| var actualAccountState types.AccountState | ||
| s.tApp.App.AccountPlusKeeper.GetCdc().MustUnmarshal(bzNew, &actualAccountState) | ||
| expectedAccountState := accountplustypes.AccountState{ | ||
| Address: addr, | ||
| TimestampNonceDetails: accountplustypes.TimestampNonceDetails{ | ||
| TimestampNonces: []uint64{1, 2, 3}, | ||
| MaxEjectedNonce: 0, | ||
| }, | ||
| } | ||
| s.Require().NotNil(bzNew, "Prefixed AccountState should exist for %s", addr) | ||
| s.Require().Equal(expectedAccountState, actualAccountState, "Incorrect AccountState after migration for %s", addr) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the implementation found, I can now provide a final response.
Additional test cases needed with specific focus on error handling
The migration function contains critical error paths that aren't currently tested:
- It panics when encountering keys with unexpected prefixes
- It panics on unmarshaling errors for invalid account states
The review suggestion for additional test cases is valid, but should specifically include:
- Test for panic on prefixed keys:
func (s *UpgradeTestSuite) TestUpgrade_MigrateAccountplusAccountState_PrefixedKeyPanic() {
// Create an account state with the prefix that should trigger panic
}- Test for panic on invalid state:
func (s *UpgradeTestSuite) TestUpgrade_MigrateAccountplusAccountState_InvalidStatePanic() {
// Store corrupted/invalid bytes that will fail unmarshaling
}- Empty state and idempotency tests as originally suggested
The implementation shows these are critical scenarios that should be verified through tests.
🔗 Analysis chain
Enhance test coverage with edge cases
The test covers the happy path but should also verify:
- Empty account states
- Invalid account states
- Migration idempotency (running migration multiple times)
- Large number of accounts to verify performance
Consider adding these test cases:
func (s *UpgradeTestSuite) TestUpgrade_MigrateAccountplusAccountState_Empty() {
// Test migration with no existing account states
migrateAccountplusAccountState(s.Ctx, s.tApp.App.AccountPlusKeeper)
// Verify no errors occur
}
func (s *UpgradeTestSuite) TestUpgrade_MigrateAccountplusAccountState_Idempotent() {
// Run original test setup
s.TestUpgrade_MigrateAccountplusAccountState()
// Run migration again
migrateAccountplusAccountState(s.Ctx, s.tApp.App.AccountPlusKeeper)
// Verify state remains correct
}Let me gather more information about the test coverage and error handling in the codebase.
Let me check the implementation of the migration function to understand the error handling better.
Let me try one final check to locate the migration function implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing error handling tests
rg -l "TestUpgrade.*Error" --type go
Length of output: 38
Script:
#!/bin/bash
# Check the implementation of migrateAccountplusAccountState to understand error handling
ast-grep --pattern 'func migrateAccountplusAccountState'
# Look for any existing test files related to accountplus migration
fd "migrate.*test.go"
# Check for any error handling patterns in the test file
rg "s\.Require\(\)" protocol/app/upgrades/v8.0/migrate_accountplus_test.go
Length of output: 561
Script:
#!/bin/bash
# Get the implementation of migrateAccountplusAccountState
rg -A 20 "func migrateAccountplusAccountState" protocol/app/upgrades/v8.0/migrate_accountplus.go
# Check if there are any other test files in v8.0 that might have error handling patterns
fd ".*test.go" protocol/app/upgrades/v8.0/
# Look for any existing test setup patterns
rg "func.*TestUpgrade" protocol/app/upgrades/v8.0/migrate_accountplus_test.go -A 5
Length of output: 661
Script:
#!/bin/bash
# Search for the migration function in the entire codebase
rg -A 20 "func migrateAccountplusAccountState"
# Check if there are any similar migration functions in v8.0
rg "func migrate" protocol/app/upgrades/v8.0/
# Look for the AccountState type definition to understand the data structure
ast-grep --pattern 'type AccountState struct'
Length of output: 1960
| } | ||
|
|
||
| // SetAccountState stores with prefix | ||
| k.SetAccountState(ctx, key, accountState) |
There was a problem hiding this comment.
Does this break the contract of iterators?
// Iterator over a domain of keys in ascending order. End is exclusive.
// Start must be less than end, or the Iterator is invalid.
// Iterator must be closed by caller.
// To iterate over entire domain, use store.Iterator(nil, nil)
// CONTRACT: No writes may happen within a domain while an iterator exists over it.
// Exceptionally allowed for cachekv.Store, safe to write in the modules.
Iterator(start, end []byte) Iterator
e.g. is it safe to modify state while having an open iterator?
There was a problem hiding this comment.
To be safe, I will collect the items and set and remove outside of iteration
8d8231a to
3d9b80a
Compare
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
Release Notes
New Features
Bug Fixes
Tests