Skip to content

Commit

Permalink
refactor: Fix RefundFeesOnChannel (cosmos#1244)
Browse files Browse the repository at this point in the history 
## Description

read cosmos#1060 

closes: cosmos#860
closes: cosmos#780 

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Review `Codecov Report` in the comment section below once CI passes
  • Loading branch information
@colin-axner
colin-axner authored Apr 13, 2022
1 parent 960a49e commit e59a6d2
Show file tree
Hide file tree
Showing 7 changed files with 402 additions and 239 deletions.
37 changes: 10 additions & 27 deletions modules/apps/29-fee/ibc_module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,19 +148,8 @@ func (im IBCModule) OnChanCloseInit(
return err
}

// 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)
if err := im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID); err != nil {
return err
}

return nil
Expand All @@ -172,21 +161,15 @@ func (im IBCModule) OnChanCloseConfirm(
portID,
channelID string,
) error {
// 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)
if err := im.app.OnChanCloseConfirm(ctx, portID, channelID); err != nil {
return err
}
return im.app.OnChanCloseConfirm(ctx, portID, channelID)

if err := im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID); err != nil {
return err
}

return nil
}

// OnRecvPacket implements the IBCModule interface.
Expand Down
191 changes: 92 additions & 99 deletions modules/apps/29-fee/ibc_module_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ import (
)

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)}}
defaultRecvFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}}
defaultAckFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(200)}}
defaultTimeoutFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(300)}}
)

// Tests OnChanOpenInit on ChainA
Expand Down Expand Up @@ -291,47 +291,39 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() {
}
}

// Tests OnChanCloseInit on chainA
func (suite *FeeTestSuite) TestOnChanCloseInit() {
var (
refundAcc sdk.AccAddress
fee types.Fee
)

testCases := []struct {
name string
setup func(suite *FeeTestSuite)
disabled bool
malleate func()
expPass bool
}{
{
"success",
func(suite *FeeTestSuite) {
packetID := channeltypes.NewPacketId(
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
1,
)
refundAcc := suite.chainA.SenderAccount.GetAddress()
packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)
},
false,
"success", func() {}, true,
},
{
"module account balance insufficient",
func(suite *FeeTestSuite) {
packetID := channeltypes.NewPacketId(
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
1,
)
refundAcc := suite.chainA.SenderAccount.GetAddress()
packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3)
"application callback fails", func() {
suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanCloseInit = func(
ctx sdk.Context, portID, channelID string,
) error {
return fmt.Errorf("application callback fails")
}
}, false,
},
{
"RefundFeesOnChannelClosure fails - invalid refund address", func() {
// store the fee in state & update escrow account balance
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)})

// set fee enabled on different channel
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7")
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)
suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
},
true,
false,
},
}

Expand All @@ -341,108 +333,109 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() {
suite.SetupTest()
suite.coordinator.Setup(suite.path) // setup channel

origBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress())
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
fee = types.Fee{
RecvFee: defaultRecvFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
}

tc.setup(suite)
refundAcc = suite.chainA.SenderAccount.GetAddress()
packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

tc.malleate()

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)
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",
)
err = cbs.OnChanCloseInit(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)

if tc.expPass {
suite.Require().NoError(err)
} 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")
suite.Require().Error(err)
}
})
}
}

// Tests OnChanCloseConfirm on chainA
func (suite *FeeTestSuite) TestOnChanCloseConfirm() {
var (
refundAcc sdk.AccAddress
fee types.Fee
)

testCases := []struct {
name string
setup func(suite *FeeTestSuite)
disabled bool
malleate func()
expPass 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()
packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)
},
false,
"success", func() {}, true,
},
{
"module account balance insufficient",
func(suite *FeeTestSuite) {
packetID := channeltypes.PacketId{
PortId: suite.path.EndpointA.ChannelConfig.PortID,
ChannelId: suite.path.EndpointA.ChannelID,
Sequence: 1,
"application callback fails", func() {
suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanCloseConfirm = func(
ctx sdk.Context, portID, channelID string,
) error {
return fmt.Errorf("application callback fails")
}
refundAcc := suite.chainA.SenderAccount.GetAddress()
packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3)
}, false,
},
{
"RefundChannelFeesOnClosure fails - refund address is invalid", func() {
// store the fee in state & update escrow account balance
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)})

// set fee enabled on different channel
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7")
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)
suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
},
true,
false,
},
}

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())
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
fee = types.Fee{
RecvFee: defaultRecvFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
}

tc.setup(suite)
refundAcc = suite.chainA.SenderAccount.GetAddress()
packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

tc.malleate()

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)
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",
)
err = cbs.OnChanCloseConfirm(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)

if tc.expPass {
suite.Require().NoError(err)
} 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")
suite.Require().Error(err)
}

})
}
}
Expand Down Expand Up @@ -657,9 +650,9 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
packetID := channeltypes.NewPacketId(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
packetFee = types.NewPacketFee(
types.Fee{
RecvFee: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
RecvFee: defaultRecvFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
},
suite.chainA.SenderAccount.GetAddress().String(),
[]string{},
Expand Down Expand Up @@ -788,9 +781,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {

packetFee = types.NewPacketFee(
types.Fee{
RecvFee: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
RecvFee: defaultRecvFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
},
suite.chainA.SenderAccount.GetAddress().String(),
[]string{},
Expand Down
Loading

0 comments on commit e59a6d2

Please sign in to comment.