Skip to content

Commit

Permalink
fix: event caching for fee distribution (#661)
Browse files Browse the repository at this point in the history 
* proto file

* initial impl

* apply self review suggestions

Deduplicate fee distribution code.
Rename DistributeFee to DistributePacketFees.
Rename DistributeFeeTimeout to DistributePacketFeesOnTimeout

* fixup tests

rename validCoins.
DistributePacketFeesOnTimeout no longer has a valid error case
Add test case for invalid forward relayer address on DistributePacketFees.

* partially fix tests

timeout fee is still being distributed depsite WriteFn() not being called

* fix tests

* address code nit

Co-authored-by: Colin Axnér <[email protected]>
  • Loading branch information
@charleenfei @colin-axner
charleenfei and colin-axner authored Jan 12, 2022
1 parent dbda885 commit 9285133
Show file tree
Hide file tree
Showing 7 changed files with 251 additions and 273 deletions.
34 changes: 5 additions & 29 deletions modules/apps/29-fee/ibc_module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,28 +213,16 @@ func (im IBCModule) OnAcknowledgementPacket(
}

packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)
identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId)

identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId)
if !found {
// return underlying callback if no fee found for given packetID
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
}

// cache context before trying to distribute the fee
cacheCtx, writeFn := ctx.CacheContext()

forwardRelayer, _ := sdk.AccAddressFromBech32(ack.ForwardRelayerAddress)
refundAcc, _ := sdk.AccAddressFromBech32(identifiedPacketFee.RefundAddress)
im.keeper.DistributePacketFees(ctx, identifiedPacketFee.RefundAddress, ack.ForwardRelayerAddress, relayer, identifiedPacketFee)

err := im.keeper.DistributeFee(cacheCtx, refundAcc, forwardRelayer, relayer, packetId)

if err == nil {
// write the cache and then call underlying callback
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())
}
// otherwise discard cache and call underlying callback
// call underlying callback
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
}

Expand All @@ -252,25 +240,13 @@ func (im IBCModule) OnTimeoutPacket(
packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)

identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId)

if !found {
// return underlying callback if fee not found for given packetID
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

// cache context before trying to distribute the fee
cacheCtx, writeFn := ctx.CacheContext()

refundAcc, _ := sdk.AccAddressFromBech32(identifiedPacketFee.RefundAddress)
err := im.keeper.DistributeFeeTimeout(cacheCtx, refundAcc, relayer, packetId)

if err == nil {
// write the cache and then call underlying callback
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())
}
im.keeper.DistributePacketFeesOnTimeout(ctx, identifiedPacketFee.RefundAddress, relayer, identifiedPacketFee)

// otherwise discard cache and call underlying callback
// call underlying callback
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}
158 changes: 99 additions & 59 deletions modules/apps/29-fee/ibc_module_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,15 +508,24 @@ func (suite *FeeTestSuite) TestOnRecvPacket() {

// different channel than sending chain
func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
var ack []byte
var (
ack []byte
identifiedFee *types.IdentifiedPacketFee
originalBalance sdk.Coins
expectedBalance sdk.Coins
expectedRelayerBalance sdk.Coins
)

testCases := []struct {
name string
malleate func()
expPass bool
}{
{
"success",
func() {},
func() {
expectedRelayerBalance = identifiedFee.Fee.ReceiveFee.Add(identifiedFee.Fee.AckFee[0])
},
true,
},
{
Expand All @@ -529,13 +538,17 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(),
ForwardRelayerAddress: suite.chainA.SenderAccount.GetAddress().String(),
}.Acknowledgement()

expectedBalance = originalBalance
},
false,
true,
},
{
"ack wrong format",
func() {
ack = []byte("unsupported acknowledgement format")

expectedBalance = originalBalance
},
false,
},
Expand All @@ -544,27 +557,32 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
func() {
suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
ack = channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement()

expectedBalance = originalBalance
},
false,
true,
},
{
"error on distribute fee (blocked address)",
"fail on distribute receive fee (blocked address)",
func() {
blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), types.ModuleName).GetAddress()
blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()

ack = types.IncentivizedAcknowledgement{
Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(),
ForwardRelayerAddress: blockedAddr.String(),
}.Acknowledgement()

expectedRelayerBalance = identifiedFee.Fee.AckFee
},
false,
true,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
expectedRelayerBalance = sdk.Coins{} // reset

// open incentivized channel
suite.coordinator.Setup(suite.path)
Expand All @@ -582,7 +600,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {

// escrow the packet fee
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence())
identifiedFee := types.NewIdentifiedPacketFee(
identifiedFee = types.NewIdentifiedPacketFee(
packetId,
types.Fee{
ReceiveFee: validCoins,
Expand All @@ -595,76 +613,94 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)

relayerAddr := suite.chainB.SenderAccount.GetAddress()

// must be changed explicitly
ack = types.IncentivizedAcknowledgement{
Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(),
ForwardRelayerAddress: suite.chainA.SenderAccount.GetAddress().String(),
ForwardRelayerAddress: relayerAddr.String(),
}.Acknowledgement()

// log original sender balance
// NOTE: balance is logged after escrowing tokens
originalBalance = sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom))

// default to success case
expectedBalance = originalBalance.
Add(identifiedFee.Fee.TimeoutFee[0])

// malleate test case
tc.malleate()

err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ack, suite.chainA.SenderAccount.GetAddress())
err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ack, relayerAddr)

if tc.expPass {
suite.Require().NoError(err, "unexpected error for case: %s", tc.name)

expectedAmt, ok := sdk.NewIntFromString("10000000000000000000")
suite.Require().True(ok)
suite.Require().Equal(
sdk.Coin{
Denom: ibctesting.TestCoin.Denom,
Amount: expectedAmt,
},
suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom))
suite.Require().NoError(err)
} else {
expectedAmt, ok := sdk.NewIntFromString("9999999999999999400")
suite.Require().True(ok)
suite.Require().Equal(
sdk.Coin{
Denom: ibctesting.TestCoin.Denom,
Amount: expectedAmt,
},
suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom))
suite.Require().Error(err)
}

suite.Require().Equal(
expectedBalance,
sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)),
)

relayerBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, ibctesting.TestCoin.Denom))
suite.Require().Equal(
expectedRelayerBalance,
relayerBalance,
)

})
}
}

func (suite *FeeTestSuite) TestOnTimeoutPacket() {
var (
relayerAddr sdk.AccAddress
relayerAddr sdk.AccAddress
identifiedFee *types.IdentifiedPacketFee
originalBalance sdk.Coins
expectedBalance sdk.Coins
)
testCases := []struct {
name string
malleate func()
expPass bool
name string
malleate func()
expFeeDistributed bool
}{
{
"success",
func() {},
true,
},
{
"no op if identified packet fee doesn't exist",
"fee not enabled",
func() {
// delete packet fee
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence())
suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeInEscrow(suite.chainA.GetContext(), packetId)
suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)

expectedBalance = originalBalance.Add(ibctesting.TestCoin) // timeout refund for ics20 transfer
},
false,
},
{
"error on distribute fee (blocked address)",
"no op if identified packet fee doesn't exist",
func() {
relayerAddr = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), types.ModuleName).GetAddress()
// delete packet fee
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence())
suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeInEscrow(suite.chainA.GetContext(), packetId)

expectedBalance = originalBalance.Add(ibctesting.TestCoin) // timeout refund for ics20 transfer
},
false,
},
{
"fee not enabled",
"distribute fee fails for timeout fee (blocked address)",
func() {
suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
relayerAddr = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress()

expectedBalance = originalBalance.
Add(identifiedFee.Fee.ReceiveFee[0]).
Add(identifiedFee.Fee.AckFee[0]).
Add(ibctesting.TestCoin) // timeout refund for ics20 transfer
},
false,
},
Expand Down Expand Up @@ -696,9 +732,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence())

// must be explicitly changed
relayerAddr = suite.chainA.SenderAccount.GetAddress()
relayerAddr = suite.chainB.SenderAccount.GetAddress()

identifiedFee := types.NewIdentifiedPacketFee(
identifiedFee = types.NewIdentifiedPacketFee(
packetId,
types.Fee{
ReceiveFee: validCoins,
Expand All @@ -712,31 +748,35 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee)
suite.Require().NoError(err)

// log original sender balance
// NOTE: balance is logged after escrowing tokens
originalBalance = sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom))

// default to success case
expectedBalance = originalBalance.
Add(identifiedFee.Fee.ReceiveFee[0]).
Add(identifiedFee.Fee.AckFee[0]).
Add(coin) // timeout refund from ics20 transfer

// malleate test case
tc.malleate()

err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr)
suite.Require().NoError(err)

if tc.expPass {
suite.Require().NoError(err, "unexpected error for case: %s", tc.name)
suite.Require().Equal(
expectedBalance,
sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)),
)

expectedAmt, ok := sdk.NewIntFromString("10000000000000000100")
suite.Require().True(ok)
relayerBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, ibctesting.TestCoin.Denom))
if tc.expFeeDistributed {
suite.Require().Equal(
sdk.Coin{
Denom: ibctesting.TestCoin.Denom,
Amount: expectedAmt,
},
suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom))
identifiedFee.Fee.TimeoutFee,
relayerBalance,
)
} else {
expectedAmt, ok := sdk.NewIntFromString("9999999999999999500")
suite.Require().True(ok)
suite.Require().Equal(
sdk.Coin{
Denom: ibctesting.TestCoin.Denom,
Amount: expectedAmt,
},
suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom))
suite.Require().Empty(relayerBalance)
}
})
}
Expand Down
Loading

0 comments on commit 9285133

Please sign in to comment.