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 4 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
20 changes: 18 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,16 @@ 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.
if err != nil {
panic(err)
Copy link
Member Author

Choose a reason for hiding this comment

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

This should only happen if there was a bug in the code and the module account has insufficient funds to refund all escrowed fees. So I panic here, open to a different suggestion on what to do

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems logical to me. Is there any way that the fee's could be refunded just before the channel closes? If so this would panic and we wouldn't call im.app.OnChanCloseInit

Copy link
Contributor

@charleenfei charleenfei Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

why not just return the error without closing the channel? if we panic then this call would crash the node, which seems excessive just for funds not being refundable, even though we still want to keep the channel open.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any way that the fee's could be refunded just before the channel closes?

The fee only gets refunded before channel-closing if it gets processed (Ack or Timeout). In both cases, the fee is deleted from the store, so it won't get retrieved again in channel closing.

why not just return the error without closing the channel?

If the funds are missing in the escrow account, this is an invalid state. Some bug or malicious actor is responsible. I will look into how transfer deals with this situation and follow its lead.

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

Expand All @@ -141,8 +149,16 @@ 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.
if err != nil {
panic(err)
}
return im.app.OnChanCloseConfirm(ctx, portID, channelID)
}

Expand Down
148 changes: 148 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,143 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() {
})
}
}

// Tests OnChanCloseInit on chainA
func (suite *FeeTestSuite) TestOnChanCloseInit() {
testCases := []struct {
name string
setup func(suite *FeeTestSuite)
panics 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)
},
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.panics {
suite.Require().Panics(func() {
cbs.OnChanCloseInit(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
}, "did not panic as expected on refund fees")
} 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)
panics 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)
},
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.panics {
suite.Require().Panics(func() {
cbs.OnChanCloseConfirm(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
}, "did not panic as expected on refund fees")
} 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")
}
})
}
}
37 changes: 37 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 @@ -108,3 +108,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
}
50 changes: 50 additions & 0 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 @@ -282,3 +282,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")
}
17 changes: 16 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,14 +144,29 @@ 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)
Expand Down
1 change: 1 addition & 0 deletions modules/apps/29-fee/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,6 @@ type PortKeeper interface {
type BankKeeper interface {
HasBalance(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coin) bool
SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error
SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error
SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error
}
7 changes: 6 additions & 1 deletion modules/apps/29-fee/types/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,10 @@ func KeyRelayerAddress(address string) []byte {

// KeyFeeInEscrow returns the key for escrowed fees
func KeyFeeInEscrow(packetID *channeltypes.PacketId) []byte {
return []byte(fmt.Sprintf("%s/%s/%s/packet/%d", FeeInEscrowPrefix, packetID.PortId, packetID.ChannelId, packetID.Sequence))
return []byte(fmt.Sprintf("%s/%d", KeyFeeInEscrowChannelPrefix(packetID.PortId, packetID.ChannelId), packetID.Sequence))
}

// KeyFeeInEscrowChannelPrefix returns the key prefix for escrowed fees on the given channel
func KeyFeeInEscrowChannelPrefix(portID, channelID string) []byte {
return []byte(fmt.Sprintf("%s/%s/%s/packet", FeeInEscrowPrefix, portID, channelID))
}