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

refactor: Fix RefundFeesOnChannel #1244

Merged
merged 16 commits into from
Apr 13, 2022
Merged

refactor: Fix RefundFeesOnChannel #1244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
43bfadf
apply code changes from #1029
colin-axner Apr 11, 2022
1dbc78a
merge main and fix build
colin-axner Apr 11, 2022
b965eac
begin implementating RefundFeesOnChannelClosure tests
colin-axner Apr 11, 2022
06e0a9f
fix test cases for RefundPacketFeesOnChannelClosure
colin-axner Apr 12, 2022
f9cff1f
test: add test case for packet fees on different channel
colin-axner Apr 12, 2022
8189026
refactor: deduplicate code and add flexibility for module refund addr…
colin-axner Apr 12, 2022
8780b97
chore: update godoc
colin-axner Apr 12, 2022
f4f69bf
test: add test for GetIdentifiedFeesForChannel
colin-axner Apr 12, 2022
754aa9f
test: add tests for OnChanCloseInit and OnChanCloseConfirm
colin-axner Apr 12, 2022
44b90b3
Merge branch 'main' into colin/860-refund-fees-onclosure
colin-axner Apr 12, 2022
602d5a4
Merge branch 'main' into colin/860-refund-fees-onclosure
colin-axner Apr 12, 2022
93d3fe2
chore: packetId -> packetID
colin-axner Apr 13, 2022
1167196
chore: types.Fee{} -> types.NewFee()
colin-axner Apr 13, 2022
de21aa7
chore: rename validCoins to defaultRecvFee, rename validCoins2 to def…
colin-axner Apr 13, 2022
a71cd92
Merge branch 'colin/860-refund-fees-onclosure' of github.com:cosmos/i…
colin-axner Apr 13, 2022
b063717
Merge branch 'main' into colin/860-refund-fees-onclosure
colin-axner Apr 13, 2022
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
37 changes: 10 additions & 27 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 @@ -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
Open in desktop
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() {
seantking marked this conversation as resolved.
Show resolved Hide resolved
// 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