refactor: replace panic with error return #740
Conversation
WalkthroughThe pull request introduces significant modifications to the error handling mechanisms within the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
x/crosschain/keeper/pending_execute_claim_test.go (1)
53-54: Improved error handling for GetPendingExecuteClaimThe changes here are consistent with the PR objective and the previous modification. The
GetPendingExecuteClaimmethod now returns both a claim and an error, which are properly handled in the test case. This approach follows Go best practices for error handling.Consider swapping the order of the assertions for better readability:
s.Require().NoError(err) s.Require().Equal(tt.claim, claim)This way, we first ensure there's no error before comparing the claims, which is a more logical order of operations.
x/crosschain/keeper/genesis.go (1)
Line range hint
1-180: Summary: Reconsider error handling strategy in InitGenesisThe changes in this file replace error-checking unpacking with
MustUnpackAttestationClaim, which can lead to panics. This seems to contradict the PR's objective of "replace panic with error return". Consider the following recommendations:
- Revert to the previous error-checking approach for unpacking attestations.
- If simplification is desired, implement a custom error handling mechanism that logs errors and continues initialization, rather than panicking.
- Add validation for attestations earlier in the genesis import process to ensure all attestations are valid before unpacking.
These changes will align better with the PR's objective and improve the robustness of the genesis initialization process.
x/crosschain/keeper/oracle_set.go (1)
Line range hint
1-34: Approve changes with a minor suggestion for improvementThe modification successfully replaces the panic with an error return, which aligns with the PR objective and allows for more graceful error handling. This change is consistent with Go's error handling conventions and doesn't introduce any new logic errors.
To further improve the code, consider wrapping the error returned from the
Equalmethod to provide more context:- return err + return types.ErrInvalid.Wrap(err)This will maintain consistency with the error wrapping used earlier in the function and provide more context about the nature of the error.
x/crosschain/keeper/abci.go (2)
163-173: Approve changes with a minor suggestion for error loggingThe modifications to
cleanupTimedOutBatchesalign well with the PR objective of replacing panics with error returns. The error handling is implemented correctly, and the function now properly returns an error.To further improve error handling and debugging:
func (k Keeper) cleanupTimedOutBatches(ctx sdk.Context) error { externalBlockHeight := k.GetLastObservedBlockHeight(ctx).ExternalBlockHeight k.IterateOutgoingTxBatches(ctx, func(batch *types.OutgoingTxBatch) bool { if batch.BatchTimeout < externalBlockHeight { if err := k.RefundOutgoingTxBatch(ctx, batch.TokenContract, batch.BatchNonce); err != nil { k.Logger(ctx).Error("failed to refund outgoing tx batch", "error", err) return true } } return false }) return nil }This change adds error logging, which can be helpful for debugging issues in production environments.
176-191: Approve changes with a minor suggestion for error loggingThe modifications to
cleanupTimeOutBridgeCallare in line with the PR objective of replacing panics with error returns. The error handling is implemented correctly, and the function now properly returns an error.To further improve error handling and debugging:
func (k Keeper) cleanupTimeOutBridgeCall(ctx sdk.Context) error { externalBlockHeight := k.GetLastObservedBlockHeight(ctx).ExternalBlockHeight k.IterateOutgoingBridgeCalls(ctx, func(data *types.OutgoingBridgeCall) bool { if data.Timeout > externalBlockHeight { return true } if err := k.RefundOutgoingBridgeCall(ctx, data); err != nil { k.Logger(ctx).Error("failed to refund outgoing bridge call", "error", err, "nonce", data.Nonce) return true } k.DeleteOutgoingBridgeCallRecord(ctx, data.Nonce) return false }) return nil }This change adds error logging, which can be helpful for debugging issues in production environments. It also includes the nonce in the error log for easier tracing.
x/crosschain/keeper/pending_execute_claim.go (1)
18-18: Consider returningnilexplicitlyAt the end of
SavePendingExecuteClaim, theerrshould benilif no error occurred. Returningnilexplicitly enhances code clarity.Apply the following diff to improve readability:
- return err + return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- x/crosschain/keeper/abci.go (2 hunks)
- x/crosschain/keeper/attestation.go (5 hunks)
- x/crosschain/keeper/attestation_handler.go (2 hunks)
- x/crosschain/keeper/genesis.go (2 hunks)
- x/crosschain/keeper/oracle_set.go (1 hunks)
- x/crosschain/keeper/outgoing_tx.go (2 hunks)
- x/crosschain/keeper/pending_execute_claim.go (1 hunks)
- x/crosschain/keeper/pending_execute_claim_test.go (1 hunks)
- x/crosschain/types/msgs.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (24)
x/crosschain/keeper/pending_execute_claim_test.go (2)
50-51: Improved error handling for SavePendingExecuteClaimThe changes here align well with the PR objective of replacing panic with error returns. The
SavePendingExecuteClaimmethod now returns an error, which is properly checked in the test case. This approach follows Go best practices for error handling and makes the code more robust.
50-54: Overall improvement in error handlingThe changes in this file significantly improve the error handling in the
TestKeeper_SavePendingExecuteClaimfunction. By updating bothSavePendingExecuteClaimandGetPendingExecuteClaimto return errors and properly checking these errors in the test cases, the code now follows Go best practices more closely.These modifications align well with the PR objective of replacing panic with error returns. They enhance the robustness of the test cases and provide more explicit error handling, which will make debugging easier if issues arise in the future.
The functionality of the tests remains intact, with the added benefit of more comprehensive error checking. This update contributes to the overall quality and reliability of the codebase.
x/crosschain/keeper/genesis.go (1)
95-95:⚠️ Potential issueCaution: Potential panic introduced with
MustUnpackAttestationClaimThe change from
UnpackAttestationClaimtoMustUnpackAttestationClaimsimplifies the code by removing explicit error handling. However, this introduces a potential panic if the unpacking fails. Consider the following:
- Is it guaranteed that all attestations in the genesis state are valid?
- How would a panic at this point affect the node's ability to start up?
Consider keeping the previous error-checking approach or wrapping this call in a recovery mechanism to handle potential panics gracefully.
To verify the safety of this change, let's check for any validation of attestations before this point:
x/crosschain/keeper/oracle_set.go (1)
Line range hint
1-34: Verify impact on callers of UpdateOracleSetExecutedThe change in the
UpdateOracleSetExecutedfunction signature (now returning an error) is correct and aligns with the PR objective. However, it's crucial to ensure that all callers of this function are updated to handle the new error return.Please run the following script to identify and verify all callers of
UpdateOracleSetExecuted:Please review the output to ensure all callers properly handle the returned error. Update any callers that don't handle the error appropriately.
✅ Verification successful
Callers Handle Error from UpdateOracleSetExecuted Properly
Verified that all callers of
UpdateOracleSetExecutedappropriately handle the returned error.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all callers of UpdateOracleSetExecuted and verify they handle the new error return # Search for function calls to UpdateOracleSetExecuted echo "Searching for UpdateOracleSetExecuted callers:" rg --type go "UpdateOracleSetExecuted\(" -A 5 # Search for method calls on Keeper struct echo "\nSearching for Keeper.UpdateOracleSetExecuted callers:" rg --type go "\w+\.UpdateOracleSetExecuted\(" -A 5Length of output: 1575
x/crosschain/keeper/abci.go (1)
Line range hint
1-291: Overall assessment: Changes align well with PR objectivesThe modifications in this file successfully replace panic calls with error returns in the
cleanupTimedOutBatchesandcleanupTimeOutBridgeCallfunctions. These changes improve error handling and align with the PR's objective of refactoring to use error returns instead of panics.The
isNeedOracleSetRequestfunction still uses a method that can panic, which could be further improved to fully align with the PR's goals.The changes enhance the robustness of the code by providing better error propagation and the potential for more detailed error logging. This will likely improve debugging and error handling in production environments.
x/crosschain/keeper/pending_execute_claim.go (7)
4-5: Approved: Addition of "fmt" package for error handlingThe import of the
"fmt"package is necessary for usingfmt.Errorfin error handling.
11-11: Updated function signature to return errorChanging the
SavePendingExecuteClaimfunction to return anerrorenhances error handling by allowing callers to handle potential failures gracefully.
15-15: Replaced panic with error return inSavePendingExecuteClaimReturning the error instead of panicking improves robustness and allows for proper error propagation.
21-21: Updated function signature to return errorModifying the
GetPendingExecuteClaimfunction to return anerrorallows for more informative error handling.
25-25: Return error when claim not foundReturning an error when the claim is not found provides clearer feedback to the caller compared to a boolean value.
29-29: Replaced panic with error return inGetPendingExecuteClaimReturning the unmarshalling error instead of panicking enhances the method's robustness and allows callers to handle errors appropriately.
31-31: Return claim withnilerror upon successReturning the claim along with a
nilerror aligns with Go conventions for successful operations.x/crosschain/keeper/attestation_handler.go (3)
14-14: Properly propagate errors fromSavePendingExecuteClaimThe change at line 14 ensures that errors from
SavePendingExecuteClaimare correctly returned, enhancing error handling and preventing silent failures.
17-17: Properly propagate errors fromOutgoingTxBatchExecutedThe change at line 17 ensures that any errors from
OutgoingTxBatchExecutedare properly returned, improving error propagation and maintaining consistent error handling across the codebase.
31-33: Handle errors fromGetPendingExecuteClaimappropriatelyBy checking and returning errors from
GetPendingExecuteClaim, the code at lines 31-33 now correctly handles potential errors, ensuring that the function does not proceed with invalid data.x/crosschain/keeper/outgoing_tx.go (4)
74-74: Good refactoring: Return errors instead of panickingChanging the function to return errors instead of panicking enhances robustness and allows callers to handle errors gracefully.
Also applies to: 103-103
84-85: Ensure proper error handling within iterationIn the
IterateOutgoingTxBatchescallback, you're assigningerrand breaking the loop by returningtruewhen an error occurs. Ensure that this pattern correctly propagates the first encountered error and that subsequent errors won't be silently ignored.
90-92: Properly propagating errors after batch processingGood job checking for errors after the iteration and returning them to the caller. This ensures that any issues encountered during batch refunds are not overlooked.
74-74: Verify all callers handle the new error returnSince
OutgoingTxBatchExecutednow returns an error, ensure that all callers of this function have been updated to handle the error appropriately.Run the following script to find all usages of
OutgoingTxBatchExecutedand check for proper error handling:x/crosschain/keeper/attestation.go (5)
49-51: Proper Error Handling inAttestFunctionAdding error checking for
k.TryAttestation(ctx, att, claim)and returning the error improves the robustness of theAttestfunction by ensuring that any issues during attestation are correctly propagated to the caller.
64-64: UpdatedTryAttestationSignature to Return ErrorChanging the
TryAttestationmethod to return anerrorallows for better error propagation and handling throughout the codebase, enhancing overall reliability.
76-76: Return Error When Oracle Not FoundReturning
types.ErrNoFoundOraclewhen an oracle is not found ensures that this scenario is properly reported and can be handled by the calling function, preventing potential nil pointer dereferences or unexpected behavior.
107-112: Added Error Checks for Cleanup FunctionsIntroducing error handling for
k.cleanupTimedOutBatches(ctx)andk.cleanupTimeOutBridgeCall(ctx)ensures that any errors during the cleanup process are not silently ignored but are properly propagated and can be addressed.
117-117: Explicit Return Statement inTryAttestationIncluding an explicit
return nilat the end of theTryAttestationfunction clarifies the function's exit points and ensures consistent return behavior.
| if err != nil { | ||
| panic("couldn't cast to claim") | ||
| } | ||
| claim := types.MustUnpackAttestationClaim(k.cdc, &att) |
There was a problem hiding this comment.
Consistent change, but reiterate caution on MustUnpackAttestationClaim
This change is consistent with the modification on line 95. However, the same concerns apply here regarding the potential for panics during genesis initialization.
Consider applying the same solution as suggested for line 95 to maintain consistency in error handling throughout the function.
| if err != nil { | ||
| panic(fmt.Errorf("covert power diff to dec err, powerDiff: %v, err: %w", powerDiff, err)) | ||
| } | ||
| powerDiffDec := sdkmath.LegacyMustNewDecFromStr(powerDiff) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using error handling instead of LegacyMustNewDecFromStr
While using sdkmath.LegacyMustNewDecFromStr simplifies the code, it still has the potential to panic if the input string is not a valid decimal. This doesn't fully align with the PR objective of replacing panics with error returns.
Consider using LegacyNewDecFromStr instead and propagating the error:
powerDiffDec, err := sdkmath.LegacyNewDecFromStr(powerDiff)
if err != nil {
return currentOracleSet, false
}This approach would handle potential errors gracefully without panicking, which is more in line with the PR's objectives.
| batch := k.GetOutgoingTxBatch(ctx, tokenContract, batchNonce) | ||
| if batch == nil { | ||
| panic(fmt.Sprintf("unknown batch nonce for outgoing tx batch %s %d", tokenContract, batchNonce)) | ||
| return fmt.Errorf("unknown batch nonce for outgoing tx batch %s %d", tokenContract, batchNonce) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error generation using types package
Instead of using fmt.Errorf, consider using the error types defined in the types package for consistency. For example:
-return fmt.Errorf("unknown batch nonce for outgoing tx batch %s %d", tokenContract, batchNonce)
+return types.ErrInvalid.Wrapf("unknown batch nonce for outgoing tx batch %s %d", tokenContract, batchNonce)This aligns with existing error handling patterns in the codebase.
📝 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.
| return fmt.Errorf("unknown batch nonce for outgoing tx batch %s %d", tokenContract, batchNonce) | |
| return types.ErrInvalid.Wrapf("unknown batch nonce for outgoing tx batch %s %d", tokenContract, batchNonce) |
| if err != nil { | ||
| panic("couldn't cast to claim") | ||
| } | ||
| claim := types.MustUnpackAttestationClaim(k.cdc, att) |
There was a problem hiding this comment.
Handle Potential Panic from MustUnpackAttestationClaim
Using types.MustUnpackAttestationClaim can lead to a panic if unpacking fails. To improve reliability, consider handling the error explicitly by using types.UnpackAttestationClaim and managing any potential errors.
Apply this change to handle the error gracefully:
- claim := types.MustUnpackAttestationClaim(k.cdc, att)
+ claim, err := types.UnpackAttestationClaim(k.cdc, att)
+ if err != nil {
+ k.Logger(ctx).Error("Failed to unpack attestation claim", "error", err)
+ continue // Skip this attestation or handle the error appropriately
+ }📝 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.
| claim := types.MustUnpackAttestationClaim(k.cdc, att) | |
| claim, err := types.UnpackAttestationClaim(k.cdc, att) | |
| if err != nil { | |
| k.Logger(ctx).Error("Failed to unpack attestation claim", "error", err) | |
| continue // Skip this attestation or handle the error appropriately | |
| } |
| func MustUnpackAttestationClaim(cdc codectypes.AnyUnpacker, att *Attestation) ExternalClaim { | ||
| var msg ExternalClaim | ||
| err := cdc.UnpackAny(att.Claim, &msg) | ||
| return msg, err | ||
| if err := cdc.UnpackAny(att.Claim, &msg); err != nil { | ||
| panic(err) | ||
| } | ||
| return msg |
There was a problem hiding this comment.
Reconsider using panic for error handling in MustUnpackAttestationClaim
Changing the function to panic on error can lead to unintended crashes if the panic is not recovered elsewhere in the application. It's generally better practice to return an error and allow the caller to handle it gracefully.
Consider modifying the function to return an error instead of panicking.
Apply this diff to change the function to return an error:
-func MustUnpackAttestationClaim(cdc codectypes.AnyUnpacker, att *Attestation) ExternalClaim {
+func UnpackAttestationClaim(cdc codectypes.AnyUnpacker, att *Attestation) (ExternalClaim, error) {
var msg ExternalClaim
if err := cdc.UnpackAny(att.Claim, &msg); err != nil {
- panic(err)
+ return nil, err
}
return msg
+ return msg, nil
}Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests