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

ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

Merged
merged 10 commits into from
Apr 14, 2022
Merged

ics29: gracefully handle escrow out of balance edge case during fee distribution #1251

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4d5a747
port code from #1056
colin-axner Apr 13, 2022
ca65d4f
refactor: deduplicate distribution code
colin-axner Apr 13, 2022
e91f849
add test case for on acknowledgement packet distribution
colin-axner Apr 13, 2022
317aaa1
add check for timeout packet distribution
colin-axner Apr 13, 2022
27ccd3b
merge main
colin-axner Apr 14, 2022
8e73b5a
revert to duplicate code approach
colin-axner Apr 14, 2022
f968b78
add tests for DistributePacketFeesOnTimeout
colin-axner Apr 14, 2022
558e852
chore: remove unnecessary comment
colin-axner Apr 14, 2022
b73b439
add back godoc
colin-axner Apr 14, 2022
3ac48a2
Merge branch 'main' into colin/821-escrow-out-of-balance-check
colin-axner Apr 14, 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
4 changes: 2 additions & 2 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 @@ -229,7 +229,7 @@ func (im IBCModule) OnAcknowledgementPacket(
packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence)
feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
if found {
im.keeper.DistributePacketFees(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees)
im.keeper.DistributePacketFees(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, false)

// removes the fees from the store as fees are now paid
im.keeper.DeleteFeesInEscrow(ctx, packetID)
Expand Down Expand Up @@ -258,7 +258,7 @@ func (im IBCModule) OnTimeoutPacket(
packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence)
feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
if found {
im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees)
im.keeper.DistributePacketFees(ctx, "", relayer, feesInEscrow.PacketFees, true) // timeouts have no forward relayer

// removes the fee from the store as fee is now paid
im.keeper.DeleteFeesInEscrow(ctx, packetID)
Expand Down
82 changes: 54 additions & 28 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 @@ -50,50 +50,76 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId,
}

// DistributePacketFees pays the acknowledgement fee & receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee.
func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) {
func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee, isTimeout bool) {
Copy link
Contributor

@seantking seantking Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The refactor here is great, I think in some ways splitting up the timeout and ack logic as you have is an improvement. I can't help but feel like I'd prefer to have two separate functions though rather than passing a boolean. I feel like the fact that we're adding a boolean here could be a sign this fn has too much responsibility.

I think I would probably prefer keeping two separate functions like DistributeAcknowledgementPacketFees & DistributeTimeoutPacketFees.

I'm pretty open to either way though :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree. I think it might be worth exploring the option of two separate functions in favour of the boolean arg, considering that the forward relayer is always an empty string for timeout scenarios also.

I do appreciate that it would be a decent bit of duplicate code nonetheless, but I would lean towards having explicit APIs for each scenario (happy path/timeout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main concern is the sensitivity of performing the cacheCtx. It can be hard to verify that we pass in cacheCtx and ctx in the correct functions. Doing this redundantly in two separate functions seems like a recipe for one of the functions to get out of sync with the other and potentially lead to a bug.

The actual distribution of fees on acknowledgement and fees on timeout are still separated. The logic this function is performing is wrapping packet fee distribution with an escrow out of balance check. The previously functionality not only distributed the packet fees for either ack or timeout but also wrapped it with an escrow out of balance check.

For reference you can see how this file differed before splitting. I actually find it harder to verify the distributeAcknowledgementPacketFees and distirbuteTimeoutPacketFees functionality in comparison to the proposed way distributePacketFeeOnAcknowledgement and distirbutePacketFeeOnTimeout

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @seantking and @damiannolan. I think having two separate functions will make the code more explicit and easier to read.

If that's not possible because of the concerns that @colin-axner has regarding the context, then a couple of other alternatives could be:

  1. To have an enum value instead of a boolean as parameter. Then at least reading the function at call site would look like ..., OnTimeout) or ..., OnAcknowledgement).
  2. To pass the function that actually does the distribution instead of a boolean as parameter. So at call site you would have something like ..., distributePacketFeeOnAcknowledgement) or ..., distributePacketFeeOnTimeout). But then you need to have both functions with the same signature, which is possible to work around, but might not lead to the most beautiful result...

// 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()

forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer)

for _, packetFee := range feesInEscrow {
for _, packetFee := range 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)

return
}

// check if refundAcc address works
refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress))
}

// distribute fee to valid forward relayer address otherwise refund the fee
if !forwardAddr.Empty() && !k.bankKeeper.BlockedAddr(forwardAddr) {
// distribute fee for forward relaying
k.distributeFee(ctx, forwardAddr, refundAddr, packetFee.Fee.RecvFee)
if isTimeout {
k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, reverseRelayer, packetFee)
} else {
// refund onRecv fee as forward relayer is not valid address
k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee)
k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee)
}
}

// distribute fee for reverse relaying
k.distributeFee(ctx, reverseRelayer, refundAddr, packetFee.Fee.AckFee)
// write the cache
writeFn()

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

// refund timeout fee for unused timeout
k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee)
}
}

// DistributePacketsFeesTimeout pays the timeout fee for a given packetID while refunding the acknowledgement fee & receive fee to the refund account associated with the Fee
func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) {
for _, feeInEscrow := range feesInEscrow {
// check if refundAcc address works
refundAddr, err := sdk.AccAddressFromBech32(feeInEscrow.RefundAddress)
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", feeInEscrow.RefundAddress))
}
// distributePacketFeeOnAcknowledgement pays the acknowledgement fee & receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee.
func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr, forwardRelayer, reverseRelayer sdk.AccAddress, packetFee types.PacketFee) {
// distribute fee to valid forward relayer address otherwise refund the fee
if !forwardRelayer.Empty() && !k.bankKeeper.BlockedAddr(forwardRelayer) {
// distribute fee for forward relaying
k.distributeFee(ctx, forwardRelayer, refundAddr, packetFee.Fee.RecvFee)
} else {
// refund onRecv fee as forward relayer is not valid address
k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee)
}

// refund receive fee for unused forward relaying
k.distributeFee(ctx, refundAddr, refundAddr, feeInEscrow.Fee.RecvFee)
// distribute fee for reverse relaying
k.distributeFee(ctx, reverseRelayer, refundAddr, packetFee.Fee.AckFee)

// refund ack fee for unused reverse relaying
k.distributeFee(ctx, refundAddr, refundAddr, feeInEscrow.Fee.AckFee)
// refund timeout fee for unused timeout
k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee)

// distribute fee for timeout relaying
k.distributeFee(ctx, timeoutRelayer, refundAddr, feeInEscrow.Fee.TimeoutFee)
}
}

// distributePacketFeeOnTimeout pays the timeout fee for a given packetID while refunding the acknowledgement fee & receive fee to the refund account associated with the Fee
func (k Keeper) distributePacketFeeOnTimeout(ctx sdk.Context, refundAddr sdk.AccAddress, timeoutRelayer sdk.AccAddress, packetFee types.PacketFee) {
// refund receive fee for unused forward relaying
k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee)

// refund ack fee for unused reverse relaying
k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.AckFee)

// distribute fee for timeout relaying
k.distributeFee(ctx, timeoutRelayer, refundAddr, packetFee.Fee.TimeoutFee)
}

// distributeFee will attempt to distribute the escrowed fee to the receiver address.
Expand Down
29 changes: 26 additions & 3 deletions modules/apps/29-fee/keeper/escrow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
refundAcc sdk.AccAddress
refundAccBal sdk.Coin
packetFee types.PacketFee
packetFees []types.PacketFee
)

testCases := []struct {
Expand Down Expand Up @@ -162,6 +163,20 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance)
},
},
{
"escrow account out of balance, fee module becomes locked - no distribution", func() {
// pass in an extra packet fee
packetFees = append(packetFees, packetFee)
},
func() {
suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))

// check if the module acc contains all the fees
expectedModuleAccBal := packetFee.Fee.Total().Add(packetFee.Fee.Total()...)
balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress())
suite.Require().Equal(expectedModuleAccBal, balance)
},
},
{
"invalid forward address",
func() {
Expand Down Expand Up @@ -201,7 +216,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
{
"invalid refund address: no-op, timeout fee remains in escrow",
func() {
packetFee.RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
packetFees[0].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
packetFees[1].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
},
func() {
// check if the module acc contains the timeoutFee
Expand Down Expand Up @@ -229,6 +245,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {

// escrow the packet fee & store the fee in state
packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{})
packetFees = []types.PacketFee{packetFee, packetFee}
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

Expand All @@ -244,7 +261,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
reverseRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), reverseRelayer, sdk.DefaultBondDenom)
refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)

suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.PacketFee{packetFee, packetFee})
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, false)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

tc.expResult()
})
Expand Down Expand Up @@ -282,7 +299,7 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
// refundAcc balance after escrow
refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)

suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, []types.PacketFee{packetFee, packetFee})
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), "", timeoutRelayer, []types.PacketFee{packetFee, packetFee}, true)

// check if the timeoutRelayer has been paid
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), timeoutRelayer, fee.TimeoutFee[0])
Expand All @@ -297,6 +314,12 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
// check the module acc wallet is now empty
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)})
suite.Require().True(hasBalance)

// attempt to distribute with empty escrow balance
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), "", timeoutRelayer, []types.PacketFee{packetFee, packetFee}, true)

suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))

}

func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
Expand Down