chore: allow ics27 to select and return default JSON encoded metadata #1550
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,26 +28,35 @@ func (k Keeper) OnChanOpenInit( | |
| chanCap *capabilitytypes.Capability, | ||
| counterparty channeltypes.Counterparty, | ||
| version string, | ||
| ) (string, error) { | ||
|
There was a problem hiding this comment. what is the implication of changing an There was a problem hiding this comment. In this instance yes, the In other situations we may need to more careful changing function sigs but this doesn't really cause any issues. There was a problem hiding this comment. to add to that: in general, we would also update the ibc spec for changes to these fn sigs. But like @damiannolan said the calling func already has this signature. |
||
| if order != channeltypes.ORDERED { | ||
| return "", sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order) | ||
| } | ||
|
|
||
| if !strings.HasPrefix(portID, icatypes.PortPrefix) { | ||
| return "", sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.PortPrefix, portID) | ||
| } | ||
|
|
||
| if counterparty.PortId != icatypes.PortID { | ||
| return "", sdkerrors.Wrapf(icatypes.ErrInvalidHostPort, "expected %s, got %s", icatypes.PortID, counterparty.PortId) | ||
| } | ||
|
|
||
| var metadata icatypes.Metadata | ||
| if strings.TrimSpace(version) == "" { | ||
| connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0]) | ||
|
There was a problem hiding this comment. why is connectionHops hardcoded to the first value of the array? There was a problem hiding this comment. The first value is the source chain connection end AFAIK There was a problem hiding this comment. ConnectionHops is an ordered list of connection ids, so using Channel spec says:
|
||
| if err != nil { | ||
|
There was a problem hiding this comment. just a super tiny nit but you have the err statement in the second else clause in the There was a problem hiding this comment. But we need to use the if err := invokeFunc(); err != nil {
// handle err
}If we use the following then we can't use the connection below, outside the scope of the conditional: if connection, err := k.channelKeeper.GetConnection(ctx, connectionHops[0]); err != nil {
// connection and err are only available at this level of scope
}There was a problem hiding this comment. ah yes, that makes sense 🙈 There was a problem hiding this comment. you should technically be able to if you put the usage in an else clause, but I personally prefer the way you have it now rather than adding additional |
||
| return "", err | ||
| } | ||
|
|
||
| metadata = icatypes.NewDefaultMetadata(connectionHops[0], connection.GetCounterparty().GetConnectionID()) | ||
chatton marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } else { | ||
| if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil { | ||
|
Comment on lines
+52
to
+53
There was a problem hiding this comment. [nit] rather than having nested if/else clauses it might be cleaner to have a helper function to return the metadata (using early returns) There was a problem hiding this comment. I took a look at this and feel like there's not a massive difference making a helper function. You'd either need to return a new metadata type or take a pointer and a bunch of other args too... I'm pretty indifferent about it |
||
| return "", sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata") | ||
| } | ||
| } | ||
|
|
||
| if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], portID) | ||
|
|
@@ -58,15 +67,15 @@ func (k Keeper) OnChanOpenInit( | |
| } | ||
|
|
||
| if channel.State == channeltypes.OPEN { | ||
| return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID) | ||
| } | ||
|
|
||
| if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) { | ||
| return "", sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version") | ||
| } | ||
| } | ||
|
|
||
| return string(icatypes.ModuleCdc.MustMarshalJSON(&metadata)), nil | ||
|
There was a problem hiding this comment. just to make sure I understand, if this panics this is handled at the sdk level right? There was a problem hiding this comment. Yep, its caught by the grpc recovery interceptor (essentially a middleware handler in grpc land) |
||
| } | ||
|
|
||
| // OnChanOpenAck sets the active channel for the interchain account/owner pair | ||
|
|
||
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.
nice readability improvement 🥇