diff --git a/CHANGELOG.md b/CHANGELOG.md index a9676b85f4..a2d99cc6d9 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/x/fbridge/keeper/abci.go b/x/fbridge/keeper/abci.go index 28b5333b1f..6f194ecef3 100644 --- a/x/fbridge/keeper/abci.go +++ b/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) + } } } } diff --git a/x/fbridge/keeper/auth.go b/x/fbridge/keeper/auth.go index 66e6d2a01c..833e53399c 100644 --- a/x/fbridge/keeper/auth.go +++ b/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 diff --git a/x/fbridge/keeper/auth_test.go b/x/fbridge/keeper/auth_test.go index de6d33001b..0946037a7d 100644 --- a/x/fbridge/keeper/auth_test.go +++ b/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") } diff --git a/x/fbridge/keeper/genesis.go b/x/fbridge/keeper/genesis.go index 479abedd8d..82788f3a24 100644 --- a/x/fbridge/keeper/genesis.go +++ b/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 } } diff --git a/x/fbridge/keeper/transfer.go b/x/fbridge/keeper/transfer.go index 8dd492c499..333fa72f23 100644 --- a/x/fbridge/keeper/transfer.go +++ b/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) diff --git a/x/fbridge/keeper/transfer_test.go b/x/fbridge/keeper/transfer_test.go index 8d4fc4986d..15813ea862 100644 --- a/x/fbridge/keeper/transfer_test.go +++ b/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) {