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

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

Merged
merged 9 commits into from
May 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e165abe
delete the refunded fees in case an error happens in the loop that re…
crodriguezvega May 1, 2024
24e40bd
test simplifications
crodriguezvega May 2, 2024
6c3b674
fix typo
crodriguezvega May 5, 2024
9eb3bd4
clean up code
crodriguezvega May 5, 2024
9027b6e
fix logic
crodriguezvega May 5, 2024
5389f1e
add changelog
crodriguezvega May 5, 2024
cfe3b11
Merge branch 'main' into carlos/delete-refunded-fees
crodriguezvega May 6, 2024
dd1c478
Merge branch 'main' into carlos/delete-refunded-fees
crodriguezvega May 7, 2024
eda884d
Merge branch 'main' into carlos/delete-refunded-fees
crodriguezvega May 7, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Bug Fixes

* (apps/27-interchain-accounts) [\#6167](https://github.com/cosmos/ibc-go/pull/6167) Fixed an edge case bug where migrating params for a pre-existing ica module which implemented controller functionality only could panic when migrating params for newly added host, and align controller param migration with host.
* (app/29-fee) [\#6255](https://github.com/cosmos/ibc-go/pull/6255) Delete refunded fees from state if some fee(s) cannot be refunded on channel closure.

## [v8.2.0](https://github.com/cosmos/ibc-go/releases/tag/v8.2.0) - 2024-04-05

Expand Down
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
86 changes: 32 additions & 54 deletions modules/apps/29-fee/keeper/escrow_test.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

might be nice to refactor this test in future to be a bit more clear, maybe we some expResult func or something. But happy to move ahead with this for now

Original file line number Diff line number Diff line change
Expand Up @@ -395,51 +395,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 @@ -453,12 +444,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 @@ -467,13 +456,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 @@ -494,42 +480,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 @@ -539,10 +529,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
suite.path.Setup() // setup channel
expIdentifiedPacketFees = []types.IdentifiedPacketFees{}
expIdentifiedPacketFees = []types.IdentifiedPacketFees(nil)
expEscrowBal = sdk.Coins{}
locked = false
expectEscrowFeesToBeDeleted = true

// setup
refundAcc = suite.chainA.SenderAccount.GetAddress()
Expand All @@ -565,32 +553,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