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

Add convenience func to reverse callbacks and refactor other methods #7142

Merged
merged 5 commits into from
Aug 13, 2024
Merged

Add convenience func to reverse callbacks and refactor other methods #7142

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
21ba317
chore: updated handlers to use reverseCallbacks function
chatton Aug 13, 2024
4277f6e
chore: add reconstructVersion to im
chatton Aug 13, 2024
8451d4f
chore: merge feature branch
chatton Aug 13, 2024
100d149
chore: remove duplicate import
chatton Aug 13, 2024
f3d4b4a
chore: pr feedback
chatton Aug 13, 2024
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
124 changes: 67 additions & 57 deletions modules/core/05-port/types/ibc_legacy_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,8 @@ func (im *LegacyIBCModule) OnChanOpenInit(
counterparty channeltypes.Counterparty,
version string,
) (string, error) {
negotiatedVersions := make([]string, len(im.cbs))

for i := len(im.cbs) - 1; i >= 0; i-- {
var negotiatedVersions []string
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] we could be slightly more efficient by doing:

negotiatedVersions := make([]string,0, len(im.cbs)) which would pre-allocate the needed capacity (but not the length).

If we know there's a 1:1 relationship between im.cbs and negotiatedVersions (so that the length is exactly the same) we could even do

negotiatedVersions := make([]string,len(im.cbs)

for i, cb := range im.reverseCallbacks() {
     ...
     negotiatedversions[i] =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah no harm!

for _, cb := range im.reverseCallbacks() {
cbVersion := version

// To maintain backwards compatibility, we must handle two cases:
Expand All @@ -59,24 +58,24 @@ func (im *LegacyIBCModule) OnChanOpenInit(
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(version) != "" {
if wrapper, ok := cb.(VersionWrapper); ok && strings.TrimSpace(version) != "" {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(version)
if err != nil {
// middleware disabled
negotiatedVersions[i] = ""
negotiatedVersions = append(negotiatedVersions, "")
continue
}
cbVersion, version = appVersion, underlyingAppVersion
}

negotiatedVersion, err := im.cbs[i].OnChanOpenInit(ctx, order, connectionHops, portID, channelID, counterparty, cbVersion)
negotiatedVersion, err := cb.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, counterparty, cbVersion)
if err != nil {
return "", errorsmod.Wrapf(err, "channel open init callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
negotiatedVersions[i] = negotiatedVersion
negotiatedVersions = append(negotiatedVersions, negotiatedVersion)
}

return reconstructVersion(im.cbs, negotiatedVersions)
return im.reconstructVersion(negotiatedVersions)
}

// OnChanOpenTry implements the IBCModule interface.
Expand All @@ -93,9 +92,8 @@ func (im *LegacyIBCModule) OnChanOpenTry(
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
negotiatedVersions := make([]string, len(im.cbs))

for i := len(im.cbs) - 1; i >= 0; i-- {
var negotiatedVersions []string
for _, cb := range im.reverseCallbacks() {
cbVersion := counterpartyVersion

// To maintain backwards compatibility, we must handle two cases:
Expand All @@ -107,24 +105,24 @@ func (im *LegacyIBCModule) OnChanOpenTry(
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
if wrapper, ok := cb.(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion)
if err != nil {
// middleware disabled
negotiatedVersions[i] = ""
negotiatedVersions = append(negotiatedVersions, "")
continue
}
cbVersion, counterpartyVersion = appVersion, underlyingAppVersion
}

negotiatedVersion, err := im.cbs[i].OnChanOpenTry(ctx, order, connectionHops, portID, channelID, counterparty, cbVersion)
negotiatedVersion, err := cb.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, counterparty, cbVersion)
if err != nil {
return "", errorsmod.Wrapf(err, "channel open try callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
negotiatedVersions[i] = negotiatedVersion
negotiatedVersions = append(negotiatedVersions, negotiatedVersion)
}

return reconstructVersion(im.cbs, negotiatedVersions)
return im.reconstructVersion(negotiatedVersions)
}

// OnChanOpenAck implements the IBCModule interface.
Expand All @@ -138,14 +136,14 @@ func (im *LegacyIBCModule) OnChanOpenAck(
counterpartyChannelID string,
counterpartyVersion string,
) error {
for i := len(im.cbs) - 1; i >= 0; i-- {
for _, cb := range im.reverseCallbacks() {
cbVersion := counterpartyVersion

// To maintain backwards compatibility, we must handle counterparty version negotiation.
// This means the version may have changed, and applications must be allowed to be disabled.
// Applications should be disabled when receiving an empty counterparty version. Callbacks
// for all applications must occur to allow disabling.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok {
if wrapper, ok := cb.(VersionWrapper); ok {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion)
if err != nil {
cbVersion = "" // disable application
Expand All @@ -154,7 +152,7 @@ func (im *LegacyIBCModule) OnChanOpenAck(
}
}

err := im.cbs[i].OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, cbVersion)
err := cb.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, cbVersion)
if err != nil {
return errorsmod.Wrapf(err, "channel open ack callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
Expand All @@ -169,8 +167,8 @@ func (im *LegacyIBCModule) OnChanOpenConfirm(
portID,
channelID string,
) error {
for i := len(im.cbs) - 1; i >= 0; i-- {
err := im.cbs[i].OnChanOpenConfirm(ctx, portID, channelID)
for _, cb := range im.reverseCallbacks() {
err := cb.OnChanOpenConfirm(ctx, portID, channelID)
if err != nil {
return errorsmod.Wrapf(err, "channel open confirm callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
Expand All @@ -184,8 +182,8 @@ func (im *LegacyIBCModule) OnChanCloseInit(
portID,
channelID string,
) error {
for i := len(im.cbs) - 1; i >= 0; i-- {
if err := im.cbs[i].OnChanCloseInit(ctx, portID, channelID); err != nil {
for _, cb := range im.reverseCallbacks() {
if err := cb.OnChanCloseInit(ctx, portID, channelID); err != nil {
return errorsmod.Wrapf(err, "channel close init callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
}
Expand All @@ -198,8 +196,8 @@ func (im *LegacyIBCModule) OnChanCloseConfirm(
portID,
channelID string,
) error {
for i := len(im.cbs) - 1; i >= 0; i-- {
if err := im.cbs[i].OnChanCloseConfirm(ctx, portID, channelID); err != nil {
for _, cb := range im.reverseCallbacks() {
if err := cb.OnChanCloseConfirm(ctx, portID, channelID); err != nil {
return errorsmod.Wrapf(err, "channel close confirm callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
}
Expand Down Expand Up @@ -230,7 +228,7 @@ func (im *LegacyIBCModule) OnSendPacket(
// is returned if the packet data is successfully decoded and the receive application
// logic returns without error.
// A nil acknowledgement may be returned when using the packet forwarding feature. This signals to core IBC that the acknowledgement will be written asynchronously.
func (LegacyIBCModule) OnRecvPacket(
func (*LegacyIBCModule) OnRecvPacket(
ctx sdk.Context,
channelVersion string,
packet channeltypes.Packet,
Expand All @@ -247,21 +245,21 @@ func (im *LegacyIBCModule) OnAcknowledgementPacket(
acknowledgement []byte,
relayer sdk.AccAddress,
) error {
for i := len(im.cbs) - 1; i >= 0; i-- {
for _, cb := range im.reverseCallbacks() {
var (
cbVersion = channelVersion
cbAck = acknowledgement
)

if wrapper, ok := im.cbs[i].(VersionWrapper); ok {
if wrapper, ok := cb.(VersionWrapper); ok {
cbVersion, channelVersion = wrapper.UnwrapVersionSafe(ctx, packet.SourcePort, packet.SourceChannel, cbVersion)
}

if wrapper, ok := im.cbs[i].(AcknowledgementWrapper); ok {
if wrapper, ok := cb.(AcknowledgementWrapper); ok {
cbAck, acknowledgement = wrapper.UnwrapAcknowledgement(ctx, packet.SourcePort, packet.SourceChannel, cbAck)
}

err := im.cbs[i].OnAcknowledgementPacket(ctx, cbVersion, packet, cbAck, relayer)
err := cb.OnAcknowledgementPacket(ctx, cbVersion, packet, cbAck, relayer)
if err != nil {
return errorsmod.Wrap(err, "acknowledge packet callback failed")
}
Expand All @@ -276,9 +274,7 @@ func (im *LegacyIBCModule) OnTimeoutPacket(
packet channeltypes.Packet,
relayer sdk.AccAddress,
) error {
cbs := slices.Clone(im.cbs)
slices.Reverse(cbs)
for _, cb := range cbs {
for _, cb := range im.reverseCallbacks() {
cbVersion := channelVersion

if wrapper, ok := cb.(VersionWrapper); ok {
Expand All @@ -294,9 +290,8 @@ func (im *LegacyIBCModule) OnTimeoutPacket(

// OnChanUpgradeInit implements the IBCModule interface
func (im *LegacyIBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, proposedVersion string) (string, error) {
negotiatedVersions := make([]string, len(im.cbs))

for i := len(im.cbs) - 1; i >= 0; i-- {
var negotiatedVersions []string
for _, cb := range im.reverseCallbacks() {
cbVersion := proposedVersion

// To maintain backwards compatibility, we must handle two cases:
Expand All @@ -308,18 +303,18 @@ func (im *LegacyIBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(proposedVersion) != "" {
if wrapper, ok := cb.(VersionWrapper); ok && strings.TrimSpace(proposedVersion) != "" {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(proposedVersion)
if err != nil {
// middleware disabled
negotiatedVersions[i] = ""
negotiatedVersions = append(negotiatedVersions, "")
continue
}
cbVersion, proposedVersion = appVersion, underlyingAppVersion
}

// in order to maintain backwards compatibility, every callback in the stack must implement the UpgradableModule interface.
upgradableModule, ok := im.cbs[i].(UpgradableModule)
upgradableModule, ok := cb.(UpgradableModule)
if !ok {
return "", errorsmod.Wrap(ErrInvalidRoute, "upgrade route not found to module in application callstack")
}
Expand All @@ -328,17 +323,17 @@ func (im *LegacyIBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID
if err != nil {
return "", errorsmod.Wrapf(err, "channel open init callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
negotiatedVersions[i] = negotiatedVersion
negotiatedVersions = append(negotiatedVersions, negotiatedVersion)
}

return reconstructVersion(im.cbs, negotiatedVersions)
return im.reconstructVersion(negotiatedVersions)
}

// OnChanUpgradeTry implements the IBCModule interface
func (im *LegacyIBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, counterpartyVersion string) (string, error) {
negotiatedVersions := make([]string, len(im.cbs))
var negotiatedVersions []string

for i := len(im.cbs) - 1; i >= 0; i-- {
for _, cb := range im.reverseCallbacks() {
cbVersion := counterpartyVersion

// To maintain backwards compatibility, we must handle two cases:
Expand All @@ -350,18 +345,18 @@ func (im *LegacyIBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID s
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
if wrapper, ok := cb.(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion)
if err != nil {
// middleware disabled
negotiatedVersions[i] = ""
negotiatedVersions = append(negotiatedVersions, "")
continue
}
cbVersion, counterpartyVersion = appVersion, underlyingAppVersion
}

// in order to maintain backwards compatibility, every callback in the stack must implement the UpgradableModule interface.
upgradableModule, ok := im.cbs[i].(UpgradableModule)
upgradableModule, ok := cb.(UpgradableModule)
if !ok {
return "", errorsmod.Wrap(ErrInvalidRoute, "upgrade route not found to module in application callstack")
}
Expand All @@ -370,15 +365,15 @@ func (im *LegacyIBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID s
if err != nil {
return "", errorsmod.Wrapf(err, "channel open init callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
negotiatedVersions[i] = negotiatedVersion
negotiatedVersions = append(negotiatedVersions, negotiatedVersion)
}

return reconstructVersion(im.cbs, negotiatedVersions)
return im.reconstructVersion(negotiatedVersions)
}

// OnChanUpgradeAck implements the IBCModule interface
func (im *LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
for i := len(im.cbs) - 1; i >= 0; i-- {
for _, cb := range im.reverseCallbacks() {
cbVersion := counterpartyVersion

// To maintain backwards compatibility, we must handle two cases:
Expand All @@ -390,7 +385,7 @@ func (im *LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID,
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
if wrapper, ok := cb.(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion)
if err != nil {
// middleware disabled
Expand All @@ -400,7 +395,7 @@ func (im *LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID,
}

// in order to maintain backwards compatibility, every callback in the stack must implement the UpgradableModule interface.
upgradableModule, ok := im.cbs[i].(UpgradableModule)
upgradableModule, ok := cb.(UpgradableModule)
if !ok {
return errorsmod.Wrap(ErrInvalidRoute, "upgrade route not found to module in application callstack")
}
Expand All @@ -415,7 +410,7 @@ func (im *LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID,

// OnChanUpgradeOpen implements the IBCModule interface
func (im *LegacyIBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, proposedVersion string) {
for i := len(im.cbs) - 1; i >= 0; i-- {
for _, cb := range im.reverseCallbacks() {
cbVersion := proposedVersion

// To maintain backwards compatibility, we must handle two cases:
Expand All @@ -427,7 +422,7 @@ func (im *LegacyIBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok {
if wrapper, ok := cb.(VersionWrapper); ok {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(proposedVersion)
if err != nil {
cbVersion = "" // disable application
Expand All @@ -437,7 +432,7 @@ func (im *LegacyIBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID
}

// in order to maintain backwards compatibility, every callback in the stack must implement the UpgradableModule interface.
upgradableModule, ok := im.cbs[i].(UpgradableModule)
upgradableModule, ok := cb.(UpgradableModule)
if !ok {
panic(errorsmod.Wrap(ErrInvalidRoute, "upgrade route not found to module in application callstack"))
}
Expand All @@ -453,13 +448,28 @@ func (*LegacyIBCModule) UnmarshalPacketData(ctx sdk.Context, portID, channelID s
return nil, nil
}

// reverseCallbacks returns a copy of the callbacks in reverse order.
// the majority of handlers are called in reverse order, so this can be used
// in those cases to prevent needing to iterate backwards over the callbacks.
func (im *LegacyIBCModule) reverseCallbacks() []ClassicIBCModule {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] since this doesn't actually reverse the callbacks, but returns the "reversed", what about calling it reversedCallbacks?

Copy link
Member

Choose a reason for hiding this comment

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

sgtm, I can't think of any better naming tbh

cbs := slices.Clone(im.cbs)
slices.Reverse(cbs)
return cbs
}

// reconstructVersion will generate the channel version by applying any version wrapping as necessary.
// Version wrapping will only occur if the negotiated version is non=empty and the application is a VersionWrapper.
func reconstructVersion(cbs []ClassicIBCModule, negotiatedVersions []string) (string, error) {
func (im *LegacyIBCModule) reconstructVersion(negotiatedVersions []string) (string, error) {
// the negotiated versions are expected to be in reverse order, as callbacks are executed in reverse order.
// in order to ensure that the indices match im.cbs, they must be reversed.
// the slice is cloned to prevent modifying the input argument.
negotiatedVersions = slices.Clone(negotiatedVersions)
slices.Reverse(negotiatedVersions)
Comment on lines +466 to +467
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the one thing I don't like about this PR, the negotiatedVersions are provided in reverse order.

I think we can either do this, require the caller to provide the calling fn to reverse them (easy to forget), or just reverse directly without copying, which will likely not be a problem but this version is just being extra careful.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah. Not beautiful, but its all part of this legacy module construction so I'm fine with it


version := negotiatedVersions[0] // base version
for i := 1; i < len(cbs); i++ { // iterate over the remaining callbacks
for i := 1; i < len(im.cbs); i++ {
if strings.TrimSpace(negotiatedVersions[i]) != "" {
wrapper, ok := cbs[i].(VersionWrapper)
wrapper, ok := im.cbs[i].(VersionWrapper)
if !ok {
return "", ibcerrors.ErrInvalidVersion
}
Expand Down
Loading