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

chore(ics29): attempt to refund fees on distribution failure #1245

Merged
merged 7 commits into from
Apr 12, 2022
Merged

chore(ics29): attempt to refund fees on distribution failure #1245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3d86e6d
attempt to refund fees if distribution fails
damiannolan Apr 11, 2022
e3e29a9
Merge branch 'main' into damian/862-fee-distribution-failure
damiannolan Apr 11, 2022
4e388d5
adapting logic and updating testcases
damiannolan Apr 12, 2022
27b42ca
Merge branch 'main' into damian/862-fee-distribution-failure
damiannolan Apr 12, 2022
34c7506
updating inline comments
damiannolan Apr 12, 2022
0632873
updating refund address testcase
damiannolan Apr 12, 2022
ff6bcc6
Merge branch 'main' into damian/862-fee-distribution-failure
damiannolan Apr 12, 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
7 changes: 4 additions & 3 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 @@ -631,7 +631,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
}.Acknowledgement()

expectedRelayerBalance = packetFee.Fee.AckFee
expectedBalance = expectedBalance.Add(packetFee.Fee.RecvFee[0])
expectedBalance = expectedBalance.Add(packetFee.Fee.RecvFee...)
},
true,
},
Expand Down Expand Up @@ -759,8 +759,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
relayerAddr = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()

expectedBalance = originalBalance.
Add(packetFee.Fee.RecvFee[0]).
Add(packetFee.Fee.AckFee[0])
Add(packetFee.Fee.RecvFee...).
Add(packetFee.Fee.AckFee...).
Add(packetFee.Fee.TimeoutFee...)
},
false,
},
Expand Down
38 changes: 25 additions & 13 deletions modules/apps/29-fee/keeper/escrow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"bytes"
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -61,17 +62,17 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev
// 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, packetFee.Fee.RecvFee)
k.distributeFee(ctx, forwardAddr, refundAddr, packetFee.Fee.RecvFee)
} else {
// refund onRecv fee as forward relayer is not valid address
k.distributeFee(ctx, refundAddr, packetFee.Fee.RecvFee)
k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee)
}

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

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

Expand All @@ -85,31 +86,42 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
}

// refund receive fee for unused forward relaying
k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.RecvFee)
k.distributeFee(ctx, refundAddr, refundAddr, feeInEscrow.Fee.RecvFee)

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

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

// distributeFee will attempt to distribute the escrowed fee to the receiver address.
// If the distribution fails for any reason (such as the receiving address being blocked),
// the state changes will be discarded.
func (k Keeper) distributeFee(ctx sdk.Context, receiver sdk.AccAddress, fee sdk.Coins) {
func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.AccAddress, fee sdk.Coins) {
// cache context before trying to distribute fees
cacheCtx, writeFn := ctx.CacheContext()

err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, receiver, fee)
if err == nil {
// write the cache
writeFn()
if err != nil {
if bytes.Equal(receiver, refundAccAddress) {
return // if sending to the refund address already failed, then return (no-op)
}

// 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())
// if an error is returned from x/bank and the receiver is not the refundAccAddress
// then attempt to refund the fee to the original sender
err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddress, fee)
if err != nil {
return // if sending to the refund address fails, no-op
}
}

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

func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) error {
Expand Down
110 changes: 69 additions & 41 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 @@ -122,64 +122,93 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() {

func (suite *KeeperTestSuite) TestDistributeFee() {
var (
reverseRelayer sdk.AccAddress
forwardRelayer string
refundAcc sdk.AccAddress
refundAccBal sdk.Coin
fee types.Fee
packetID channeltypes.PacketId
forwardRelayer string
forwardRelayerBal sdk.Coin
reverseRelayer sdk.AccAddress
reverseRelayerBal sdk.Coin
refundAcc sdk.AccAddress
refundAccBal sdk.Coin
packetFee types.PacketFee
)

validSeq := uint64(1)

testCases := []struct {
name string
malleate func()
expResult func()
}{
{
"success", func() {}, func() {
"success",
func() {},
func() {
// check if the reverse relayer is paid
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0]))
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should be using HasBalance as it just ensures it is greater than or equal to.

Updated to be explicit.
Fetch the account balances below, pre fee distribution, then assert the required amounts are added.

suite.Require().True(hasBalance)
expectedReverseAccBal := reverseRelayerBal.Add(defaultAckFee[0]).Add(defaultAckFee[0])
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), reverseRelayer, sdk.DefaultBondDenom)
suite.Require().Equal(expectedReverseAccBal, balance)

// check if the forward relayer is paid
forward, err := sdk.AccAddressFromBech32(forwardRelayer)
suite.Require().NoError(err)
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0]))
suite.Require().True(hasBalance)

expectedForwardAccBal := forwardRelayerBal.Add(defaultReceiveFee[0]).Add(defaultReceiveFee[0])
balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), forward, sdk.DefaultBondDenom)
suite.Require().Equal(expectedForwardAccBal, balance)

// check if the refund acc has been refunded the timeoutFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0]))
hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0].Add(defaultTimeoutFee[0]))
balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)

// 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)
balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom)
suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance)
},
},
{
"invalid forward address", func() {
"invalid forward address",
func() {
forwardRelayer = "invalid address"
},
func() {
// check if the refund acc has been refunded the timeoutFee & onRecvFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0])
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)

// check if the refund acc has been refunded the timeoutFee & recvFee
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]).Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0])
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
},
{
"invalid forward address: blocked address", func() {
"invalid forward address: blocked address",
func() {
forwardRelayer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()
},
func() {
// check if the refund acc has been refunded the timeoutFee & onRecvFee
expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0])
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal)
suite.Require().True(hasBalance)
// check if the refund acc has been refunded the timeoutFee & recvFee
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]).Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0])
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
},
{
"invalid receiver address: ack fee returned to sender",
func() {
reverseRelayer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()
},
func() {
// check if the refund acc has been refunded the timeoutFee & ackFee
expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultAckFee[0]).Add(defaultTimeoutFee[0]).Add(defaultAckFee[0])
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
},
{
"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()
},
func() {
// check if the module acc contains the timeoutFee
expectedModuleAccBal := sdk.NewCoin(sdk.DefaultBondDenom, defaultTimeoutFee.Add(defaultTimeoutFee...).AmountOf(sdk.DefaultBondDenom))
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom)
suite.Require().Equal(expectedModuleAccBal, balance)
},
},
}
Expand All @@ -191,30 +220,29 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
suite.SetupTest() // reset
suite.coordinator.Setup(suite.path) // setup channel

// setup
refundAcc = suite.chainA.SenderAccount.GetAddress()
reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
// setup accounts
forwardRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()
reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
refundAcc = suite.chainA.SenderAccount.GetAddress()

packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, validSeq)
fee = types.Fee{
RecvFee: defaultReceiveFee,
AckFee: defaultAckFee,
TimeoutFee: defaultTimeoutFee,
}
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee)

// escrow the packet fee & store the fee in state
packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{})

packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{})
err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

// escrow a second packet fee to test with multiple fees distributed
err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee)
suite.Require().NoError(err)

tc.malleate()

// refundAcc balance after escrow
// fetch the account balances before fee distribution (forward, reverse, refund)
forwardAccAddress, _ := sdk.AccAddressFromBech32(forwardRelayer)
forwardRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), forwardAccAddress, sdk.DefaultBondDenom)
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})
Expand Down