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

Simplify SendPacket API #1703

Merged
merged 27 commits into from
Oct 6, 2022
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
05e1d86
fix compile issues and channel keeper tests
AdityaSripal Jul 14, 2022
99db505
add back deleted code
AdityaSripal Jul 14, 2022
a146b33
fix merge
AdityaSripal Jul 14, 2022
3d4a0eb
CHANGELOG
AdityaSripal Jul 14, 2022
3cbbef6
fix var naming and remove unnecessary test cases
AdityaSripal Sep 30, 2022
847fd98
add sequence to return argument of SendPacket
AdityaSripal Sep 30, 2022
3527aed
fix merge
AdityaSripal Sep 30, 2022
7a94343
Merge branch 'main' into aditya/send-packet-api
AdityaSripal Sep 30, 2022
8386d5d
remove print
AdityaSripal Sep 30, 2022
302d32a
Remove unnecessary diffs
AdityaSripal Sep 30, 2022
8dc0c4a
update changelog
AdityaSripal Sep 30, 2022
60c740b
Merge branch 'aditya/send-packet-api' of github.com:cosmos/ibc-go int…
AdityaSripal Sep 30, 2022
c6d9861
Apply suggestions from code review
colin-axner Oct 3, 2022
36742b0
Apply suggestions from code review
colin-axner Oct 3, 2022
e8b8a33
chore: remove unnecessary test
colin-axner Oct 3, 2022
c348aa4
chore: add migration docs
colin-axner Oct 3, 2022
c30f011
revert breaking telemetry
colin-axner Oct 3, 2022
079c3a1
fix: migration docs was in wrong location
colin-axner Oct 3, 2022
90e5e08
Update docs/migrations/v5-to-v6.md
colin-axner Oct 5, 2022
827529a
update 'SendPacket' godoc
colin-axner Oct 5, 2022
e48d77e
Merge branch 'main' of github.com:cosmos/ibc-go into aditya/send-pack…
colin-axner Oct 5, 2022
acaf109
Merge branch 'aditya/send-packet-api' of github.com:cosmos/ibc-go int…
colin-axner Oct 5, 2022
9a7737e
chore: update changelog entry to include packet seq return
colin-axner Oct 5, 2022
6ad76d1
update documentation
crodriguezvega Oct 5, 2022
2faf51e
Merge branch 'main' into aditya/send-packet-api
crodriguezvega Oct 5, 2022
17882e6
chore: fix test case to increase code coverage
colin-axner Oct 6, 2022
d9eb17f
Merge branch 'aditya/send-packet-api' of github.com:cosmos/ibc-go int…
colin-axner Oct 6, 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
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (transfer) [\#2034](https://github.com/cosmos/ibc-go/pull/2034) Transfer Keeper now expects a keeper which fulfills the expected `ScopedKeeper` interface for the capability keeper.
* (05-port) [\#2025](https://github.com/cosmos/ibc-go/pull/2025) Port Keeper now expects a keeper which fulfills the expected `ScopedKeeper` interface for the capability keeper.
* (04-channel) [\#2024](https://github.com/cosmos/ibc-go/pull/2024) Channel Keeper now expects a keeper which fulfills the expected `ScopedKeeper` interface for the capability keeper.
* (core/04-channel)[\#1703](https://github.com/cosmos/ibc-go/pull/1703) Update `SendPacket` API to take in necessary arguments and construct rest of packet rather than taking in entire packet. The generated packet sequence is returned by the `SendPacket` function.
* (modules/apps/27-interchain-accounts) [\#2433](https://github.com/cosmos/ibc-go/pull/2450) Renamed icatypes.PortPrefix to icatypes.ControllerPortPrefix & icatypes.PortID to icatypes.HostPortID

### State Machine Breaking
Expand Down
24 changes: 21 additions & 3 deletions docs/ibc/apps.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,10 +238,21 @@ DecodePacketData(encoded []byte) (CustomPacketData) {
Then a module must encode its packet data before sending it through IBC.
Copy link
Contributor

Choose a reason for hiding this comment

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

I realized that this file is actually not used in the docs website, so could probably be removed...

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, looks like we took this info and split it up into separate files? Lets open an issue whose task is to see if any info in here should be moved before deleting the file

Copy link
Contributor

Choose a reason for hiding this comment

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


```go
// retrieve the dynamic capability for this channel
channelCap := scopedKeeper.GetCapability(ctx, channelCapName)
// Sending custom application packet data
data := EncodePacketData(customPacketData)
packet.Data = data
IBCChannelKeeper.SendPacket(ctx, packet)
// Send packet to IBC, authenticating with channelCap
sequence, err := IBCChannelKeeper.SendPacket(
ctx,
channelCap,
sourcePort,
sourceChannel,
timeoutHeight,
timeoutTimestamp,
data,
)
```

A module receiving a packet must decode the `PacketData` into a structure it expects so that it can
Expand Down Expand Up @@ -284,9 +295,16 @@ packet a module simply needs to call `SendPacket` on the `IBCChannelKeeper`.
channelCap := scopedKeeper.GetCapability(ctx, channelCapName)
// Sending custom application packet data
data := EncodePacketData(customPacketData)
packet.Data = data
// Send packet to IBC, authenticating with channelCap
IBCChannelKeeper.SendPacket(ctx, channelCap, packet)
sequence, err := IBCChannelKeeper.SendPacket(
ctx,
channelCap,
sourcePort,
sourceChannel,
timeoutHeight,
timeoutTimestamp,
data,
)
```

::: warning
Expand Down
11 changes: 9 additions & 2 deletions docs/ibc/apps/ibcmodule.md
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,16 @@ module must trigger execution on the port-bound module through the use of callba
channelCap := scopedKeeper.GetCapability(ctx, channelCapName)
// Sending custom application packet data
data := EncodePacketData(customPacketData)
packet.Data = data
// Send packet to IBC, authenticating with channelCap
IBCChannelKeeper.SendPacket(ctx, channelCap, packet)
sequence, err := IBCChannelKeeper.SendPacket(
ctx,
channelCap,
sourcePort,
sourceChannel,
timeoutHeight,
timeoutTimestamp,
data,
)
```

::: warning
Expand Down
14 changes: 12 additions & 2 deletions docs/ibc/apps/packets_acks.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,20 @@ DecodePacketData(encoded []byte) (CustomPacketData) {
Then a module must encode its packet data before sending it through IBC.

```go
// retrieve the dynamic capability for this channel
channelCap := scopedKeeper.GetCapability(ctx, channelCapName)
// Sending custom application packet data
data := EncodePacketData(customPacketData)
packet.Data = data
IBCChannelKeeper.SendPacket(ctx, packet)
// Send packet to IBC, authenticating with channelCap
sequence, err := IBCChannelKeeper.SendPacket(
ctx,
channelCap,
sourcePort,
sourceChannel,
timeoutHeight,
timeoutTimestamp,
data,
)
```

A module receiving a packet must decode the `PacketData` into a structure it expects so that it can
Expand Down
47 changes: 39 additions & 8 deletions docs/ibc/middleware/develop.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,28 @@ type Middleware interface {
// The base application will call `sendPacket` or `writeAcknowledgement` of the middleware directly above them
// which will call the next middleware until it reaches the core IBC handler.
type ICS4Wrapper interface {
SendPacket(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet exported.Packet) error
WriteAcknowledgement(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet exported.Packet, ack exported.Acknowledgement) error
GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool)
SendPacket(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
sourcePort string,
sourceChannel string,
timeoutHeight clienttypes.Height,
timeoutTimestamp uint64,
data []byte,
) (sequence uint64, err error)

WriteAcknowledgement(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
ack exported.Acknowledgement,
) error

GetAppVersion(
ctx sdk.Context,
portID,
channelID string,
) (string, bool)
}
```

Expand Down Expand Up @@ -352,12 +371,24 @@ Middleware must also wrap ICS-4 so that any communication from the application t
func SendPacket(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
appPacket exported.PacketI,
sourcePort string,
sourceChannel string,
timeoutHeight clienttypes.Height,
timeoutTimestamp uint64,
appData []byte,
) {
// middleware may modify packet
packet = doCustomLogic(appPacket)

return ics4Keeper.SendPacket(ctx, chanCap, packet)
// middleware may modify data
data = doCustomLogic(appData)

return ics4Keeper.SendPacket(
ctx,
chanCap,
sourcePort,
sourceChannel,
timeoutHeight,
timeoutTimestamp,
data,
)
}
```

Expand Down
40 changes: 40 additions & 0 deletions docs/migrations/v5-to-v6.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@

# Migrating from ibc-go v5 to v6

This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG.
Any changes that must be done by a user of ibc-go should be documented here.

There are four sections based on the four potential user groups of this document:
- Chains
- IBC Apps
- Relayers
- IBC Light Clients

**Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated to bump the version number on major releases.

## Chains

## IBC Apps

### ICS04 - `SendPacket` API change

The `SendPacket` API has been simplified:

```diff
// SendPacket is called by a module in order to send an IBC packet on a channel
func (k Keeper) SendPacket(
ctx sdk.Context,
channelCap *capabilitytypes.Capability,
- packet exported.PacketI,
-) error {
+ sourcePort string,
+ sourceChannel string,
+ timeoutHeight clienttypes.Height,
+ timeoutTimestamp uint64,
+ data []byte,
+) (uint64, error) {
```

Callers no longer need to pass in a pre-constructed packet.
The destination port/channel identifiers and the packet sequence will be determined by core IBC.
`SendPacket` will return the packet sequence.
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/controller/keeper"
"github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/controller/types"
icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types"
clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v6/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
Expand Down Expand Up @@ -227,8 +228,12 @@ func (im IBCMiddleware) OnTimeoutPacket(
func (im IBCMiddleware) SendPacket(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet ibcexported.PacketI,
) error {
sourcePort string,
sourceChannel string,
timeoutHeight clienttypes.Height,
timeoutTimestamp uint64,
data []byte,
) (uint64, error) {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
panic("SendPacket not supported for ICA controller module. Please use SendTx")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
genesistypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/genesis/types"
icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v6/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
)

Expand All @@ -25,7 +26,7 @@ type Keeper struct {
cdc codec.BinaryCodec
paramSpace paramtypes.Subspace

ics4Wrapper icatypes.ICS4Wrapper
ics4Wrapper porttypes.ICS4Wrapper
Copy link
Member Author

Choose a reason for hiding this comment

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

No reason for us to duplicate ICS4Wrapper in the expected keepers. the interface already exists in port types. This also prevents accidentally missing a function. Done for all apps

channelKeeper icatypes.ChannelKeeper
portKeeper icatypes.PortKeeper

Expand All @@ -37,7 +38,7 @@ type Keeper struct {
// NewKeeper creates a new interchain accounts controller Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key storetypes.StoreKey, paramSpace paramtypes.Subspace,
ics4Wrapper icatypes.ICS4Wrapper, channelKeeper icatypes.ChannelKeeper, portKeeper icatypes.PortKeeper,
ics4Wrapper porttypes.ICS4Wrapper, channelKeeper icatypes.ChannelKeeper, portKeeper icatypes.PortKeeper,
scopedKeeper icatypes.ScopedKeeper, msgRouter icatypes.MessageRouter,
) Keeper {
// set KeyTable if it has not already been set
Expand Down
43 changes: 3 additions & 40 deletions modules/apps/27-interchain-accounts/controller/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,6 @@ func (k Keeper) sendTx(ctx sdk.Context, connectionID, portID string, icaPacketDa
return 0, sdkerrors.Wrapf(icatypes.ErrActiveChannelNotFound, "failed to retrieve active channel on connection %s for port %s", connectionID, portID)
}

sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID)
if !found {
return 0, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelID)
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

destinationPort := sourceChannelEnd.GetCounterparty().GetPortID()
destinationChannel := sourceChannelEnd.GetCounterparty().GetChannelID()

chanCap, found := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(portID, activeChannelID))
if !found {
return 0, sdkerrors.Wrapf(capabilitytypes.ErrCapabilityNotFound, "failed to find capability: %s", host.ChannelCapabilityPath(portID, activeChannelID))
Expand All @@ -43,45 +35,16 @@ func (k Keeper) sendTx(ctx sdk.Context, connectionID, portID string, icaPacketDa
return 0, icatypes.ErrInvalidTimeoutTimestamp
}

return k.createOutgoingPacket(ctx, portID, activeChannelID, destinationPort, destinationChannel, chanCap, icaPacketData, timeoutTimestamp)
}

func (k Keeper) createOutgoingPacket(
ctx sdk.Context,
sourcePort,
sourceChannel,
destinationPort,
destinationChannel string,
chanCap *capabilitytypes.Capability,
icaPacketData icatypes.InterchainAccountPacketData,
timeoutTimestamp uint64,
) (uint64, error) {
if err := icaPacketData.ValidateBasic(); err != nil {
return 0, sdkerrors.Wrap(err, "invalid interchain account packet data")
}

// get the next sequence
sequence, found := k.channelKeeper.GetNextSequenceSend(ctx, sourcePort, sourceChannel)
if !found {
return 0, sdkerrors.Wrapf(channeltypes.ErrSequenceSendNotFound, "failed to retrieve next sequence send for channel %s on port %s", sourceChannel, sourcePort)
}

packet := channeltypes.NewPacket(
icaPacketData.GetBytes(),
sequence,
sourcePort,
sourceChannel,
destinationPort,
destinationChannel,
clienttypes.ZeroHeight(),
timeoutTimestamp,
)

if err := k.ics4Wrapper.SendPacket(ctx, chanCap, packet); err != nil {
sequence, err := k.ics4Wrapper.SendPacket(ctx, chanCap, portID, activeChannelID, clienttypes.ZeroHeight(), timeoutTimestamp, icaPacketData.GetBytes())
if err != nil {
return 0, err
}

return packet.Sequence, nil
return sequence, nil
}

// OnTimeoutPacket removes the active channel associated with the provided packet, the underlying channel end is closed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,6 @@ func (suite *KeeperTestSuite) TestSendTx() {
},
false,
},
{
"channel does not exist",
func() {
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, "channel-100")
},
false,
},
{
"channel in INIT state - optimistic packet sends fail",
func() {
Expand Down
5 changes: 3 additions & 2 deletions modules/apps/27-interchain-accounts/host/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v6/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
)

Expand All @@ -24,7 +25,7 @@ type Keeper struct {
cdc codec.BinaryCodec
paramSpace paramtypes.Subspace

ics4Wrapper icatypes.ICS4Wrapper
ics4Wrapper porttypes.ICS4Wrapper
channelKeeper icatypes.ChannelKeeper
portKeeper icatypes.PortKeeper
accountKeeper icatypes.AccountKeeper
Expand All @@ -37,7 +38,7 @@ type Keeper struct {
// NewKeeper creates a new interchain accounts host Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key storetypes.StoreKey, paramSpace paramtypes.Subspace,
ics4Wrapper icatypes.ICS4Wrapper, channelKeeper icatypes.ChannelKeeper, portKeeper icatypes.PortKeeper,
ics4Wrapper porttypes.ICS4Wrapper, channelKeeper icatypes.ChannelKeeper, portKeeper icatypes.PortKeeper,
accountKeeper icatypes.AccountKeeper, scopedKeeper icatypes.ScopedKeeper, msgRouter icatypes.MessageRouter,
) Keeper {
// ensure ibc interchain accounts module account is set
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@ type AccountKeeper interface {
GetModuleAddress(name string) sdk.AccAddress
}

// ICS4Wrapper defines the expected ICS4Wrapper for middleware
type ICS4Wrapper interface {
SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error
GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool)
}

// ChannelKeeper defines the expected IBC channel keeper
type ChannelKeeper interface {
GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool)
Expand Down
11 changes: 8 additions & 3 deletions modules/apps/29-fee/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/cosmos/ibc-go/v6/modules/apps/29-fee/keeper"
"github.com/cosmos/ibc-go/v6/modules/apps/29-fee/types"
clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v6/modules/core/05-port/types"
"github.com/cosmos/ibc-go/v6/modules/core/exported"
Expand Down Expand Up @@ -337,9 +338,13 @@ func (im IBCMiddleware) OnTimeoutPacket(
func (im IBCMiddleware) SendPacket(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
) error {
return im.keeper.SendPacket(ctx, chanCap, packet)
sourcePort string,
sourceChannel string,
timeoutHeight clienttypes.Height,
timeoutTimestamp uint64,
data []byte,
) (uint64, error) {
return im.keeper.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
}

// WriteAcknowledgement implements the ICS4 Wrapper interface
Expand Down
Loading