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 ChanUpgradeInit to use new upgrade type #3456

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Apr 13, 2023

Description

closes: #3447
closes: #3456


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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@chatton chatton marked this pull request as ready for review April 18, 2023 10:49
Comment on lines 284 to 287
sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgrade.Fields.ConnectionHops[0]),
sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgrade.Fields.Version),
sdk.NewAttribute(types.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", currentChannel.UpgradeSequence)),
sdk.NewAttribute(types.AttributeKeyUpgradeOrder, upgrade.Fields.Ordering.String()),
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've added new keys to be explicit about the upgrade fields.

modules/core/04-channel/types/upgrade.go Outdated Show resolved Hide resolved
func (k Keeper) extractUpgradeFromMessage(ctx sdk.Context, msg *channeltypes.MsgChannelUpgradeInit) (channeltypes.Upgrade, error) {
seq, found := k.ChannelKeeper.GetNextSequenceSend(ctx, msg.PortId, msg.ChannelId)
if !found {
return channeltypes.Upgrade{}, channeltypes.ErrSequenceSendNotFound
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'm unsure if this is the desired behaviour. If we don't want to error here, what value should be set for LatestSequenceSend, 0?

Copy link
Member

Choose a reason for hiding this comment

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

I think if next sequence send is not found then we should error

proto/ibc/core/channel/v1/tx.proto Outdated Show resolved Hide resolved
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.

Really great work pumping this out so quickly with all the various changes ❤️

I left some nits and suggestion/open discussion about the ChanUpgradeInit method sig and return values.

modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved
proto/ibc/core/channel/v1/tx.proto Outdated Show resolved Hide resolved
modules/core/keeper/msg_server.go Outdated Show resolved Hide resolved
func (k Keeper) extractUpgradeFromMessage(ctx sdk.Context, msg *channeltypes.MsgChannelUpgradeInit) (channeltypes.Upgrade, error) {
seq, found := k.ChannelKeeper.GetNextSequenceSend(ctx, msg.PortId, msg.ChannelId)
if !found {
return channeltypes.Upgrade{}, channeltypes.ErrSequenceSendNotFound
Copy link
Member

Choose a reason for hiding this comment

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

I think if next sequence send is not found then we should error

modules/core/04-channel/types/upgrade.go Outdated Show resolved Hide resolved
proto/ibc/core/channel/v1/upgrade.proto Show resolved Hide resolved
modules/core/04-channel/types/events.go Outdated Show resolved Hide resolved
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.

Excellent work!! Code is looking great to me. Left various comments

e2e/tests/core/04-channel/channel_test.go Outdated Show resolved Hide resolved
types.Channel{State: types.INITUPGRADE, Version: mock.Version},
clienttypes.NewHeight(0, 10000),
0,
types.NewUpgradeFields(types.UNORDERED, []string{ibctesting.FirstChannelID}, mock.Version),
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be the connection id?

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 it should be, interesting, we are not validating a valid connection id format in validate basic. Is this something we should be doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could be. Only the first connection ID can be required to be in the "connection-X" format (thinking of multihop non len 1 context). I don't have a strong preference since it will fail on the handler trying to get the connection

proto/ibc/core/channel/v1/upgrade.proto Show resolved Hide resolved
modules/core/keeper/msg_server.go Outdated Show resolved Hide resolved
modules/core/04-channel/types/upgrade.go Outdated Show resolved Hide resolved
modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved
modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved
modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved
Height: timeout.Height,
Timestamp: timeout.Timestamp,
},
LatestSequenceSend: seq - 1,
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 add a comment for why we do the minus 1?

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 can add a comment if you think it will help, I think the combination of the names GetNextSequenceSend and LatestSequenceSend make it pretty clear, but I can add a comment to make it extra explicit if you think it makes it more clear.

Copy link
Member

Choose a reason for hiding this comment

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

we could also consider adding a new keeper method to sweep it under the rug a little. E.g. k.GetLastSequenceSend(ctx, portID, channelID) which would read the same key/value as GetNextSequenceSend but handle the off by one.

I think if we do that it can be done later in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like @damiannolan's idea!

upgradeSequence uint64,
channelUpgrade types.Channel,
) {
func (k Keeper) WriteUpgradeInitChannel(ctx sdk.Context, portID, channelID string, currentChannel types.Channel, upgrade types.Upgrade) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing we have to be careful with about passing in the channel is to ensure it is obtained and not modified in state before calling this function

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 could do an additional read of the channel directly in the function, I'm not sure it's necessary though, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

It is probably safer to do an additional read but I'm also happy to leave as is for now. @colin-axner and I discussed yesterday briefly about refactoring grpc handlers into their associated submodules rather than having the entry point at the core layer. This would mean we could reduce the exported APIs and prevent them from being misused. If this were a private method then it would be much safer passing currentChannel.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of doing the additional read for safety and then optimizing later if there's a clean way to do so, but I don't have a strong preference here. Will go with your gut!

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.

Looks very clean!

"invalid proposed channel counterparty",
func() {
channelUpgrade.Counterparty = types.NewCounterparty(mock.PortID, "channel-100")
upgrade.Fields.ConnectionHops = []string{"connection-100"}
},
false,
},
{
"invalid proposed channel upgrade ordering",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"invalid proposed channel upgrade ordering",
"stricter proposed channel upgrade ordering",

sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgrade.Fields.ConnectionHops[0]),
sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgrade.Fields.Version),
sdk.NewAttribute(types.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", currentChannel.UpgradeSequence)),
sdk.NewAttribute(types.AttributeKeyUpgradeOrdering, upgrade.Fields.Ordering.String()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add a test for this event, so that if in the future new upgrade fields are added, then the test should fail, so that we don't forget to add the field here. Alternatively, instead of adding each upgrade field with its own key, would it work to have one key for all the upgrade fields and the value is a JSON-encoded string of all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you happy if we do this in a followup, I think a few changes would need to happen to the tests (as we are testing the channel keeper function but events are emitted at the message server layer)

_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
}

return nil
if !msg.Timeout.IsValid() {
return errorsmod.Wrap(ErrInvalidUpgrade, "upgrade timeout cannot be empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

An error message like this would make it similar to the one for Packet.

Suggested change
return errorsmod.Wrap(ErrInvalidUpgrade, "upgrade timeout cannot be empty")
return errorsmod.Wrap(ErrInvalidUpgrade, "upgrade timeout height and upgrade timeout timestamp cannot both be 0")

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.

Excellent work ❤️

LGTM!

upgradeSequence uint64,
channelUpgrade types.Channel,
) {
func (k Keeper) WriteUpgradeInitChannel(ctx sdk.Context, portID, channelID string, currentChannel types.Channel, upgrade types.Upgrade) {
Copy link
Member

Choose a reason for hiding this comment

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

It is probably safer to do an additional read but I'm also happy to leave as is for now. @colin-axner and I discussed yesterday briefly about refactoring grpc handlers into their associated submodules rather than having the entry point at the core layer. This would mean we could reduce the exported APIs and prevent them from being misused. If this were a private method then it would be much safer passing currentChannel.

Height: timeout.Height,
Timestamp: timeout.Timestamp,
},
LatestSequenceSend: seq - 1,
Copy link
Member

Choose a reason for hiding this comment

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

we could also consider adding a new keeper method to sweep it under the rug a little. E.g. k.GetLastSequenceSend(ctx, portID, channelID) which would read the same key/value as GetNextSequenceSend but handle the off by one.

I think if we do that it can be done later in a different PR.

errReceipt, errReceiptPresent := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
_, err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeInit(
suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgrade.Fields, upgrade.Timeout,
)

if tc.expPass {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should check that the channel state is in INITUPGRADE now?

@chatton chatton mentioned this pull request Apr 19, 2023
9 tasks
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.

Yay! 🎉 Thanks @chatton for pumping this out! :)

modules/core/04-channel/keeper/upgrade_test.go Outdated Show resolved Hide resolved
channel.State = types.INITUPGRADE
if reflect.DeepEqual(channel, proposedUpgradeChannel) {
return 0, "", errorsmod.Wrap(types.ErrChannelExists, "existing channel end is identical to proposed upgrade channel end")
if err := k.ValidateUpgradeFields(ctx, upgradeFields, channel); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

In a separate pr, how would folks feel about making this function private and placed in upgrade.go?

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 think I made the exported as it was used in another package, if it's possible though I'm in favour of making it unexported.

Copy link
Contributor

Choose a reason for hiding this comment

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

What package? I feel like validation of the upgrade fields against the current fields should only happen in the init and try handlers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like here

@chatton chatton merged commit 81a709b into 04-channel-upgrades Apr 19, 2023
@chatton chatton deleted the cian/issue#3447-modify-upgradeinit-to-use-new-upgrade-type branch April 19, 2023 14:30
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.

5 participants