refactor(storev2): change error namespace #19698
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
WalkthroughWalkthroughThe recent update involves a significant restructuring of error handling mechanisms across various components within the system. This includes standardizing error references, introducing new error packages, updating error codes, and enhancing error messages for improved clarity and consistency throughout the system. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (10)
- store/db/goleveldb.go (12 hunks)
- store/db/memdb.go (11 hunks)
- store/db/prefixdb.go (8 hunks)
- store/errors/errors.go (2 hunks)
- store/snapshots/chunk.go (3 hunks)
- store/snapshots/manager.go (10 hunks)
- store/snapshots/store.go (5 hunks)
- store/storage/pebbledb/db.go (5 hunks)
- store/storage/rocksdb/db.go (4 hunks)
- store/storage/sqlite/db.go (3 hunks)
Additional comments: 68
store/errors/errors.go (2)
- 10-10: The update to the
StoreCodespaceconstant to "storev2" is appropriate and aligns with the PR's objective to refactor the error namespace within thestore/v2module.- 13-14: The addition of
ErrInvalidProofwith code 2 is correctly implemented and well-documented. This enhances error specificity and handling within the module.store/snapshots/chunk.go (3)
- 8-8: The update to the import path for errors to
cosmossdk.io/store/v2/errorsis correct, ensuring that the new error namespace is utilized throughout the file.- 75-75: The error wrapping in
ChunkWriterusingstoreerrors.ErrLogicfor attempting to write to a closed writer is appropriately handled and clearly communicates the error condition.- 177-177: The error wrapping in
ValidRestoreHeightusingstoreerrors.ErrLogicfor an invalid height (0) is correctly implemented, providing a clear and specific error message.store/storage/rocksdb/db.go (3)
- 17-17: The update to import
cosmossdk.io/store/v2/errorsfor error handling is correct, ensuring that the new error namespace is utilized throughout the file.- 103-103: The use of
errors.ErrVersionPrunedingetSlicefor handling attempts to access pruned versions is appropriately implemented, providing a clear and specific error message.- 170-170: The error handling in iterators using
errors.ErrKeyEmptyanderrors.ErrStartAfterEndis correctly implemented, providing clear messages for invalid iterator conditions.Also applies to: 174-174, 186-186, 190-190
store/db/prefixdb.go (2)
- 10-10: The update to import
cosmossdk.io/store/v2/errorsfor error handling is correct, ensuring that the new error namespace is utilized throughout the file.- 33-33: The replacement of error references with those from the
cosmossdk.io/store/v2/errorspackage across various methods in thePrefixDBstruct is correctly implemented, providing clear and specific error messages for invalid conditions.Also applies to: 47-47, 61-61, 82-82, 281-281, 284-284, 293-293, 317-317
store/storage/sqlite/db.go (3)
- 15-15: The update to import
storeerrorsfromcosmossdk.io/store/v2/errorsfor error handling is correct, ensuring that the new error namespace is utilized throughout the file.- 142-142: The use of
storeerrors.ErrVersionPrunedinGetfor handling attempts to access pruned versions is appropriately implemented, providing a clear and specific error message.- 221-221: The error handling in iterators using
storeerrors.ErrKeyEmptyandstoreerrors.ErrStartAfterEndis correctly implemented, providing clear messages for invalid iterator conditions.Also applies to: 225-225, 233-233, 237-237
store/db/goleveldb.go (12)
- 10-10: The import alias
dberrorsfor the packagegitpro.ttaallkk.top/syndtr/goleveldb/leveldb/errorsis correctly introduced to distinguish it from the newly importedcosmossdk.io/store/v2/errorspackage. This change improves readability and avoids potential naming conflicts.- 18-18: The import of the new error package
cosmossdk.io/store/v2/errorsis correctly added to use the updated error handling mechanism within the Cosmos SDK'sstore/v2module. This aligns with the PR's objective to refactor the error namespace.- 58-58: The use of
errors.ErrKeyEmptyfrom thecosmossdk.io/store/v2/errorspackage for handling empty keys is appropriate and aligns with the Cosmos SDK's error handling conventions. This ensures consistency across the SDK.- 62-62: The comparison
err == dberrors.ErrNotFoundcorrectly utilizes thedberrorsalias to distinguish between the leveldb error and the Cosmos SDK's store errors. This ensures that the specific leveldb not found error is correctly handled.- 82-82: The use of
errors.ErrKeyEmptyfor handling empty keys in theSetmethod is consistent with the SDK's error handling strategy. This change ensures that the error handling is uniform across different database implementations.- 85-85: The introduction of
errors.ErrValueNilfor handling nil values in theSetmethod is a good practice. It ensures that the method's preconditions are explicitly checked, improving the robustness of the database interface.- 96-96: Repeating the use of
errors.ErrKeyEmptyin theSetSyncmethod for empty key validation maintains consistency in error handling across synchronous and asynchronous write operations.- 99-99: The use of
errors.ErrValueNilin theSetSyncmethod for nil value checks is consistent with theSetmethod, ensuring uniform error handling for both synchronous and asynchronous writes.- 110-110: Applying
errors.ErrKeyEmptyin theDeletemethod for empty key validation aligns with the error handling strategy used in other methods, ensuring a consistent approach across the database interface.- 121-121: The use of
errors.ErrKeyEmptyin theDeleteSyncmethod for empty key checks is appropriate and consistent with theDeletemethod, maintaining uniform error handling for deletion operations.- 199-199: The introduction of
errors.ErrKeyEmptyfor handling empty keys in theIteratormethod is consistent with the error handling strategy used in other methods, ensuring uniformity across the database interface.- 208-208: The use of
errors.ErrKeyEmptyin theReverseIteratormethod for empty key validation is appropriate and maintains consistency in error handling across iterator operations.store/storage/pebbledb/db.go (8)
- 15-15: The introduction of the
storeerrorsalias for thecosmossdk.io/store/v2/errorspackage is correctly done to distinguish it from other error packages. This change is necessary for using the updated error handling mechanism within the Cosmos SDK'sstore/v2module.- 152-152: The use of
storeerrors.ErrVersionPrunedfor handling attempts to access pruned versions is appropriate and aligns with the Cosmos SDK's error handling conventions. This ensures that the error is descriptive and specific to the version pruning functionality.- 157-157: The check for
storeerrors.ErrRecordNotFoundusingerrors.Isis correctly implemented. This approach is idiomatic in Go for handling wrapped errors and ensures that the specific record not found error is correctly identified and handled.- 272-272: The use of
storeerrors.ErrKeyEmptyfor handling empty keys in theIteratormethod is consistent with the SDK's error handling strategy. This change ensures that the error handling is uniform across different database implementations.- 276-276: The introduction of
storeerrors.ErrStartAfterEndfor handling cases where the start key is after the end key in range queries is a good practice. It ensures that the method's preconditions are explicitly checked, improving the robustness of the database interface.- 296-296: Repeating the use of
storeerrors.ErrKeyEmptyin theReverseIteratormethod for empty key validation maintains consistency in error handling across iterator operations.- 300-300: The use of
storeerrors.ErrStartAfterEndin theReverseIteratormethod for handling invalid range queries is appropriate and consistent with theIteratormethod, ensuring uniform error handling for both forward and reverse iterations.- 382-382: The check for
storeerrors.ErrRecordNotFoundin thegetMVCCSlicefunction is correctly implemented. This ensures that attempts to access non-existent records are handled with a specific and descriptive error.store/db/memdb.go (15)
- 13-13: The import of the
cosmossdk.io/store/v2/errorspackage is correctly added to use the updated error handling mechanism within the Cosmos SDK'sstore/v2module. This aligns with the PR's objective to refactor the error namespace.- 68-68: The use of
errors.ErrKeyEmptyfrom thecosmossdk.io/store/v2/errorspackage for handling empty keys in theGetmethod is appropriate and aligns with the Cosmos SDK's error handling conventions. This ensures consistency across the SDK.- 83-83: The introduction of
errors.ErrKeyEmptyfor handling empty keys in theHasmethod is consistent with the SDK's error handling strategy. This change ensures that the error handling is uniform across different database implementations.- 94-94: The use of
errors.ErrKeyEmptyfor handling empty keys in theSetmethod is consistent with the error handling strategy used in other methods, ensuring a consistent approach across the database interface.- 97-97: The introduction of
errors.ErrValueNilfor handling nil values in theSetmethod is a good practice. It ensures that the method's preconditions are explicitly checked, improving the robustness of the database interface.- 119-119: Applying
errors.ErrKeyEmptyin theDeletemethod for empty key validation aligns with the error handling strategy used in other methods, ensuring a consistent approach across the database interface.- 185-185: The use of
errors.ErrKeyEmptyfor handling empty keys in theIteratormethod is appropriate and maintains consistency in error handling across iterator operations.- 194-194: Repeating the use of
errors.ErrKeyEmptyin theReverseIteratormethod for empty key validation maintains consistency in error handling across synchronous and asynchronous write operations.- 202-202: The introduction of
errors.ErrKeyEmptyfor handling empty keys in theIteratorNoMtxmethod is consistent with the SDK's error handling strategy. This change ensures that the error handling is uniform across different database implementations.- 210-210: The use of
errors.ErrKeyEmptyin theReverseIteratorNoMtxmethod for empty key checks is appropriate and consistent with theIteratorNoMtxmethod, maintaining uniform error handling for iterator operations without mutex locking.- 399-399: The use of
errors.ErrKeyEmptyin thememDBBatchSetmethod for handling empty keys is appropriate and aligns with the Cosmos SDK's error handling conventions. This ensures consistency across the SDK.- 402-402: The introduction of
errors.ErrValueNilfor handling nil values in thememDBBatchSetmethod is a good practice. It ensures that the method's preconditions are explicitly checked, improving the robustness of the database interface.- 415-415: Applying
errors.ErrKeyEmptyin thememDBBatchDeletemethod for empty key validation aligns with the error handling strategy used in other methods, ensuring a consistent approach across the database interface.- 428-428: The check for
errors.ErrBatchClosedin thememDBBatchWritemethod ensures that attempts to write a closed batch are handled with a specific and descriptive error. This is a good practice for robust error handling.- 463-463: The check for
errors.ErrBatchClosedin thememDBBatchGetByteSizemethod is correctly implemented. This ensures that attempts to get the byte size of a closed batch are handled with a specific and descriptive error.store/snapshots/store.go (7)
- 21-21: The import alias
storeerrorsforcosmossdk.io/store/v2/errorsis correctly introduced to handle error namespacing. This change aligns with the PR's objective to refactor error handling within thestore/v2module.- 41-41: The usage of
storeerrors.ErrLogicfor error handling when the snapshot directory is not provided is appropriate and improves the clarity of the error's origin.- 64-64: Correctly utilizing
storeerrors.ErrConflictto indicate a conflict error when a snapshot is currently being saved. This change enhances error specificity and maintainability.- 252-252: The use of
storeerrors.ErrLogicfor validating the snapshot height is a good practice, ensuring that the error is descriptive and originates from a consistent namespace.- 260-260: Appropriately using
storeerrors.ErrConflictto handle the error when a snapshot for a specific height is already being saved, maintaining consistency in error handling.- 403-403: The application of
storeerrors.ErrLogicfor validating the length of a snapshot key is correct and improves the error's descriptiveness.- 406-406: Utilizing
storeerrors.ErrLogicto indicate an invalid snapshot key prefix is appropriate, ensuring that the error message is clear and originates from a consistent error namespace.store/snapshots/manager.go (13)
- 17-17: The import alias
storeerrorsforcosmossdk.io/store/v2/errorsis correctly introduced to handle error namespacing. This change aligns with the PR's objective to refactor error handling within thestore/v2module.- 121-121: Correctly using
storeerrors.ErrLogicto indicate a logical error when attempting to begin a "none" operation. This enhances the clarity and maintainability of error handling.- 124-124: Appropriately utilizing
storeerrors.ErrConflictto handle the error when an operation is already in progress, maintaining consistency in error handling across the module.- 170-170: The use of
storeerrors.ErrLogicfor handling a nilSnapshot Managerinstance is appropriate, ensuring that the error is descriptive and originates from a consistent namespace.- 184-184: Correctly using
storeerrors.ErrConflictto indicate a conflict error when a more recent snapshot already exists. This change enhances error specificity and maintainability.- 241-241: The application of
storeerrors.ErrLogicfor handling a nilSnapshot Managerinstance during migration is correct, improving the error's descriptiveness.- 310-310: Utilizing
storeerrors.ErrLogicto indicate a logical error when attempting to restore a snapshot at height 0 is appropriate, ensuring that the error message is clear and originates from a consistent error namespace.- 419-419: The use of
storeerrors.ErrLogicfor handling unknown snapshot items during restoration is correct, enhancing the clarity and maintainability of error handling.- 423-423: Appropriately using
storeerrors.ErrLogicto indicate an error when an unknown extension snapshotter is encountered during restoration. This maintains consistency in error handling across the module.- 452-452: Correctly utilizing
storeerrors.ErrLogicto handle the absence of a restore operation in progress, ensuring that the error is descriptive and originates from a consistent namespace.- 456-456: The application of
storeerrors.ErrLogicfor handling unexpected chunks during restoration is appropriate, improving the error's descriptiveness and maintainability.- 466-466: Utilizing
storeerrors.ErrLogicto indicate that the restoration ended unexpectedly is correct, ensuring that the error message is clear and originates from a consistent error namespace.- 502-502: The use of
storeerrors.ErrLogicfor indicating that the restoration ended prematurely is appropriate, enhancing the clarity and maintainability of error handling.
tac0turtle
approved these changes
Mar 8, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (1)
- store/proof/proof.go (3 hunks)
Additional comments: 4
store/proof/proof.go (4)
- 9-9: The import alias
storeerrorsforcosmossdk.io/store/v2/errorsis correctly defined and follows Go conventions for package aliasing. This change aligns with the PR's objective to refactor error handling within thestore/v2module.- 101-101: The error wrapping using
storeerrors.ErrInvalidProofwitherrors.Wrapfis correctly implemented. However, consider the user's preference for not making error messages too generic. Ensure that the error message provides enough context without revealing sensitive information such as the key.Would you like suggestions for making error messages detailed yet secure?
Verification successful
The error message in the provided code snippet appears to be appropriately detailed without revealing sensitive information, such as specific keys. It strikes a balance between providing context about the error and adhering to the user's preference for not making error messages too generic. This approach seems to align with ensuring error messages are secure yet informative.
* 110-110: The use of `storeerrors.ErrInvalidProof` for indicating the absence of a key is appropriate. However, the error message includes the key, which might conflict with the user alexanderbez's preference for not revealing the key in error messages. Consider rephrasing the error message to avoid potential security concerns.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Manual review recommended to ensure error messages are appropriately detailed without revealing sensitive information. echo "Review error messages for sensitivity and appropriateness."Length of output: 126
Consider removing or obfuscating the key in the error message to align with security best practices and user preferences.
* 120-120: The error handling for invalid argument lengths using `storeerrors.ErrInvalidProof` is correctly implemented. This is a good practice as it provides clear feedback to the caller about the nature of the error.Skipped due to learnings
User: alexanderbez
PR: #18633
File: store/proof.go:130-130
Timestamp: 2023-12-05T16:58:16.746Z
Learning: User alexanderbez has requested to ignore the suggestion to make the error message more generic to avoid potential sensitive information leakage.
| } | ||
|
|
||
| case 1: | ||
| // Args is length 1, verify existence of key with value args[0] | ||
| if !ics23.VerifyMembership(op.Spec, root, op.Proof, op.Key, args[0]) { | ||
| return nil, errors.Wrapf(ErrInvalidProof, "proof did not verify existence of key %s with given value %x", op.Key, args[0]) | ||
| return nil, errors.Wrapf(storeerrors.ErrInvalidProof, "proof did not verify existence of key %s with given value %x", op.Key, args[0]) |
There was a problem hiding this comment.
While the error handling logic is correct, including both the key and value in the error message could potentially reveal sensitive information. This is especially relevant given the user's preference for not disclosing the key in error messages. Consider how to provide useful feedback in errors without compromising security.
Reevaluate the inclusion of key and value in error messages to ensure they do not inadvertently expose sensitive information.
facundomedica
approved these changes
Mar 8, 2024
alexanderbez
approved these changes
Mar 8, 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
Change error namespace from store/v2 to prohibit double error registration.
Moves the error in an error package, to have all errors at a same place instead of in random files for avoiding cycling deps
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