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 11 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
173 changes: 83 additions & 90 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 @@ -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: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
}
Copy link
Member

Choose a reason for hiding this comment

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

should we rename validCoins to defaultRecvFee..etc, to be consistent with keeper tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this as well. I think we should, but would prefer to have this be done in a followup since this pr already has a bit of diffs

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, no worries. Sounds good to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to do it in this pr


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: validCoins,
AckFee: validCoins2,
TimeoutFee: validCoins3,
}

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
64 changes: 42 additions & 22 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 @@ -112,37 +112,57 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver sdk.AccAddress, fee sdk.
}
}

func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) error {
// RefundFeesOnChannelClosure will refund all fees associated with the given port and channel identifiers.
// If the escrow account runs out of balance then fee module will become locked as this implies the presence
// of a severe bug. When the fee module is locked, no fee distributions will be performed.
// Please see ADR 004 for more information.
func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm starting to wonder if this function should never return an error. The only place it errors is with regards to the refund address. We could either panic or lock the fee module since this is a bug. If we decide to change it, I think it'd be best to do in a follow up pr

Copy link
Member

Choose a reason for hiding this comment

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

I think its fine for now 👍
Agree on follow up if things change!

identifiedPacketFees := k.GetIdentifiedPacketFeesForChannel(ctx, portID, channelID)

var refundErr error
// cache context before trying to distribute fees
// if the escrow account has insufficient balance then we want to avoid partially distributing fees
cacheCtx, writeFn := ctx.CacheContext()

for _, identifiedPacketFee := range identifiedPacketFees {
for _, packetFee := range identifiedPacketFee.PacketFees {

if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) {
// if the escrow account does not have sufficient funds then there must exist a severe bug
// the fee module should be locked until manual intervention fixes the issue
// a locked fee module will simply skip fee logic, all channels will temporarily function as
// fee disabled channels
// NOTE: we use the uncached context to lock the fee module so that the state changes from
// locking the fee module are persisted
k.lockFeeModule(ctx)

k.IteratePacketFeesInEscrow(ctx, portID, channelID, func(packetFees types.PacketFees) (stop bool) {
for _, identifiedFee := range packetFees.PacketFees {
refundAccAddr, err := sdk.AccAddressFromBech32(identifiedFee.RefundAddress)
// return a nil error so state changes are committed but distribution stops
return nil
}

refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
if err != nil {
refundErr = err
return true
return err
}

// if the refund address is blocked, skip and continue distribution
if k.bankKeeper.BlockedAddr(refundAddr) {
continue
Comment on lines +160 to +161
Copy link
Member

Choose a reason for hiding this comment

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

Clean solution!

}

// 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
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.RecvFee); err != nil {
refundErr = err
return true
}
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.AckFee); err != nil {
refundErr = err
return true
}
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.TimeoutFee); err != nil {
refundErr = err
return true
moduleAcc := k.GetFeeModuleAddress()
if err = k.bankKeeper.SendCoins(cacheCtx, moduleAcc, refundAddr, packetFee.Fee.Total()); err != nil {
return err
}

}

return false
})
k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId)
}

// write the cache
writeFn()

return refundErr
return nil
}
Loading