-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add convenience func to reverse callbacks and refactor other methods #7142
Changes from 4 commits
21ba317
4277f6e
8451d4f
100d149
f3d4b4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
for _, cb := range im.reverseCallbacks() { | ||
cbVersion := version | ||
|
||
// To maintain backwards compatibility, we must handle two cases: | ||
|
@@ -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. | ||
|
@@ -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: | ||
|
@@ -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. | ||
|
@@ -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 | ||
|
@@ -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) | ||
} | ||
|
@@ -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) | ||
} | ||
|
@@ -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) | ||
} | ||
} | ||
|
@@ -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) | ||
} | ||
} | ||
|
@@ -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, | ||
|
@@ -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") | ||
} | ||
|
@@ -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 { | ||
|
@@ -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: | ||
|
@@ -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") | ||
} | ||
|
@@ -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: | ||
|
@@ -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") | ||
} | ||
|
@@ -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: | ||
|
@@ -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 | ||
|
@@ -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") | ||
} | ||
|
@@ -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: | ||
|
@@ -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 | ||
|
@@ -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")) | ||
} | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
|
There was a problem hiding this comment.
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
andnegotiatedVersions
(so that the length is exactly the same) we could even doThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah no harm!