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

feat: allow multiple addrs to incentivize packets #915

Merged
merged 12 commits into from
Feb 16, 2022
+526 −191
Merged

feat: allow multiple addrs to incentivize packets #915

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8717641
[WIP] allow multiple addresses to incentivize a packet
damiannolan Feb 11, 2022
0120019
distribute multiple fees, fix broken tests
damiannolan Feb 11, 2022
c190888
use NewIdentifiedPacketFees in EscrowPacketFee
damiannolan Feb 11, 2022
313d125
cleanup var naming
damiannolan Feb 11, 2022
e640784
removing commented out code and adding test case
damiannolan Feb 12, 2022
77f31f8
Update modules/apps/29-fee/ibc_module.go
damiannolan Feb 15, 2022
d4db040
fix: refund RecvFee if ForwardAddr is invalid
seantking Feb 15, 2022
b234d48
test: update tests to distribute multiple identified fees
seantking Feb 15, 2022
4085fc1
Merge branch 'ics29-fee-middleware' into damian/761-allow-multiple-addrs
seantking Feb 15, 2022
9ca8eee
refactor: clean up DistrPacketFees
seantking Feb 15, 2022
9d8f139
refactor: using .Empty() helper func for code hygiene
seantking Feb 16, 2022
d0f2b39
Merge branch 'ics29-fee-middleware' into damian/761-allow-multiple-addrs
seantking Feb 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: 16 additions & 0 deletions docs/ibc/proto-docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
- [ibc/applications/fee/v1/fee.proto](#ibc/applications/fee/v1/fee.proto)
- [Fee](#ibc.applications.fee.v1.Fee)
- [IdentifiedPacketFee](#ibc.applications.fee.v1.IdentifiedPacketFee)
- [IdentifiedPacketFees](#ibc.applications.fee.v1.IdentifiedPacketFees)

- [ibc/applications/fee/v1/genesis.proto](#ibc/applications/fee/v1/genesis.proto)
- [FeeEnabledChannel](#ibc.applications.fee.v1.FeeEnabledChannel)
Expand Down Expand Up @@ -736,6 +737,21 @@ and an optional list of relayers that are permitted to receive the fee.




<a name="ibc.applications.fee.v1.IdentifiedPacketFees"></a>

### IdentifiedPacketFees
IdentifiedPacketFees contains a list of packet fees


| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `packet_fees` | [IdentifiedPacketFee](#ibc.applications.fee.v1.IdentifiedPacketFee) | repeated | |





<!-- end messages -->

<!-- end enums -->
Expand Down
20 changes: 12 additions & 8 deletions modules/apps/29-fee/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,15 +223,17 @@ func (im IBCModule) OnAcknowledgementPacket(
return sdkerrors.Wrapf(err, "cannot unmarshal ICS-29 incentivized packet acknowledgement: %v", ack)
}

packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)

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

im.keeper.DistributePacketFees(ctx, identifiedPacketFee.RefundAddress, ack.ForwardRelayerAddress, relayer, identifiedPacketFee)
im.keeper.DistributePacketFees(ctx, ack.ForwardRelayerAddress, relayer, identifiedPacketFees.PacketFees)

// removes the fee from the store as fee is now paid
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
im.keeper.DeleteFeesInEscrow(ctx, packetID)

// call underlying callback
return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
Expand All @@ -248,15 +250,17 @@ func (im IBCModule) OnTimeoutPacket(
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)

identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId)
packetID := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence)
identifiedPacketFees, found := im.keeper.GetFeesInEscrow(ctx, packetID)
if !found {
// return underlying callback if fee not found for given packetID
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

im.keeper.DistributePacketFeesOnTimeout(ctx, identifiedPacketFee.RefundAddress, relayer, identifiedPacketFee)
im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, identifiedPacketFees.PacketFees)

// removes the fee from the store as fee is now paid
im.keeper.DeleteFeesInEscrow(ctx, packetID)

// call underlying callback
return im.app.OnTimeoutPacket(ctx, packet, relayer)
Expand Down
16 changes: 8 additions & 8 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 @@ -526,7 +526,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
"no op success without a packet fee",
func() {
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)
Copy link
Member

Choose a reason for hiding this comment

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

nit: The tests in these files could also use multiple fees

suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeesInEscrow(suite.chainA.GetContext(), packetId)

ack = types.IncentivizedAcknowledgement{
Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(),
Expand Down Expand Up @@ -620,8 +620,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
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])
expectedBalance = originalBalance.Add(identifiedFee.Fee.TimeoutFee[0])

// malleate test case
tc.malleate()
Expand Down Expand Up @@ -680,7 +679,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
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.DeleteFeesInEscrow(suite.chainA.GetContext(), packetId)

expectedBalance = originalBalance.Add(ibctesting.TestCoin) // timeout refund for ics20 transfer
},
Expand Down Expand Up @@ -765,10 +764,11 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {

relayerBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, ibctesting.TestCoin.Denom))
if tc.expFeeDistributed {
suite.Require().Equal(
identifiedFee.Fee.TimeoutFee,
relayerBalance,
)
// there should no longer be a fee in escrow for this packet
found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetId)
suite.Require().False(found)

suite.Require().Equal(identifiedFee.Fee.TimeoutFee, relayerBalance)
} else {
suite.Require().Empty(relayerBalance)
}
Expand Down
126 changes: 64 additions & 62 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 @@ -26,64 +26,63 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee types.IdentifiedP
return sdkerrors.Wrapf(types.ErrRefundAccNotFound, "account with address: %s not found", refundAcc)
}

coins := identifiedFee.Fee.RecvFee
coins = coins.Add(identifiedFee.Fee.AckFee...)
coins = coins.Add(identifiedFee.Fee.TimeoutFee...)

if err := k.bankKeeper.SendCoinsFromAccountToModule(
ctx, refundAcc, types.ModuleName, coins,
); err != nil {
coins := identifiedFee.Fee.Total()
if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, refundAcc, types.ModuleName, coins); err != nil {
return err
}

// Store fee in state for reference later
k.SetFeeInEscrow(ctx, identifiedFee)
packetFees := []types.IdentifiedPacketFee{identifiedFee}
if feesInEscrow, found := k.GetFeesInEscrow(ctx, identifiedFee.PacketId); found {
packetFees = append(packetFees, feesInEscrow.PacketFees...)
}

identifiedFees := types.NewIdentifiedPacketFees(packetFees)
k.SetFeesInEscrow(ctx, identifiedFee.PacketId, identifiedFees)
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

return nil
}

// DistributePacketFees pays the acknowledgement fee & receive fee for a given packetId while refunding the timeout fee to the refund account associated with the Fee.
func (k Keeper) DistributePacketFees(ctx sdk.Context, refundAcc, forwardRelayer string, reverseRelayer sdk.AccAddress, feeInEscrow types.IdentifiedPacketFee) {
// distribute fee for forward relaying
forward, err := sdk.AccAddressFromBech32(forwardRelayer)
if err == nil {
k.distributeFee(ctx, forward, feeInEscrow.Fee.RecvFee)
}

// distribute fee for reverse relaying
k.distributeFee(ctx, reverseRelayer, feeInEscrow.Fee.AckFee)
func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, feesInEscrow []types.IdentifiedPacketFee) {
for _, packetFee := range feesInEscrow {
// distribute fee for forward relaying
forward, err := sdk.AccAddressFromBech32(forwardRelayer)
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
if err == nil {
k.distributeFee(ctx, forward, packetFee.Fee.RecvFee)
}

// refund timeout fee refund,
refundAddr, err := sdk.AccAddressFromBech32(refundAcc)
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", refundAcc))
}
// distribute fee for reverse relaying
k.distributeFee(ctx, reverseRelayer, packetFee.Fee.AckFee)

// refund timeout fee for unused timeout
k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.TimeoutFee)
// refund timeout fee refund,
refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress))
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// removes the fee from the store as fee is now paid
k.DeleteFeeInEscrow(ctx, feeInEscrow.PacketId)
// refund timeout fee for unused timeout
k.distributeFee(ctx, refundAddr, packetFee.Fee.TimeoutFee)
}
}

// DistributePacketsFeesTimeout pays the timeout fee for a given packetId while refunding the acknowledgement fee & receive fee to the refund account associated with the Fee
func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, refundAcc string, timeoutRelayer sdk.AccAddress, feeInEscrow types.IdentifiedPacketFee) {
// check if refundAcc address works
refundAddr, err := sdk.AccAddressFromBech32(refundAcc)
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", refundAcc))
}

// refund receive fee for unused forward relaying
k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.RecvFee)
func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, feesInEscrow []types.IdentifiedPacketFee) {
for _, feeInEscrow := range feesInEscrow {
// check if refundAcc address works
refundAddr, err := sdk.AccAddressFromBech32(feeInEscrow.RefundAddress)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", feeInEscrow.RefundAddress))
}

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

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

// removes the fee from the store as fee is now paid
k.DeleteFeeInEscrow(ctx, feeInEscrow.PacketId)
// distribute fee for timeout relaying
k.distributeFee(ctx, timeoutRelayer, feeInEscrow.Fee.TimeoutFee)
}
}

// distributeFee will attempt to distribute the escrowed fee to the receiver address.
Expand All @@ -107,28 +106,31 @@ func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) e

var refundErr error

k.IterateChannelFeesInEscrow(ctx, portID, channelID, func(identifiedFee types.IdentifiedPacketFee) (stop bool) {
refundAccAddr, err := sdk.AccAddressFromBech32(identifiedFee.RefundAddress)
if err != nil {
refundErr = err
return true
k.IterateIdentifiedChannelFeesInEscrow(ctx, portID, channelID, func(identifiedFees types.IdentifiedPacketFees) (stop bool) {
for _, identifiedFee := range identifiedFees.PacketFees {
refundAccAddr, err := sdk.AccAddressFromBech32(identifiedFee.RefundAddress)
if err != nil {
refundErr = err
return true
}

// refund all fees to refund address
// Use SendCoins rather than the module account send functions since refund address may be a user account or module address.
// if any `SendCoins` call returns an error, we return error and stop iteration
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.RecvFee); err != nil {
refundErr = err
return true
}
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.AckFee); err != nil {
refundErr = err
return true
}
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.TimeoutFee); err != nil {
refundErr = err
return true
}
}

// refund all fees to refund address
// Use SendCoins rather than the module account send functions since refund address may be a user account or module address.
// if any `SendCoins` call returns an error, we return error and stop iteration
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.RecvFee); err != nil {
refundErr = err
return true
}
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.AckFee); err != nil {
refundErr = err
return true
}
if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.TimeoutFee); err != nil {
refundErr = err
return true
}
return false
})

Expand Down
34 changes: 24 additions & 10 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 @@ -27,6 +27,23 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() {
{
"success", func() {}, true,
},
{
"success with existing packet fee", func() {
fee := types.Fee{
RecvFee: receiveFee,
AckFee: ackFee,
TimeoutFee: timeoutFee,
}

identifiedPacketFee := types.NewIdentifiedPacketFee(packetId, fee, refundAcc.String(), []string{})

feesInEscrow := types.IdentifiedPacketFees{
PacketFees: []types.IdentifiedPacketFee{identifiedPacketFee},
}

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetId, feesInEscrow)
}, true,
},
{
"fee not enabled on this channel", func() {
packetId.ChannelId = "disabled_channel"
Expand Down Expand Up @@ -84,11 +101,12 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() {
err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedPacketFee)

if tc.expPass {
feeInEscrow, _ := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeInEscrow(suite.chainA.GetContext(), packetId)
feesInEscrow, found := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeesInEscrow(suite.chainA.GetContext(), packetId)
suite.Require().True(found)
// check if the escrowed fee is set in state
suite.Require().True(feeInEscrow.Fee.AckFee.IsEqual(fee.AckFee))
suite.Require().True(feeInEscrow.Fee.RecvFee.IsEqual(fee.RecvFee))
suite.Require().True(feeInEscrow.Fee.TimeoutFee.IsEqual(fee.TimeoutFee))
suite.Require().True(feesInEscrow.PacketFees[0].Fee.AckFee.IsEqual(fee.AckFee))
suite.Require().True(feesInEscrow.PacketFees[0].Fee.RecvFee.IsEqual(fee.RecvFee))
suite.Require().True(feesInEscrow.PacketFees[0].Fee.TimeoutFee.IsEqual(fee.TimeoutFee))
// check if the fee is escrowed correctly
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(600)})
suite.Require().True(hasBalance)
Expand Down Expand Up @@ -158,7 +176,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
// refundAcc balance after escrow
refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)

suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), refundAcc.String(), forwardRelayer, reverseRelayer, identifiedPacketFee)
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.IdentifiedPacketFee{identifiedPacketFee})
Copy link
Member

Choose a reason for hiding this comment

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

Can we test with multiple fees distributed

Copy link
Contributor

Choose a reason for hiding this comment

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

b234d48


if tc.expPass {
// there should no longer be a fee in escrow for this packet
Expand Down Expand Up @@ -225,11 +243,7 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
// refundAcc balance after escrow
refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)

suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), refundAcc.String(), timeoutRelayer, identifiedPacketFee)

// there should no longer be a fee in escrow for this packet
found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), packetId)
suite.Require().False(found)
suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, []types.IdentifiedPacketFee{identifiedPacketFee})
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

b234d48


// check if the timeoutRelayer has been paid
hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), timeoutRelayer, fee.TimeoutFee[0])
Expand Down
Loading