chore(storage): cleanup NotFound errors
#2065
Conversation
| return deposits, nil | ||
| default: | ||
| return nil, err |
There was a problem hiding this comment.
technically a change since upon err we don't return deposits cumulated so far. However err client does drop all deposits upon error, so we're fine
abi87
force-pushed
the
storage-cleanup-err-not-found
branch
5 times, most recently
from
0108886
to
b535cdf
Compare
abi87
force-pushed
the
storage-cleanup-err-not-found
branch
from
b535cdf
to
bba5071
Compare
| @@ -108,7 +110,6 @@ func (kv *KVStore[DepositT]) Prune(start, end uint64) error { | |||
| kv.mu.Lock() | |||
| defer kv.mu.Unlock() | |||
| for i := range end { | |||
| // This only errors if the key passed in cannot be encoded. | |||
There was a problem hiding this comment.
I think this comment is wrong. There are a few internal errors possible, depending on the backend chosen
abi87
force-pushed
the
storage-cleanup-err-not-found
branch
4 times, most recently
from
ef0e1da
to
cf7c9b9
Compare
abi87
force-pushed
the
storage-cleanup-err-not-found
branch
from
cf7c9b9
to
3917a9a
Compare
WalkthroughThis pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
abi87
changed the title
NotFound errors
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range comments (2)
mod/storage/pkg/deposit/store.go (2)
Line range hint
66-69: Incorrect use of 'range' on 'uint64'; use a standard for loop insteadIn
GetDepositsByIndex, the loop usesfor i := range numView, butnumViewis auint64. Therangekeyword cannot be applied directly to an integer, which will result in a compilation error.To fix this, use a standard for loop:
-func (kv *KVStore[DepositT]) GetDepositsByIndex( - startIndex uint64, - numView uint64, -) ([]DepositT, error) { - kv.mu.RLock() - defer kv.mu.RUnlock() - deposits := []DepositT{} - for i := range numView { +func (kv *KVStore[DepositT]) GetDepositsByIndex( + startIndex uint64, + numView uint64, +) ([]DepositT, error) { + kv.mu.RLock() + defer kv.mu.RUnlock() + deposits := []DepositT{} + for i := uint64(0); i < numView; i++ {
Line range hint
104-108: Incorrect use of 'range' on 'uint64'; use a standard for loop inPrunemethodIn the
Prunemethod, the loop usesfor i := range end, butendis auint64. Therangekeyword cannot be applied directly to an integer, which will result in a compilation error.To fix this, use a standard for loop to iterate from
starttoend:func (kv *KVStore[DepositT]) Prune(start, end uint64) error { var ctx = context.TODO() kv.mu.Lock() defer kv.mu.Unlock() - for i := range end { + for i := start; i < end; i++ { if err := kv.store.Remove(ctx, start+i); err != nil { return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (13)
- mod/storage/pkg/beacondb/eth1.go (5 hunks)
- mod/storage/pkg/beacondb/fork.go (2 hunks)
- mod/storage/pkg/beacondb/history.go (4 hunks)
- mod/storage/pkg/beacondb/kvstore.go (2 hunks)
- mod/storage/pkg/beacondb/randao.go (2 hunks)
- mod/storage/pkg/beacondb/registry.go (7 hunks)
- mod/storage/pkg/beacondb/slashing.go (3 hunks)
- mod/storage/pkg/beacondb/versioning.go (3 hunks)
- mod/storage/pkg/beacondb/withdrawals.go (2 hunks)
- mod/storage/pkg/block/store.go (5 hunks)
- mod/storage/pkg/block/store_test.go (2 hunks)
- mod/storage/pkg/deposit/store.go (1 hunks)
- mod/storage/pkg/encoding/ssz.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (32)
mod/storage/pkg/beacondb/fork.go (3)
23-28: LGTM: New imports are appropriate for the changes.The addition of the "errors" and "fmt" packages is necessary and appropriate for the improved error handling in the
GetForkmethod.
45-53: LGTM: Improved error handling in GetFork method.The changes in the
GetForkmethod significantly improve error handling:
- It now distinguishes between different error types.
- It provides more informative error messages.
- It uses
errors.Is()for proper error type checking.These improvements align well with the PR objectives and Go best practices.
50-50: Verify the existence and import of ErrNotFound.The code uses
ErrNotFound, but this error type is not defined in the current file. Please ensure thatErrNotFoundis properly defined and imported from the correct package.To verify the existence and import of
ErrNotFound, please run the following script:mod/storage/pkg/beacondb/randao.go (2)
23-29: LGTM: Import changes are appropriate.The new imports (
errors,fmt, andcosmossdk.io/collections) are necessary for the enhanced error handling in theGetRandaoMixAtIndexfunction. These additions improve the code's functionality and readability.
Line range hint
1-64: Overall assessment: Changes align well with PR objectives.The modifications to this file successfully implement the new error handling mechanism described in the PR objectives. The introduction of
ErrNotFoundand the improved error reporting inGetRandaoMixAtIndexprovide a more uniform and informative approach to handling "not found" cases.These changes contribute to the overall goal of ensuring that
collections.ErrNotFoundis no longer returned to clients, instead exposing a non-Cosmos SDK error (ErrNotFound) to the rest of the codebase.mod/storage/pkg/block/store_test.go (3)
42-42: LGTM: Improved type safety inHashTreeRootmethodThe change from
[32]bytetocommon.Rootenhances type safety and aligns with the refactoring objectives of the PR. This modification ensures consistency in return types across the codebase.
50-50: LGTM: Consistent type improvement inGetStateRootmethodThe change from
[32]bytetocommon.Rootin theGetStateRootmethod maintains consistency with theHashTreeRootmethod modification. This change contributes to the overall refactoring effort and improves type safety.
84-89: LGTM: Improved error handling precisionThe change from
require.ErrorContainstorequire.ErrorIsenhances the precision of error checking in the test cases. This modification aligns with the PR objectives of improving error handling and provides more accurate test results by checking for specific error types.mod/storage/pkg/beacondb/slashing.go (1)
Line range hint
1-108: Overall, excellent improvements in error handling.The changes in this file effectively address the PR's objectives by improving error handling in the
GetSlashingAtIndexandGetTotalSlashingmethods. The new switch statement approach provides clearer, more idiomatic Go code and ensures thatcollections.ErrNotFoundis not directly returned to clients.These modifications enhance the clarity and maintainability of the code while aligning perfectly with the stated goal of introducing a new error type (
beacondb.ErrNotFound) and providing a more uniform error handling mechanism.Great job on this refactoring effort!
mod/storage/pkg/block/store.go (5)
24-24: LGTM: Import statement addition is appropriate.The addition of the "errors" import is necessary for creating the new error variable, which aligns with the PR objectives to improve error handling.
33-33: LGTM: New error variable improves error handling consistency.The introduction of
ErrSlotNotFoundaligns with the PR objectives by providing a standardized error for slot not found cases. This will enhance error handling consistency across the package.
87-91: LGTM: Improved error handling in GetSlotByBlockRoot.The modification enhances error reporting by using the new
ErrSlotNotFounderror and including the block root in the error message. This change provides more context and aligns with the PR objectives for improved error handling.
104-105: LGTM: Consistent error handling improvements across methods.The changes in
GetSlotByExecutionNumberandGetSlotByStateRootmethods follow the same pattern asGetSlotByBlockRoot. They use the newErrSlotNotFounderror and include the relevant identifier in the error message. This consistency improves the overall error handling in the package.Also applies to: 118-122
24-24: Summary: Successful implementation of improved error handling.The changes in this file consistently enhance error handling across the
KVStoremethods. The introduction ofErrSlotNotFoundand the inclusion of relevant identifiers in error messages align well with the PR objectives. These modifications will provide more informative error reporting, facilitating easier debugging and improving the overall robustness of the package.Great job on maintaining consistency across all affected methods!
Also applies to: 33-33, 87-91, 104-105, 118-122
mod/storage/pkg/encoding/ssz.go (5)
24-24: LGTM: Improved error handling with standardized error.The addition of the
errorsimport and the newerrNotImplementedvariable enhances the error handling in the package. This change aligns well with the PR objective of improving error handling and provides a consistent way to represent unimplemented functionality.Also applies to: 31-31
53-53: LGTM: Improved error message for unimplemented method.The update to use
errNotImplementedin the panic message ofEncodeJSONenhances error clarity and consistency. The use offmt.Errorffor error wrapping is a good practice.
58-58: LGTM: Consistent error handling for unimplemented method.The update to
DecodeJSONfollows the same pattern asEncodeJSON, usingerrNotImplementedin the panic message. This maintains consistency in error handling across the struct's methods.
102-102: LGTM: Consistent error handling across codec types.The update to
EncodeJSONinSSZInterfaceCodecmirrors the changes made inSSZValueCodec, ensuring consistency in error handling across different codec types.
Line range hint
1-118: Overall assessment: Consistent improvement in error handling.The changes in this file successfully implement a more consistent and clear approach to error handling for unimplemented methods. This aligns well with the PR objectives of improving error handling and clarity. The use of a standardized error variable and consistent error wrapping across different codec types enhances the maintainability and readability of the code.
While the current changes are appropriate and improve the codebase, consider the suggestion for future refactoring to return errors instead of using panics, as it could further enhance the API's usability and error handling capabilities.
mod/storage/pkg/beacondb/kvstore.go (2)
36-37: 🧹 Nitpick (assertive)Approve the addition of ErrNotFound and suggest its usage
The addition of
ErrNotFoundis a good step towards improving error handling in thebeacondbpackage. This aligns well with the PR objectives.To fully leverage this new error type, consider updating relevant methods in this file (and other files in the package) to use
ErrNotFoundinstead of returningnilor other generic errors when an object is not found. This will provide more consistent and informative error handling throughout the package.Let's check if there are any potential places in this file where
ErrNotFoundcould be used:#!/bin/bash # Search for potential places to use ErrNotFound rg --type go -i 'not found|nil' mod/storage/pkg/beacondb/kvstore.go
36-37: 🧹 Nitpick (assertive)Review other files for potential usage of ErrNotFound
While the addition of
ErrNotFoundis beneficial, its full potential isn't realized in this file alone.Consider reviewing other files in the
beacondbpackage to identify methods that currently returnnilor use generic errors for "not found" scenarios. Updating these methods to useErrNotFoundwould improve consistency and error handling across the package.Let's check for potential usage sites in other files of the package:
✅ Verification successful
Run a corrected search for
ErrNotFoundin other files within thebeacondbpackageThe previous script encountered an error and did not yield the expected results. Let's perform a more targeted search to identify potential usage sites of
ErrNotFound:
Run a corrected search for
ErrNotFoundin other files within thebeacondbpackageThe previous script encountered an error due to an invalid flag. Let's perform a corrected search to identify potential usage sites of
ErrNotFound:
ErrNotFound is consistently used across the beacondb package
The addition of
ErrNotFoundinkvstore.goaligns with its established usage in other files within thebeacondbpackage, ensuring consistent error handling for "not found" scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential places to use ErrNotFound in other files rg --type go -i 'not found|nil' mod/storage/pkg/beacondb/ | grep -v 'kvstore.go'Length of output: 241
Script:
# Searching for 'ErrNotFound' in Go files within beacondb package, excluding kvstore.go rg --type go 'ErrNotFound' mod/storage/pkg/beacondb/ --invert-file 'kvstore.go'Length of output: 523
Script:
# Searching for 'ErrNotFound' in Go files within beacondb package, excluding kvstore.go rg --type go 'ErrNotFound' mod/storage/pkg/beacondb/ -g '!kvstore.go'Length of output: 2586
mod/storage/pkg/beacondb/versioning.go (3)
24-26: Addition of Necessary ImportsThe imports
errorsandfmtare added to support enhanced error handling in the methods below. This inclusion is appropriate and necessary for the new error wrapping and formatting.
50-62: Improved Error Handling inGetGenesisValidatorsRootThe introduction of the
switchstatement effectively differentiates betweencollections.ErrNotFoundand other errors, providing clearer and more informative error messages. Mappingcollections.ErrNotFoundtobeacondb.ErrNotFoundstandardizes error handling across the codebase and improves client understanding.
72-85: Enhanced Error Handling inGetSlotThe revised error handling in the
GetSlotmethod mirrors the improvements made inGetGenesisValidatorsRoot. The use oferrors.Isto check forcollections.ErrNotFoundand the subsequent mapping toErrNotFoundenhances consistency and clarity in error reporting.mod/storage/pkg/beacondb/withdrawals.go (2)
23-29: Imports are appropriately updatedThe addition of
"errors","fmt","cosmossdk.io/collections", and"github.com/berachain/beacon-kit/mod/primitives/pkg/math"imports is necessary for the enhanced error handling and mathematical operations introduced in the code.
36-50: Verify the definition and usage ofErrNotFoundIn the
GetNextWithdrawalIndexmethod,ErrNotFoundis returned whencollections.ErrNotFoundis encountered. Please ensure thatErrNotFoundis properly defined in thebeacondbpackage and accessible in this context.Run the following script to verify the definition of
ErrNotFound:✅ Verification successful
ErrNotFoundis Properly Defined and AccessibleThe
ErrNotFoundvariable is correctly defined inmod/storage/pkg/beacondb/kvstore.goand is accessible inwithdrawals.go. No issues were found regarding its definition and usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'ErrNotFound' is defined in the 'beacondb' package. # Search for the definition of 'ErrNotFound' in the 'beacondb' package. rg --type go --max-depth 1 -A 2 'var ErrNotFound' mod/storage/pkg/beacondb/Length of output: 284
mod/storage/pkg/beacondb/eth1.go (1)
94-98: Add validation for payloadHeader.Version()In the
SetLatestExecutionPayloadHeadermethod (lines 94-98), you usepayloadHeader.Version()without validating the returned version:version = payloadHeader.Version()If there's a possibility that
Version()could return an invalid value, consider adding validation to ensure the version is acceptable before proceeding.Check whether
Version()can return invalid data:#!/bin/bash # Description: Verify the implementation of the Version() method. # Search for the Version method in the ExecutionPayloadHeader implementation. rg --type go 'func \(.*ExecutionPayloadHeader\) Version\(' -A 5 # Review for potential issues or lack of validation.mod/storage/pkg/beacondb/registry.go (5)
24-25: Approved: Necessary imports addedThe addition of
"errors"and"fmt"imports is appropriate for the enhanced error handling in the code.
149-165: Approved: Enhanced error handling in 'ValidatorByIndex'The introduction of a switch-case structure for error handling in the
ValidatorByIndexmethod improves clarity and consistency. Returning a zero-valueValidatorTalongside the error provides a clear indication of failure without risking the use of uninitialized data.
245-245: Approved: Proper error propagation in 'GetValidatorsByEffectiveBalance'The error from
ValidatorByIndexis appropriately propagated, ensuring any issues encountered during validator retrieval are correctly handled. This enhances the robustness of the method.
261-276: Approved: Enhanced error handling in 'GetBalance'The updated error handling using a switch-case structure provides more informative feedback. Wrapping the errors with additional context helps in debugging and maintaining the code.
327-333: Approved: Use of 'GetSlot' method and error handlingUtilizing the
GetSlotmethod improves encapsulation and abstraction within the codebase. The enhanced error handling ensures that failures in retrieving the slot are properly reported and handled.
mod/storage/pkg/beacondb/randao.go
Outdated
| switch { | ||
| case err == nil: | ||
| return common.Bytes32(bz), nil | ||
| case errors.Is(err, collections.ErrNotFound): | ||
| return common.Bytes32{}, fmt.Errorf( | ||
| "failed retrieving randao mix at index %d: %w", | ||
| index, | ||
| ErrNotFound, | ||
| ) | ||
| default: | ||
| return common.Bytes32{}, fmt.Errorf( | ||
| "failed retrieving randao mix at index %d: %w", | ||
| index, | ||
| err, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Improved error handling and alignment with PR objectives.
The changes to GetRandaoMixAtIndex function significantly improve error handling and align well with the PR objectives. The introduction of the ErrNotFound error type and the use of a switch statement for error handling provide more granular and informative error reporting.
Suggestions for minor improvements:
- Consider extracting the error message formatting into a separate function to improve readability and reduce duplication.
- Add a comment explaining the significance of
ErrNotFoundand how it differs fromcollections.ErrNotFound.
Here's a suggested refactor to address the above points:
func formatRandaoMixError(index uint64, err error) error {
return fmt.Errorf("failed retrieving randao mix at index %d: %w", index, err)
}
func (kv *KVStore[...]) GetRandaoMixAtIndex(index uint64) (common.Bytes32, error) {
bz, err := kv.randaoMix.Get(kv.ctx, index)
switch {
case err == nil:
return common.Bytes32(bz), nil
case errors.Is(err, collections.ErrNotFound):
// ErrNotFound is used to indicate that the randao mix was not found in the database,
// as opposed to collections.ErrNotFound which is specific to the collections package.
return common.Bytes32{}, formatRandaoMixError(index, ErrNotFound)
default:
return common.Bytes32{}, formatRandaoMixError(index, err)
}
}
mod/storage/pkg/block/store_test.go
Outdated
| _, err = blockStore.GetSlotByBlockRoot(common.Root{byte(8)}) | ||
| require.ErrorIs(t, err, block.ErrSlotNotFound) | ||
| _, err = blockStore.GetSlotByExecutionNumber(2) | ||
| require.ErrorContains(t, err, "not found") | ||
| require.ErrorIs(t, err, block.ErrSlotNotFound) | ||
| _, err = blockStore.GetSlotByStateRoot(common.Root{byte(8)}) | ||
| require.ErrorIs(t, err, block.ErrSlotNotFound) |
There was a problem hiding this comment.
💡 Codebase verification
Align Error Types with PR Objectives
The test in mod/storage/pkg/block/store_test.go is currently using block.ErrSlotNotFound. To maintain consistency with the PR's objective of introducing beacondb.ErrNotFound, please update the test to use beacondb.ErrNotFound instead.
- File:
mod/storage/pkg/block/store_test.go - Lines: 84-89
🔗 Analysis chain
Verify error type consistency with PR objectives
The PR objectives mention introducing a new error type beacondb.ErrNotFound, but the test cases are using block.ErrSlotNotFound. Could you please verify if this is intentional or if there's a need to align the error types across the codebase?
To check for consistency, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of error types across the codebase
# Search for beacondb.ErrNotFound
echo "Occurrences of beacondb.ErrNotFound:"
rg --type go "beacondb\.ErrNotFound"
# Search for block.ErrSlotNotFound
echo "Occurrences of block.ErrSlotNotFound:"
rg --type go "block\.ErrSlotNotFound"
# Search for any other ErrNotFound variations
echo "Other ErrNotFound variations:"
rg --type go "ErrNotFound" | grep -v "beacondb\.ErrNotFound" | grep -v "block\.ErrSlotNotFound"
Length of output: 4147
mod/storage/pkg/beacondb/slashing.go
Outdated
| switch { | ||
| case err == nil: | ||
| return math.Gwei(total), nil | ||
| case errors.Is(err, collections.ErrNotFound): | ||
| return 0, nil | ||
| } else if err != nil { | ||
| return 0, err | ||
| default: | ||
| return 0, fmt.Errorf("failed retrieving total slashing: %w", err) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consistent error handling improvement, with a minor suggestion for alignment.
The switch statement for error handling in GetTotalSlashing is a great improvement and consistent with the changes in GetSlashingAtIndex. It effectively addresses the PR objectives by ensuring that collections.ErrNotFound is not directly returned to clients.
For consistency with the GetSlashingAtIndex method, consider expanding the error message to include more context:
default:
- return 0, fmt.Errorf("failed retrieving total slashing: %w", err)
+ return 0, fmt.Errorf("failed retrieving total slashing amount: %w", err)
}This minor change aligns the error message style with GetSlashingAtIndex, providing slightly more descriptive context.
📝 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.
| switch { | |
| case err == nil: | |
| return math.Gwei(total), nil | |
| case errors.Is(err, collections.ErrNotFound): | |
| return 0, nil | |
| } else if err != nil { | |
| return 0, err | |
| default: | |
| return 0, fmt.Errorf("failed retrieving total slashing: %w", err) | |
| switch { | |
| case err == nil: | |
| return math.Gwei(total), nil | |
| case errors.Is(err, collections.ErrNotFound): | |
| return 0, nil | |
| default: | |
| return 0, fmt.Errorf("failed retrieving total slashing amount: %w", err) | |
| } |
mod/storage/pkg/beacondb/slashing.go
Outdated
| switch { | ||
| case err == nil: | ||
| return math.Gwei(amount), nil | ||
| case errors.Is(err, collections.ErrNotFound): | ||
| return 0, nil | ||
| } else if err != nil { | ||
| return 0, err | ||
| default: | ||
| return 0, fmt.Errorf( | ||
| "failed retrieving slashing at index %d: %w", | ||
| index, | ||
| err, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improved error handling looks good, with a minor suggestion.
The switch statement for error handling is a great improvement. It's more idiomatic Go and easier to read. The error cases are now more explicitly handled, which aligns well with the PR objectives.
Consider using a named return value for amount to make the code slightly more concise:
-func (kv *KVStore[
- BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
- ForkT, ValidatorT, ValidatorsT,
-]) GetSlashingAtIndex(
- index uint64,
-) (math.Gwei, error) {
- amount, err := kv.slashings.Get(kv.ctx, index)
+func (kv *KVStore[
+ BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
+ ForkT, ValidatorT, ValidatorsT,
+]) GetSlashingAtIndex(
+ index uint64,
+) (amount math.Gwei, err error) {
+ var rawAmount uint64
+ rawAmount, err = kv.slashings.Get(kv.ctx, index)
switch {
case err == nil:
- return math.Gwei(amount), nil
+ amount = math.Gwei(rawAmount)
+ return
case errors.Is(err, collections.ErrNotFound):
- return 0, nil
+ return
default:
- return 0, fmt.Errorf(
+ err = fmt.Errorf(
"failed retrieving slashing at index %d: %w",
index,
err,
)
+ return
}
}This change would eliminate the need for explicit return statements with values, as the named return values are automatically returned.
Committable suggestion was skipped due to low confidence.
| } | ||
|
|
||
| // DecodeJSON is not implemented and will panic if called. | ||
| func (SSZInterfaceCodec[T]) DecodeJSON(_ []byte) (T, error) { | ||
| panic("not implemented") | ||
| panic(fmt.Errorf("DecodeJSON %w", errNotImplemented)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Consistent error handling maintained.
The update to DecodeJSON in SSZInterfaceCodec completes the consistent application of the new error handling approach across all relevant methods.
For future consideration: Instead of using panic, you might want to return the error directly. This would allow callers to handle the error more gracefully. For example:
func (SSZInterfaceCodec[T]) DecodeJSON(_ []byte) (T, error) {
var zero T
return zero, fmt.Errorf("DecodeJSON %w", errNotImplemented)
}This change would make the API more idiomatic and easier to use, but it's outside the scope of the current PR.
mod/storage/pkg/beacondb/eth1.go
Outdated
| var h ExecutionPayloadHeaderT | ||
| v, err := kv.getLatestExecutionPayloadVersion() | ||
| if err != nil { | ||
| var t ExecutionPayloadHeaderT | ||
| return t, err | ||
| return h, fmt.Errorf( | ||
| "failed retrieving latest execution payload header: %w", | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| kv.latestExecutionPayloadCodec.SetActiveForkVersion(v) | ||
| h, err = kv.latestExecutionPayloadHeader.Get(kv.ctx) | ||
| switch { | ||
| case err == nil: | ||
| return h, nil | ||
| case errors.Is(err, collections.ErrNotFound): | ||
| return h, fmt.Errorf( | ||
| "failed retrieving latest execution payload header: %w", | ||
| ErrNotFound, | ||
| ) | ||
| default: | ||
| return h, fmt.Errorf( | ||
| "failed retrieving latest execution payload header: %w", | ||
| err, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify error messages for clarity
In the GetLatestExecutionPayloadHeader method (lines 38-63), the error messages can be simplified to improve readability. Currently, the same error message prefix is used multiple times.
Consider defining a constant error message prefix or using a helper function to reduce repetition.
const errMsg = "failed retrieving latest execution payload header"
func (kv *KVStore[...]) GetLatestExecutionPayloadHeader() (ExecutionPayloadHeaderT, error) {
// ...
if err != nil {
return h, fmt.Errorf("%s: %w", errMsg, err)
}
// ...
switch {
case err == nil:
return h, nil
case errors.Is(err, collections.ErrNotFound):
return h, fmt.Errorf("%s: %w", errMsg, ErrNotFound)
default:
return h, fmt.Errorf("%s: %w", errMsg, err)
}
}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range comments (1)
mod/storage/pkg/beacondb/withdrawals.go (1)
Line range hint
54-61: Apply error mapping inSetNextWithdrawalValidatorIndexSimilar to the previous suggestion,
SetNextWithdrawalValidatorIndexreturns errors directly fromkv.nextWithdrawalValidatorIndex.Set(kv.ctx, index.Unwrap())without usingmapErrors(err). For uniform error handling, consider mapping errors in this method as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
- mod/storage/pkg/beacondb/eth1.go (5 hunks)
- mod/storage/pkg/beacondb/fork.go (2 hunks)
- mod/storage/pkg/beacondb/helpers.go (1 hunks)
- mod/storage/pkg/beacondb/history.go (4 hunks)
- mod/storage/pkg/beacondb/randao.go (2 hunks)
- mod/storage/pkg/beacondb/registry.go (7 hunks)
- mod/storage/pkg/beacondb/slashing.go (3 hunks)
- mod/storage/pkg/beacondb/versioning.go (3 hunks)
- mod/storage/pkg/beacondb/withdrawals.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (28)
mod/storage/pkg/beacondb/helpers.go (2)
1-20: LGTM: License header is correctly included.The BUSL-1.1 license header is properly included at the beginning of the file, which is a good practice for maintaining clear licensing information.
21-27: LGTM: Package declaration and imports are appropriate.The package is correctly declared as
beacondb, and the necessary imports for "errors" and "cosmossdk.io/collections" are included. These imports align with the function's requirements.mod/storage/pkg/beacondb/randao.go (2)
23-27: LGTM: Import changes align with new error handling.The addition of the
fmtpackage import is appropriate for the new error formatting inGetRandaoMixAtIndex.
48-48: Verify the implementation ofmapErrorsfunction.The
mapErrorsfunction is crucial for convertingcollections.ErrNotFoundtobeacondb.ErrNotFound. Please ensure that this function is correctly implemented to fulfill the PR objectives.Run the following script to verify the
mapErrorsimplementation:✅ Verification successful
mapErrors Function Implementation Verified
The
mapErrorsfunction is correctly implemented inmod/storage/pkg/beacondb/helpers.goand effectively convertscollections.ErrNotFoundtobeacondb.ErrNotFound.
- Defined in
helpers.gowith proper error handling.- Utilized in
registry.goto maintain consistent error mapping.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of mapErrors function # Test: Search for the mapErrors function definition echo "Searching for mapErrors function definition:" rg --type go -A 10 'func mapErrors' # Test: Check if mapErrors handles collections.ErrNotFound echo "Checking if mapErrors handles collections.ErrNotFound:" rg --type go -A 5 'case errors.Is\(.*?, collections.ErrNotFound\)'Length of output: 1934
mod/storage/pkg/beacondb/versioning.go (3)
24-25: Importing the 'fmt' package is appropriate for error formatting.The addition of the
fmtpackage is necessary sincefmt.Errorfis used for wrapping errors in the updated methods below.
64-71: Consistent error handling in 'GetSlot' method enhances clarity.Applying
mapErrors(err)and wrapping the error with contextual information usingfmt.Errorfimproves the consistency and clarity of error reporting in theGetSlotmethod.
48-53: Ensure 'mapErrors' correctly maps storage errors to 'beacondb.ErrNotFound'.The use of
mapErrors(err)standardizes error handling by mapping underlying storage errors to the newbeacondb.ErrNotFound. Please verify thatmapErrorsproperly mapscollections.ErrNotFoundtobeacondb.ErrNotFoundas intended in the PR objectives.Run the following script to confirm the error mapping:
✅ Verification successful
'mapErrors' correctly maps 'collections.ErrNotFound' to 'beacondb.ErrNotFound'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'mapErrors' function maps 'collections.ErrNotFound' to 'beacondb.ErrNotFound'. # Search for the 'mapErrors' function implementation that maps the errors. ast-grep --lang go --pattern $'func mapErrors(err error) error { if err == collections.ErrNotFound { return ErrNotFound } $$$ }'Length of output: 134
Script:
#!/bin/bash # Description: Locate the 'mapErrors' function implementation. ast-grep --lang go --pattern $'func mapErrors($_) $_ { $$$ }'Length of output: 623
Script:
#!/bin/bash # Description: Find all return statements that return 'ErrNotFound' in the codebase. rg 'return ErrNotFound' --type goLength of output: 92
mod/storage/pkg/beacondb/withdrawals.go (1)
66-70: Avoid using potentially uninitializedidxin error messageAs previously noted, when an error occurs in retrieving
idx, the variableidxmay not hold a meaningful value. Including it in the error message could be misleading.mod/storage/pkg/beacondb/slashing.go (2)
86-88: Consistent error mapping in 'GetTotalSlashing'Ensure that
mapErrorsis correctly used inGetTotalSlashingto map errors as intended, maintaining consistency across methods.
58-64: Verify 'mapErrors' correctly maps errors as per PR objectivesEnsure that the
mapErrorsfunction properly mapscollections.ErrNotFoundtobeacondb.ErrNotFoundto fulfill the PR's goal of providing a uniform error handling mechanism to clients.You can run the following script to verify the error mapping:
mod/storage/pkg/beacondb/history.go (7)
23-25: Importingfmtpackage for error formattingThe addition of the
fmtpackage is appropriate, as it is used for formatting error messages withfmt.Errorf.
48-54: Improved error handling with contextual messagesIn the
GetBlockRootAtIndexmethod, the updated error handling provides more context by including the index in the error message. This enhances debugging and user feedback.
76-84: Enhanced error handling inGetLatestBlockHeaderThe method now includes detailed error messages when retrieving the latest block header fails, improving clarity and maintainability.
106-112: Improved error messages inStateRootAtIndexThe updated error handling provides detailed context, including the index, which aids in debugging and error tracing.
78-82: Verify unwrapping of wrapped errors for consistent handlingEnsure that the wrapped errors can be unwrapped by callers to identify specific error types, enabling consistent error handling across the codebase.
Use the following script to check error unwrapping:
#!/bin/bash # Description: Check that wrapped errors can be unwrapped and specific error types are handled. # Locate usages of fmt.Errorf with %w in the codebase. rg --type go 'fmt\.Errorf\([^)]*%w[^)]*\)' -A 2 # Verify that callers use errors.Is or errors.As to handle specific errors.
108-112: Confirm error unwrapping for proper error propagationWhen errors are wrapped, it's important that they can be unwrapped by calling code to check for errors like
beacondb.ErrNotFound.Execute the following script to confirm error unwrapping:
#!/bin/bash # Description: Ensure that wrapped errors are properly propagated and can be unwrapped. # Search for error wrapping with fmt.Errorf and %w. rg --type go 'fmt\.Errorf\([^)]*%w[^)]*\)' -A 2 # Check that error handling in the codebase uses errors.Is or errors.As appropriately.
50-54: Ensure errors can be unwrapped for type checkingWhen wrapping errors using
fmt.Errorfwith the%wverb, verify that callers can unwrap the error usingerrors.Isorerrors.Asto check for specific error types likebeacondb.ErrNotFound.You can run the following script to verify error unwrapping:
mod/storage/pkg/beacondb/eth1.go (5)
23-25: Approved: Import statements are appropriateThe necessary packages are imported, and the import statements are clean and concise.
35-54: Approved: Implementation ofGetLatestExecutionPayloadHeaderThe
GetLatestExecutionPayloadHeadermethod correctly initializes variables and handles errors consistently. The use of the helper functiongetLatestExecutionPayloadVersionimproves code modularity and readability.
56-68: Approved: Addition ofgetLatestExecutionPayloadVersionmethodThe new method
getLatestExecutionPayloadVersionencapsulates the logic for retrieving the latest execution payload version, enhancing the maintainability of the code. Error handling is clear and consistent with the rest of the codebase.
122-130: Approved: Consistent error handling inGetEth1DataThe
GetEth1Datamethod implements error handling with clear and informative messages. The use ofmapErrorsstandardizes error mapping across the codebase.
79-84: Consider verifying error handling forSetActiveForkVersionIn
SetLatestExecutionPayloadHeader, the call tokv.latestExecutionPayloadCodec.SetActiveForkVersion(payloadHeader.Version())does not handle any potential errors. IfSetActiveForkVersioncan fail or might do so in future implementations, it's advisable to handle any possible errors returned from it.Please confirm whether
SetActiveForkVersioncan fail, and if so, consider adding error handling. You can run the following script to check ifSetActiveForkVersionreturns an error:mod/storage/pkg/beacondb/registry.go (6)
24-25: Added imports are appropriateThe addition of
errorsandfmtpackages is necessary for the updated error handling.
94-108: Previous comment on formatting 'pubkey' in error messages is still validThe issue regarding the formatting of
pubkeyin error messages has not been addressed. Please refer to the previous review comment for details.
123-137: Previous comment on formatting 'cometBFTAddress' in error messages is still validThe issue regarding the formatting of
cometBFTAddressin error messages has not been addressed. Please refer to the previous review comment for details.
149-149: Error handling inValidatorByIndexis correctly implementedThe addition of error mapping and detailed error messages enhances the robustness of the
ValidatorByIndexfunction.Also applies to: 152-156, 158-158
237-237: Validator retrieval by index is correctly updatedThe use of
kv.ValidatorByIndex(math.U64(idx))ensures correct retrieval of validators.
253-261: Enhanced error handling inGetBalancefunctionThe inclusion of error mapping and improved error messages enhances the function's reliability.
abi87
force-pushed
the
storage-cleanup-err-not-found
branch
from
022b099
to
5261a79
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
mod/storage/pkg/beacondb/withdrawals.go (1)
Line range hint
46-53: Consider applying consistent error mapping to setter methodsFor consistency with the getter methods, consider applying the
mapErrorsfunction to theSetNextWithdrawalIndexandSetNextWithdrawalValidatorIndexmethods. This would align with the PR's goal of improving error handling across the package.For
SetNextWithdrawalIndex, consider applying this change:func (kv *KVStore[ BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT, ForkT, ValidatorT, ValidatorsT, ]) SetNextWithdrawalIndex( index uint64, ) error { - return kv.nextWithdrawalIndex.Set(kv.ctx, index) + err := kv.nextWithdrawalIndex.Set(kv.ctx, index) + return mapErrors(err) }Apply a similar change to
SetNextWithdrawalValidatorIndex.Also applies to: 75-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
- mod/storage/pkg/beacondb/eth1.go (5 hunks)
- mod/storage/pkg/beacondb/fork.go (2 hunks)
- mod/storage/pkg/beacondb/helpers.go (1 hunks)
- mod/storage/pkg/beacondb/history.go (4 hunks)
- mod/storage/pkg/beacondb/randao.go (2 hunks)
- mod/storage/pkg/beacondb/registry.go (14 hunks)
- mod/storage/pkg/beacondb/slashing.go (4 hunks)
- mod/storage/pkg/beacondb/versioning.go (3 hunks)
- mod/storage/pkg/beacondb/withdrawals.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (29)
mod/storage/pkg/beacondb/helpers.go (1)
33-42: 🛠️ Refactor suggestionConsider using a type switch for more idiomatic error handling
While the current implementation is correct, using a type switch instead of
errors.Is()would make the code more idiomatic and potentially more efficient. This aligns better with Go's error handling patterns.Consider refactoring the function as follows:
func mapErrors(err error) error { - switch { - case err == nil: + switch err { + case nil: return nil - case errors.Is(err, collections.ErrNotFound): + case collections.ErrNotFound: return ErrNotFound default: return err } }This change maintains the same functionality while improving code idiomaticity and potential performance.
Likely invalid or redundant comment.
mod/storage/pkg/beacondb/randao.go (1)
48-54: 🧹 Nitpick (assertive)LGTM: Improved error handling with suggestions for further enhancement.
The changes align well with the PR objectives by improving error handling and providing more context in the error message. The use of
mapErrorslikely handles the conversion ofcollections.ErrNotFoundtobeacondb.ErrNotFoundas intended.Suggestions for further improvement:
- Consider extracting the error formatting into a separate function for better readability and maintainability:
func formatRandaoMixError(index uint64, err error) error { return fmt.Errorf("failed retrieving randao mix at index %d: %w", index, err) } // Usage in GetRandaoMixAtIndex if err != nil { return common.Bytes32{}, formatRandaoMixError(index, err) }
- Add a comment explaining the purpose of
mapErrorsfunction, especially its role in handlingcollections.ErrNotFound.To ensure the
mapErrorsfunction is correctly handlingcollections.ErrNotFound, please run the following verification script:✅ Verification successful
Verification Successful:
mapErrorscorrectly handlescollections.ErrNotFound.The
mapErrorsfunction is properly implemented to mapcollections.ErrNotFoundtobeacondb.ErrNotFound, aligning with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of mapErrors function # Test: Search for the mapErrors function definition echo "Searching for mapErrors function definition:" rg --type go -A 10 'func mapErrors' # Test: Check if collections.ErrNotFound is being handled echo "Checking if collections.ErrNotFound is being handled:" rg --type go 'case errors\.Is\(.*collections\.ErrNotFound'Length of output: 1012
mod/storage/pkg/beacondb/withdrawals.go (1)
34-42: Improved error handling and consistencyThe changes in
GetNextWithdrawalIndexalign well with the PR objectives. The introduction ofmapErrorsand the new error formatting enhance error handling consistency and provide more informative error messages. This improvement addresses the past review comment about standardizing error messages.mod/storage/pkg/beacondb/history.go (2)
76-84: Improved error handling looks good!The changes in
GetLatestBlockHeaderare consistent with the improvements made in other methods. The new error handling provides more context, which aligns with the PR objectives.
48-48: Please clarify the implementation ofmapErrorsandErrNotFound.The changes consistently use a
mapErrorsfunction, which seems to be addressing theErrNotFoundcase mentioned in the PR objectives. However, the implementation ofmapErrorsand the definition ofErrNotFoundare not visible in this file.Could you please provide more information on:
- The implementation of the
mapErrorsfunction?- The definition and location of the
ErrNotFounderror type?This information will help ensure that the error handling is implemented correctly and consistently across the package.
To verify the implementation, please run the following script:
Also applies to: 77-77, 106-106
✅ Verification successful
Verification Successful.
The implementations of
mapErrorsandErrNotFoundare correctly defined and utilized within the codebase. No issues were found regarding their usage or definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mapErrors function and ErrNotFound definition echo "Searching for mapErrors function:" rg --type go -A 5 'func mapErrors' echo "Searching for ErrNotFound definition:" rg --type go -A 2 'var ErrNotFound'Length of output: 802
mod/storage/pkg/beacondb/fork.go (2)
23-25: Addition of"fmt"import is appropriate.The import of the
"fmt"package is necessary for error formatting usingfmt.Errorf, which enhances the clarity of error messages.
42-47: Enhanced error handling inGetForkmethod.The use of
mapErrors(err)to map internal errors tobeacondb.ErrNotFound, along with wrapping the error usingfmt.Errorf, improves error transparency. This change aligns with the PR objective to provide a uniform error handling mechanism.mod/storage/pkg/beacondb/versioning.go (3)
24-25: Importing "fmt" package for error formattingThe addition of the
"fmt"package is appropriate to support error wrapping withfmt.Errorf.
48-53: Enhanced error handling inGetGenesisValidatorsRoot()The updated error handling improves clarity by mapping errors and providing informative messages. This change aids in debugging and maintains consistency across the codebase.
64-71: Consistent error handling inGetSlot()The inclusion of error mapping and wrapping enhances error reporting, aligning with best practices.
mod/storage/pkg/beacondb/slashing.go (4)
24-25: Appropriate addition of the 'fmt' packageThe inclusion of the
fmtpackage is necessary for enhanced error formatting introduced in the updated code.
35-42: Consistent use of 'mapErrors' for error mappingThe implementation of
mapErrors(err)enhances consistency in error handling across the codebase. The updated error messages provide clear context when iteration fails inGetSlashings.
63-69: Improved error handling in 'GetSlashingAtIndex'The use of
mapErrors(err)along with detailed error messages improves clarity when retrieving slashing amounts by index fails. This change aligns with the PR objectives of refining error handling.
91-93: Enhanced error reporting in 'GetTotalSlashing'Applying
mapErrors(err)and providing a formatted error message ensures that any failures during the retrieval of the total slashing amount are clearly communicated.mod/storage/pkg/beacondb/eth1.go (6)
23-25: ApprovedThe import statements have been correctly updated to include only the necessary packages.
35-54: ApprovedThe
GetLatestExecutionPayloadHeaderfunction properly retrieves the latest execution payload version and header with appropriate error handling. The separation of concerns by introducing thegetLatestExecutionPayloadVersionhelper function enhances code readability and maintainability.
56-68: ApprovedThe
getLatestExecutionPayloadVersionhelper function encapsulates the retrieval of the latest execution payload version with consistent error handling, improving code reusability.
79-86: ApprovedThe
SetLatestExecutionPayloadHeaderfunction correctly sets the latest execution payload version and header. The use of local variables for context and version improves code clarity.
95-104: ApprovedThe
GetEth1DepositIndexfunction includes appropriate retrieval logic with consistent error mapping and handling.
122-130: ApprovedThe
GetEth1Datafunction correctly retrieves the Eth1 data with proper error handling and mapping.mod/storage/pkg/beacondb/registry.go (9)
103-106: Corrected formatting ofpubkeyin error messageThe error message now uses
%xto formatpubkey, which is appropriate for byte slices.
125-128: Corrected formatting ofcometBFTAddressin error messageThe error message now correctly formats
cometBFTAddressusing%x, suitable for byte slices.
141-150: Enhanced error handling inValidatorByIndexThe error mapping and detailed error message improve the clarity of error reporting.
163-166: Improved error message inGetValidatorsThe error message now more accurately describes the operation, enhancing clarity.
177-180: Improved error message inGetValidatorsiterationThe error message now clearly indicates that the iteration over validators failed.
229-232: Enhanced error message inGetValidatorsByEffectiveBalanceThe error message has been clarified to indicate the failure in iterating validators by effective balance.
257-265: Enhanced error handling inGetBalanceThe added error mapping and detailed error message improve error reporting in the
GetBalancemethod.
288-291: Improved error handling inGetBalancesThe error mapping and clear error message enhance robustness.
318-321: Improved error message inGetTotalActiveBalancesThe error message now more clearly indicates the failure in iterating validators' effective balances.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/storage/pkg/beacondb/errors.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
mod/storage/pkg/beacondb/errors.go (2)
1-27: LGTM: Package declaration and imports are correct.The file includes appropriate license information, correct package declaration, and relevant imports for the implemented functionality.
1-44: Overall assessment: Well-implemented error handling improvements.This new file successfully introduces the
ErrNotFounderror type and implements error mapping as per the PR objectives. The code is well-structured, follows Go best practices, and effectively standardizes error handling for the Beacon database.Key points:
- Custom
ErrNotFounderror is appropriately defined.- The
mapErrorsfunction effectively mapscollections.ErrNotFoundtoErrNotFound.- The implementation reduces direct dependencies on Cosmos SDK error types.
The suggested minor improvements (adding a comment for
ErrNotFoundand renamingmapErrorstomapError) would further enhance the code's clarity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range comments (3)
mod/storage/pkg/beacondb/randao.go (1)
Line range hint
23-54: Summary: Partial alignment with PR objectivesThe changes in this file improve error handling and message formatting, which is a step in the right direction. However, to fully align with the PR objectives:
- Ensure that
beacondb.ErrNotFoundis used explicitly when appropriate.- Clarify the role of the
mapErrorfunction in error handling.- Verify that
collections.ErrNotFoundis no longer returned to clients.Consider addressing these points to complete the implementation of the PR objectives.
mod/storage/pkg/beacondb/slashing.go (2)
Line range hint
59-64: Returnbeacondb.ErrNotFoundinstead of default valueIn
GetSlashingAtIndex, to align with the PR objective of introducingbeacondb.ErrNotFoundfor uniform error handling, consider returning this specific error whencollections.ErrNotFoundis encountered, rather than returning0andnil. This change explicitly informs the client that the object was not found.Suggested change:
amount, err := kv.slashings.Get(kv.ctx, index) if errors.Is(err, collections.ErrNotFound) { - return 0, nil + return 0, beacondb.ErrNotFound } else if err != nil { return 0, err } return math.Gwei(amount), nil
Line range hint
76-81: Returnbeacondb.ErrNotFoundwhen total slashing amount is missingIn
GetTotalSlashing, consider returningbeacondb.ErrNotFoundwhencollections.ErrNotFoundis encountered, instead of returning0andnil. This approach aligns with the PR's goal of standardizing error handling and clearly indicates to clients that the total slashing amount was not found.Suggested change:
total, err := kv.totalSlashing.Get(kv.ctx) if errors.Is(err, collections.ErrNotFound) { - return 0, nil + return 0, beacondb.ErrNotFound } else if err != nil { return 0, err } return math.Gwei(total), nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
- mod/storage/pkg/beacondb/errors.go (1 hunks)
- mod/storage/pkg/beacondb/eth1.go (5 hunks)
- mod/storage/pkg/beacondb/fork.go (2 hunks)
- mod/storage/pkg/beacondb/history.go (4 hunks)
- mod/storage/pkg/beacondb/randao.go (2 hunks)
- mod/storage/pkg/beacondb/registry.go (14 hunks)
- mod/storage/pkg/beacondb/slashing.go (2 hunks)
- mod/storage/pkg/beacondb/versioning.go (3 hunks)
- mod/storage/pkg/beacondb/withdrawals.go (2 hunks)
🧰 Additional context used
📓 Learnings (2)
mod/storage/pkg/beacondb/errors.go (2)
Learnt from: abi87 PR: berachain/beacon-kit#2065 File: mod/storage/pkg/beacondb/errors.go:29-29 Timestamp: 2024-10-13T16:17:45.589Z Learning: When defining error variables like `ErrNotFound` in the `beacondb` package, the user prefers not to add comments if the error is self-explanatory.Learnt from: abi87 PR: berachain/beacon-kit#2065 File: mod/storage/pkg/beacondb/helpers.go:0-0 Timestamp: 2024-10-13T15:05:46.809Z Learning: In the Go code at `mod/storage/pkg/beacondb/helpers.go`, using `errors.Is` in the `mapErrors` function is preferred over direct error equality checks, as it handles wrapped errors and provides more general error matching.mod/storage/pkg/beacondb/registry.go (1)
Learnt from: abi87 PR: berachain/beacon-kit#2065 File: mod/storage/pkg/beacondb/registry.go:330-331 Timestamp: 2024-10-13T14:18:59.472Z Learning: When reviewing PRs that are purely for refactoring, avoid suggesting logic changes or code modifications that are out of scope.
🔇 Additional comments (34)
mod/storage/pkg/beacondb/errors.go (3)
1-20: LGTM: Appropriate license header.The BUSL-1.1 license header is correctly included and formatted, which is crucial for open-source projects.
29-29: LGTM: Clear and concise error definition.The
ErrNotFounderror is well-defined and aligns with the PR objectives. It provides a clear, package-specific error message without unnecessary comments, as per your preference.
31-44: LGTM: Well-implemented error mapping function.The
mapErrorfunction effectively achieves the goal of standardizing error handling by mappingcollections.ErrNotFoundto the customErrNotFound. Notable points:
- The function is well-commented, explaining its purpose clearly.
- It correctly uses
errors.Isfor error checking, which is the preferred method as it handles wrapped errors.- The function name is already in singular form (
mapError), which is appropriate for its functionality.The implementation aligns well with the PR objectives and follows good practices in Go error handling.
mod/storage/pkg/beacondb/randao.go (1)
50-54: Approved: Improved error message formattingThe error message formatting has been improved as suggested in a previous review comment. This change enhances the clarity of error reporting.
mod/storage/pkg/beacondb/history.go (4)
23-27: LGTM: Import changes are appropriate.The addition of the "fmt" package to the import list is necessary for the new error formatting using
fmt.Errorf. This change aligns with the modifications made in the methods below.
76-84: LGTM: Improved error handling inGetLatestBlockHeader.The changes in this method are consistent with the improvements made in
GetBlockRootAtIndex. The new error handling provides more context, which aligns with the PR objectives. The use ofmapErroris consistent, and the error message is clear and informative.
Line range hint
1-114: Summary: Improved error handling with some points needing attention.Overall, the changes in this file consistently improve error handling by providing more context in error messages, which aligns with the PR objectives. However, there are a few points that need attention:
- The
mapErrorfunction is used throughout the file but is not defined here. Please ensure it's properly defined and imported.- The PR objectives mention introducing
beacondb.ErrNotFound, but it's not directly used in the reviewed code. Clarify if this is intentional or if it should be incorporated.- Consider standardizing the error message format across all methods for consistency (e.g., "failed to retrieve" vs "failed retrieving").
These improvements enhance the clarity and specificity of error reporting in the specified methods, but addressing the above points would further strengthen the implementation.
To verify the
ErrNotFounddefinition:#!/bin/bash # Search for the definition of ErrNotFound rg --type go 'var ErrNotFound'
48-54: Improved error handling, but clarification needed onmapError.The new error handling provides more context by including the index in the error message, which aligns with the PR objectives. However, there are a couple of points that need attention:
- The
mapErrorfunction is used but not defined in this file. Please ensure it's properly defined and imported.- The PR objectives mention introducing
beacondb.ErrNotFound, but it's not directly used here. Is this intentional?To verify the
mapErrorfunction:✅ Verification successful
mapErroris properly defined and accessible.The
mapErrorfunction is defined inmod/storage/pkg/beacondb/errors.goand is correctly used withinhistory.go. Since both files are part of the samebeacondbpackage, there is no need for additional imports, and the usage aligns with Go's package structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition of mapError function rg --type go 'func mapError'Length of output: 213
Script:
#!/bin/bash # Search for any occurrences of mapError in Go files rg --type go 'mapError'Length of output: 1684
mod/storage/pkg/beacondb/fork.go (2)
23-25: Importing 'fmt' package is appropriateThe addition of the
fmtpackage is necessary for error formatting.
42-46: Ensure to return zero value ofForkTon errorAs previously noted, when an error occurs, returning the potentially invalid
fmay lead to unintended behavior. It's safer to return the zero value ofForkTto prevent propagation of invalid data.mod/storage/pkg/beacondb/versioning.go (3)
24-25: Approved: Necessary import addedThe import of the
fmtpackage is required for the enhanced error handling usingfmt.Errorfbelow.
48-54: VerifymapErrorcorrectly mapscollections.ErrNotFoundtobeacondb.ErrNotFoundEnsure that the
mapError(err)function properly convertscollections.ErrNotFoundtobeacondb.ErrNotFoundas per the PR objective to clean upNotFounderrors. This change ensures that clients receive the uniform error typebeacondb.ErrNotFoundwhen an object is not found.You can verify this by checking the implementation of
mapError. Run the following script to confirm:#!/bin/bash # Description: Verify that mapError correctly maps collections.ErrNotFound to beacondb.ErrNotFound # Search for the implementation of mapError rg --type go -A5 -B5 'func mapError\(err error\) error' # Check if collections.ErrNotFound is mapped to beacondb.ErrNotFound within mapError
64-70: Ensure consistent error mapping inGetSlotmethodAs with
GetGenesisValidatorsRoot, confirm thatmapError(err)in theGetSlotmethod correctly mapscollections.ErrNotFoundtobeacondb.ErrNotFound. This maintains uniform error handling across the codebase.You can verify this by reviewing the
mapErrorimplementation:✅ Verification successful
Error mapping in
GetSlotis consistent and correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that mapError correctly maps collections.ErrNotFound to beacondb.ErrNotFound # Search for the implementation of mapError rg --type go -A5 -B5 'func mapError\(err error\) error' # Ensure that collections.ErrNotFound is being mapped appropriatelyLength of output: 831
mod/storage/pkg/beacondb/withdrawals.go (2)
23-27: Appropriate Addition of ImportsThe
fmtpackage is correctly imported for formatting error messages, andmathis imported formath.ValidatorIndex. These additions are necessary and properly included.
34-42: Enhanced Error Handling inGetNextWithdrawalIndexThe method now uses
mapError(err)to process errors and provides a clear, formatted error message when retrieval fails. This improves error clarity and consistency across the codebase.mod/storage/pkg/beacondb/slashing.go (2)
24-25: Importing necessary packages for error handlingThe addition of the
"errors"and"fmt"packages is appropriate for improved error handling and formatting.
37-42: EnsuremapErrorcorrectly maps errorsThe use of
err = mapError(err)is essential for consistent error handling. Please verify that themapErrorfunction correctly transformscollections.ErrNotFoundintobeacondb.ErrNotFoundor handles errors as intended, in line with the PR objectives.You can run the following script to locate and review the implementation of
mapError:✅ Verification successful
Verification Successful:
mapErrorcorrectly mapscollections.ErrNotFoundtobeacondb.ErrNotFound.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate and display the implementation of the mapError function. # Search for the definition of mapError in Go source files. rg --type go '^func mapError\(err error\) error' -A 5Length of output: 407
mod/storage/pkg/beacondb/eth1.go (5)
23-25: Imports are appropriateThe necessary packages are imported correctly.
35-54:GetLatestExecutionPayloadHeaderis correctly implementedThe function effectively retrieves the latest execution payload header with proper error handling.
60-68:getLatestExecutionPayloadVersionis correctly implementedThe helper function encapsulates the retrieval of the latest execution payload version and handles errors appropriately.
95-104:GetEth1DepositIndexis correctly implementedThe function retrieves the Eth1 deposit index with appropriate error handling.
122-130:GetEth1Datais correctly implementedThe function retrieves the Eth1 data and handles errors properly.
mod/storage/pkg/beacondb/registry.go (12)
24-25: Importing the "fmt" packageThe addition of the
"fmt"package is appropriate, as it is required for the formatted error messages introduced in this refactoring.
38-43: Improved error handling inAddValidatorThe use of
mapError(err)followed by a check forerr != nilenhances error handling. The formatted error message provides clear context about the failure when adding a validator.
61-66: Consistent error handling inAddValidatorBartioThe introduction of
mapError(err)and a descriptive error message improves the clarity and consistency of error handling when adding a Bartio validator.
100-106: Proper formatting ofpubkeyin error messagesFormatting
pubkeywith%xdisplays the public key in hexadecimal format, which enhances readability and ensures correct representation of theBLSPubkey.
122-128: Proper formatting ofcometBFTAddressin error messagesUsing
%xto formatcometBFTAddresscorrectly displays the address in hexadecimal, which is appropriate for byte slices and improves the clarity of error messages.
141-148: Enhanced error handling inValidatorByIndexMapping the error and providing a detailed error message when retrieval fails improves debuggability and user understanding.
161-166: Improved error handling inGetValidatorsThe error handling enhancements, including the use of
mapError(err)and a clear error message, make it easier to diagnose issues when peeking the next validator index fails.
175-180: Consistent error handling during validators iterationThe addition of error mapping and a descriptive error message in the iteration process enhances consistency and aids in troubleshooting.
227-232: Improved error handling inGetValidatorsByEffectiveBalanceThe updates ensure that any errors encountered during iteration are mapped and clearly reported, improving maintainability and debugging.
257-265: Enhanced error handling inGetBalanceMapping the error and providing a specific error message when retrieving a balance fails enhances the clarity of error reporting.
286-291: Improved error handling during balances iterationThe consistent use of
mapError(err)and detailed error messages improves error handling when iterating over balances.
316-321: Consistent error handling inGetTotalActiveBalancesThe addition of error mapping and a clear message when iterating over validators' effective balances aligns with the refactoring objective and enhances error clarity.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range comments (3)
mod/storage/pkg/block/store.go (1)
Line range hint
1-124: Overall: Excellent improvements in error handling and consistency.The changes in this file successfully implement the new
storage.ErrNotFounderror type across all relevant methods, aligning perfectly with the PR objectives. The error messages now provide more context by including specific identifiers (block root, execution number, state root), which will greatly aid in debugging.To further improve code maintainability and reduce duplication, consider introducing a helper function for error creation:
func newNotFoundError(identifier string, value interface{}) error { return fmt.Errorf("%w, %s: %v", storage.ErrNotFound, identifier, value) }This function could then be used in all three methods:
return 0, newNotFoundError("block root", blockRoot) return 0, newNotFoundError("execution number", executionNumber) return 0, newNotFoundError("state root", stateRoot)This approach would centralize the error creation logic and make future modifications easier.
mod/storage/pkg/beacondb/withdrawals.go (1)
Line range hint
75-82: Consistent error mapping inSetNextWithdrawalValidatorIndexIn
GetNextWithdrawalValidatorIndex, errors are mapped usingstorage.MapError(err)before handling. However, inSetNextWithdrawalValidatorIndex, errors fromkv.nextWithdrawalValidatorIndex.Set(kv.ctx, index.Unwrap())are returned directly without mapping. For consistency in error handling across methods, consider mapping errors here as well.Apply this diff to map errors:
func (kv *KVStore[ BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT, ForkT, ValidatorT, ValidatorsT, ]) SetNextWithdrawalValidatorIndex( index math.ValidatorIndex, ) error { - return kv.nextWithdrawalValidatorIndex.Set(kv.ctx, index.Unwrap()) + err := kv.nextWithdrawalValidatorIndex.Set(kv.ctx, index.Unwrap()) + return storage.MapError(err) }mod/storage/pkg/beacondb/slashing.go (1)
Line range hint
64-79: Align error handling with PR objectives by returningbeacondb.ErrNotFoundAccording to the PR objectives, the goal is to expose a
beacondb.ErrNotFounderror when an object is not found, instead of returning default values with anilerror. In theGetSlashingAtIndexandGetTotalSlashingmethods, whencollections.ErrNotFoundis encountered, the functions currently return a zero value (0) andnilerror. This can lead to confusion for clients, as it does not clearly indicate that the object was not found.To align with the PR objectives and provide a more uniform error handling mechanism, consider returning
beacondb.ErrNotFoundwhen an object is not found.Apply this diff to update the error handling:
func (kv *KVStore[ BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT, ForkT, ValidatorT, ValidatorsT, ]) GetSlashingAtIndex( index uint64, ) (math.Gwei, error) { amount, err := kv.slashings.Get(kv.ctx, index) - if errors.Is(err, collections.ErrNotFound) { - return 0, nil + err = storage.MapError(err) + if errors.Is(err, beacondb.ErrNotFound) { + return 0, err + } else if err != nil { + return 0, err } return math.Gwei(amount), nil } // GetTotalSlashing retrieves the total slashing amount from the store. func (kv *KVStore[ BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT, ForkT, ValidatorT, ValidatorsT, ]) GetTotalSlashing() (math.Gwei, error) { total, err := kv.totalSlashing.Get(kv.ctx) - if errors.Is(err, collections.ErrNotFound) { - return 0, nil + err = storage.MapError(err) + if errors.Is(err, beacondb.ErrNotFound) { + return 0, err + } else if err != nil { + return 0, err } return math.Gwei(total), nil }This change ensures that when an object is not found, the functions return
beacondb.ErrNotFound, making it clear to clients that the requested data is missing.Also applies to: 89-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (11)
- mod/storage/pkg/beacondb/eth1.go (5 hunks)
- mod/storage/pkg/beacondb/fork.go (2 hunks)
- mod/storage/pkg/beacondb/history.go (4 hunks)
- mod/storage/pkg/beacondb/randao.go (2 hunks)
- mod/storage/pkg/beacondb/registry.go (14 hunks)
- mod/storage/pkg/beacondb/slashing.go (2 hunks)
- mod/storage/pkg/beacondb/versioning.go (3 hunks)
- mod/storage/pkg/beacondb/withdrawals.go (2 hunks)
- mod/storage/pkg/block/store.go (4 hunks)
- mod/storage/pkg/block/store_test.go (3 hunks)
- mod/storage/pkg/errors.go (1 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/storage/pkg/beacondb/registry.go (1)
Learnt from: abi87 PR: berachain/beacon-kit#2065 File: mod/storage/pkg/beacondb/registry.go:330-331 Timestamp: 2024-10-13T14:18:59.472Z Learning: When reviewing PRs that are purely for refactoring, avoid suggesting logic changes or code modifications that are out of scope.
🔇 Additional comments (25)
mod/storage/pkg/errors.go (3)
1-21: LGTM: License header and package declaration.The license header is complete and the package declaration is appropriate for the file's purpose.
23-29: LGTM: Imports and error variable declaration.The imports are appropriate for the file's functionality. The
ErrNotFoundvariable is well-defined and aligns with the PR objective of introducing a new error type for improved error handling in cases where queried objects are not found.
1-44: Overall assessment: Well-implemented error handling.This new file successfully introduces the
ErrNotFounderror type and theMapErrorfunction, fulfilling the PR objective of improving error handling for cases where queried objects are not found. The implementation is clean, follows Go best practices, and effectively reduces dependencies on the Cosmos SDK package.The changes align perfectly with the PR's goal of ensuring that
collections.ErrNotFoundis no longer returned to clients, instead providing a more uniform error handling mechanism throughbeacondb.ErrNotFound.Great job on this refactoring effort!
mod/storage/pkg/beacondb/randao.go (3)
23-28: LGTM: Import changes are appropriate.The new imports for "fmt" and the storage package are correctly added and necessary for the changes made in the
GetRandaoMixAtIndexmethod.
Line range hint
1-55: Overall improvements with room for alignmentThe changes in this file improve error handling and provide more informative error messages. The use of
storage.MapErrorand formatted error messages enhances the clarity of error reporting. However, to fully align with the PR objectives of introducingbeacondb.ErrNotFound, consider implementing the suggested changes in the previous comment.These modifications will ensure that the code meets the stated goal of not returning
collections.ErrNotFoundto clients and provides a more uniform error handling mechanism across the codebase.
49-55: 🛠️ Refactor suggestionSuggestion: Explicitly handle
collections.ErrNotFoundWhile the changes improve error handling, they don't fully align with the PR objectives of introducing
beacondb.ErrNotFound. Consider explicitly checking forcollections.ErrNotFoundand returningbeacondb.ErrNotFoundto better meet the PR goals.Here's a suggested implementation:
bz, err := kv.randaoMix.Get(kv.ctx, index) if errors.Is(err, collections.ErrNotFound) { return common.Bytes32{}, fmt.Errorf( "failed retrieving randao mix at index %d: %w", index, beacondb.ErrNotFound, ) } else if err != nil { return common.Bytes32{}, fmt.Errorf( "failed retrieving randao mix at index %d: %w", index, err, ) } return common.Bytes32(bz), nilThis change ensures that
collections.ErrNotFoundis no longer returned to clients, as specified in the PR objectives.To verify the current implementation of
storage.MapError, please run the following script:mod/storage/pkg/block/store_test.go (2)
29-29: LGTM: Import added for storage packageThe addition of the storage package import is in line with the PR objectives and enables the use of
storage.ErrNotFoundin the test cases.
85-90: LGTM: Improved error handling in test casesThe changes to use
require.ErrorIswithstorage.ErrNotFoundalign well with the PR objectives and provide more precise error checking. This addresses the concern raised in previous reviews about aligning error types.To ensure consistency across the codebase, let's verify the usage of error types:
✅ Verification successful
Verified: Consistent use of
storage.ErrNotFoundin test casesThe use of
require.ErrorIswithstorage.ErrNotFoundinstore_test.gois consistent and aligns with the project's error handling standards.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of error types across the codebase # Search for storage.ErrNotFound echo "Occurrences of storage.ErrNotFound:" rg --type go "storage\.ErrNotFound" # Search for any other ErrNotFound variations echo "Other ErrNotFound variations:" rg --type go "ErrNotFound" | grep -v "storage\.ErrNotFound"Length of output: 2093
mod/storage/pkg/beacondb/history.go (1)
23-28: Overall improvements in error handlingThe changes made to this file significantly improve error handling across multiple functions. Key improvements include:
- Consistent use of
storage.MapErrorto handle potentialErrNotFoundcases.- More informative error messages that include context (such as index numbers) to aid in debugging.
- Structured error creation using
fmt.Errorf, which allows for better error wrapping and context preservation.These changes align well with the PR objectives of introducing a new error type and improving error handling. The code is now more consistent and provides clearer information when errors occur.
To further enhance consistency, consider applying the suggested minor wording changes in the error messages across all functions.
Also applies to: 49-55, 77-85, 107-113
mod/storage/pkg/block/store.go (1)
29-29: LGTM: New import added correctly.The new import for the
storagepackage is correctly added and aliased. This import is necessary for using theErrNotFounderror, which aligns with the PR objective of introducing a new error type.mod/storage/pkg/beacondb/fork.go (1)
44-48: Previous issue still applies: Return zero value ofForkTon errorAs previously noted, when an error occurs, you are returning
falong with the error. The value offmay be invalid or undefined whenerris notnil. Returning this potentially invalidfcould lead to unintended behavior in the caller's code. It's safer to return the zero value ofForkTto ensure no invalid data is propagated.mod/storage/pkg/beacondb/versioning.go (3)
24-25: Necessary import added for error formattingThe addition of the
fmtpackage import is appropriate, enabling the use offmt.Errorffor enhanced error messages.
49-54: Enhanced error handling inGetGenesisValidatorsRootThe inclusion of
storage.MapError(err)and wrapping the error withfmt.Errorfprovides better error context and aids in debugging. This is a good practice for consistent error handling.
65-72: Improved error handling inGetSlotApplying
storage.MapError(err)and formatting the error message withfmt.Errorfenhances clarity and consistency in error reporting.mod/storage/pkg/beacondb/withdrawals.go (2)
23-28: Imports are correctly updatedThe necessary packages have been imported to support error formatting and mapping. This enhances error handling consistency across the module.
35-43: Proper error handling inGetNextWithdrawalIndexThe method correctly maps errors using
storage.MapError(err)and returns a formatted error message with appropriate context. This improves the clarity of error reporting.mod/storage/pkg/beacondb/slashing.go (2)
24-29: Necessary imports addedThe added imports (
errors,fmt, andstorage) are appropriate and necessary for the enhanced error handling in the code.
38-43: Improved error handling withstorage.MapErrorUsing
storage.MapError(err)improves the clarity and consistency of error handling, aligning with the PR objectives to avoid exposingcollections.ErrNotFoundto clients.mod/storage/pkg/beacondb/eth1.go (5)
37-56: Correct Implementation ofGetLatestExecutionPayloadHeaderThe method
GetLatestExecutionPayloadHeadercorrectly retrieves the latest execution payload header with appropriate error handling. The use ofstorage.MapErrorensures that storage-specific errors are properly mapped, and the error messages provide clear context if retrieval fails.
58-70: Proper Error Handling ingetLatestExecutionPayloadVersionThe helper method
getLatestExecutionPayloadVersioneffectively abstracts the retrieval of the latest execution payload version. It includes appropriate error mapping and returns clear, informative error messages, enhancing maintainability and readability.
81-86: Previous Refactor Suggestions Still ApplicableThe earlier refactor suggestions regarding the
SetLatestExecutionPayloadHeadermethod are still valid:
- Simplify the use of the
ctxvariable: Sincectxis assignedkv.ctxand used only once, consider usingkv.ctxdirectly to enhance readability.- Reuse the
versionvariable: You have already assignedversion = payloadHeader.Version(). When setting the active fork version, consider reusing theversionvariable instead of callingpayloadHeader.Version()again.
100-104: Avoid Including Uninitialized Variables in Error MessagesIncluding
idxin the error message when it may not have been successfully retrieved (and defaults to zero) could be misleading. The previous suggestion to excludeidxfrom the error message remains applicable.
124-132: Consistent and Clear Error Handling inGetEth1DataThe method
GetEth1Datacorrectly retrieves the eth1 data with appropriate error handling. The use ofstorage.MapErrorensures consistent error mapping, and the error message provides clear information in case of failure.mod/storage/pkg/beacondb/registry.go (2)
Line range hint
24-266: Code changes enhance error handling and clarityThe addition of
storage.MapError(err)and the consistent wrapping of errors usingfmt.Errorfacross the methods improve the error handling mechanism. These changes provide more informative error messages and enhance the maintainability of the code.
Line range hint
287-343: Error handling improvements are appropriateThe updates to the error handling in the balance-related methods, including
GetBalancesandGetTotalActiveBalances, streamline the error reporting and align with the rest of the codebase's practices.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range comments (1)
mod/storage/pkg/block/store.go (1)
Line range hint
1-124: Overall LGTM with a suggestion for further improvement.The changes in this file consistently improve error handling across different methods by using the new
errors.ErrNotFoundtype and providing more context in error messages. This aligns well with the PR objectives of introducing a new error type and improving error handling.To further enhance maintainability and consistency, consider implementing the following suggestion across all methods:
- Define constants for error message formats at the package level.
- Use these constants in the respective error handling sections.
Example:
const ( errMsgBlockRootNotFound = "%w, block root: %s" errMsgExecutionNumberNotFound = "%w, execution number: %d" errMsgStateRootNotFound = "%w, state root: %s" ) // Then use these constants in the respective methods: return 0, fmt.Errorf(errMsgBlockRootNotFound, errors.ErrNotFound, blockRoot)This approach will centralize error message definitions, making future updates easier and ensuring consistency across the package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (11)
- mod/storage/pkg/beacondb/eth1.go (5 hunks)
- mod/storage/pkg/beacondb/fork.go (2 hunks)
- mod/storage/pkg/beacondb/history.go (4 hunks)
- mod/storage/pkg/beacondb/randao.go (2 hunks)
- mod/storage/pkg/beacondb/registry.go (14 hunks)
- mod/storage/pkg/beacondb/slashing.go (2 hunks)
- mod/storage/pkg/beacondb/versioning.go (3 hunks)
- mod/storage/pkg/beacondb/withdrawals.go (2 hunks)
- mod/storage/pkg/block/store.go (4 hunks)
- mod/storage/pkg/block/store_test.go (3 hunks)
- mod/storage/pkg/errors/errors.go (1 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/storage/pkg/beacondb/registry.go (1)
Learnt from: abi87 PR: berachain/beacon-kit#2065 File: mod/storage/pkg/beacondb/registry.go:330-331 Timestamp: 2024-10-13T14:18:59.472Z Learning: When reviewing PRs that are purely for refactoring, avoid suggesting logic changes or code modifications that are out of scope.
🔇 Additional comments (35)
mod/storage/pkg/errors/errors.go (5)
1-20: LGTM: License header is present and correctly formatted.The Business Source License (BSL) 1.1 is properly included at the beginning of the file, providing clear terms of use and necessary disclaimers.
21-27: LGTM: Package declaration and imports are appropriate.The package name
errorsis suitable for an error handling package. The imports are relevant to the functionality provided in this file, including the standarderrorspackage andcosmossdk.io/collections.
29-29: LGTM: ErrNotFound declaration is well-defined.The
ErrNotFoundvariable is appropriately declared as a package-level variable with a clear and descriptive error message. This approach allows for consistent error handling across the package and follows Go naming conventions for errors.
31-44: LGTM: MapError function is well-implemented and aligns with PR objectives.The
MapErrorfunction effectively achieves the goal of replacingcollections.ErrNotFoundwith the customErrNotFound. The implementation is clear, concise, and handles all cases appropriately:
- Returns
nilfor nil errors.- Maps
collections.ErrNotFoundtoErrNotFound.- Returns the original error for all other cases.
This implementation aligns perfectly with the PR objectives of providing a more uniform error handling mechanism and reducing direct dependencies on Cosmos SDK error types.
1-44: Excellent implementation that fully meets PR objectives.This new
errors.gofile successfully introduces a custom error handling mechanism for storage-related operations, aligning perfectly with the PR objectives. Key points:
- The custom
ErrNotFounderror provides a clear, storage-specific error message.- The
MapErrorfunction effectively replacescollections.ErrNotFoundwithErrNotFound, reducing direct dependencies on Cosmos SDK error types.- The implementation allows for more uniform error handling across the package.
This change will indeed improve the ability of clients to understand when an object is not found through a specific error rather than through silent mapping in the storage package.
mod/storage/pkg/beacondb/randao.go (1)
23-28: LGTM: Necessary imports addedThe new imports for "fmt" and "errors" are correctly added to support the changes in the
GetRandaoMixAtIndexmethod.mod/storage/pkg/beacondb/history.go (2)
23-28: LGTM: Import statements updated appropriately.The new imports for "fmt" and the custom errors package are correctly added to support the enhanced error handling introduced in this PR.
Line range hint
1-116: Overall LGTM: Error handling improvements align well with PR objectives.The changes in this file successfully implement the improved error handling as described in the PR objectives. Key improvements include:
- Introduction of
errors.MapError, which likely handles the mapping to the newbeacondb.ErrNotFoundtype.- More informative error messages that include context (such as index values) where applicable.
- Consistent error handling approach across multiple methods.
These changes will provide clearer indications to clients when objects are not found, addressing the main goal of the PR. The suggested minor wording adjustments for consistency will further enhance the overall quality of the error messages.
Great job on this refactoring effort!
mod/storage/pkg/block/store.go (4)
29-29: LGTM: New import aligns with PR objective.The addition of the
errorspackage import is necessary for using the newerrors.ErrNotFoundtype, which aligns with the PR objective of introducing a new error type for improved error handling.
85-89: LGTM: Error handling improved with more context.The changes align with the PR objective by using the new
errors.ErrNotFounderror type. The error message now includes the block root, providing more context for debugging.As previously suggested, consider using a constant for the error message format to improve maintainability:
const errMsgBlockRootNotFound = "%w, block root: %s" return 0, fmt.Errorf(errMsgBlockRootNotFound, errors.ErrNotFound, blockRoot)
102-103: LGTM: Error handling improved with more context.The changes align with the PR objective by using the new
errors.ErrNotFounderror type. The error message now includes the execution number, providing more context for debugging.As previously suggested, consider using a constant for the error message format to improve maintainability:
const errMsgExecutionNumberNotFound = "%w, execution number: %d" return 0, fmt.Errorf(errMsgExecutionNumberNotFound, errors.ErrNotFound, executionNumber)
116-120: LGTM: Error handling improved with more context.The changes align with the PR objective by using the new
errors.ErrNotFounderror type. The error message now includes the state root, providing more context for debugging.As previously suggested, consider using a constant for the error message format to improve maintainability:
const errMsgStateRootNotFound = "%w, state root: %s" return 0, fmt.Errorf(errMsgStateRootNotFound, errors.ErrNotFound, stateRoot)mod/storage/pkg/beacondb/fork.go (1)
44-49: Previous issue regarding returning potentially invalidfon error remains unaddressedmod/storage/pkg/beacondb/versioning.go (3)
24-28: Approved: Correct import and error mapping implementationThe addition of the
fmtimport and the usage oferrors.MapError(err)enhance the error handling by properly mapping and formatting errors.
46-54: Approved: Improved error handling inGetGenesisValidatorsRootThe
GetGenesisValidatorsRootmethod now correctly maps errors and provides a descriptive error message when retrieval fails.
65-72: Approved: Improved error handling inGetSlotThe
GetSlotmethod now correctly maps errors and provides a descriptive error message when retrieval fails.mod/storage/pkg/beacondb/withdrawals.go (3)
23-28: Imports Add Necessary PackagesThe addition of the
fmtanderrorspackages is appropriate and necessary for the enhanced error handling implemented in the methods.
35-43: Improved Error Handling inGetNextWithdrawalIndexThe use of
errors.MapError(err)and the detailed error message enhances the clarity and consistency of error handling in theGetNextWithdrawalIndexmethod.
64-72: Enhanced Error Handling inGetNextWithdrawalValidatorIndexThe introduction of
errors.MapError(err)and the informative error message improves the robustness of theGetNextWithdrawalValidatorIndexmethod.mod/storage/pkg/beacondb/slashing.go (3)
24-26: Addition of imports is appropriateThe added imports of
"errors"and"fmt"are necessary for the updated error handling and formatting.
Line range hint
58-64: Consistent error handling in 'GetSlashingAtIndex'The error handling in
GetSlashingAtIndexis clear and returns zero when the slashing at the specified index is not found, which aligns with the PR objectives.
Line range hint
76-82: Consistent error handling in 'GetTotalSlashing'The updated error handling in
GetTotalSlashingappropriately returns zero when no total slashing is found, maintaining consistency across methods.mod/storage/pkg/beacondb/eth1.go (2)
37-56: Well-structured error handling inGetLatestExecutionPayloadHeaderThe revisions to the
GetLatestExecutionPayloadHeadermethod improve error handling by utilizing the newgetLatestExecutionPayloadVersionhelper function. This enhances code readability and ensures consistent error messages across the codebase.
58-70: Effective use of helper functiongetLatestExecutionPayloadVersionIntroducing the
getLatestExecutionPayloadVersionmethod encapsulates the logic for retrieving the latest execution payload version. This refactoring reduces code duplication and simplifies error handling, making the code more maintainable.mod/storage/pkg/beacondb/registry.go (11)
24-25: Addition of necessary imports for error handlingThe inclusion of the
"fmt"and"github.com/berachain/beacon-kit/mod/storage/pkg/errors"packages is appropriate for the enhanced error handling implemented in the code.Also applies to: 29-29
39-44: Improved error handling inAddValidatorThe mapping of errors using
errors.MapError(err)and the detailed error message improve the clarity and robustness of theAddValidatormethod.
62-67: Consistent error handling inAddValidatorBartioApplying
errors.MapError(err)and providing a descriptive error message enhances consistency and maintainability in error handling across methods.
101-107: Appropriate error formatting inValidatorIndexByPubkeyUsing
%xto format thepubkeyin the error message ensures that the public key is displayed correctly as a hexadecimal string, improving readability.
123-129: Correct error formatting inValidatorIndexByCometBFTAddressFormatting
cometBFTAddresswith%xin the error message appropriately represents the address in hexadecimal form, enhancing clarity.
142-151: Enhanced error handling inValidatorByIndexThe addition of error mapping and a detailed error message provides better insight when retrieval by index fails, improving debugging efficiency.
162-167: Improved error handling inGetValidatorsMapping errors and providing descriptive error messages when peeking the next validator index and iterating over validators enhance the robustness of the method.
Also applies to: 176-181
228-233: Consistent error handling inGetValidatorsByEffectiveBalanceThe method now consistently handles errors during iteration, and the enhancements improve maintainability and error reporting.
Also applies to: 242-242
258-266: Robust error handling inGetBalanceMapping the error and providing a detailed message when retrieving a balance ensures that issues are clearly communicated.
287-292: Enhanced error reporting inGetBalancesThe improved error handling during balance iteration aligns with the error handling strategy used in other methods, promoting consistency.
317-322: Refined error handling and encapsulation inGetTotalActiveBalancesThe changes include:
- Error mapping and descriptive messages enhance clarity when iterations fail.
- Refactoring to use the
GetSlot()method improves encapsulation and aligns with best practices.These adjustments collectively improve the method's readability and maintainability.
Also applies to: 325-343
| err = errors.MapError(err) | ||
| if err != nil { | ||
| return common.Bytes32{}, err | ||
| return common.Bytes32{}, fmt.Errorf( | ||
| "failed retrieving randao mix at index %d: %w", | ||
| index, | ||
| err, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improved error handling, but consider explicit ErrNotFound check
The changes improve error handling by using errors.MapError and providing a formatted error message. This aligns with the PR objectives to enhance error reporting.
However, to fully meet the PR objectives, consider explicitly checking for collections.ErrNotFound and returning beacondb.ErrNotFound. This would ensure that collections.ErrNotFound is never returned to clients, as specified in the PR summary.
Here's a suggested implementation:
bz, err := kv.randaoMix.Get(kv.ctx, index)
if errors.Is(err, collections.ErrNotFound) {
return common.Bytes32{}, fmt.Errorf(
"failed retrieving randao mix at index %d: %w",
index,
beacondb.ErrNotFound,
)
} else if err != nil {
return common.Bytes32{}, fmt.Errorf(
"failed retrieving randao mix at index %d: %w",
index,
err,
)
}
return common.Bytes32(bz), nilThis change would make the use of beacondb.ErrNotFound explicit and ensure that collections.ErrNotFound is never returned to clients.
| err = errors.MapError(err) | ||
| if err != nil { | ||
| return common.Root{}, err | ||
| return common.Root{}, fmt.Errorf( | ||
| "failed retrieving block root at index %d: %w", | ||
| index, | ||
| err, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM with a minor suggestion for consistency.
The error handling improvements in GetBlockRootAtIndex align well with the PR objectives. The use of errors.MapError likely handles the mapping to the new ErrNotFound type, and the error message now includes the index, providing more context.
For consistency with other error messages, consider updating the error message:
- "failed retrieving block root at index %d: %w",
+ "failed to retrieve block root at index %d: %w",This minor change would make the error messages more uniform across the file.
📝 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.
| err = errors.MapError(err) | |
| if err != nil { | |
| return common.Root{}, err | |
| return common.Root{}, fmt.Errorf( | |
| "failed retrieving block root at index %d: %w", | |
| index, | |
| err, | |
| ) | |
| err = errors.MapError(err) | |
| if err != nil { | |
| return common.Root{}, fmt.Errorf( | |
| "failed to retrieve block root at index %d: %w", | |
| index, | |
| err, | |
| ) |
| err = errors.MapError(err) | ||
| if err != nil { | ||
| return common.Root{}, err | ||
| return common.Root{}, fmt.Errorf( | ||
| "failed retrieving state root at index %d: %w", | ||
| idx, | ||
| err, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM with minor suggestions for consistency and clarity.
The error handling improvements in StateRootAtIndex align well with the PR objectives. The use of errors.MapError likely handles the mapping to the new ErrNotFound type, and the error message now includes the index, providing more context.
For consistency with other error messages and to improve clarity, consider the following changes:
- Use "failed to retrieve" instead of "failed retrieving" for consistency.
- Use "index" instead of "idx" in the error message for clarity.
Apply this diff to implement these changes:
- "failed retrieving state root at index %d: %w",
- idx,
+ "failed to retrieve state root at index %d: %w",
+ index,These changes will make the error messages more uniform and clear across the file.
📝 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.
| err = errors.MapError(err) | |
| if err != nil { | |
| return common.Root{}, err | |
| return common.Root{}, fmt.Errorf( | |
| "failed retrieving state root at index %d: %w", | |
| idx, | |
| err, | |
| ) | |
| err = errors.MapError(err) | |
| if err != nil { | |
| return common.Root{}, fmt.Errorf( | |
| "failed to retrieve state root at index %d: %w", | |
| index, | |
| err, | |
| ) |
| h, err := kv.latestBlockHeader.Get(kv.ctx) | ||
| err = errors.MapError(err) | ||
| if err != nil { | ||
| return h, fmt.Errorf( | ||
| "failed retrieving latest block header: %w", | ||
| err, | ||
| ) | ||
| } | ||
| return h, nil |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM with a minor suggestion for consistency.
The error handling improvements in GetLatestBlockHeader are well-aligned with the PR objectives. The use of errors.MapError likely handles the mapping to the new ErrNotFound type, and the error message is now more descriptive.
For consistency with the suggested changes in other methods, consider updating the error message:
- "failed retrieving latest block header: %w",
+ "failed to retrieve latest block header: %w",This minor change would make the error messages more uniform across the file.
📝 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.
| h, err := kv.latestBlockHeader.Get(kv.ctx) | |
| err = errors.MapError(err) | |
| if err != nil { | |
| return h, fmt.Errorf( | |
| "failed retrieving latest block header: %w", | |
| err, | |
| ) | |
| } | |
| return h, nil | |
| h, err := kv.latestBlockHeader.Get(kv.ctx) | |
| err = errors.MapError(err) | |
| if err != nil { | |
| return h, fmt.Errorf( | |
| "failed to retrieve latest block header: %w", | |
| err, | |
| ) | |
| } | |
| return h, nil |
mod/storage/pkg/beacondb/slashing.go
Outdated
| err = storageerr.MapError(err) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf( | ||
| "failed iterating slashings: %w", | ||
| err, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure the iterator is properly closed to prevent resource leaks
After obtaining the iterator from kv.slashings.Iterate, it should be closed after use to prevent potential resource leaks. It's recommended to add defer iter.Close() after checking for errors.
Apply this diff to close the iterator:
func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
]) GetSlashings() ([]math.Gwei, error) {
var slashings []math.Gwei
iter, err := kv.slashings.Iterate(kv.ctx, nil)
err = storageerr.MapError(err)
if err != nil {
return nil, fmt.Errorf(
"failed iterating slashings: %w",
err,
)
}
+ defer iter.Close()
for iter.Valid() {
var slashing uint64
slashing, err = iter.Value()
if err != nil {
return nil, err
}
slashings = append(slashings, math.Gwei(slashing))
iter.Next()
}
return slashings, nil
}📝 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.
| err = storageerr.MapError(err) | |
| if err != nil { | |
| return nil, err | |
| return nil, fmt.Errorf( | |
| "failed iterating slashings: %w", | |
| err, | |
| ) | |
| } | |
| err = storageerr.MapError(err) | |
| if err != nil { | |
| return nil, fmt.Errorf( | |
| "failed iterating slashings: %w", | |
| err, | |
| ) | |
| } | |
| defer iter.Close() | |
🛠️ Refactor suggestion
Consider handling 'collections.ErrNotFound' to return an empty slice
In GetSlashings, if there are no slashing entries, kv.slashings.Iterate might return a collections.ErrNotFound error. To make the behavior consistent with GetSlashingAtIndex and GetTotalSlashing, consider checking for collections.ErrNotFound and returning an empty slice without an error.
Apply this diff:
err = storageerr.MapError(err)
+if errors.Is(err, collections.ErrNotFound) {
+ return []math.Gwei{}, nil
+} else if err != nil {
return nil, fmt.Errorf(
"failed iterating slashings: %w",
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.
| err = storageerr.MapError(err) | |
| if err != nil { | |
| return nil, err | |
| return nil, fmt.Errorf( | |
| "failed iterating slashings: %w", | |
| err, | |
| ) | |
| } | |
| err = storageerr.MapError(err) | |
| if errors.Is(err, collections.ErrNotFound) { | |
| return []math.Gwei{}, nil | |
| } else if err != nil { | |
| return nil, fmt.Errorf( | |
| "failed iterating slashings: %w", | |
| err, | |
| ) | |
| } | |
There was a problem hiding this comment.
Right! I will fix the iterator issue in #2077
| var ( | ||
| ctx = kv.ctx | ||
| version = payloadHeader.Version() | ||
| ) | ||
| if err := kv.latestExecutionPayloadVersion.Set(ctx, version); err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify SetLatestExecutionPayloadHeader by removing unnecessary variable
The variable ctx is assigned kv.ctx but is used only once. You can simplify the code by using kv.ctx directly, eliminating the need for the extra variable.
Apply this diff to streamline the function:
func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
]) SetLatestExecutionPayloadHeader(
payloadHeader ExecutionPayloadHeaderT,
) error {
- var (
- ctx = kv.ctx
- version = payloadHeader.Version()
- )
+ version := payloadHeader.Version()
- if err := kv.latestExecutionPayloadVersion.Set(ctx, version); err != nil {
+ if err := kv.latestExecutionPayloadVersion.Set(kv.ctx, version); err != nil {
return err
}
kv.latestExecutionPayloadCodec.SetActiveForkVersion(payloadHeader.Version())
return kv.latestExecutionPayloadHeader.Set(kv.ctx, payloadHeader)
}📝 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.
| var ( | |
| ctx = kv.ctx | |
| version = payloadHeader.Version() | |
| ) | |
| if err := kv.latestExecutionPayloadVersion.Set(ctx, version); err != nil { | |
| version := payloadHeader.Version() | |
| if err := kv.latestExecutionPayloadVersion.Set(kv.ctx, version); err != nil { |
| idx, err := kv.eth1DepositIndex.Get(kv.ctx) | ||
| err = errors.MapError(err) | ||
| if err != nil { | ||
| return 0, fmt.Errorf( | ||
| "failed retrieving eth1 deposit index %d, %w", | ||
| idx, | ||
| err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Avoid including uninitialized variables in error messages
In the GetEth1DepositIndex method, including idx in the error message before it's successfully retrieved could be misleading, as it may contain a default value (e.g., zero). For clarity, consider modifying the error message to exclude idx.
Apply this diff to adjust the error message:
if err != nil {
- return 0, fmt.Errorf(
- "failed retrieving eth1 deposit index %d, %w",
- idx,
- err,
- )
+ return 0, fmt.Errorf(
+ "failed retrieving eth1 deposit index: %w",
+ 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.
| idx, err := kv.eth1DepositIndex.Get(kv.ctx) | |
| err = errors.MapError(err) | |
| if err != nil { | |
| return 0, fmt.Errorf( | |
| "failed retrieving eth1 deposit index %d, %w", | |
| idx, | |
| err, | |
| ) | |
| } | |
| idx, err := kv.eth1DepositIndex.Get(kv.ctx) | |
| err = errors.MapError(err) | |
| if err != nil { | |
| return 0, fmt.Errorf( | |
| "failed retrieving eth1 deposit index: %w", | |
| err, | |
| ) | |
| } |
| ) | ||
| } | ||
|
|
||
| kv.latestExecutionPayloadCodec.SetActiveForkVersion(v) |
There was a problem hiding this comment.
💡 Codebase verification
SetActiveForkVersion Does Not Validate Version Inputs
The SetActiveForkVersion method in mod/storage/pkg/encoding/ssz.go assigns the provided version directly without validating whether it is valid or supported. This lack of validation may lead to unexpected behavior if an invalid or unsupported version is set.
- File:
mod/storage/pkg/encoding/ssz.goSetActiveForkVersionlacks error handling for invalid versions.
🔗 Analysis chain
Ensure SetActiveForkVersion handles invalid versions
When setting the active fork version with kv.latestExecutionPayloadCodec.SetActiveForkVersion(v), it's important to verify that invalid or unsupported versions are properly handled. Lack of validation may lead to unexpected behavior elsewhere in the code.
Run the following script to check the implementation of SetActiveForkVersion:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that SetActiveForkVersion handles invalid versions.
# Search for the SetActiveForkVersion method implementation and check for error handling.
rg --type go 'func\s+\([^)]+\)\s+SetActiveForkVersion\s*\(' -A 15
# Review the implementation to ensure it includes validation logic.
Length of output: 1081
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (1)
mod/state-transition/pkg/core/state/statedb.go (1)
Line range hint
1-424: Consider reviewing other methods for error handling improvementsWhile the changes in
UpdateSlashingAtIndexalign with the PR objectives, it might be beneficial to review other methods in this file (e.g.,IncreaseBalance,DecreaseBalance,ExpectedWithdrawals) for potential error handling improvements. This would ensure consistency across the entireStateDBstruct and further enhance the robustness of the code.For instance, methods that interact with storage (like
GetBalanceinIncreaseBalanceandDecreaseBalance) might benefit from similar error handling patterns introduced inUpdateSlashingAtIndex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
mod/state-transition/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
- mod/state-transition/go.mod (4 hunks)
- mod/state-transition/pkg/core/state/statedb.go (2 hunks)
- mod/storage/pkg/beacondb/slashing.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (9)
mod/state-transition/go.mod (3)
12-12: LGTM: Updated errors module version.The update to
github.com/berachain/beacon-kit/mod/errorsaligns with the PR objectives of improving error handling. This change likely introduces the newbeacondb.ErrNotFounderror type mentioned in the PR summary.
22-23: LGTM: New indirect dependencies added.The addition of new indirect dependencies, including
cosmossdk.io/collections,cosmossdk.io/core, andgithub.com/google/go-cmp, appears to support the new error handling mechanisms and integration with Cosmos SDK. The specific versions used contribute to build stability.Also applies to: 56-56
28-28: LGTM: Updated chain-spec module version.The update to
github.com/berachain/beacon-kit/mod/chain-specis likely related to changes supporting the new error handling mechanisms. The minor version change suggests incremental improvements that align with the PR objectives.mod/state-transition/pkg/core/state/statedb.go (1)
27-27: LGTM: New import for improved error handlingThe addition of the
storageerrorsimport aligns with the PR objective of improving error handling for storage-related operations. This change will allow for more specific error handling in theStateDBmethods.mod/storage/pkg/beacondb/slashing.go (5)
24-27: Appropriate import additionsThe inclusion of
"fmt"and"github.com/berachain/beacon-kit/mod/storage/pkg/errors"is necessary and appropriate for the enhanced error handling implemented in the code.
36-41: Ensure the iterator is properly closed to prevent resource leaksAs previously mentioned, after obtaining the iterator from
kv.slashings.Iterate, it should be closed after use to prevent potential resource leaks. Consider addingdefer iter.Close()after checking for errors.
36-41: Consider handling 'beacondb.ErrNotFound' to return an empty sliceIf there are no slashing entries,
kv.slashings.Iteratemight return abeacondb.ErrNotFounderror. To maintain consistency with other methods and enhance client experience, consider checking for this error and returning an empty slice without an error.
Line range hint
64-69: Proper error mapping and handling inGetSlashingAtIndexThe use of
errors.MapError(err)followed by returning the error directly ensures that specific errors likebeacondb.ErrNotFoundare properly propagated to the clients. This aligns with the PR objectives to improve error uniformity and clarity.
Line range hint
88-93: Consistent error handling inGetTotalSlashingThe addition of
errors.MapError(err)and the unified error handling approach enhance the method's reliability. Propagating mapped errors directly allows clients to receive more informative error messages, fulfilling the PR's intent.
| github.com/berachain/beacon-kit/mod/primitives v0.0.0-20240911165923-82f71ec86570 | ||
| github.com/berachain/beacon-kit/mod/storage v0.0.0-00010101000000-000000000000 |
There was a problem hiding this comment.
Update the placeholder version for the storage module.
The github.com/berachain/beacon-kit/mod/storage module is added with a placeholder version (v0.0.0-00010101000000-000000000000). This is likely due to the local replacement directive. Before merging, ensure that this is replaced with a proper version or commit hash to maintain reproducible builds.
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- mod/storage/pkg/beacondb/registry.go (14 hunks)
- mod/storage/pkg/beacondb/slashing.go (4 hunks)
- mod/storage/pkg/block/store.go (4 hunks)
- mod/storage/pkg/block/store_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (8)
mod/storage/pkg/block/store.go (1)
30-30: LGTM: New import added for error handling.The addition of the
errorspackage import is necessary and aligns with the changes made in the file.mod/storage/pkg/beacondb/slashing.go (2)
67-68: Consistent error handling inGetSlashingAtIndexThe error mapping using
errors.MapError(err)and subsequent checks align with the updated error handling strategy.
91-92: Consistent error mapping inGetTotalSlashingThe use of
errors.MapError(err)ensures uniform error processing across the codebase.mod/storage/pkg/beacondb/registry.go (5)
42-45: Clarify error message inAddValidatorConsider rephrasing the error message for better clarity, as previously suggested.
66-68: Clarify error message inAddValidatorBartioConsider rephrasing the error message for consistency and clarity, similar to prior feedback.
104-108: Proper formatting ofpubkeyin error messagesGood job updating the error message to use
%xforpubkey, ensuring correct hexadecimal representation.
126-130: Proper formatting ofcometBFTAddressin error messagesWell done on changing the format specifier to
%xforcometBFTAddress, providing accurate hexadecimal output.
324-324: Clarify error message when retrieving slotThe previous suggestion to specify the failure in retrieving the slot value remains applicable.
| @@ -39,15 +40,15 @@ func (m MockBeaconBlock) GetSlot() math.Slot { | |||
| } | |||
|
|
|||
| func (m MockBeaconBlock) HashTreeRoot() common.Root { | |||
| return [32]byte{byte(m.slot)} | |||
| return common.Root{byte(m.slot)} | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider full initialization of common.Root
While the change to common.Root improves type safety, consider initializing it with a full 32-byte array for completeness and to avoid potential issues with uninitialized bytes.
Consider updating the return statement as follows:
return common.Root{byte(m.slot), 0, 0, /* ... */ 0}This ensures all 32 bytes of the common.Root are explicitly initialized.
| } | ||
|
|
||
| func (m MockBeaconBlock) GetTimestamp() math.U64 { | ||
| return m.slot | ||
| } | ||
|
|
||
| func (m MockBeaconBlock) GetStateRoot() common.Root { | ||
| return [32]byte{byte(m.slot)} | ||
| return common.Root{byte(m.slot)} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider full initialization of common.Root
Similar to the HashTreeRoot method, consider initializing the common.Root with a full 32-byte array for completeness and to avoid potential issues with uninitialized bytes.
Consider updating the return statement as follows:
return common.Root{byte(m.slot), 0, 0, /* ... */ 0}This ensures all 32 bytes of the common.Root are explicitly initialized.
| @@ -27,6 +27,7 @@ | |||
| "github.com/berachain/beacon-kit/mod/primitives/pkg/common" | |||
| "github.com/berachain/beacon-kit/mod/primitives/pkg/math" | |||
| "github.com/berachain/beacon-kit/mod/storage/pkg/block" | |||
| "github.com/berachain/beacon-kit/mod/storage/pkg/errors" | |||
There was a problem hiding this comment.
Update import to use beacondb package
To align with the PR's objective of introducing beacondb.ErrNotFound, please update the import statement to use the beacondb package instead of the errors package.
Apply this diff to make the necessary change:
- "github.com/berachain/beacon-kit/mod/storage/pkg/errors"
+ "github.com/berachain/beacon-kit/mod/storage/pkg/beacondb"📝 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.
| "github.com/berachain/beacon-kit/mod/storage/pkg/errors" | |
| "github.com/berachain/beacon-kit/mod/storage/pkg/beacondb" |
| _, err = blockStore.GetSlotByBlockRoot(common.Root{byte(8)}) | ||
| require.ErrorIs(t, err, errors.ErrNotFound) | ||
| _, err = blockStore.GetParentSlotByTimestamp(2) | ||
| require.ErrorContains(t, err, "not found") | ||
| require.ErrorIs(t, err, errors.ErrNotFound) | ||
| _, err = blockStore.GetSlotByStateRoot(common.Root{byte(8)}) | ||
| require.ErrorIs(t, err, errors.ErrNotFound) |
There was a problem hiding this comment.
Use beacondb.ErrNotFound for consistency
To align with the PR's objective of introducing beacondb.ErrNotFound, please update the error checks to use beacondb.ErrNotFound instead of errors.ErrNotFound. This ensures consistent error handling across the codebase.
Apply this diff to make the necessary changes:
- require.ErrorIs(t, err, errors.ErrNotFound)
+ require.ErrorIs(t, err, beacondb.ErrNotFound)
- require.ErrorIs(t, err, errors.ErrNotFound)
+ require.ErrorIs(t, err, beacondb.ErrNotFound)
- require.ErrorIs(t, err, errors.ErrNotFound)
+ require.ErrorIs(t, err, beacondb.ErrNotFound)Remember to update the import statement as suggested in the previous comment.
📝 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.
| _, err = blockStore.GetSlotByBlockRoot(common.Root{byte(8)}) | |
| require.ErrorIs(t, err, errors.ErrNotFound) | |
| _, err = blockStore.GetParentSlotByTimestamp(2) | |
| require.ErrorContains(t, err, "not found") | |
| require.ErrorIs(t, err, errors.ErrNotFound) | |
| _, err = blockStore.GetSlotByStateRoot(common.Root{byte(8)}) | |
| require.ErrorIs(t, err, errors.ErrNotFound) | |
| _, err = blockStore.GetSlotByBlockRoot(common.Root{byte(8)}) | |
| require.ErrorIs(t, err, beacondb.ErrNotFound) | |
| _, err = blockStore.GetParentSlotByTimestamp(2) | |
| require.ErrorIs(t, err, beacondb.ErrNotFound) | |
| _, err = blockStore.GetSlotByStateRoot(common.Root{byte(8)}) | |
| require.ErrorIs(t, err, beacondb.ErrNotFound) |
| return 0, fmt.Errorf( | ||
| "%w, block root: %s", | ||
| errors.ErrNotFound, | ||
| blockRoot, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Error handling improved with more context.
The changes align with the PR objective by using the new errors.ErrNotFound error type. The error message now includes the block root, providing more context for debugging.
Consider using a constant for the error message format to improve maintainability:
const errMsgBlockRootNotFound = "%w, block root: %s"
return 0, fmt.Errorf(errMsgBlockRootNotFound, errors.ErrNotFound, blockRoot)| return 0, fmt.Errorf( | ||
| "%w, timestamp: %d", | ||
| errors.ErrNotFound, | ||
| timestamp, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Error handling improved with more context.
The changes align with the PR objective by using the new errors.ErrNotFound error type. The error message now includes the timestamp, providing more context for debugging. The use of stderrors.New for the genesis slot error is appropriate.
Consider using constants for the error message formats to improve maintainability:
const (
errMsgTimestampNotFound = "%w, timestamp: %d"
errMsgGenesisSlotParent = "parent slot not supported for genesis slot 0"
)
// In the first error case
return 0, fmt.Errorf(errMsgTimestampNotFound, errors.ErrNotFound, timestamp)
// In the genesis slot case
return slot, stderrors.New(errMsgGenesisSlotParent)Also applies to: 117-117
| return 0, fmt.Errorf( | ||
| "%w, state root: %s", | ||
| errors.ErrNotFound, | ||
| stateRoot, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Error handling improved with more context.
The changes align with the PR objective by using the new errors.ErrNotFound error type. The error message now includes the state root, providing more context for debugging.
Consider using a constant for the error message format to improve maintainability:
const errMsgStateRootNotFound = "%w, state root: %s"
return 0, fmt.Errorf(errMsgStateRootNotFound, errors.ErrNotFound, stateRoot)| err = errors.MapError(err) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf( | ||
| "failed iterating slashings: %w", | ||
| err, | ||
| ) | ||
| } | ||
| defer func() { | ||
| err = errors.Join(err, iter.Close()) | ||
| err = stderrors.Join(err, iter.Close()) |
There was a problem hiding this comment.
Ensure err is a named return value to capture deferred errors properly
In the GetSlashings function, the deferred function modifies err to include any error from iter.Close(). However, since err is not a named return parameter, this modification will not affect the returned error value. To ensure that any error from iter.Close() is correctly returned, err should be a named return parameter.
Apply this diff to modify the function signature:
-func (kv *KVStore[
+func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
-]) GetSlashings() ([]math.Gwei, error) {
+]) GetSlashings() (slashings []math.Gwei, err error) {
var slashings []math.Gwei
iter, err := kv.slashings.Iterate(kv.ctx, nil)
err = errors.MapError(err)
if err != nil {
return nil, fmt.Errorf(
"failed iterating slashings: %w",
err,
)
}
defer func() {
err = stderrors.Join(err, iter.Close())
}()
// rest of the code remains unchanged
}Committable suggestion was skipped due to low confidence.
abi87
added
DO NOT MERGE
Introducing
beacondb.ErrNotFoundto duly handle cases where queried object from storage is not found.Currently, whenever an item is not found we:
collections.ErrNotFoundas is to the clientcollections.ErrNotFoundto the default value, nil error pair.There are a few things that I don't like in this arrangement:
This PR is a step towards solving the issue: The PR inspects errors returned by cosmos sdk collections and maps
collections.ErrNotFoundto a package levelErrNotFoundThis PR is a pure refactoring; it only cares of making sure that
collections.ErrNotFoundis not returned to the clients anymore. Changes to the mapping will be carried out in separate PRs to simplify code review.Note: This PR focuses on errors from getting from or iterating over collections. Cosmos sdk errors from item settings are not modified since they cannot return
collections.ErrNotFounderrors.TODO: consider handling absence of validators as
ErrNotFoundas well?