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

imp(ica/host): removed previous version validation check #5613

Merged
merged 5 commits into from
Jan 16, 2024
Merged

imp(ica/host): removed previous version validation check #5613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
36e664b
imp: removed validation check
srdtrk Jan 16, 2024
67074b3
test: updated icahost test
srdtrk Jan 16, 2024
69221f8
Merge branch 'feat/allow-unordered-ica-channels' into serdar/issue#56…
srdtrk Jan 16, 2024
ef55c7e
docs: added godocs
srdtrk Jan 16, 2024
198b525
docs: added godocs
srdtrk Jan 16, 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
11 changes: 3 additions & 8 deletions modules/apps/27-interchain-accounts/host/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,9 @@ func (k Keeper) OnChanOpenTry(
return "", errorsmod.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)
}

appVersion, found := k.GetAppVersion(ctx, portID, activeChannelID)
Copy link
Contributor

Choose a reason for hiding this comment

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

just for my own reasoning. The previous metadata has:
version, controller/host connection id's, address, encoding, tx type

Only the host connection id and address cannot change? The host connection id check above in ValidateHostMetadata, but is the address? What happens if the controller reopens a channel modifying the interchain acc address to be different than what it is. It looks like it'll be overwritten later on. I feel like we should document these cases or make them more explicit

Copy link
Member

@damiannolan damiannolan Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm understanding correctly, the address is looked up with a key built from host connection id and counterparty port id, right? And counterparty port ID needs to proven by core before getting here (correction: is used for the key in proving, not value) and if host connection id cannot change then it should be all fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just a bit odd to me because the relayer can pass in:
metadata.Address = invalid-address
and our code won't error
it'll return
metadata.Address = valid-address

just feels a bit odd if a middleware relies on the value before the callback

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. Moreover, the removed code (IsPreviousMetadataEqual) never actually checked if the addresses were the same... It only checked other fields

Copy link
Member Author

Choose a reason for hiding this comment

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

So this was always possible

Copy link
Member Author

@srdtrk srdtrk Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But this damages the backwards compatibility if we check the address since the users are used to being able to pass invalid stuff there (it is common to pass ""! This is because a normal channel handshake must start with this field empty, so users just usually reuse the same version string, including my hackathon project. There are other reasons it is convenient to pass "").

Try step allows the app to propose a new version anyway, so any middleware should use the result of the Try step, and not what the counterparty proposes. Also, #5533 is going to allow host connection id to be fully empty anyway.

So I suggest we don't add these checks, but if you still want the check, then I can add, I suspect it will break some stuff for our users initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay, thanks for the context. Should we add a comment in case someone has the same question as me?

maybe something like:

// if a channel is being reopened, we allow the controller to propose new fields
// which are not exactly the same as the previous. The provided address will 
// be overwritten with the correct one before the metadata is returned

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check at least that version is ics27-1?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is still being checked here @crodriguezvega

if !found {
panic(fmt.Errorf("active channel mapping set for %s, but channel does not exist in channel store", activeChannelID))
}

if !icatypes.IsPreviousMetadataEqual(appVersion, metadata) {
return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
}
// if a channel is being reopened, we allow the controller to propose new fields
// which are not exactly the same as the previous. The provided address will
// be overwritten with the correct one before the metadata is returned.
}

// On the host chain the capability may only be claimed during the OnChanOpenTry
Expand Down
43 changes: 22 additions & 21 deletions modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,28 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
suite.Require().False(found)
}, true,
},
{
"success - previous metadata is different",
func() {
// create a new channel and set it in state
ch := channeltypes.NewChannel(channeltypes.CLOSED, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion)
suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)

// set the active channelID in state
suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID)

// set the previous encoding to be proto3json.
// the new encoding is set to be protobuf in the test below.
metadata.Encoding = icatypes.EncodingProto3JSON

versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)

channel.Version = string(versionBytes)

path.EndpointB.SetChannel(*channel)
}, true,
},
{
"reopening account fails - no existing account",
func() {
Expand Down Expand Up @@ -148,27 +170,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
},
false,
},
{
"invalid metadata - previous metadata is different",
func() {
// create a new channel and set it in state
ch := channeltypes.NewChannel(channeltypes.CLOSED, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion)
suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)

// set the active channelID in state
suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID)

// attempt to downgrade version by reinitializing channel with version 1, but setting channel to version 2
metadata.Version = "ics27-2"

versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)

channel.Version = string(versionBytes)

path.EndpointB.SetChannel(*channel)
}, false,
},
{
"invalid port ID",
func() {
Expand Down
Loading