fix: return error instead of panic for behaviors triggered by client (#…

…1395) (#1400)

(cherry picked from commit 6c90f1e)

Co-authored-by: Jeseung Lee <[email protected]>

CHANGELOG.md

@@ -54,7 +54,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/fbridge) [\#1369](https://github.com/Finschia/finschia-sdk/pull/1369) Add the event of `SetBridgeStatus`
 * (x/fswap) [\#1372](https://github.com/Finschia/finschia-sdk/pull/1372) support message based proposals
 * (x/fswap) [\#1387](https://github.com/Finschia/finschia-sdk/pull/1387) add new Swap query to get a single swap
-* (x/fswap) [\#1382](https://github.com/Finschia/finschia-sdk/pull/1382) add validation & unit tests in fswap module
+* (x/fswap) [\#1382](https://github.com/Finschia/finschia-sdk/pull/1382) add validation & unit tests in fswap module 
+* (x/fbridge) [\#1395](https://github.com/Finschia/finschia-sdk/pull/1395) Return error instead of panic for behaviors triggered by client
 * (x/fswap) [\#1396](https://github.com/Finschia/finschia-sdk/pull/1396) refactor to use snake_case in proto
 * (x/fswap) [\#1391](https://github.com/Finschia/finschia-sdk/pull/1391) add cli_test for fswap module
 

x/fbridge/keeper/abci.go

@@ -13,7 +13,9 @@ func (k Keeper) BeginBlocker(ctx sdk.Context) {
 	proposals := k.GetRoleProposals(ctx)
 	for _, proposal := range proposals {
 		if ctx.BlockTime().After(proposal.ExpiredAt) {
-			k.deleteRoleProposal(ctx, proposal.Id)
+			if err := k.deleteRoleProposal(ctx, proposal.Id); err != nil {
+				panic(err)
+			}
 		}
 	}
 }
@@ -36,7 +38,9 @@ func (k Keeper) EndBlocker(ctx sdk.Context) {
 				panic(err)
 			}
 
-			k.deleteRoleProposal(ctx, proposal.Id)
+			if err := k.deleteRoleProposal(ctx, proposal.Id); err != nil {
+				panic(err)
+			}
 		}
 	}
 }

x/fbridge/keeper/auth.go

@@ -2,7 +2,6 @@ package keeper
 
 import (
 	"encoding/binary"
-	"fmt"
 	"time"
 
 	sdk "github.com/Finschia/finschia-sdk/types"
@@ -62,7 +61,7 @@ func (k Keeper) addVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress
 func (k Keeper) updateRole(ctx sdk.Context, role types.Role, addr sdk.AccAddress) error {
 	previousRole := k.GetRole(ctx, addr)
 	if previousRole == role {
-		return sdkerrors.ErrInvalidRequest.Wrap("target already has same role")
+		return nil
 	}
 
 	roleMeta := k.GetRoleMetadata(ctx)
@@ -173,12 +172,14 @@ func (k Keeper) GetRoleProposal(ctx sdk.Context, id uint64) (proposal types.Role
 	return proposal, true
 }
 
-func (k Keeper) deleteRoleProposal(ctx sdk.Context, id uint64) {
+func (k Keeper) deleteRoleProposal(ctx sdk.Context, id uint64) error {
 	store := ctx.KVStore(k.storeKey)
 	if _, found := k.GetRoleProposal(ctx, id); !found {
-		panic(fmt.Sprintf("role proposal #%d not found", id))
+		return sdkerrors.ErrNotFound.Wrapf("role proposal #%d not found", id)
 	}
+
 	store.Delete(types.ProposalKey(id))
+	return nil
 }
 
 // IterateProposals iterates over the all the role proposals and performs a callback function

x/fbridge/keeper/auth_test.go

@@ -55,7 +55,7 @@ func TestAssignRole(t *testing.T) {
 
 	// 4. Guardian assigns an address to a same role
 	err = k.updateRole(ctx, types.RoleOperator, addrs[1])
-	require.Error(t, err, "role must not be updated to the same role")
+	require.NoError(t, err)
 }
 
 func TestBridgeHaltAndResume(t *testing.T) {
@@ -86,4 +86,7 @@ func TestBridgeHaltAndResume(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, types.StatusActive, k.GetBridgeStatus(ctx), "bridge status must be active (2/3)")
 	require.Equal(t, types.BridgeStatusMetadata{Active: 2, Inactive: 1}, k.GetBridgeStatusMetadata(ctx))
+
+	err = k.updateBridgeSwitch(ctx, addrs[0], 3)
+	require.Error(t, err, "invalid bridge status must be rejected")
 }

x/fbridge/keeper/genesis.go

@@ -19,13 +19,13 @@ func (k Keeper) InitGenesis(ctx sdk.Context, gs *types.GenesisState) error {
 
 	for _, pair := range gs.Roles {
 		if err := k.setRole(ctx, pair.Role, sdk.MustAccAddressFromBech32(pair.Address)); err != nil {
-			panic(err)
+			return err
 		}
 	}
 
 	for _, sw := range gs.BridgeSwitches {
 		if err := k.setBridgeSwitch(ctx, sdk.MustAccAddressFromBech32(sw.Guardian), sw.Status); err != nil {
-			panic(err)
+			return err
 		}
 	}
 

x/fbridge/keeper/transfer.go

@@ -23,11 +23,11 @@ func (k Keeper) handleBridgeTransfer(ctx sdk.Context, sender sdk.AccAddress, amo
 	}
 
 	if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, token); err != nil {
-		panic(err)
+		return 0, err
 	}
 
 	if err := k.bankKeeper.BurnCoins(ctx, types.ModuleName, token); err != nil {
-		panic(fmt.Errorf("cannot burn coins after a successful send to a module account: %v", err))
+		panic(fmt.Errorf("cannot burn coins after a successful send to a module account: %s", err))
 	}
 
 	seq := k.GetNextSequence(ctx)

x/fbridge/keeper/transfer_test.go

@@ -2,6 +2,7 @@ package keeper
 
 import (
 	"encoding/binary"
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -18,10 +19,9 @@ func TestHandleBridgeTransfer(t *testing.T) {
 	amt := sdk.NewInt(1000000)
 	denom := "stake"
 	token := sdk.Coins{sdk.Coin{Denom: denom, Amount: amt}}
-
-	bankKeeper.EXPECT().IsSendEnabledCoins(ctx, token).Return(nil)
-	bankKeeper.EXPECT().SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, token).Return(nil)
-	bankKeeper.EXPECT().BurnCoins(ctx, types.ModuleName, token).Return(nil)
+	bankKeeper.EXPECT().IsSendEnabledCoins(ctx, token).Return(nil).Times(1)
+	bankKeeper.EXPECT().SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, token).Return(nil).Times(1)
+	bankKeeper.EXPECT().BurnCoins(ctx, types.ModuleName, token).Return(nil).Times(1)
 
 	k := NewKeeper(encCfg.Codec, key, memKey, authKeeper, bankKeeper, types.DefaultAuthority().String())
 	params := types.DefaultParams()
@@ -41,6 +41,17 @@ func TestHandleBridgeTransfer(t *testing.T) {
 	h, err := k.GetSeqToBlocknum(ctx, handledSeq)
 	require.NoError(t, err)
 	require.Equal(t, uint64(ctx.BlockHeight()), h)
+
+	// test error cases
+	bankKeeper.EXPECT().IsSendEnabledCoins(ctx, token).Return(nil).Times(1)
+	bankKeeper.EXPECT().SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, token).Return(errors.New("insufficient funds")).Times(1)
+	_, err = k.handleBridgeTransfer(ctx, sender, amt)
+	require.Error(t, err)
+
+	bankKeeper.EXPECT().IsSendEnabledCoins(ctx, token).Return(nil).Times(1)
+	bankKeeper.EXPECT().SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, token).Return(nil).Times(1)
+	bankKeeper.EXPECT().BurnCoins(ctx, types.ModuleName, token).Return(errors.New("failed to burn coins")).Times(1)
+	require.Panics(t, func() { _, _ = k.handleBridgeTransfer(ctx, sender, amt) }, "cannot burn coins after a successful send to a module account: failed to burn coins")
 }
 
 func TestIsValidEthereumAddress(t *testing.T) {