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.

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

ics4 callbacks fee middleware #580

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
18 changes: 18 additions & 0 deletions docs/ibc/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

- [ibc/applications/fee/v1/genesis.proto](#ibc/applications/fee/v1/genesis.proto)
- [FeeEnabledChannel](#ibc.applications.fee.v1.FeeEnabledChannel)
- [ForwardRelayerAddress](#ibc.applications.fee.v1.ForwardRelayerAddress)
- [GenesisState](#ibc.applications.fee.v1.GenesisState)
- [RegisteredRelayerAddress](#ibc.applications.fee.v1.RegisteredRelayerAddress)

Expand Down Expand Up @@ -724,6 +725,22 @@ Contains the PortID & ChannelID for a fee enabled channel



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

### ForwardRelayerAddress
Contains the forward relayer address and packetId used for async acknowledgements


| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `address` | [string](#string) | | |
| `packet_id` | [ibc.core.channel.v1.PacketId](#ibc.core.channel.v1.PacketId) | | |






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

### GenesisState
Expand All @@ -735,6 +752,7 @@ GenesisState defines the fee middleware genesis state
| `identified_fees` | [IdentifiedPacketFee](#ibc.applications.fee.v1.IdentifiedPacketFee) | repeated | |
| `fee_enabled_channels` | [FeeEnabledChannel](#ibc.applications.fee.v1.FeeEnabledChannel) | repeated | |
| `registered_relayers` | [RegisteredRelayerAddress](#ibc.applications.fee.v1.RegisteredRelayerAddress) | repeated | |
| `forward_relayers` | [ForwardRelayerAddress](#ibc.applications.fee.v1.ForwardRelayerAddress) | repeated | |



Expand Down
9 changes: 3 additions & 6 deletions modules/apps/29-fee/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,14 +184,11 @@ func (im IBCModule) OnRecvPacket(
ack := im.app.OnRecvPacket(ctx, packet, relayer)

forwardRelayer, found := im.keeper.GetCounterpartyAddress(ctx, relayer.String())
if !found {
forwardRelayer = ""
if ack == nil && found {
im.keeper.SetForwardRelayerAddress(ctx, channeltypes.NewPacketId(packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence()), forwardRelayer)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment explaining this functionality. I didn't understand what it was doing until I read the godoc for SetForwardRelayerAddress

Copy link
Member

Choose a reason for hiding this comment

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

Agreed on comment


return types.IncentivizedAcknowledgement{
Result: ack.Acknowledgement(),
ForwardRelayerAddress: forwardRelayer,
}
return types.NewIncentivizedAcknowledgement(forwardRelayer, ack.Acknowledgement())
}

// OnAcknowledgementPacket implements the IBCModule interface
Expand Down
1 change: 0 additions & 1 deletion modules/apps/29-fee/keeper/escrow.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee *types.Identified
}

// Store fee in state for reference later
// feeInEscrow/<port-id>/<channel-id>/packet/<sequence-id>/ -> Fee (timeout, ack, onrecv)
k.SetFeeInEscrow(ctx, identifiedFee)
return nil
}
Expand Down
5 changes: 5 additions & 0 deletions modules/apps/29-fee/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ func (k Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) {
k.SetCounterpartyAddress(ctx, addr.Address, addr.CounterpartyAddress)
}

for _, forwardAddr := range state.ForwardRelayers {
k.SetForwardRelayerAddress(ctx, forwardAddr.PacketId, forwardAddr.Address)
}

for _, enabledChan := range state.FeeEnabledChannels {
k.SetFeeEnabled(ctx, enabledChan.PortId, enabledChan.ChannelId)
}
Expand All @@ -27,5 +31,6 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
IdentifiedFees: k.GetAllIdentifiedPacketFees(ctx),
FeeEnabledChannels: k.GetAllFeeEnabledChannels(ctx),
RegisteredRelayers: k.GetAllRelayerAddresses(ctx),
ForwardRelayers: k.GetAllForwardRelayerAddresses(ctx),
}
}
7 changes: 7 additions & 0 deletions modules/apps/29-fee/keeper/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ func (suite *KeeperTestSuite) TestExportGenesis() {
// set counterparty address
suite.chainA.GetSimApp().IBCFeeKeeper.SetCounterpartyAddress(suite.chainA.GetContext(), sender, counterparty)

// set forward relayer address
suite.chainA.GetSimApp().IBCFeeKeeper.SetForwardRelayerAddress(suite.chainA.GetContext(), packetId, sender)

// export genesis
genesisState := suite.chainA.GetSimApp().IBCFeeKeeper.ExportGenesis(suite.chainA.GetContext())

Expand All @@ -110,4 +113,8 @@ func (suite *KeeperTestSuite) TestExportGenesis() {
// check registered relayer addresses
suite.Require().Equal(sender, genesisState.RegisteredRelayers[0].Address)
suite.Require().Equal(counterparty, genesisState.RegisteredRelayers[0].CounterpartyAddress)

// check registered relayer addresses
suite.Require().Equal(sender, genesisState.ForwardRelayers[0].Address)
suite.Require().Equal(packetId, genesisState.ForwardRelayers[0].PacketId)
}
72 changes: 55 additions & 17 deletions modules/apps/29-fee/keeper/keeper.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"strconv"
"strings"

"github.com/cosmos/cosmos-sdk/codec"
Expand All @@ -12,7 +13,6 @@ import (
"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
ibcexported "github.com/cosmos/ibc-go/modules/core/exported"
)

// Middleware must implement types.ChannelKeeper and types.PortKeeper expected interfaces
Expand Down Expand Up @@ -77,22 +77,6 @@ func (k Keeper) GetFeeModuleAddress() sdk.AccAddress {
return k.authKeeper.GetModuleAddress(types.ModuleName)
}

// SendPacket wraps IBC ChannelKeeper's SendPacket function
func (k Keeper) SendPacket(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet ibcexported.PacketI) error {
return k.ics4Wrapper.SendPacket(ctx, chanCap, packet)
}

// WriteAcknowledgement wraps IBC ChannelKeeper's WriteAcknowledgement function
func (k Keeper) WriteAcknowledgement(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet ibcexported.PacketI,
acknowledgement []byte,
) error {
return nil
// return k.channelKeeper.WriteAcknowledgement(ctx, chanCap, packet, acknowledgement)
}

// SetFeeEnabled sets a flag to determine if fee handling logic should run for the given channel
// identified by channel and port identifiers.
func (k Keeper) SetFeeEnabled(ctx sdk.Context, portID, channelID string) {
Expand Down Expand Up @@ -169,6 +153,7 @@ func (k Keeper) GetCounterpartyAddress(ctx sdk.Context, address string) (string,
return addr, true
}

// GetAllRelayerAddresses returns all registered relayer addresses
func (k Keeper) GetAllRelayerAddresses(ctx sdk.Context) []*types.RegisteredRelayerAddress {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, []byte(types.RelayerAddressKeyPrefix))
Expand All @@ -189,6 +174,59 @@ func (k Keeper) GetAllRelayerAddresses(ctx sdk.Context) []*types.RegisteredRelay
return registeredAddrArr
}

// SetForwardRelayerAddress sets the forward relayer address during OnRecvPacket in case of async acknowledgement
func (k Keeper) SetForwardRelayerAddress(ctx sdk.Context, packetId *channeltypes.PacketId, address string) {
store := ctx.KVStore(k.storeKey)
store.Set(types.KeyForwardRelayerAddress(packetId), []byte(address))
}

// GetForwardRelayerAddress gets forward relayer address for a particular packet
func (k Keeper) GetForwardRelayerAddress(ctx sdk.Context, packetId *channeltypes.PacketId) (string, bool) {
store := ctx.KVStore(k.storeKey)
key := types.KeyForwardRelayerAddress(packetId)
if !store.Has(key) {
return "", false
}

addr := string(store.Get(key))
return addr, true
}

AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
// GetAllForwardRelayerAddresses returns all forward relayer addresses stored for async acknowledgements
func (k Keeper) GetAllForwardRelayerAddresses(ctx sdk.Context) []*types.ForwardRelayerAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a unit test for this? A followup pr is fine

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 create an issue. Thanks

store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, []byte(types.ForwardRelayerPrefix))
defer iterator.Close()

var forwardRelayerAddr []*types.ForwardRelayerAddress
for ; iterator.Valid(); iterator.Next() {
keySplit := strings.Split(string(iterator.Key()), "/")

seq, err := strconv.ParseUint(keySplit[3], 0, 64)
if err != nil {
panic("Error parsing sequence")
seantking marked this conversation as resolved.
Show resolved Hide resolved
}

packetId := channeltypes.NewPacketId(keySplit[2], keySplit[1], seq)

addr := &types.ForwardRelayerAddress{
Address: string(iterator.Value()),
PacketId: packetId,
}

forwardRelayerAddr = append(forwardRelayerAddr, addr)
}

return forwardRelayerAddr
}

// Deletes the forwardRelayerAddr associated with the packetId
func (k Keeper) DeleteForwardRelayerAddress(ctx sdk.Context, packetId *channeltypes.PacketId) {
store := ctx.KVStore(k.storeKey)
key := types.KeyForwardRelayerAddress(packetId)
store.Delete(key)
}

// Stores a Fee for a given packet in state
func (k Keeper) SetFeeInEscrow(ctx sdk.Context, fee *types.IdentifiedPacketFee) {
store := ctx.KVStore(k.storeKey)
Expand Down
34 changes: 34 additions & 0 deletions modules/apps/29-fee/keeper/relay.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package keeper

import (
sdk "github.com/cosmos/cosmos-sdk/types"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
ibcexported "github.com/cosmos/ibc-go/modules/core/exported"
)

// SendPacket wraps IBC ChannelKeeper's SendPacket function
func (k Keeper) SendPacket(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet ibcexported.PacketI) error {
return k.ics4Wrapper.SendPacket(ctx, chanCap, packet)
}

// WriteAcknowledgement wraps IBC ChannelKeeper's WriteAcknowledgement function
// ICS29 WriteAcknowledgement is used for asynchronous acknowledgements
func (k Keeper) WriteAcknowledgement(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet ibcexported.PacketI, acknowledgement []byte) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AdityaSripal I guess I need the OnRecvPacket functionality finished before I can write a test for this?

#262

Copy link
Member

Choose a reason for hiding this comment

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

Yea because OnRecvPacket should ONLY set ForwardRelayerAddress if the underlying application returns a nil acknowledgement. cc: @charleenfei

No need to store it if you're going to send it out immediately

// retrieve the forward relayer that was stored in `onRecvPacket`
packetId := channeltypes.NewPacketId(packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence())
relayer, found := k.GetForwardRelayerAddress(ctx, packetId)

if found {
Copy link
Contributor Author

@seantking seantking Jan 10, 2022

Choose a reason for hiding this comment

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

@AdityaSripal Changed this to if found and added the DeleteForwardRelayer here as we don't want to panic trying to delete the address if it hasn't been set.

Copy link
Member

Choose a reason for hiding this comment

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

Not really necessary since this will no op if the key isn't set but doesn't matter either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok good to know, I was thinking it might panic.

// delete the forward relayer address as it is no longer used
k.DeleteForwardRelayerAddress(ctx, packetId)
}

AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
ack := types.NewIncentivizedAcknowledgement(relayer, acknowledgement)
bz := ack.Acknowledgement()

// ics4Wrapper may be core IBC or higher-level middleware
return k.ics4Wrapper.WriteAcknowledgement(ctx, chanCap, packet, bz)
}
57 changes: 57 additions & 0 deletions modules/apps/29-fee/keeper/relay_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package keeper_test

import (
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
)

func (suite *KeeperTestSuite) TestWriteAcknowledgementAsync() {
testCases := []struct {
name string
malleate func()
expPass bool
}{
{
"success",
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a test case for the forward relayer address being set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added a check to ensure it gets successfully deleted if set, and no-ops otherwise.

func() {},
true,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()

// open incentivized channel
suite.coordinator.Setup(suite.path)

// build packet
timeoutTimestamp := ^uint64(0)
packet := channeltypes.NewPacket(
[]byte("packetData"),
1,
suite.path.EndpointA.ChannelConfig.PortID,
suite.path.EndpointA.ChannelID,
suite.path.EndpointB.ChannelConfig.PortID,
suite.path.EndpointB.ChannelID,
clienttypes.ZeroHeight(),
timeoutTimestamp,
)

ack := []byte("ack")
chanCap := suite.chainB.GetChannelCapability(suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID)

// malleate test case
tc.malleate()

err := suite.chainB.GetSimApp().IBCFeeKeeper.WriteAcknowledgement(suite.chainB.GetContext(), chanCap, packet, ack)

if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}
})
}
}
8 changes: 8 additions & 0 deletions modules/apps/29-fee/types/ack.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

// NewIncentivizedAcknowledgement creates a new instance of IncentivizedAcknowledgement
func NewIncentivizedAcknowledgement(relayer string, ack []byte) IncentivizedAcknowledgement {
return IncentivizedAcknowledgement{
Result: ack,
ForwardRelayerAddress: relayer,
}
}

Copy link
Member

Choose a reason for hiding this comment

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

You also need to add a Success method here. Just return false if ForwardRelayerAddress is empty, and true otherwise

Copy link
Contributor Author

@seantking seantking Dec 2, 2021

Choose a reason for hiding this comment

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

I think @charleenfei has this on her branch. I'll copy it over

// Success implements the Acknowledgement interface. The acknowledgement is
// considered successful if the forward relayer address is empty. Otherwise it is
// considered a failed acknowledgement.
Expand Down
15 changes: 8 additions & 7 deletions modules/apps/29-fee/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ import (

// 29-fee sentinel errors
var (
ErrInvalidVersion = sdkerrors.Register(ModuleName, 2, "invalid ICS29 middleware version")
ErrRefundAccNotFound = sdkerrors.Register(ModuleName, 3, "no account found for given refund address")
ErrBalanceNotFound = sdkerrors.Register(ModuleName, 4, "balance not found for given account address")
ErrFeeNotFound = sdkerrors.Register(ModuleName, 5, "there is no fee escrowed for the given packetID")
ErrRelayersNotNil = sdkerrors.Register(ModuleName, 6, "relayers must be nil. This feature is not supported")
ErrCounterpartyAddressEmpty = sdkerrors.Register(ModuleName, 7, "counterparty address must not be empty")
ErrFeeNotEnabled = sdkerrors.Register(ModuleName, 8, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled")
ErrInvalidVersion = sdkerrors.Register(ModuleName, 2, "invalid ICS29 middleware version")
ErrRefundAccNotFound = sdkerrors.Register(ModuleName, 3, "no account found for given refund address")
ErrBalanceNotFound = sdkerrors.Register(ModuleName, 4, "balance not found for given account address")
ErrFeeNotFound = sdkerrors.Register(ModuleName, 5, "there is no fee escrowed for the given packetID")
ErrRelayersNotNil = sdkerrors.Register(ModuleName, 6, "relayers must be nil. This feature is not supported")
ErrCounterpartyAddressEmpty = sdkerrors.Register(ModuleName, 7, "counterparty address must not be empty")
ErrForwardRelayerAddressNotFound = sdkerrors.Register(ModuleName, 8, "forward relayer address not found")
ErrFeeNotEnabled = sdkerrors.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled")
)
Loading