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

Jump to bottom

Simplify SendPacket API #1703

Merged
merged 27 commits into from
Oct 6, 2022
Merged

Simplify SendPacket API #1703

Show file tree
Hide file tree
Changes from 8 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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (transfer)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`.
* (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of non-deterministic writes to application state.
* (core/04-channel)[\#1636](https://github.com/cosmos/ibc-go/pull/1636) Removing `SplitChannelVersion` and `MergeChannelVersions` functions since they are not used.
* (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.

### State Machine Breaking

Expand Down
9 changes: 7 additions & 2 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions modules/apps/27-interchain-accounts/controller/keeper/keeper.go
View file
Open in desktop
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
View file
Open in desktop
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
7 changes: 0 additions & 7 deletions modules/apps/27-interchain-accounts/controller/keeper/relay_test.go
View file
Open in desktop
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
View file
Open in desktop
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
8 changes: 1 addition & 7 deletions modules/apps/27-interchain-accounts/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,9 @@ 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)
GetChannel(ctx sdk.Context, sourcePort, srcChan string) (channel channeltypes.Channel, found bool)
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool)
GetConnection(ctx sdk.Context, connectionID string) (ibcexported.ConnectionI, error)
GetAllChannelsWithPortPrefix(ctx sdk.Context, portPrefix string) []channeltypes.IdentifiedChannel
Expand Down
11 changes: 8 additions & 3 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 @@ -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
5 changes: 3 additions & 2 deletions modules/apps/29-fee/keeper/keeper.go
View file
Open in desktop
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/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

authKeeper types.AccountKeeper
ics4Wrapper types.ICS4Wrapper
ics4Wrapper porttypes.ICS4Wrapper
channelKeeper types.ChannelKeeper
portKeeper types.PortKeeper
bankKeeper types.BankKeeper
Expand All @@ -34,7 +35,7 @@ type Keeper struct {
// NewKeeper creates a new 29-fee Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key storetypes.StoreKey,
ics4Wrapper types.ICS4Wrapper, channelKeeper types.ChannelKeeper,
ics4Wrapper porttypes.ICS4Wrapper, channelKeeper types.ChannelKeeper,
portKeeper types.PortKeeper, authKeeper types.AccountKeeper, bankKeeper types.BankKeeper,
) Keeper {
return Keeper{
Expand Down
13 changes: 11 additions & 2 deletions modules/apps/29-fee/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,22 @@ import (
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

"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"
ibcexported "github.com/cosmos/ibc-go/v6/modules/core/exported"
)

// SendPacket wraps IBC ChannelKeeper's SendPacket function
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func (k Keeper) SendPacket(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet ibcexported.PacketI) error {
return k.ics4Wrapper.SendPacket(ctx, chanCap, packet)
func (k Keeper) SendPacket(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
sourcePort string,
sourceChannel string,
timeoutHeight clienttypes.Height,
timeoutTimestamp uint64,
data []byte,
) (uint64, error) {
return k.ics4Wrapper.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
}

// WriteAcknowledgement wraps IBC ChannelKeeper's WriteAcknowledgement function
Expand Down
10 changes: 1 addition & 9 deletions modules/apps/29-fee/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

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

// AccountKeeper defines the contract required for account APIs.
Expand All @@ -15,16 +14,9 @@ type AccountKeeper interface {
GetAccount(sdk.Context, sdk.AccAddress) types.AccountI
}

// ICS4Wrapper defines the expected ICS4Wrapper for middleware
type ICS4Wrapper interface {
WriteAcknowledgement(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet ibcexported.PacketI, acknowledgement ibcexported.Acknowledgement) error
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)
GetChannel(ctx sdk.Context, sourcePort, srcChan string) (channel channeltypes.Channel, found bool)
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
GetPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64) []byte
GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool)
}
Expand Down
5 changes: 3 additions & 2 deletions modules/apps/transfer/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/tendermint/tendermint/libs/log"

"github.com/cosmos/ibc-go/v6/modules/apps/transfer/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 @@ -20,7 +21,7 @@ type Keeper struct {
cdc codec.BinaryCodec
paramSpace paramtypes.Subspace

ics4Wrapper types.ICS4Wrapper
ics4Wrapper porttypes.ICS4Wrapper
channelKeeper types.ChannelKeeper
portKeeper types.PortKeeper
authKeeper types.AccountKeeper
Expand All @@ -31,7 +32,7 @@ type Keeper struct {
// NewKeeper creates a new IBC transfer Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key storetypes.StoreKey, paramSpace paramtypes.Subspace,
ics4Wrapper types.ICS4Wrapper, channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper,
ics4Wrapper porttypes.ICS4Wrapper, channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper,
authKeeper types.AccountKeeper, bankKeeper types.BankKeeper, scopedKeeper types.ScopedKeeper,
) Keeper {
// ensure ibc transfer module account is set
Expand Down
36 changes: 2 additions & 34 deletions modules/apps/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,23 +91,6 @@ func (k Keeper) sendTransfer(
return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", sender)
}

sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel)
if !found {
return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", sourcePort, sourceChannel)
}

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

// get the next sequence
sequence, found := k.channelKeeper.GetNextSequenceSend(ctx, sourcePort, sourceChannel)
if !found {
return sdkerrors.Wrapf(
channeltypes.ErrSequenceSendNotFound,
"source port: %s, source channel: %s", sourcePort, sourceChannel,
)
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// begin createOutgoingPacket logic
// See spec for this logic: https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer#packet-relay
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(sourcePort, sourceChannel))
Expand All @@ -129,10 +112,7 @@ func (k Keeper) sendTransfer(
}
}

labels := []metrics.Label{
telemetry.NewLabel(coretypes.LabelDestinationPort, destinationPort),
telemetry.NewLabel(coretypes.LabelDestinationChannel, destinationChannel),
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
labels := []metrics.Label{}
Copy link
Member

Choose a reason for hiding this comment

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

Are we losing this telemetry info now for destination port and channel?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. cc: @colin-axner

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is okay, so long as there is telemetry within core IBC showing where packets are being sent from/to, then the telemetry could filter on port ID to get the same info. This might break external UX though, so might make more sense to do in a followup

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think telemetry is used on external UX? Also I just think it makes sense to keep it clean here, rather than grabbing channel just so that we can emit the same telemetry.

Afaiu, breaking telemetry does not count as an api breaking or state machine breaking change. So i don't want to be so restrictive and conservative in this case.

I'm in favor of just removing it. We can add it in the changelog so that people who need this information are informed of it beforehand

Copy link
Contributor

@colin-axner colin-axner Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Changing any of the labels will break the telemetry counter. Thus any IBC apps reporting statistics via Prometheus would likely break. I think it makes sense to check with external apps to ensure we don't make a disruptive change out of the blue

Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove this telemetry entirely and replace it with one in core IBC that emits per packet sent, rather than duplicating for every application


// NOTE: SendTransfer simply sends the denomination as it exists on its own
// chain inside the packet data. The receiving chain will perform denom
Expand All @@ -150,7 +130,6 @@ func (k Keeper) sendTransfer(
); err != nil {
return err
}

} else {
labels = append(labels, telemetry.NewLabel(coretypes.LabelSource, "false"))

Expand All @@ -175,18 +154,7 @@ func (k Keeper) sendTransfer(
fullDenomPath, token.Amount.String(), sender.String(), receiver,
)

packet := channeltypes.NewPacket(
packetData.GetBytes(),
sequence,
sourcePort,
sourceChannel,
destinationPort,
destinationChannel,
timeoutHeight,
timeoutTimestamp,
)

if err := k.ics4Wrapper.SendPacket(ctx, channelCap, packet); err != nil {
if _, err := k.ics4Wrapper.SendPacket(ctx, channelCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, packetData.GetBytes()); err != nil {
return err
}

Expand Down
9 changes: 0 additions & 9 deletions modules/apps/transfer/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,6 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
amount = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, sdk.DefaultBondDenom, sdk.NewInt(100))
}, false, true,
},
{
"source channel not found",
func() {
// channel references wrong ID
suite.coordinator.CreateTransferChannels(path)
path.EndpointA.ChannelID = ibctesting.InvalidID
amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
}, true, false,
},
{
"next seq send not found",
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func() {
Expand Down
Loading