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 2 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
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)
}
51 changes: 45 additions & 6 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,11 +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)
}

// 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 @@ -158,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 @@ -178,6 +174,49 @@ 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, _ := strconv.ParseUint(keySplit[3], 0, 64)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure if/how we should handle this error. If this errs the code is broken elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

I think you could probably just panic. I'd say it's nicer than just ignoring the error, which could maybe be considered "bad code hygiene", and it seems to be done that way in other parts of the core codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Just panicing makes sense here. This isn't run within the state machine anyway. So it actually is preferable to just panic if someone runs this command and finds this issue out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a panic. Not sure about the error message.

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

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

forwardRelayerAddr = append(forwardRelayerAddr, addr)
}

return forwardRelayerAddr
}

// 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 (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
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`
relayer, found := k.GetForwardRelayerAddress(ctx, channeltypes.NewPacketId(packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence()))
if !found {
return sdkerrors.Wrap(types.ErrForwardRelayerAddressNotFound, fmt.Sprintf("packet: %s, %s, %d", packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()))
seantking marked this conversation as resolved.
Show resolved Hide resolved
}

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)
}
22 changes: 22 additions & 0 deletions modules/apps/29-fee/types/ack.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package types

import (
"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

// NewMsgRegisterCounterpartyAddress creates a new instance of MsgRegisterCounterpartyAddress
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

// Acknowledgement implements the Acknowledgement interface. It returns the
// acknowledgement serialised using JSON.
func (ack IncentivizedAcknowledgement) Acknowledgement() []byte {
var SubModuleCdc = codec.NewProtoCodec(codectypes.NewInterfaceRegistry())
Copy link
Contributor

Choose a reason for hiding this comment

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

use the ModuleCdc in codec.go (rename to SubModuleCdc)

return sdk.MustSortJSON(SubModuleCdc.MustMarshalJSON(&ack))
}
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