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

Jump to bottom

fix: delete already refunded fees from state if some fee cannot be refunded on channel closure (backport #6255) #6269

Merged
merged 4 commits into from
May 10, 2024
Merged

fix: delete already refunded fees from state if some fee cannot be refunded on channel closure (backport #6255) #6269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
010c4c5
fix: delete already refunded fees from state if some fee cannot be re…
crodriguezvega May 7, 2024
47a5761
fix conflicts
crodriguezvega May 7, 2024
4f5c182
add import
crodriguezvega May 7, 2024
3b11d85
Merge branch 'release/v7.5.x' into mergify/bp/release/v7.5.x/pr-6255
crodriguezvega May 8, 2024
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
12 changes: 8 additions & 4 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 @@ -189,7 +189,7 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st
cacheCtx, writeFn := ctx.CacheContext()

for _, identifiedPacketFee := range identifiedPacketFees {
var failedToSendCoins bool
var unRefundedFees []types.PacketFee
for _, packetFee := range identifiedPacketFee.PacketFees {

if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) {
Expand All @@ -207,18 +207,22 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st

refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
if err != nil {
failedToSendCoins = true
unRefundedFees = append(unRefundedFees, packetFee)
continue
}

// refund all fees to refund address
if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAddr, packetFee.Fee.Total()); err != nil {
failedToSendCoins = true
unRefundedFees = append(unRefundedFees, packetFee)
continue
}
}

if !failedToSendCoins {
if len(unRefundedFees) > 0 {
// update packet fees to keep only the unrefunded fees
packetFees := types.NewPacketFees(unRefundedFees)
k.SetFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId, packetFees)
} else {
k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId)
}
}
Expand Down
87 changes: 33 additions & 54 deletions modules/apps/29-fee/keeper/escrow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
sdkmath "cosmossdk.io/math"
"github.com/cometbft/cometbft/crypto/secp256k1"
sdk "github.com/cosmos/cosmos-sdk/types"

Expand Down Expand Up @@ -313,51 +314,42 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {

func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
var (
expIdentifiedPacketFees []types.IdentifiedPacketFees
expEscrowBal sdk.Coins
expRefundBal sdk.Coins
refundAcc sdk.AccAddress
fee types.Fee
locked bool
expectEscrowFeesToBeDeleted bool
expIdentifiedPacketFees []types.IdentifiedPacketFees
expEscrowBal sdk.Coins
expRefundBal sdk.Coins
refundAcc sdk.AccAddress
fee types.Fee
)

testCases := []struct {
name string
malleate func()
expPass bool
locked bool
}{
{
"success", func() {
for i := 1; i < 6; i++ {
// store the fee in state & update escrow account balance
packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
suite.Require().NoError(err)

expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees)
}
}, true,
}, false,
},
{
"success with undistributed packet fees on a different channel", func() {
for i := 1; i < 6; i++ {
// store the fee in state & update escrow account balance
packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
suite.Require().NoError(err)

expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees)
}

// set packet fee for a different channel
Expand All @@ -371,12 +363,10 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {

expEscrowBal = fee.Total()
expRefundBal = expRefundBal.Sub(fee.Total()...)
}, true,
}, false,
},
{
"escrow account empty, module should become locked", func() {
locked = true

// store the fee in state without updating 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, refundAcc.String(), nil)})
Expand All @@ -385,13 +375,10 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees}
},
true,
}, true,
},
{
"escrow account goes negative on second packet, module should become locked", func() {
locked = true

// store 2 fees in state
packetID1 := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
packetID2 := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(2))
Expand All @@ -412,42 +399,46 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
{
"invalid refund acc address", func() {
// store the fee in state & update escrow account balance
expectEscrowFeesToBeDeleted = false
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)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

packetFees := types.NewPacketFees([]types.PacketFee{
types.NewPacketFee(fee, refundAcc.String(), nil), // this packet fee will be refunded, and will be deleted from state
types.NewPacketFee(fee, "invalid refund address", nil),
})
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
// escrow twice the fee amount to account for the packet to have been incentivized twice
err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total().MulInt(sdkmath.NewInt(2)))
suite.Require().NoError(err)

expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees}
// only the first packet fee should be refunded, the second should remain in state
expIdentifiedPacketFees = []types.IdentifiedPacketFees{types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees[1:])}

expEscrowBal = fee.Total()
expRefundBal = expRefundBal.Sub(fee.Total()...)
}, true,
}, false,
},
{
"distributing to blocked address is skipped", func() {
expectEscrowFeesToBeDeleted = false
blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()

// 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, blockedAddr, nil)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

packetFees := types.NewPacketFees([]types.PacketFee{
types.NewPacketFee(fee, refundAcc.String(), nil), // this packet fee will be refunded, and will be deleted from state
types.NewPacketFee(fee, blockedAddr, nil),
})
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
// escrow twice the fee amount to account for the packet to have been incentivized twice
err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total().MulInt(sdkmath.NewInt(2)))
suite.Require().NoError(err)

expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees}
// only the first packet fee should be refunded, the second should remain in state
expIdentifiedPacketFees = []types.IdentifiedPacketFees{types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees[1:])}

expEscrowBal = fee.Total()
expRefundBal = expRefundBal.Sub(fee.Total()...)
}, true,
}, false,
},
}

Expand All @@ -457,10 +448,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
suite.coordinator.Setup(suite.path) // setup channel
expIdentifiedPacketFees = []types.IdentifiedPacketFees{}
expIdentifiedPacketFees = []types.IdentifiedPacketFees(nil)
expEscrowBal = sdk.Coins{}
locked = false
expectEscrowFeesToBeDeleted = true

// setup
refundAcc = suite.chainA.SenderAccount.GetAddress()
Expand All @@ -483,32 +472,22 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
originalEscrowBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), moduleAcc)

err := suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannelClosure(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
suite.Require().NoError(err)

// refundAcc balance after RefundFeesOnChannelClosure
refundBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc)
escrowBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), moduleAcc)

if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}
suite.Require().Equal(tc.locked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
suite.Require().Equal(expIdentifiedPacketFees, suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))

suite.Require().Equal(locked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))

if locked || !tc.expPass {
if tc.locked {
// refund account and escrow account balances should remain unchanged
suite.Require().Equal(originalRefundBal, refundBal)
suite.Require().Equal(originalEscrowBal, escrowBal)

// ensure none of the fees were deleted
suite.Require().Equal(expIdentifiedPacketFees, suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))
} else {
suite.Require().Equal(expEscrowBal, escrowBal) // escrow balance should be empty
suite.Require().Equal(expRefundBal, refundBal) // all packets should have been refunded

// all fees in escrow should be deleted if expected for this channel
suite.Require().Equal(expectEscrowFeesToBeDeleted, len(suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)) == 0)
}
})
}
Expand Down
Loading