-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: replace panic with error return #740
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -46,7 +46,9 @@ func (k Keeper) Attest(ctx sdk.Context, oracleAddr sdk.AccAddress, claim types.E | |||||||||||||
k.SetAttestation(ctx, claim.GetEventNonce(), claim.ClaimHash(), att) | ||||||||||||||
|
||||||||||||||
if !att.Observed && claim.GetEventNonce() == k.GetLastObservedEventNonce(ctx)+1 { | ||||||||||||||
k.TryAttestation(ctx, att, claim) | ||||||||||||||
if err = k.TryAttestation(ctx, att, claim); err != nil { | ||||||||||||||
return nil, err | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
ctx = ctx.WithGasMeter(gasMeter) | ||||||||||||||
|
@@ -59,7 +61,7 @@ func (k Keeper) Attest(ctx sdk.Context, oracleAddr sdk.AccAddress, claim types.E | |||||||||||||
// TryAttestation checks if an attestation has enough votes to be applied to the consensus state | ||||||||||||||
// and has not already been marked Observed, then calls processAttestation to actually apply it to the state, | ||||||||||||||
// and then marks it Observed and emits an event. | ||||||||||||||
func (k Keeper) TryAttestation(ctx sdk.Context, att *types.Attestation, claim types.ExternalClaim) { | ||||||||||||||
func (k Keeper) TryAttestation(ctx sdk.Context, att *types.Attestation, claim types.ExternalClaim) error { | ||||||||||||||
// If the attestation has not yet been Observed, sum up the votes and see if it is ready to apply to the state. | ||||||||||||||
// This conditional stops the attestation from accidentally being applied twice. | ||||||||||||||
// Sum the current powers of all validators who have voted and see if it passes the current threshold | ||||||||||||||
|
@@ -71,9 +73,7 @@ func (k Keeper) TryAttestation(ctx sdk.Context, att *types.Attestation, claim ty | |||||||||||||
oracleAddr := sdk.MustAccAddressFromBech32(oracleStr) | ||||||||||||||
oracle, found := k.GetOracle(ctx, oracleAddr) | ||||||||||||||
if !found { | ||||||||||||||
k.Logger(ctx).Error("TryAttestation", "not found oracle", oracleAddr.String(), "claimEventNonce", | ||||||||||||||
claim.GetEventNonce(), "claimType", claim.GetType(), "claimHeight", claim.GetBlockHeight()) | ||||||||||||||
continue | ||||||||||||||
return types.ErrNoFoundOracle | ||||||||||||||
} | ||||||||||||||
oraclePower := oracle.GetPower() | ||||||||||||||
// Add it to the attestation power's sum | ||||||||||||||
|
@@ -104,12 +104,17 @@ func (k Keeper) TryAttestation(ctx sdk.Context, att *types.Attestation, claim ty | |||||||||||||
ctx.EventManager().EmitEvent(event) | ||||||||||||||
|
||||||||||||||
// execute the timeout logic | ||||||||||||||
k.cleanupTimedOutBatches(ctx) | ||||||||||||||
k.cleanupTimeOutBridgeCall(ctx) | ||||||||||||||
if err = k.cleanupTimedOutBatches(ctx); err != nil { | ||||||||||||||
return err | ||||||||||||||
} | ||||||||||||||
if err = k.cleanupTimeOutBridgeCall(ctx); err != nil { | ||||||||||||||
return err | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
k.pruneAttestations(ctx) | ||||||||||||||
break | ||||||||||||||
} | ||||||||||||||
return nil | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// processAttestation actually applies the attestation to the consensus state | ||||||||||||||
|
@@ -168,10 +173,7 @@ func (k Keeper) IterateAttestationAndClaim(ctx sdk.Context, cb func(*types.Attes | |||||||||||||
for ; iter.Valid(); iter.Next() { | ||||||||||||||
att := new(types.Attestation) | ||||||||||||||
k.cdc.MustUnmarshal(iter.Value(), att) | ||||||||||||||
claim, err := types.UnpackAttestationClaim(k.cdc, att) | ||||||||||||||
if err != nil { | ||||||||||||||
panic("couldn't cast to claim") | ||||||||||||||
} | ||||||||||||||
claim := types.MustUnpackAttestationClaim(k.cdc, att) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle Potential Panic from Using 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
Suggested change
|
||||||||||||||
// cb returns true to stop early | ||||||||||||||
if cb(att, claim) { | ||||||||||||||
return | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,10 +92,7 @@ func InitGenesis(ctx sdk.Context, k Keeper, state *types.GenesisState) { | |
// reset attestations in state | ||
for i := 0; i < len(state.Attestations); i++ { | ||
att := state.Attestations[i] | ||
claim, err := types.UnpackAttestationClaim(k.cdc, &att) | ||
if err != nil { | ||
panic("couldn't cast to claim") | ||
} | ||
claim := types.MustUnpackAttestationClaim(k.cdc, &att) | ||
|
||
// 0x17 | ||
k.SetAttestation(ctx, claim.GetEventNonce(), claim.ClaimHash(), &att) | ||
|
@@ -106,10 +103,7 @@ func InitGenesis(ctx sdk.Context, k Keeper, state *types.GenesisState) { | |
for i := 0; i < len(state.Attestations); i++ { | ||
|
||
att := state.Attestations[i] | ||
claim, err := types.UnpackAttestationClaim(k.cdc, &att) | ||
if err != nil { | ||
panic("couldn't cast to claim") | ||
} | ||
claim := types.MustUnpackAttestationClaim(k.cdc, &att) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consistent change, but reiterate caution on 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. |
||
// reconstruct the latest event nonce for every validator | ||
// if somehow this genesis state is saved when all attestations | ||
// have been cleaned up GetLastEventNonceByOracle handles that case | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -71,22 +71,25 @@ func (k Keeper) BuildOutgoingTxBatch(ctx sdk.Context, sender sdk.AccAddress, rec | |||||
return batch.BatchNonce, nil | ||||||
} | ||||||
|
||||||
func (k Keeper) OutgoingTxBatchExecuted(ctx sdk.Context, tokenContract string, batchNonce uint64) { | ||||||
func (k Keeper) OutgoingTxBatchExecuted(ctx sdk.Context, tokenContract string, batchNonce uint64) (err error) { | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Standardize error generation using Instead of using -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
Suggested change
|
||||||
} | ||||||
|
||||||
// Iterate through remaining batches | ||||||
k.IterateOutgoingTxBatches(ctx, func(iterBatch *types.OutgoingTxBatch) bool { | ||||||
// If the iterated batches nonce is lower than the one that was just executed, cancel it | ||||||
if iterBatch.BatchNonce < batch.BatchNonce && iterBatch.TokenContract == tokenContract { | ||||||
if err := k.RefundOutgoingTxBatch(ctx, tokenContract, iterBatch.BatchNonce); err != nil { | ||||||
panic(fmt.Sprintf("Failed cancel out batch %s %d while trying to execute failed: %s", batch.TokenContract, batch.BatchNonce, err)) | ||||||
if err = k.RefundOutgoingTxBatch(ctx, tokenContract, iterBatch.BatchNonce); err != nil { | ||||||
return true | ||||||
} | ||||||
} | ||||||
return false | ||||||
}) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
|
||||||
// Delete batch since it is finished | ||||||
k.DeleteBatch(ctx, batch) | ||||||
|
@@ -97,6 +100,7 @@ func (k Keeper) OutgoingTxBatchExecuted(ctx sdk.Context, tokenContract string, b | |||||
k.erc20Keeper.DeleteOutgoingTransferRelation(ctx, k.moduleName, tx.Id) | ||||||
} | ||||||
} | ||||||
return nil | ||||||
} | ||||||
|
||||||
// --- OUTGOING TX BATCH --- // | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -280,10 +280,12 @@ var ( | |
_ ExternalClaim = &MsgBridgeCallResultClaim{} | ||
) | ||
|
||
func UnpackAttestationClaim(cdc codectypes.AnyUnpacker, att *Attestation) (ExternalClaim, error) { | ||
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 | ||
Comment on lines
+283
to
+288
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reconsider using 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
}
|
||
} | ||
|
||
func (m *MsgClaim) ValidateBasic() (err error) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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:This approach would handle potential errors gracefully without panicking, which is more in line with the PR's objectives.