fix(observer): remove error return on staking hooks
#2968
Conversation
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing error handling within the Changes
Sequence Diagram(s)sequenceDiagram
participant Hooks
participant Logger
participant Voting
Hooks->>Logger: Log error "Error cleaning observer set"
Hooks->>Hooks: Return nil
Voting->>Voting: CheckObserverSelfDelegation
Voting->>Voting: Wrap error with context
Voting->>Logger: Log wrapped error
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
Documentation and Community
|
lumtis
changed the title
observer): remove error return on staking hooks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
x/observer/keeper/hooks.go (2)
38-44: Implement a comprehensive error handling strategyThe modifications across all methods in this file follow a consistent pattern of logging errors without returning them. While this approach maintains method signatures and backwards compatibility, it raises concerns about error propagation and system observability.
To address these concerns comprehensively, consider the following recommendations:
- Implement a uniform error handling strategy across all methods in this file.
- Create a custom error type that encapsulates both the original error and additional context.
- Refactor the error handling logic into a separate function to reduce duplication.
- Ensure that errors are both logged and returned to maintain proper error propagation.
Here's a suggested implementation that combines these recommendations:
type ObserverError struct { OriginalError error Context string } func (e *ObserverError) Error() string { return fmt.Sprintf("%s: %v", e.Context, e.OriginalError) } func logAndWrapError(ctx sdk.Context, err error, message string) error { if err != nil { ctx.Logger().Error(message, "error", err) return &ObserverError{OriginalError: err, Context: message} } return nil } // Then in each method: func (h Hooks) BeforeValidatorSlashed(ctx sdk.Context, valAddr sdk.ValAddress, fraction sdk.Dec) error { err := h.k.CleanSlashedValidator(ctx, valAddr, fraction) return logAndWrapError(ctx, err, "Error cleaning observer set") }This approach would provide a robust and consistent error handling mechanism throughout the file, improving both error reporting and system maintainability.
Line range hint
1-44: Summary: Revise error handling strategy across the codebaseThe changes in this file represent a significant shift in error handling strategy, moving from returning errors to logging them. While this approach maintains method signatures, it introduces potential issues with error propagation and system observability.
Recommendations for next steps:
- Conduct a thorough review of the error handling strategy across the entire codebase.
- Implement a consistent and robust error handling mechanism that balances logging and error propagation.
- Consider introducing a custom error type that preserves context while allowing for detailed logging.
- Refactor common error handling logic into reusable functions to reduce code duplication.
- Update documentation to reflect the new error handling approach and its implications for system behavior and debugging.
These changes have far-reaching implications and should be carefully considered in the context of the entire system's architecture and operational requirements.
x/observer/keeper/voting.go (1)
138-143: Approval: Improved error context in CheckObserverSelfDelegation.The modifications to error handling in the
CheckObserverSelfDelegationfunction are commendable. The use oferrors.Wrapfprovides more detailed error context, which will significantly aid in debugging and error tracing.However, for consistency and further improvement, consider the following suggestion:
- return errors.Wrapf(types.ErrNotValidator, "validator : %s", valAddress) + return errors.Wrapf(types.ErrNotValidator, "validator: %s", valAddress) - return errors.Wrapf(types.ErrSelfDelegation, "self delegation : %s , valAddres : %s", selfdelAddr, valAddress) + return errors.Wrapf(types.ErrSelfDelegation, "self delegation: %s, valAddress: %s", selfdelAddr, valAddress)This minor adjustment removes the space before the colon, corrects the spelling of "valAddress", and ensures consistent spacing in the error messages. These changes will maintain a uniform style throughout the codebase.
x/observer/keeper/hooks_test.go (1)
142-155: Approve changes with a suggestion for improved test isolation.The new test case effectively covers the scenario where the validator is not found, ensuring that no error is returned and the observer set remains unchanged. This addition enhances the robustness of the test suite.
To further improve test isolation, consider the following suggestion:
t.Run("should not error if validator not found", func(t *testing.T) { - k, ctx, _, _ := keepertest.ObserverKeeper(t) + k, ctx, _, _ := keepertest.ObserverKeeper(t) + ctx = ctx.WithBlockHeight(1) // Ensure a consistent block height for the test r := rand.New(rand.NewSource(9)) validator := sample.Validator(t, r) os := sample.ObserverSet(10) k.SetObserverSet(ctx, os) hooks := k.Hooks() err := hooks.BeforeValidatorSlashed(ctx, validator.GetOperator(), sdk.NewDec(1)) require.NoError(t, err) storedOs, found := k.GetObserverSet(ctx) require.True(t, found) require.Equal(t, os, storedOs) })This change ensures a consistent block height for the test, which can be crucial for deterministic behavior in blockchain-related tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- x/observer/keeper/hooks.go (1 hunks)
- x/observer/keeper/hooks_test.go (1 hunks)
- x/observer/keeper/voting.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/observer/keeper/hooks.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/hooks_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/voting.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (1)
x/observer/keeper/voting.go (1)
5-5: Approval: Enhanced error handling import.The addition of the "github.com/pkg/errors" package import is a positive change. This package provides more robust error handling capabilities, allowing for better error context and stack traces.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2968 +/- ##
========================================
Coverage 66.40% 66.40%
========================================
Files 389 389
Lines 21762 21762
========================================
Hits 14451 14451
Misses 6584 6584
Partials 727 727
|
Description
Staking hook fix was missing on develop
102b872#diff-f6d8e75e5cf1f39354b3a150db855a69895dbf5e6da8d24c89213a17256440dcR143
Summary by CodeRabbit
Bug Fixes
New Features
CheckObserverSelfDelegationmethod for better context and clarity.