chore: add defensive check to ensure metadata does not change when reopening an active channel

[colin-axner]

Description

closes: #795

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[colin-axner]

I reworked the code, checks now added for controller and host. The check added cannot be reached in tests since we don't support multiple application versions.

Running into an issue with the reopening a channel after closure test. Will debug monday, feel free to push to this branch if you see the fix

Edit: figured it out

[codecov-commenter]

Codecov Report

Merging #847 (146c7fa) into main (c7ea0e8) will increase coverage by 0.08%.
The diff coverage is 67.74%.

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
+ Coverage   79.75%   79.84%   +0.08%     
==========================================
  Files         148      150       +2     
  Lines       10759    10797      +38     
==========================================
+ Hits         8581     8621      +40     
+ Misses       1758     1752       -6     
- Partials      420      424       +4     

Impacted Files	Coverage Δ
...interchain-accounts/controller/keeper/handshake.go	86.66% <50.00%> (-7.68%)	⬇️
...ps/27-interchain-accounts/host/keeper/handshake.go	84.90% <54.54%> (-8.43%)	⬇️
...ules/apps/27-interchain-accounts/types/metadata.go	93.10% <100.00%> (+0.89%)	⬆️
.../apps/27-interchain-accounts/host/keeper/keeper.go	83.49% <0.00%> (-9.71%)	⬇️
modules/core/keeper/msg_server.go	57.71% <0.00%> (-4.70%)	⬇️
modules/core/04-channel/keeper/handshake.go	89.27% <0.00%> (-2.00%)	⬇️
modules/core/02-client/keeper/proposal.go	90.16% <0.00%> (-1.02%)	⬇️
modules/core/02-client/keeper/client.go	98.22% <0.00%> (-0.16%)	⬇️
... and 4 more

[crodriguezvega]

I might be missing something, but the version that is passed here, isn't it the same that is unmarshalled to metadata above? So here:

var metadata icatypes.Metadata
if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {
    return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
}

If that's the case, wouldn't this check here just redundant?

Oh, wait... Or shouldn't you be using here channel.Version instead?

[colin-axner]

nice catch, it should be channel.Version 👍

[colin-axner]

fixed

[crodriguezvega]

Suggestion...

We already have code to unmarhsall the version string here and the same code is found in OnChannelOpenTry and OnOpenChannelAck. And now we also have it inside this function. So we could make an extra constructor function and remove the duplication of code in all these places. So basically to create a function with a signature like this: func NewMetadata(version string) (Metadata, error).

And if you do that then you could also change this function so that it has the Metadata receiver type and change also the name and signature, so something like this: func (Metadata m) IsEqual(metadata Metadata) bool. I leave it up to you if you prefer a pointer receiver type.

The you can invoke these functions like:

previousMetadata, err := icatypes.NewMetadata(previousVersion)
if !err {
    return "", err
}
if !metadata.IsEqual(previousMetadata) {
    ...   
}

[colin-axner]

I like your suggestion. NewMetadata would need to be named something else to avoid name collision with the traditional usage of NewMetadata. IsEqual is also a good idea, but we will remove all this logic with the removal of active channels, so I'm leaning towards leaving as is. It makes the equality check explicitly to be used for active channel security and nothing else (otherwise external users may start depending on that API which should be removed in the future)

[damiannolan]

I prefer @crodriguezvega's suggestion too, but given this will be removed in a couple of months hopefully, then I think we should just leave it as is.

[seantking]

agree with @damiannolan

[crodriguezvega]

If this whole block of code is the same as the one in OnChannelOpenInit, could we extract it into a helper function?

[colin-axner]

I think it is fine to leave as is/be explicit. There's still a lot of duplicate code between OpenInit and OpenTry

[AdityaSripal]

Suggested change

	return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
	return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)

[colin-axner]

updated on controller and host

[seantking]

🤝

[AdityaSripal]

Looks great! I'm assuming there isn't error cases because it's not possible for it to even pass at the moment with some other metadata?

[AdityaSripal]

Is there an error testcase for when previous metadata is different?

[colin-axner]

Unreachable, but I might as well add one

[AdityaSripal]

test case for error when the previous channel has different metadata?

[colin-axner]

Looks great! I'm assuming there isn't error cases because it's not possible for it to even pass at the moment with some other metadata?

correct