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.

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

refactor: channel handshake version improvements #1283

Merged
merged 23 commits into from
May 10, 2022

Conversation

seantking
Copy link
Contributor

@seantking seantking commented Apr 21, 2022

Description

closes: #722


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

@seantking seantking changed the title refactor: channel handshake improvements refactor: channel handshake version improvements Apr 21, 2022
return nil, sdkerrors.Wrap(err, "channel open init callback failed")
}

// Write channel into state
k.ChannelKeeper.WriteOpenInitChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, msg.Channel.Version)
k.ChannelKeeper.WriteOpenInitChannel(ctx, msg.PortId, channelID, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.Channel.Counterparty, version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this causes the fee tests to break. Looking into it 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed: 0178c09

modules/core/05-port/types/module.go Show resolved Hide resolved
modules/apps/transfer/ibc_module.go Outdated Show resolved Hide resolved
modules/apps/29-fee/ibc_module.go Outdated Show resolved Hide resolved
@seantking seantking marked this pull request as ready for review April 26, 2022 11:00
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2022

Codecov Report

Merging #1283 (35f71cf) into main (bd08650) will increase coverage by 0.00%.
The diff coverage is 93.87%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1283   +/-   ##
=======================================
  Coverage   80.27%   80.28%           
=======================================
  Files         166      166           
  Lines       12023    12044   +21     
=======================================
+ Hits         9652     9670   +18     
- Misses       1916     1918    +2     
- Partials      455      456    +1     
Impacted Files Coverage Δ
modules/core/keeper/msg_server.go 58.29% <75.00%> (+0.09%) ⬆️
modules/apps/29-fee/ibc_middleware.go 93.75% <92.85%> (-1.43%) ⬇️
...7-interchain-accounts/controller/ibc_middleware.go 87.03% <100.00%> (+0.49%) ⬆️
...les/apps/27-interchain-accounts/host/ibc_module.go 100.00% <100.00%> (ø)
modules/apps/transfer/ibc_module.go 63.88% <100.00%> (+0.76%) ⬆️

return "", err
}

if strings.TrimSpace(version) == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add a test case for this

@seantking seantking requested a review from colin-axner April 26, 2022 17:29
CHANGELOG.md Outdated
@@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
### API Breaking

* (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used.
* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) `OnChanOpenInit` now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to improvements on the changelog message.

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Nice work, @seantking!

I just noticed that in ICA we don't use any types.Version like we have for transfer (ics20-1) or fee. Was there a reason not use one there?

}

im.keeper.SetFeeEnabled(ctx, portID, channelID)

// call underlying app's OnChanOpenInit callback with the appVersion
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, versionMetadata.AppVersion)
return string(versionBytes), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the call to underlying app's OnChanOpenInit removed here?

Copy link
Member

Choose a reason for hiding this comment

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

Its moved to above on line 53 so that the returned application version (for example: ics20-1) can be inserted into the AppVersion field of types.Metadata, and re-marshalled to a JSON encoded version string. Otherwise we would return directly from the app callback (writing ics20-1 as the channel version into state), we would expect ics29 to be enabled but subsequent handshake callbacks on would fail as the channel version in state would not be the metadata object:

{
    "AppVersion": "ics20-1",
    "FeeVersion": "ics29-1",
}

Does that make sense?

"empty version string", func() {
channel.Version = ""
}, true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a test case when channel.Version is not empty but it's a string that does not match the expected types.Version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have this test case already, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, sorry. I missed it.

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM today, my only concern is the ICA auth module handling of the version string and if it should be given the opportunity to modify it, my initial thinking is no.
Other than that just some minor nits

CHANGELOG.md Outdated
@@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
### API Breaking

* (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used.
* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) `OnChanOpenInit` now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).
Copy link
Member

Choose a reason for hiding this comment

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

Some sugar coating?

Suggested change
* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) `OnChanOpenInit` now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).
* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) The `OnChanOpenInit` application callback now supports version selection and returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the first part but left out the version selection. I found that term confusing personally.

Comment on lines 54 to 56
// call underlying app's OnChanOpenInit callback with the passed in version
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, version)
Copy link
Member

Choose a reason for hiding this comment

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

This gives an opportunity to the interchain accounts auth module to modify the version string, or perhaps mistakenly return the incorrect string, right?

Is this something we want to support? i.e. providing the auth module the ability to mutate this?
Currently the only expectation from this callback is the claiming of the channel capability.

The code below would complain about appVersion declared but not used...

Suggested change
// call underlying app's OnChanOpenInit callback with the passed in version
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, version)
// call underlying app's OnChanOpenInit callback with the passed in version
appVersion, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, version)
if err != nil {
return "", err
}
return version, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really following your point with the code example.

What would be the alternative here?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed if ibc-auth is expected not to change the version at all. Then we should ignore it's return value. And if relayer passes in empty string, then just return our default version

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if that was confusing, like @AdityaSripal says, we should ignore the appVersion field from the code example above by discarding it, as we don't want to depend on ica-auth to return the correct string(version) or risk modifying it:

_, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty version)
if err != nil {
    return "", err
}

And if relayer passes in empty string, then just return our default version

But for ICA a relayer is never expected to initiate the channel handshake, it should be from some on-chain mechanism, correct?

Copy link
Contributor Author

@seantking seantking Apr 27, 2022

Choose a reason for hiding this comment

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

But for ICA a relayer is never expected to initiate the channel handshake, it should be from some on-chain mechanism, correct?

In our implementation of the controller module, yes. The version string will always be the Metadata struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this. Good catch 👍

path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(),
)

if tc.expPass {
expMetaData := icatypes.NewMetadata(
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: casing

Suggested change
expMetaData := icatypes.NewMetadata(
expMetadata := icatypes.NewMetadata(

}

im.keeper.SetFeeEnabled(ctx, portID, channelID)

// call underlying app's OnChanOpenInit callback with the appVersion
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, versionMetadata.AppVersion)
return string(versionBytes), nil
Copy link
Member

Choose a reason for hiding this comment

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

Its moved to above on line 53 so that the returned application version (for example: ics20-1) can be inserted into the AppVersion field of types.Metadata, and re-marshalled to a JSON encoded version string. Otherwise we would return directly from the app callback (writing ics20-1 as the channel version into state), we would expect ics29 to be enabled but subsequent handshake callbacks on would fail as the channel version in state would not be the metadata object:

{
    "AppVersion": "ics20-1",
    "FeeVersion": "ics29-1",
}

Does that make sense?

modules/apps/29-fee/ibc_module.go Outdated Show resolved Hide resolved
} else {
suite.Require().Equal(version, ibcmock.Version)
}

suite.Require().NoError(err, "unexpected error from version: %s", tc.version)
} else {
suite.Require().Error(err, "error not returned for version: %s", tc.version)
Copy link
Member

Choose a reason for hiding this comment

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

Could we also assert version is empty here too?

path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, counterparty, channel.GetVersion(),
)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(types.Version, version)
} else {
suite.Require().Error(err)
Copy link
Member

Choose a reason for hiding this comment

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

ditto on empty version assertion

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Looks good so far. The default version logic you have in transfer should be extended to the other applications

CHANGELOG.md Outdated
@@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
### API Breaking

* (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used.
* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) `OnChanOpenInit` now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).
Copy link
Member

Choose a reason for hiding this comment

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

I would add that it may return a default version string if relayers pass in empty version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this out as I don't think the default is applicable to all modules.

Copy link
Member

Choose a reason for hiding this comment

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

Hence the word may, i think it could still be useful but maybe we want to leave that suggestion elsewhere (e.g. release notes or something)

return "", err
}

if strings.TrimSpace(version) == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't all of the applications/middleware have a similar code block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

responded below

@@ -36,7 +36,7 @@ func (im IBCModule) OnChanOpenInit(
chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
) error {
) (string, error) {
var versionMetadata types.Metadata
if err := types.ModuleCdc.UnmarshalJSON([]byte(version), &versionMetadata); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This will not enable fee by default. If provided string is empty then, get default version from underlying app and wrap with fee version.
If underlying app returns error with empty version then return error as well

Copy link
Contributor

@colin-axner colin-axner Apr 27, 2022

Choose a reason for hiding this comment

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

Do we want to enable fees by default? Otherwise there's no way to only use the default version of the base application. I'd think middleware applications must be explicitly activated or, the versionMetadata should be specified with an empty fee version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was my assumption also. What is the benefit of enabling the fee middleware by default with an empty version passed in?

Copy link
Member

Choose a reason for hiding this comment

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

I do think fees should be enabled by default. My preference is that relayers get the default version of the stack if they don't explicitly specify otherwise. Some middleware will want to be activated by default and some may want to be explicitly activated. For adoption purposes, I think it would be good to make incentivization the default rather than having to explicitly enable it. Because as I think the number of applications scale, most relayers will only be passing in empty strings and use non-empty strings only in very special cases (otherwise they would need to have context on every single port they relay for).

So a relayer that always defaults to what the application prefers will be opening incentivized channels.

Of course, if the relayer wants to explicitly disable fees then they can by passing in a version without the feeVersion. But we should design for a future where relayers are rarely passing in specific strings

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. In favor of @AdityaSripal's approach. It should just be well documented that ics29 is an exceptional middleware application that we believe should be enabled by default, but that other applications (who might be copy/pasting our code) should give good consideration to whether their application should be enabled by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation @AdityaSripal. Makes sense to me!

Comment on lines 54 to 56
// call underlying app's OnChanOpenInit callback with the passed in version
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, version)
Copy link
Member

Choose a reason for hiding this comment

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

Agreed if ibc-auth is expected not to change the version at all. Then we should ignore it's return value. And if relayer passes in empty string, then just return our default version

@seantking
Copy link
Contributor Author

Looks good so far. The default version logic you have in transfer should be extended to the other applications

I left the default only for ics20.

I think ics29 should not be enabled by default as @colin-axner suggests & relayers do not pass a version for ics-27. The controller for ics27 module always passes a metaData struct as a version to the OnChanOpenInit callback, it's not optional.

@seantking
Copy link
Contributor Author

Nice work, @seantking!

I just noticed that in ICA we don't use any types.Version like we have for transfer (ics20-1) or fee. Was there a reason not use one there?

Here :)

Comment on lines 54 to 57
// call underlying app's OnChanOpenInit callback with the passed in version
if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
return "", err
}
Copy link
Member

Choose a reason for hiding this comment

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

Could be useful to have a small explanation of why this is ignored in the comment.

@seantking seantking force-pushed the sean/issue#722-channel-handshake-improvements branch from 2f640f3 to d30f887 Compare April 29, 2022 16:42
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM!

CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Some testing and documentation nits :)

Would appreciate having them fixed, but doesn't need to happen in this PR

Comment on lines 108 to 109
isFeeEnabled := suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
if isFeeEnabled {
Copy link
Member

Choose a reason for hiding this comment

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

This is a strange way to test here because you're relying on the result of the callback execution to create your expected value. If the callback incorrectly creates a fee version and incorrectly sets fee enabled you won't catch it here. Similarly if the callback incorrectly doesn't set fee version and incorrectly doesn't set fee enabled you won't catch it.

I think we should add a boolean in the test cases that says feeEnabled. You then can add the expected value for each test case and then check if the feeEnabled flag is true and check if the version contains a fee version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup good catch. Thanks for the feedback 🤝

Comment on lines +18 to +19
// If the provided version string is non-empty, OnChanOpenInit should return
// the version string if valid or an error if the provided version is invalid.
Copy link
Member

Choose a reason for hiding this comment

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

Do we require that Init returns the exact same version if version is non-empty? I don't believe that is the case. Applications may want to modify the version string even if it is non-empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this directly from the spec: https://github.com/cosmos/ibc/pull/629/files#diff-508b3e7784a300436fbeb6940be22f31c74e958270a36bb851d06a8782ad7e7aR50

Should we update both? Agree with your assessment.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree we should change the spec as well

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks @seantking

@seantking seantking enabled auto-merge (squash) May 10, 2022 15:19
@seantking seantking merged commit dcd0681 into main May 10, 2022
@seantking seantking deleted the sean/issue#722-channel-handshake-improvements branch May 10, 2022 15:28
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request Nov 6, 2023
Fixes cosmos#1283 

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

Refactor:
- Improvement: Enhanced the transaction handling process in the
`TxMempool` module. Transactions are now more accurately filtered based
on their gas and data size limits, ensuring that only valid transactions
are processed. This update improves the overall efficiency and
reliability of transaction processing.

Tests:
- Test: Added a new test, `TestTxMempoolTxLargerThanMaxBytes`, to verify
the correct handling of large transactions. This ensures the robustness
of the system when dealing with transactions of varying sizes.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Ganesha Upadhyaya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Channel handshake ChanOpenInit improvements
6 participants