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

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 1 commit
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
Prev Previous commit
Next Next commit
adapting logic and updating testcases
  • Loading branch information
@damiannolan
damiannolan committed Apr 12, 2022
commit 4e388d5f607bd0460cd12aea8418dfe011ec7412
11 changes: 9 additions & 2 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 @@ -104,10 +104,17 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.Ac
cacheCtx, writeFn := ctx.CacheContext()

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

// 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
return // if sending to the refund address fails, no-op
}
}

Expand Down
138 changes: 88 additions & 50 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,31 +122,94 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() {

func (suite *KeeperTestSuite) TestDistributeFee() {
var (
reverseRelayer sdk.AccAddress
forwardRelayer string
refundAcc sdk.AccAddress
forwardRelayer string
forwardRelayerBal sdk.Coin
reverseRelayer sdk.AccAddress
reverseRelayerBal sdk.Coin
refundAcc sdk.AccAddress
refundAccBal sdk.Coin
)

validSeq := uint64(1)

testCases := []struct {
name string
malleate func()
expPass bool
name string
malleate func()
expResult func()
}{
{
"success", func() {}, true,
"success",
func() {},
func() {
// check if the reverse relayer is paid
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)

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(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
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"
}, false,
},
func() {
// 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()
}, false,
},
func() {
// 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",
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'll see if I can fix this test. Currently failing because we're relying on EscrowPacketFee and malleate() is called after this. If we call malleate() before escrowing the fees, then the RefundAddress (incentivizer) is the module account, which doesn't have balance.

Bit of a mouth full, but hope it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The simplest solution is to mutate packetFee.RefundAddress in malleate() after the fees are initially escrowed and before DistributePacketFees is called 👍

// func() {
// refundAcc = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()
// },
// 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)
// },
// },
}

for _, tc := range testCases {
Expand All @@ -156,59 +219,34 @@ 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{})

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
refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
// 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})

if tc.expPass {
// check if the reverse relayer is paid
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0]))
suite.Require().True(hasBalance)

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

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

// 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)
} else {
// 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)
}
tc.expResult()
})
}
}
Expand Down