Skip to content
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.

Sign up for GitHub

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

Fee Closing Handshake #551

Merged
merged 9 commits into from
Nov 29, 2021
Merged

Fee Closing Handshake #551

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0504176
add iterate logic
AdityaSripal Nov 18, 2021
229f2cb
Merge branch 'ics29-fee-middleware' of github.com:cosmos/ibc-go into …
AdityaSripal Nov 18, 2021
21c86be
add closing logic with tests
AdityaSripal Nov 19, 2021
cc0e5ac
add comments for panic
AdityaSripal Nov 19, 2021
4ab4b02
change invariant breaking recovery to disabling middleware rather tha…
AdityaSripal Nov 24, 2021
c479a35
fix merge and add docs
AdityaSripal Nov 24, 2021
6653378
docs, tests, minor refactor
AdityaSripal Nov 24, 2021
5d109cc
fix merge
AdityaSripal Nov 29, 2021
1d0d3fd
Merge branch 'ics29-fee-middleware' into aditya/closing-handshake
AdityaSripal Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions modules/apps/29-fee/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,18 @@ 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 damage.
if err != nil {
im.keeper.DisableAllChannels(ctx)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling all channels will effectively "turn off" the fee module. I added a check for IsFeeEnabled on EscrowFee.

Also, OnAckPacket and OnTimeoutPacket will check if fee is enabled on the channel before running middleware logic. So if we disable all channels, any further escrows will get rejected and all distributions will be bypassed. Thus, we can turn off the fee functionality without affecting underlying applications. IBC can still run even if fee module gets disabled due to invalid state

Copy link
Contributor

@seantking seantking Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have IsFeeEnabled as a param we can change via governance. That way if a bug was discovered a chain could potentially disable a channel. Although, maybe the proposal would take too long.

Either way, I think this approach works better. Nice 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially we could have a global enabled param, rather than a per-channel one. But that should be in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 👍

}
return im.app.OnChanCloseInit(ctx, portID, channelID)
}

Expand All @@ -141,8 +151,18 @@ 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 damage.
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
Open in desktop
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)

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)

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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prevents further escrows if channel gets disabled

// 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 @@ -108,3 +112,40 @@ func (k Keeper) DistributeFeeTimeout(ctx sdk.Context, refundAcc, timeoutRelayer

return nil
}

func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) error {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
// 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
}
57 changes: 50 additions & 7 deletions modules/apps/29-fee/keeper/escrow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,6 @@ import (
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
)

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)}}
)

func (suite *KeeperTestSuite) TestEscrowPacketFee() {
var (
err error
Expand Down Expand Up @@ -282,3 +275,53 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
})
}
}

func (suite *KeeperTestSuite) TestRefundFeesOnChannel() {
// setup
refundAcc := suite.chainA.SenderAccount.GetAddress()

// refundAcc balance before escrow
prevBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc)

ackFee := validCoins
receiveFee := validCoins2
timeoutFee := validCoins3

for i := 0; i < 5; i++ {
packetId := &channeltypes.PacketId{ChannelId: "channel-0", PortId: types.PortKey, Sequence: uint64(i)}
fee := types.Fee{receiveFee, ackFee, timeoutFee}

identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee)
suite.Require().NoError(err)
}

// send a packet over a different channel to ensure this fee is not refunded
packetId := &channeltypes.PacketId{ChannelId: "channel-1", PortId: types.PortKey, Sequence: 1}
fee := types.Fee{receiveFee, ackFee, timeoutFee}

identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}}
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee)
suite.Require().NoError(err)

// check that refunding all fees on channel-0 refunds all fees except for fee on channel-1
err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(suite.chainA.GetContext(), types.PortKey, "channel-0")
suite.Require().NoError(err, "refund fees returned unexpected error")

// add fee sent to channel-1 to after balance to recover original balance
afterBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc)
suite.Require().Equal(prevBal, afterBal.Add(validCoins...).Add(validCoins2...).Add(validCoins3...), "refund account not back to original balance after refunding all tokens")

// create escrow and then change module account balance to cause error on refund
packetId = &channeltypes.PacketId{ChannelId: "channel-0", PortId: types.PortKey, Sequence: uint64(6)}
fee = types.Fee{receiveFee, ackFee, timeoutFee}

identifiedPacketFee = types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}}
err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee)
suite.Require().NoError(err)

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

err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(suite.chainA.GetContext(), types.PortKey, "channel-0")
suite.Require().Error(err, "refund fees returned no error with insufficient balance on module account")
}
32 changes: 31 additions & 1 deletion modules/apps/29-fee/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,21 +144,51 @@ func (k Keeper) GetFeeInEscrow(ctx sdk.Context, packetId *channeltypes.PacketId)
return fee, true
}

// IterateChannelFeesInEscrow iterates over all the fees on the given channel currently escrowed and calls the provided callback
// if the callback returns true, then iteration is stopped.
func (k Keeper) IterateChannelFeesInEscrow(ctx sdk.Context, portID, channelID string, cb func(identifiedFee types.IdentifiedPacketFee) (stop bool)) {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, types.KeyFeeInEscrowChannelPrefix(portID, channelID))

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
identifiedFee := k.MustUnmarshalFee(iterator.Value())
if cb(identifiedFee) {
break
}
}
}

// Deletes the fee associated with the given packetId
func (k Keeper) DeleteFeeInEscrow(ctx sdk.Context, packetId *channeltypes.PacketId) {
store := ctx.KVStore(k.storeKey)
key := types.KeyFeeInEscrow(packetId)
store.Delete(key)
}

// GetFeeInEscrow returns true if there is a Fee still to be escrowed for a given packet
// HasFeeInEscrow returns true if there is a Fee still to be escrowed for a given packet
func (k Keeper) HasFeeInEscrow(ctx sdk.Context, packetId *channeltypes.PacketId) bool {
store := ctx.KVStore(k.storeKey)
key := types.KeyFeeInEscrow(packetId)

return store.Has(key)
}

// DisableAllChannels will disable the fee module for all channels.
// Only called if the module enters into an invalid state
// e.g. ModuleAccount has insufficient balance to refund users.
// In this case, chain developers should investigate the issue, fix it,
// and then re-enable the fee module in a coordinated upgrade.
func (k Keeper) DisableAllChannels(ctx sdk.Context) {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, []byte(types.FeeEnabledKeyPrefix))

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
store.Delete(iterator.Key())
}
}

// MustMarshalFee attempts to encode a Fee object and returns the
// raw encoded bytes. It panics on error.
func (k Keeper) MustMarshalFee(fee *types.IdentifiedPacketFee) []byte {
Expand Down
Loading