feat!: migrate x/group to store service #19410
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughWalkthroughThese updates focus on improving the codebase by streamlining variable declarations, loop iterations, and store initialization. A significant change is the migration to a new Store Service, impacting various components like 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -453,7 +451,7 @@ | |||
| return errorsmod.Wrap(err, "doTallyAndUpdate") | |||
| } | |||
|
|
|||
| if err := k.proposalTable.Update(ctx.KVStore(k.key), proposal.Id, &proposal); err != nil { | |||
| if err := k.proposalTable.Update(k.storeService.OpenKVStore(ctx), proposal.Id, &proposal); err != nil { | |||
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop. Error
| @@ -351,7 +349,7 @@ | |||
|
|
|||
| //nolint:gosec // "implicit memory aliasing in the for loop (because of the pointer on &v)" | |||
| for _, v := range votes { | |||
| err = k.voteTable.Delete(ctx.KVStore(k.key), &v) | |||
| err = k.voteTable.Delete(k.storeService.OpenKVStore(ctx), &v) | |||
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop. Error
| @@ -310,7 +308,7 @@ | |||
| if proposalInfo.Status == group.PROPOSAL_STATUS_SUBMITTED { | |||
| proposalInfo.Status = group.PROPOSAL_STATUS_ABORTED | |||
|
|
|||
| if err := k.proposalTable.Update(ctx.KVStore(k.key), proposalInfo.Id, &proposalInfo); err != nil { | |||
| if err := k.proposalTable.Update(k.storeService.OpenKVStore(ctx), proposalInfo.Id, &proposalInfo); err != nil { | |||
Check failure
Code scanning / gosec
Implicit memory aliasing in for loop. Error
facundomedica
changed the title
![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: 39
Configuration used: .coderabbit.yml
Files selected for processing (38)
- math/int_test.go (2 hunks)
- simapp/app.go (1 hunks)
- x/group/CHANGELOG.md (1 hunks)
- x/group/internal/orm/auto_uint64.go (1 hunks)
- x/group/internal/orm/auto_uint64_test.go (3 hunks)
- x/group/internal/orm/genesis.go (1 hunks)
- x/group/internal/orm/genesis_test.go (2 hunks)
- x/group/internal/orm/index.go (7 hunks)
- x/group/internal/orm/index_test.go (4 hunks)
- x/group/internal/orm/indexer.go (5 hunks)
- x/group/internal/orm/indexer_test.go (10 hunks)
- x/group/internal/orm/iterator_test.go (2 hunks)
- x/group/internal/orm/orm_scenario_test.go (9 hunks)
- x/group/internal/orm/prefixstore/prefixstore.go (1 hunks)
- x/group/internal/orm/primary_key.go (1 hunks)
- x/group/internal/orm/primary_key_property_test.go (1 hunks)
- x/group/internal/orm/primary_key_test.go (4 hunks)
- x/group/internal/orm/sequence.go (3 hunks)
- x/group/internal/orm/sequence_property_test.go (1 hunks)
- x/group/internal/orm/sequence_test.go (2 hunks)
- x/group/internal/orm/table.go (12 hunks)
- x/group/internal/orm/table_test.go (4 hunks)
- x/group/internal/orm/types.go (2 hunks)
- x/group/internal/orm/types_test.go (2 hunks)
- x/group/keeper/genesis.go (2 hunks)
- x/group/keeper/genesis_test.go (3 hunks)
- x/group/keeper/grpc_query.go (10 hunks)
- x/group/keeper/grpc_query_test.go (3 hunks)
- x/group/keeper/invariants.go (3 hunks)
- x/group/keeper/invariants_test.go (2 hunks)
- x/group/keeper/keeper.go (10 hunks)
- x/group/keeper/keeper_test.go (3 hunks)
- x/group/keeper/migrations.go (1 hunks)
- x/group/keeper/msg_server.go (28 hunks)
- x/group/keeper/tally.go (2 hunks)
- x/group/migrations/v2/migrate.go (2 hunks)
- x/group/migrations/v2/migrate_test.go (3 hunks)
- x/group/module/depinject.go (3 hunks)
Additional comments: 111
x/group/keeper/migrations.go (1)
- 23-23: Parameter
m.keeper.storeServiceis correctly updated to align with the new store service migration. This change is consistent with the PR's objective to migrate to a new store service.x/group/internal/orm/genesis.go (1)
- 3-3: The import path update from
"cosmossdk.io/store/types"to"cosmossdk.io/core/store"is correct and aligns with the new store service integration.x/group/CHANGELOG.md (1)
- 37-37: The changelog entry for the migration to Store Service is correctly added under the API Breaking Changes section, providing clear documentation of the change.
x/group/internal/orm/sequence_test.go (1)
- 17-19: The update to use
runtime.NewKVStoreServicefor store initialization in tests is correct and aligns with the migration to the new store service.x/group/internal/orm/genesis_test.go (1)
- 24-26: The update to use
runtime.NewKVStoreServiceandtestutil.DefaultContextWithDBfor store and context setup in tests is correct and aligns with the migration to the new store service.x/group/module/depinject.go (2)
- 33-33: Replacing
store.KVStoreKeywithstore.KVStoreServicein theGroupInputsstruct is correct and aligns with the migration to the new store service.- 49-49: The update in the
ProvideModulefunction to usein.StoreServiceinstead of a store key is correct and aligns with the migration to the new store service.x/group/keeper/tally.go (1)
- 25-25: Using
storeServiceto open a KV store instead of directly accessingctx.KVStore(k.key)is correct and aligns with the migration to the new store service.x/group/internal/orm/sequence_property_test.go (1)
- 16-21: The update to use
testutil.DefaultContextWithDBandruntime.NewKVStoreServicefor context and store setup in tests is correct and aligns with the migration to the new store service.x/group/internal/orm/types_test.go (2)
- 13-13: Importing
prefixstorefromcosmossdk.io/x/group/internal/ormis correct and aligns with the migration to the new store service.- 24-28: The update to use
prefixstore.Newinstead ofprefix.NewStoreand the adjustments in store operations within the test function are correct and align with the migration to the new store service.x/group/keeper/genesis.go (1)
- 20-20: Replacing usages of
sdkCtx.KVStore(k.key)withstorefor various table imports and exports within theInitGenesisandExportGenesisfunctions is correct and aligns with the migration to the new store service.x/group/migrations/v2/migrate.go (3)
- 7-7: Updating the import path for the
storepackage is correct and aligns with the migration to the new store service.- 28-28: Changing the
storeKeyparameter tostoreServiceof typestore.KVStoreServiceis correct and aligns with the migration to the new store service.- 33-33: Updating the store initialization to use
storeService.OpenKVStore(ctx)is correct and aligns with the migration to the new store service.x/group/internal/orm/sequence.go (2)
- 6-9: Refactoring the storage handling by introducing a new
prefixstorepackage and updating method calls accordingly is correct and aligns with the migration to the new store service.- 28-38: Adjusting error handling in the
NextValmethod to panic on errors is a significant change. Ensure that this behavior is intentional and documented, as it could affect the application's stability.x/group/keeper/invariants.go (2)
- 10-10: Updating the usage of
storetypesfrom "cosmossdk.io/store/types" to "cosmossdk.io/core/store" is correct and aligns with the migration to the new store service.- 29-29: Modifying the handling of store operations within the
GroupTotalWeightInvariantHelperfunction by introducingstoreServicefor interacting with the KVStore is correct and aligns with the migration to the new store service.x/group/keeper/invariants_test.go (2)
- 19-19: Introducing a new import for
runtimeis correct and aligns with the migration to the new store service.- 135-137: Utilizing
runtime.NewKVStoreServiceto create a key-value store service in tests is correct and aligns with the migration to the new store service.x/group/migrations/v2/migrate_test.go (3)
- 38-38: Ensure
runtime.NewKVStoreService(storeKey)is correctly initializingstoreServicewith the intended store key. This is crucial for the migration logic to function properly.- 43-43: The function
createGroupPoliciesnow correctly acceptscorestore.KVStoreServiceas an argument, aligning with the migration objectives. Verify that all calls to this function have been updated accordingly.- 46-46: The
v2.Migratefunction's signature update to acceptruntime.KVStoreServiceis in line with the migration plan. Ensure all invocations ofMigrateare updated to pass the correct type.x/group/internal/orm/auto_uint64_test.go (1)
- 29-31: The store initialization with
runtime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx)correctly reflects the migration to the new store service. Ensure the test context is appropriately set up for all tests.x/group/internal/orm/auto_uint64.go (1)
- 6-6: The update to the import path for
storetypesaligns with the migration to the new store service. Verify that all usages ofstoretypesin this file are compatible with the changes.x/group/internal/orm/primary_key_property_test.go (1)
- 19-152: The refactoring of
TestPrimaryKeyTableusing a closure forrapid.Checkimproves test encapsulation and readability. Ensure that the initialization of context, store, and table, as well as the model commands within the closure, are correctly implemented and maintain the intended test coverage.x/group/internal/orm/types.go (1)
- 119-124: The update to
NewTypeSafeRowGetterto useprefixstoreand handle errors duringGetoperations is a significant improvement. Ensure that the error handling is correctly implemented and that it aligns with the intended functionality.x/group/internal/orm/prefixstore/prefixstore.go (1)
- 1-223: The introduction of the
prefixstorepackage and its adaptation to thecosmossdk.io/core/store.KVStoreinterface is correctly implemented. Ensure that the key prefixing logic,KVStoremethods, and iterator functionality are thoroughly tested to confirm compatibility and correctness.x/group/internal/orm/indexer.go (4)
- 4-4: The import path for
storetypeshas been correctly updated to reflect the new store service structure.- 145-145: The use of
store.SetinuniqueKeysAddFuncis correct and does not require error handling as it does not return an error.- 150-153: Error handling in
checkUniqueIndexKeyforstore.Iteratoris correctly implemented, ensuring robustness in index key uniqueness checks.- 179-179: The use of
store.SetinmultiKeyAddFuncis correct and does not require error handling as it does not return an error.x/group/internal/orm/primary_key.go (1)
- 6-6: The import path for
storetypeshas been correctly updated to"cosmossdk.io/core/store", aligning with the new store service structure.x/group/internal/orm/primary_key_test.go (1)
- 9-17: The introduction of new imports related to
cosmos-sdkandcosmossdk.iois correctly done to support the updated test setup.x/group/keeper/genesis_test.go (2)
- 25-25: The introduction of the
runtimeimport is correctly done to support the new store service initialization in the test setup.- 76-76: Modification of
keeper.NewKeeperto passstoreServiceinstead ofkeyis correctly implemented, aligning with the new store service migration.x/group/internal/orm/index.go (9)
- 19-21: Update function signatures to use
storetypes.KVStorealigns with the migration requirements.- 76-86: Correct use of
storetypes.KVStoreandprefixstore.Newfor store initialization and iterator handling.- 92-102: Correct use of
storetypes.KVStoreandprefixstore.Newfor store initialization and iterator handling inGetfunction.- 121-131: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [109-128]
Correct use of
storetypes.KVStoreandprefixstore.Newfor paginated store access. Proper handling ofpageRequest.Keyfor pagination logic.
- 149-160: Correct use of
storetypes.KVStoreandprefixstore.Newfor prefix scanning. Proper error handling and iterator usage.- 173-184: Correct use of
storetypes.KVStoreandprefixstore.Newfor reverse prefix scanning. Proper error handling and iterator usage.- 223-232: Correct use of
storetypes.KVStoreandprefixstore.NewinonSetmethod. Proper handling ofoldValuefor update scenarios.- 231-232: Correct use of
storetypes.KVStoreandprefixstore.NewinonDeletemethod. Ensures proper deletion logic.- 257-257: Correct use of
storetypes.KVStoreinindexIteratorstruct. Ensures compatibility with the new store service.x/group/internal/orm/iterator_test.go (1)
- 214-216: Correct update to test setup using
storetypes.NewKVStoreKey,testutil.DefaultContextWithDB, andruntime.NewKVStoreServicefor KV store initialization. Aligns with the new store service integration.x/group/internal/orm/table.go (7)
- 128-138: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [78-135]
Correct implementation of
Create,Update, andSetmethods usingstoretypes.KVStoreandprefixstore.New. Ensures compatibility with the new store service and proper error handling.
- 159-169: Correct implementation of
Deletemethod usingstoretypes.KVStoreandprefixstore.New. Proper error handling and interceptor invocation.- 181-190: Correct use of
storetypes.KVStoreandprefixstore.NewinHasmethod. Ensures proper key existence check.- 218-238: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [196-231]
Correct implementation of
GetOne,PrefixScan, andReversePrefixScanmethods usingstoretypes.KVStoreandprefixstore.New. Ensures compatibility with the new store service and proper iterator handling.
- 245-270: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [267-281]
Correct approach in
ExportandImportmethods usingstoretypes.KVStore. Proper handling of data export and import logic.
- 311-316: Correct use of
storetypes.KVStoreandprefixstore.Newinkeysmethod for fetching all keys. Proper iterator handling and error management.- 328-328: Correct use of
storetypes.KVStoreintypeSafeIteratorstruct. Ensures compatibility with the new store service.x/group/keeper/grpc_query_test.go (3)
- 22-22: The import of the
runtimepackage is correctly added to support the new store service functionality.- 46-46: Initialization of
storeServiceusingruntime.NewKVStoreService(key)is correctly implemented to align with the migration to the new store service.- 72-72: Correct use of
storeServiceingroupkeeper.NewKeeperto replace previoussdkCtx.KVStore(k.key)andstoretypes.StoreKeyusages.x/group/internal/orm/index_test.go (5)
- 9-9: The addition of new imports for
cosmossdk.io/core/store,cosmossdk.io/errors, andcosmossdk.io/store/typesis appropriate for the updated test functionalities.- 16-17: The import of the
runtimepackage andtestutilis correctly added to support the new store service functionality in tests.- 107-109: Correct initialization of
storeusingruntime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx)for store operations in tests.- 137-137: Proper use of
storeas a parameter inmethodfunction calls within test cases, aligning with the new store service's requirements.- 308-310: Correct initialization of
storeusingruntime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx)for unique index tests.x/group/keeper/grpc_query.go (13)
- 36-36: Correct usage of
storeService.OpenKVStore(ctx)ingetGroupInfofunction to access the store.- 59-59: Proper use of
storeService.OpenKVStore(ctx)ingetGroupPolicyInfofunction for store operations.- 85-85: Correct implementation of
storeService.OpenKVStore(ctx)ingetGroupMembersfunction to retrieve group members.- 114-114: Appropriate use of
storeService.OpenKVStore(ctx)ingetGroupsByAdminfunction for querying groups by admin.- 140-140: Proper usage of
storeService.OpenKVStore(ctx)ingetGroupPoliciesByGroupfunction for querying group policies by group.- 170-170: Correct implementation of
storeService.OpenKVStore(ctx)ingetGroupPoliciesByAdminfunction for querying group policies by admin.- 211-211: Appropriate use of
storeService.OpenKVStore(ctx)ingetProposalsByGroupPolicyfunction for querying proposals by group policy.- 217-217: Correct usage of
storeService.OpenKVStore(ctx)ingetProposalfunction to retrieve proposal information.- 326-326: Proper implementation of
storeService.OpenKVStore(ctx)ingetVotefunction for retrieving vote information.- 331-331: Correct use of
storeService.OpenKVStore(ctx)ingetVotesByProposalfunction for querying votes by proposal.- 336-336: Appropriate implementation of
storeService.OpenKVStore(ctx)ingetVotesByVoterfunction for querying votes by voter.- 372-372: Correct usage of
storeService.OpenKVStore(ctx)inGroupsfunction for querying all groups.- 297-297: Proper use of
storeService.OpenKVStore(ctx)inGroupsByMemberfunction for querying groups by member.x/group/internal/orm/orm_scenario_test.go (7)
- 12-19: Imports
cosmos-sdk/runtimeandcosmos-sdk/testutilare added to replace deprecated context and store initialization methods with the new store service approach. This aligns with the PR objectives.- 30-32: The usage of
testutil.DefaultContextWithDBandruntime.NewKVStoreServiceto open KV stores is correct and aligns with the new store service integration. This change ensures compatibility withserver/v2.- 109-111: Repeating the pattern of using
testutil.DefaultContextWithDBandruntime.NewKVStoreServicefor another test function. Consistency in test setup is maintained across the file.- 196-198: Again, consistent use of
testutil.DefaultContextWithDBandruntime.NewKVStoreServicefor setting up the test context and store. This consistency is good for maintainability.- 294-296: The same pattern of using
testutil.DefaultContextWithDBandruntime.NewKVStoreServiceis applied here, ensuring that all tests are updated to use the new store service.- 356-358: The consistent application of the new store service setup across all test functions in this file is noted. This ensures that all tests are aligned with the new store service integration.
- 406-406: The function signature of
assertIndexis updated to usecorestore.KVStoreinstead of the previous store type. This change is necessary for compatibility with the new store service and follows best practices for type usage.x/group/keeper/keeper.go (11)
- 49-49: The
storeServicefield is correctly added to theKeeperstruct to replace the old store key withcorestoretypes.KVStoreService. This change is necessary for the migration to the new store service.- 86-91: The
NewKeeperfunction is correctly updated to acceptcorestoretypes.KVStoreServiceas a parameter and initialize theKeeperstruct with it. This change aligns with the migration objectives.- 245-245: The
GetGroupSequencefunction correctly usesk.storeService.OpenKVStore(ctx)to access the store, aligning with the new store service approach.- 250-250: The
GetGroupPolicySeqfunction is updated to use the new store service for accessing the store, which is consistent with the migration to the new store service.- 256-256: In
proposalsByVPEnd, the use ofk.storeService.OpenKVStore(ctx)for store access is correct and aligns with the new store service integration.- 288-288: The
pruneProposalfunction correctly uses the new store service to access and modify the store, which is in line with the migration objectives.- 311-311: The
abortProposalsfunction correctly updates proposals using the new store service. The existing comment about implicit memory aliasing is noted but not directly related to the migration changes.- 321-321: The
proposalsByGroupPolicyfunction's use of the new store service for accessing the store is correct and aligns with the migration to the new store service.- 352-352: In
pruneVotes, the deletion of votes using the new store service is correctly implemented. The existing comment about implicit memory aliasing is noted but not directly related to the migration changes.- 363-363: The
votesByProposalfunction's use of the new store service for accessing the store is correct and aligns with the migration to the new store service.- 454-454: The
TallyProposalsAtVPEndfunction correctly updates proposals using the new store service. The existing comment about implicit memory aliasing is noted but not directly related to the migration changes.x/group/internal/orm/indexer_test.go (11)
- 11-18: Ensure the newly introduced imports are used effectively within the test cases, particularly
corestore,prefixstore, andtestutil. These changes are crucial for aligning the tests with the updated module structure.- 170-172: The store initialization using
testutil.DefaultContextWithDBandruntime.NewKVStoreServicefollowed byprefixstore.Newis correctly updated to reflect the new store service integration. This pattern is consistent with the migration objectives.- 231-233: The use of
store.Haswithin theTestIndexerOnDeletetest case correctly checks for the existence of keys before deletion. This ensures the test accurately reflects the expected behavior of theIndexeron delete operations.- 244-246: The verification of key absence using
store.Hasafter theOnDeleteoperation is executed correctly, ensuring the test validates the deletion logic within theIndexer.- 256-258: Initialization of the store for
TestIndexerOnUpdateusingtestutil.DefaultContextWithDBandruntime.NewKVStoreServicefollowed byprefixstore.Newis consistent with the updated module structure and store service integration.- 345-345: The custom
addFuncprovided in theTestIndexerOnUpdatefor simulating an error case on persisting new keys demonstrates a good testing practice by covering error handling scenarios.- 371-378: Assertions within
TestIndexerOnUpdatefor checking the presence and absence of keys after the update operation are correctly implemented, ensuring the test accurately reflects the expected behavior of theIndexer.- 413-416: Store initialization in
TestUniqueKeyAddFuncusingruntime.NewKVStoreServiceandtestutil.DefaultContextWithDBis correctly updated to reflect the new store service integration, aligning with the migration objectives.- 423-426: The assertion for the existence of the expected entry after calling
uniqueKeysAddFuncinTestUniqueKeyAddFunccorrectly verifies the functionality of adding unique keys to the store.- 460-463: Initialization of the store in
TestMultiKeyAddFuncusingruntime.NewKVStoreServiceandtestutil.DefaultContextWithDBis correctly implemented, reflecting the integration of the new store service.- 470-473: The assertion for the existence of the expected entry after calling
multiKeyAddFuncinTestMultiKeyAddFunccorrectly verifies the functionality of adding keys to the store, ensuring the test accurately reflects the expected behavior.x/group/keeper/keeper_test.go (3)
- 26-26: The introduction of the
runtimepackage is correctly used for creating a newstoreServiceinstance, aligning with the migration to the new store service.- 53-53: The creation of
storeServiceusingruntime.NewKVStoreServiceis correctly implemented, reflecting the updated approach to store management in thekeepertests.- 79-79: The update to the
keeper.NewKeepercall to includestoreServiceas a parameter is correctly implemented, ensuring thekeeperis initialized with the new store service.math/int_test.go (2)
- 692-692: Use shorthand variable declaration for
intsto improve readability and align with Go idiomatic practices.- 702-702: Simplify loop iteration by removing unnecessary variable declaration. This change improves readability and is more idiomatic Go.
simapp/app.go (1)
- 369-369: The initialization of
app.GroupKeeperusingruntime.NewKVStoreServicecorrectly aligns with the objective of migrating to a new store service. Ensure that thegroupConfigpassed togroupkeeper.NewKeeperis properly configured and that all necessary parameters are correctly set for the new store service.x/group/keeper/msg_server.go (1)
- 139-139: The switch statement for error handling is correct, but ensure consistency in error handling patterns across the codebase for readability and maintainability.
| @@ -62,22 +62,23 @@ func (k Keeper) CreateGroup(goCtx context.Context, msg *group.MsgCreateGroup) (* | |||
|
|
|||
| // Create a new group in the groupTable. | |||
| ctx := sdk.UnwrapSDKContext(goCtx) | |||
| kvStore := k.storeService.OpenKVStore(goCtx) | |||
There was a problem hiding this comment.
Introduce error handling for OpenKVStore to ensure robustness in case the store cannot be opened.
| groupInfo := &group.GroupInfo{ | ||
| Id: k.groupTable.Sequence().PeekNextVal(ctx.KVStore(k.key)), | ||
| Id: k.groupTable.Sequence().PeekNextVal(kvStore), |
There was a problem hiding this comment.
The Id field assignment using PeekNextVal without incrementing the sequence might lead to ID collisions or misuse. Consider using a method that both retrieves and increments the sequence value.
| Admin: msg.Admin, | ||
| Metadata: msg.Metadata, | ||
| Version: 1, | ||
| TotalWeight: totalWeight.String(), | ||
| CreatedAt: ctx.HeaderInfo().Time, | ||
| } | ||
| groupID, err := k.groupTable.Create(ctx.KVStore(k.key), groupInfo) | ||
| groupID, err := k.groupTable.Create(kvStore, groupInfo) |
There was a problem hiding this comment.
Ensure that Create method calls that modify state are followed by appropriate error handling to maintain data integrity in case of failures.
| if err != nil { | ||
| return nil, errorsmod.Wrap(err, "could not create group") | ||
| } | ||
|
|
||
| // Create new group members in the groupMemberTable. | ||
| for i, m := range msg.Members { | ||
| err := k.groupMemberTable.Create(ctx.KVStore(k.key), &group.GroupMember{ | ||
| err := k.groupMemberTable.Create(kvStore, &group.GroupMember{ |
There was a problem hiding this comment.
Similar to previous comments, add error handling after Create method calls for robustness.
| @@ -112,6 +113,7 @@ | |||
| } | |||
|
|
|||
| ctx := sdk.UnwrapSDKContext(goCtx) | |||
| kvStore := k.storeService.OpenKVStore(goCtx) | |||
There was a problem hiding this comment.
Missing error handling for OpenKVStore here as well. This is crucial for ensuring the application does not proceed with an invalid store reference.
Comment on lines
+84
to
+86
| if err := store.Delete(indexKey); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Error handling in OnDelete function should not attempt to handle errors from store.Delete as it does not return an error. Remove the error handling logic.
- if err := store.Delete(indexKey); err != nil {
- return err
- }
+ store.Delete(indexKey)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.
Suggested change
| if err := store.Delete(indexKey); err != nil { | |
| return err | |
| } | |
| store.Delete(indexKey) |
Comment on lines
+110
to
+112
| if err := store.Delete(indexKey); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Repeat of the previous comment regarding unnecessary error handling in OnUpdate function. store.Delete does not return an error.
- if err := store.Delete(indexKey); err != nil {
- return err
- }
+ store.Delete(indexKey)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.
Suggested change
| if err := store.Delete(indexKey); err != nil { | |
| return err | |
| } | |
| store.Delete(indexKey) |
Comment on lines
+28
to
+30
| key := storetypes.NewKVStoreKey("test") | ||
| testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) | ||
| store := runtime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx) |
There was a problem hiding this comment.
Replace direct storetypes.NewKVStoreKey and manual store initialization with runtime.NewKVStoreService to align with the new store service migration. Ensure testutil.DefaultContextWithDB is correctly used for context setup in TestPrimaryKeyTablePrefixScan.
- key := storetypes.NewKVStoreKey("test")
- testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test"))
- store := runtime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx)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.
Suggested change
| key := storetypes.NewKVStoreKey("test") | |
| testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) | |
| store := runtime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx) |
Comment on lines
+215
to
+217
| key := storetypes.NewKVStoreKey("test") | ||
| testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) | ||
| store := runtime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx) |
There was a problem hiding this comment.
Repeat of the previous comment regarding store and context initialization in TestContains. Ensure consistency in the implementation across all test functions.
- key := storetypes.NewKVStoreKey("test")
- testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test"))
- store := runtime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx)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.
Suggested change
| key := storetypes.NewKVStoreKey("test") | |
| testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) | |
| store := runtime.NewKVStoreService(key).OpenKVStore(testCtx.Ctx) |
| @@ -49,6 +50,7 @@ var ( | |||
|
|
|||
| func (s *GenesisTestSuite) SetupTest() { | |||
| key := storetypes.NewKVStoreKey(group.StoreKey) | |||
| storeService := runtime.NewKVStoreService(key) | |||
There was a problem hiding this comment.
Replace direct storetypes.NewKVStoreKey and manual store initialization with runtime.NewKVStoreService to align with the new store service migration. Ensure the correct setup for the genesis functionality tests.
- key := storetypes.NewKVStoreKey(group.StoreKey)
- storeService := runtime.NewKVStoreService(key)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.
Suggested change
| storeService := runtime.NewKVStoreService(key) |
tac0turtle
approved these changes
Feb 13, 2024
12 tasks
This was referenced Apr 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Migrated x/group to store service to make it fully compatible with the upcoming server/v2
closes #19382
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
prefixstorepackage for handling key-value pairs efficiently.GroupKeeperfor better performance.