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

Return error if fee module is locked before distributing fees on acknowledgement and timeout #1340

Merged
merged 23 commits into from
May 16, 2022
Merged

Return error if fee module is locked before distributing fees on acknowledgement and timeout #1340

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8c312f6
return err && move DeleteFees in DistributePacketFee func
vuong177 May 5, 2022
9e8841c
add test
vuong177 May 5, 2022
5dfc57a
TestDistributeFee: check if fees has been deleted
vuong177 May 5, 2022
af268d9
add distributePacketError field in testCases
vuong177 May 5, 2022
6532a39
update testcase when ack and timeout in middleware test
vuong177 May 8, 2022
a9dd568
Update modules/apps/29-fee/ibc_middleware.go
vuong177 May 9, 2022
61e1343
Update modules/apps/29-fee/keeper/escrow.go
vuong177 May 9, 2022
e9a6a28
Update modules/apps/29-fee/ibc_middleware_test.go
vuong177 May 9, 2022
9d0b852
Update modules/apps/29-fee/ibc_middleware_test.go
vuong177 May 9, 2022
714dd4c
update CHANGELOG
vuong177 May 10, 2022
880a5d0
nits
vuong177 May 10, 2022
3bd5b9a
Merge branch 'main' into fee-module
vuong177 May 10, 2022
56b15c2
Merge branch 'main' into fee-module
vuong177 May 10, 2022
035b53f
remove error in DistributePacketFeesOnAcknowledgement
vuong177 May 12, 2022
8ae668c
update test
vuong177 May 12, 2022
a11869b
CHANGELOG.MD
vuong177 May 12, 2022
e4adbd9
DistributePacketFeesOnTimeout not return error
vuong177 May 15, 2022
b420e46
remove error from Distribute API and write cache before deleting fees
colin-axner May 16, 2022
2c0ded7
adjust tests to check that fee module gets locked
colin-axner May 16, 2022
f48ab8b
add fee distribution check
colin-axner May 16, 2022
9fbef45
Merge pull request #50 from cosmos/colin/1320-testing-improvements
vuong177 May 16, 2022
2519ed6
Merge branch 'main' into fee-module
vuong177 May 16, 2022
c707223
Merge branch 'main' into fee-module
vuong177 May 16, 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
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 @@ -55,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file.
* (modules/core/04-channel) [\#1279](https://github.com/cosmos/ibc-go/pull/1279) Add selected channel version to MsgChanOpenInitResponse and MsgChanOpenTryResponse. Emit channel version during OpenInit/OpenTry
* (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1`
* (app/29-fee) [\#1340](https://github.com/cosmos/ibc-go/pull/1340) Return error if fee module is locked before distributing fees on acknowledgement and timeout
vuong177 marked this conversation as resolved.
Show resolved Hide resolved

### Features

Expand Down
14 changes: 6 additions & 8 deletions modules/apps/29-fee/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,9 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence)
feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
if found {
im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees)

// removes the fees from the store as fees are now paid
im.keeper.DeleteFeesInEscrow(ctx, packetID)
if err := im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID); err != nil {
return err
}
}

// call underlying callback
Expand All @@ -261,10 +260,9 @@ func (im IBCMiddleware) 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)

// removes the fee from the store as fee is now paid
im.keeper.DeleteFeesInEscrow(ctx, packetID)
if err := im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID); err != nil {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return err
seantking marked this conversation as resolved.
Show resolved Hide resolved
}
}

// call underlying callback
Expand Down
43 changes: 42 additions & 1 deletion modules/apps/29-fee/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ var (
defaultRecvFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}}
defaultAckFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(200)}}
defaultTimeoutFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(300)}}
smallAmount = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(50)}}
)

// Tests OnChanOpenInit on ChainA
Expand Down Expand Up @@ -635,6 +636,21 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
},
true,
},
{
"fail on no distribution by escrow account out of balance",
func() {
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, suite.chainA.SenderAccount.GetSequence())
err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, suite.chainA.SenderAccount.GetAddress(), smallAmount)
suite.Require().NoError(err)

// expectedBalance and expectedRelayerBalance don't change
expectedBalance = originalBalance.Add(smallAmount...)
expectedRelayerBalance = sdk.NewCoins()

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee}))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
},
false,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -720,11 +736,13 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
name string
malleate func()
expFeeDistributed bool
expPass bool
}{
{
"success",
func() {},
true,
true,
},
{
"fee not enabled",
Expand All @@ -734,6 +752,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
expectedBalance = originalBalance
},
false,
true,
},
{
"fee module is disabled, skip fee logic",
Expand All @@ -743,6 +762,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
expectedBalance = originalBalance
},
false,
true,
},
{
"no op if identified packet fee doesn't exist",
Expand All @@ -754,6 +774,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
expectedBalance = originalBalance
},
false,
true,
},
{
"distribute fee fails for timeout fee (blocked address)",
Expand All @@ -766,6 +787,22 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
Add(packetFee.Fee.TimeoutFee...)
},
false,
true,
},
{
"fail on no distribution by escrow account out of balance",
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func() {
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, suite.chainA.SenderAccount.GetSequence())
err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, suite.chainA.SenderAccount.GetAddress(), smallAmount)
suite.Require().NoError(err)

// expectedBalance don't change
expectedBalance = originalBalance.Add(smallAmount...)

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee}))
},
false,
false,
},
}

Expand Down Expand Up @@ -815,7 +852,11 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
tc.malleate()

err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().NoError(err)
if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}

suite.Require().Equal(
expectedBalance,
Expand Down
18 changes: 14 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 @@ -45,7 +45,7 @@ func (k Keeper) escrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId,
}

// DistributePacketFeesOnAcknowledgement pays all the acknowledgement & receive fees for a given packetID while refunding the timeout fees to the refund account.
func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee) {
func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) error {
vuong177 marked this conversation as resolved.
Show resolved Hide resolved
// 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()
Expand All @@ -62,7 +62,7 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe
// locking the fee module are persisted
k.lockFeeModule(ctx)

return
return types.ErrFeeModuleLocked
}

// check if refundAcc address works
Expand All @@ -74,11 +74,16 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe
k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee)
}

// removes the fees from the store as fees are now paid
k.DeleteFeesInEscrow(ctx, packetID)

// write the cache
writeFn()
vuong177 marked this conversation as resolved.
Show resolved Hide resolved

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

return nil
}

// distributePacketFeeOnAcknowledgement pays the receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee.
Expand All @@ -102,7 +107,7 @@ func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr
}

// DistributePacketsFeesOnTimeout pays all the timeout fees for a given packetID while refunding the acknowledgement & receive fees to the refund account.
func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee) {
func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) error {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// 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()
Expand All @@ -117,7 +122,7 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
// locking the fee module are persisted
k.lockFeeModule(ctx)

return
return types.ErrFeeModuleLocked
}

// check if refundAcc address works
Expand All @@ -129,11 +134,16 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee)
}

// removing the fee from the store as the fee is now paid
k.DeleteFeesInEscrow(ctx, packetID)
seantking marked this conversation as resolved.
Show resolved Hide resolved

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

return nil
}

// distributePacketFeeOnTimeout pays the timeout fee to the timeout relayer and refunds the acknowledgement & receive fee.
Expand Down
41 changes: 32 additions & 9 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 @@ -22,14 +22,19 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
)

testCases := []struct {
name string
malleate func()
expResult func()
name string
malleate func()
expResult func()
distributePacketError error
}{
{
"success",
func() {},
func() {
// check if fees has been deleted
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID))

// 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)
Expand All @@ -52,20 +57,25 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
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)
},
nil,
},
{
"escrow account out of balance, fee module becomes locked - no distribution", func() {
// pass in an extra packet fee
packetFees = append(packetFees, packetFee)
},
func() {
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)

suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID))

// 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)
},
types.ErrFeeModuleLocked,
},
{
"invalid forward address",
Expand All @@ -78,6 +88,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
nil,
},
{
"invalid forward address: blocked address",
Expand All @@ -90,6 +101,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
nil,
},
{
"invalid receiver address: ack fee returned to sender",
Expand All @@ -102,6 +114,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
nil,
},
{
"invalid refund address: no-op, timeout fee remains in escrow",
Expand All @@ -115,6 +128,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom)
suite.Require().Equal(expectedModuleAccBal, balance)
},
nil,
},
}

Expand Down Expand Up @@ -149,8 +163,8 @@ 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.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees)

distributePacketError := suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, packetID)
suite.Require().Equal(tc.distributePacketError, distributePacketError)
tc.expResult()
})
}
Expand All @@ -167,9 +181,10 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
)

testCases := []struct {
name string
malleate func()
expResult func()
name string
malleate func()
expResult func()
distributePacketError error
}{
{
"success",
Expand All @@ -189,20 +204,25 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
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)
},
nil,
},
{
"escrow account out of balance, fee module becomes locked - no distribution", func() {
// pass in an extra packet fee
packetFees = append(packetFees, packetFee)
},
func() {
packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)

suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID))

// 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)
},
types.ErrFeeModuleLocked,
},
{
"invalid timeout relayer address: timeout fee returned to sender",
Expand All @@ -215,6 +235,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
suite.Require().Equal(expectedRefundAccBal, balance)
},
nil,
},
{
"invalid refund address: no-op, recv and ack fees remain in escrow",
Expand All @@ -228,6 +249,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom)
suite.Require().Equal(expectedModuleAccBal, balance)
},
nil,
},
}

Expand Down Expand Up @@ -259,7 +281,8 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
timeoutRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), timeoutRelayer, sdk.DefaultBondDenom)
refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)

suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees)
distributePacketError := suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees, packetID)
suite.Require().Equal(tc.distributePacketError, distributePacketError)

tc.expResult()
})
Expand Down