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 2 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
16 changes: 8 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,10 @@ 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)
err := im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID)
if err != nil {
return err
}
vuong177 marked this conversation as resolved.
Show resolved Hide resolved
}

// call underlying callback
Expand All @@ -261,10 +261,10 @@ 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)
err := im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID)
if err != nil {
vuong177 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
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)
}

// removes the fee from the store as fee is now paid
vuong177 marked this conversation as resolved.
Show resolved Hide resolved
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
10 changes: 8 additions & 2 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 @@ -59,7 +59,10 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
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()...)
Expand Down Expand Up @@ -149,7 +152,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.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees)
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, packetID)

tc.expResult()
})
Expand Down Expand Up @@ -196,7 +199,10 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
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()...)
Expand Down Expand Up @@ -259,7 +265,7 @@ 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)
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees, packetID)

tc.expResult()
})
Expand Down