Skip to content

Commit

Permalink
Fee Closing Handshake (#551)
Browse files Browse the repository at this point in the history 
* add iterate logic

* add closing logic with tests

* add comments for panic

* change invariant breaking recovery to disabling middleware rather than panicing

* docs, tests, minor refactor
  • Loading branch information
@AdityaSripal
AdityaSripal authored Nov 29, 2021
1 parent c4dff6c commit edd11c6
Show file tree
Hide file tree
Showing 14 changed files with 422 additions and 71 deletions.
28 changes: 26 additions & 2 deletions modules/apps/29-fee/ibc_module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,20 @@ func (im IBCModule) OnChanCloseInit(
portID,
channelID string,
) error {
// TODO: Unescrow all remaining funds for unprocessed packets
// delete fee enabled on channel
// and refund any remaining fees escrowed on channel
im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
err := im.keeper.RefundFeesOnChannel(ctx, portID, channelID)
// error should only be non-nil if there is a bug in the code
// that causes module account to have insufficient funds to refund
// all escrowed fees on the channel.
// Disable all channels to allow for coordinated fix to the issue
// and mitigate/reverse damage.
// NOTE: Underlying application's packets will still go through, but
// fee module will be disabled for all channels
if err != nil {
im.keeper.DisableAllChannels(ctx)
}
return im.app.OnChanCloseInit(ctx, portID, channelID)
}

Expand All @@ -141,8 +153,20 @@ func (im IBCModule) OnChanCloseConfirm(
portID,
channelID string,
) error {
// TODO: Unescrow all remaining funds for unprocessed packets
// delete fee enabled on channel
// and refund any remaining fees escrowed on channel
im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
err := im.keeper.RefundFeesOnChannel(ctx, portID, channelID)
// error should only be non-nil if there is a bug in the code
// that causes module account to have insufficient funds to refund
// all escrowed fees on the channel.
// Disable all channels to allow for coordinated fix to the issue
// and mitigate/reverse damage.
// NOTE: Underlying application's packets will still go through, but
// fee module will be disabled for all channels
if err != nil {
im.keeper.DisableAllChannels(ctx)
}
return im.app.OnChanCloseConfirm(ctx, portID, channelID)
}

Expand Down
168 changes: 168 additions & 0 deletions modules/apps/29-fee/ibc_module_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package fee_test
import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
Expand All @@ -12,6 +13,13 @@ import (
ibctesting "github.com/cosmos/ibc-go/testing"
)

var (
validCoins = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}}
validCoins2 = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(200)}}
validCoins3 = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(300)}}
invalidCoins = sdk.Coins{sdk.Coin{Denom: "invalidDenom", Amount: sdk.NewInt(100)}}
)

// Tests OnChanOpenInit on ChainA
func (suite *FeeTestSuite) TestOnChanOpenInit() {
testCases := []struct {
Expand Down Expand Up @@ -300,3 +308,163 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() {
})
}
}

// Tests OnChanCloseInit on chainA
func (suite *FeeTestSuite) TestOnChanCloseInit() {
testCases := []struct {
name string
setup func(suite *FeeTestSuite)
disabled bool
}{
{
"success",
func(suite *FeeTestSuite) {
packetId := channeltypes.PacketId{
PortId: suite.path.EndpointA.ChannelConfig.PortID,
ChannelId: suite.path.EndpointA.ChannelID,
Sequence: 1,
}
refundAcc := suite.chainA.SenderAccount.GetAddress()
identifiedFee := types.NewIdentifiedPacketFee(&packetId, types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)
},
false,
},
{
"module account balance insufficient",
func(suite *FeeTestSuite) {
packetId := channeltypes.PacketId{
PortId: suite.path.EndpointA.ChannelConfig.PortID,
ChannelId: suite.path.EndpointA.ChannelID,
Sequence: 1,
}
refundAcc := suite.chainA.SenderAccount.GetAddress()
identifiedFee := types.NewIdentifiedPacketFee(&packetId, types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)

suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3)

// set fee enabled on different channel
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7")
},
true,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
suite.coordinator.Setup(suite.path) // setup channel

origBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress())

tc.setup(suite)

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

if tc.disabled {
suite.Require().True(
suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID),

"fee is not disabled on original channel: %s", suite.path.EndpointA.ChannelID,
)
suite.Require().True(
suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7"),

"fee is not disabled on other channel: %s", "channel-7",
)
} else {
cbs.OnChanCloseInit(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
afterBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress())
suite.Require().Equal(origBal, afterBal, "balances of refund account not equal after all fees refunded")
}
})
}
}

// Tests OnChanCloseConfirm on chainA
func (suite *FeeTestSuite) TestOnChanCloseConfirm() {
testCases := []struct {
name string
setup func(suite *FeeTestSuite)
disabled bool
}{
{
"success",
func(suite *FeeTestSuite) {
packetId := channeltypes.PacketId{
PortId: suite.path.EndpointA.ChannelConfig.PortID,
ChannelId: suite.path.EndpointA.ChannelID,
Sequence: 1,
}
refundAcc := suite.chainA.SenderAccount.GetAddress()
identifiedFee := types.NewIdentifiedPacketFee(&packetId, types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)
},
false,
},
{
"module account balance insufficient",
func(suite *FeeTestSuite) {
packetId := channeltypes.PacketId{
PortId: suite.path.EndpointA.ChannelConfig.PortID,
ChannelId: suite.path.EndpointA.ChannelID,
Sequence: 1,
}
refundAcc := suite.chainA.SenderAccount.GetAddress()
identifiedFee := types.NewIdentifiedPacketFee(&packetId, types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)

suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3)

// set fee enabled on different channel
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7")
},
true,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
suite.coordinator.Setup(suite.path) // setup channel

origBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress())

tc.setup(suite)

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

if tc.disabled {
suite.Require().True(
suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID),

"fee is not disabled on original channel: %s", suite.path.EndpointA.ChannelID,
)
suite.Require().True(
suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7"),

"fee is not disabled on other channel: %s", "channel-7",
)
} else {
cbs.OnChanCloseConfirm(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
afterBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress())
suite.Require().Equal(origBal, afterBal, "balances of refund account not equal after all fees refunded")
}
})
}
}
41 changes: 41 additions & 0 deletions modules/apps/29-fee/keeper/escrow.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import (

// EscrowPacketFee sends the packet fee to the 29-fee module account to hold in escrow
func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee *types.IdentifiedPacketFee) error {
if !k.IsFeeEnabled(ctx, identifiedFee.PacketId.PortId, identifiedFee.PacketId.ChannelId) {
// users may not escrow fees on this channel. Must send packets without a fee message
return sdkerrors.Wrap(types.ErrFeeNotEnabled, "cannot escrow fee for packet")
}
// check if the refund account exists
refundAcc, err := sdk.AccAddressFromBech32(identifiedFee.RefundAddress)
if err != nil {
Expand Down Expand Up @@ -107,3 +111,40 @@ func (k Keeper) DistributeFeeTimeout(ctx sdk.Context, refundAcc, timeoutRelayer

return nil
}

func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) error {
// get module accAddr
feeModuleAccAddr := k.authKeeper.GetModuleAddress(types.ModuleName)

var refundErr error

k.IterateChannelFeesInEscrow(ctx, portID, channelID, func(identifiedFee types.IdentifiedPacketFee) (stop bool) {
refundAccAddr, err := sdk.AccAddressFromBech32(identifiedFee.RefundAddress)
if err != nil {
refundErr = err
return true
}

// refund all fees to refund address
// Use SendCoins rather than the module account send functions since refund address may be a user account or module address.
// if any `SendCoins` call returns an error, we return error and stop iteration
err = k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, refundAccAddr, identifiedFee.Fee.ReceiveFee)
if err != nil {
refundErr = err
return true
}
err = k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, refundAccAddr, identifiedFee.Fee.AckFee)
if err != nil {
refundErr = err
return true
}
err = k.bankKeeper.SendCoins(ctx, feeModuleAccAddr, refundAccAddr, identifiedFee.Fee.TimeoutFee)
if err != nil {
refundErr = err
return true
}
return false
})

return refundErr
}
Loading

0 comments on commit edd11c6

Please sign in to comment.