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

Jump to bottom

Add ChanUpgradeOpen core handler. #3844

Merged
merged 9 commits into from
Jun 29, 2023
Merged

Add ChanUpgradeOpen core handler. #3844

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b33f21f
Add ChanUpgradeOpen core handler.
DimitrisJim Jun 13, 2023
9ffd9ee
Tests tests tests.
DimitrisJim Jun 22, 2023
dcffa80
Update upgrade open handler based on feedback.
DimitrisJim Jun 24, 2023
85ffd3b
Reformat testing approach.
DimitrisJim Jun 28, 2023
17191a2
Move counterpartyhops assignment inline.
DimitrisJim Jun 28, 2023
112423e
Check err of SetChannelState.
DimitrisJim Jun 28, 2023
77d91c2
Address feedback.
DimitrisJim Jun 28, 2023
151f71e
Remove uneeded modification of version.
DimitrisJim Jun 28, 2023
81215f5
Merge branch '04-channel-upgrades' into chan_upgrade_open
DimitrisJim Jun 29, 2023
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
Prev Previous commit
Next Next commit
Tests tests tests.
  • Loading branch information
@DimitrisJim
DimitrisJim committed Jun 28, 2023
commit 9ffd9ee30c289f0967f2e1be59108d91f2de8916
2 changes: 1 addition & 1 deletion modules/core/04-channel/keeper/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ func (k Keeper) ChanUpgradeOpen(
}
counterpartyChannel = types.Channel{
State: types.OPEN,
Ordering: channel.Ordering,
Ordering: upgrade.Fields.Ordering,
ConnectionHops: upgrade.Fields.ConnectionHops,
Counterparty: types.NewCounterparty(portID, channelID),
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
Version: upgrade.Fields.GetVersion(),
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
108 changes: 85 additions & 23 deletions modules/core/04-channel/keeper/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,38 +496,108 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() {
}{
{
"success, counterparty in TRYUPGRADE",
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
func() {},
func() {
err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointB.ChanUpgradeTry()
suite.Require().NoError(err)

err = path.EndpointA.ChanUpgradeAck()
suite.Require().NoError(err)

channelB := path.EndpointB.GetChannel()
channelB.FlushStatus = types.FLUSHCOMPLETE
path.EndpointB.SetChannel(channelB)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think once #3928 is implemented, this won't be necessary, should we add a note?


channelA := path.EndpointA.GetChannel()
channelA.FlushStatus = types.FLUSHCOMPLETE
path.EndpointA.SetChannel(channelA)
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 forced to set these myself since on completion of try/ack upgrade we're still in FLUSHING. Lmk if I can go about this in a different way.


suite.coordinator.CommitBlock(suite.chainA, suite.chainB)
suite.Require().NoError(path.EndpointA.UpdateClient())
},
nil,
},
// TODO: Rest of combinations for counterparty state.
{
"success, counterparty in ACKUPGRADE",
func() {
err := path.EndpointB.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointA.ChanUpgradeTry()
suite.Require().NoError(err)

err = path.EndpointB.ChanUpgradeAck()
suite.Require().NoError(err)

channelB := path.EndpointB.GetChannel()
channelB.FlushStatus = types.FLUSHCOMPLETE
path.EndpointB.SetChannel(channelB)

channelA := path.EndpointA.GetChannel()
channelA.FlushStatus = types.FLUSHCOMPLETE
path.EndpointA.SetChannel(channelA)

suite.coordinator.CommitBlock(suite.chainA, suite.chainB)
suite.Require().NoError(path.EndpointA.UpdateClient())
},
nil,
},
/*
{
// TODO: Pending implementation of open handling on msg_server
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 open an issue to not forget about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally, I was thinking of adding it in the PR for the message server handler. Can open an issue if you think it might be an odd addition there (NOTE: there's a similar case in TRY, unsure what we went with there).

"success, counterparty in OPEN",
func() {},
nil,
},
*/
{
"channel not found",
func() {
path.EndpointA.ChannelID = ibctesting.InvalidID
path.EndpointA.ChannelConfig.PortID = ibctesting.InvalidID
},
types.ErrChannelNotFound,
},
{
"in-flight packets still exist",
"channel state is not in TRYUPGRADE or ACKUPGRADE",
func() {
// TODO:
channel := path.EndpointA.GetChannel()
channel.State = types.OPEN
path.EndpointA.SetChannel(channel)
},
types.ErrInvalidChannelState,
},
{
"channel has in-flight packets",
func() {
portID := path.EndpointA.ChannelConfig.PortID
channelID := path.EndpointA.ChannelID
// Set a dummy packet commitment to simulate in-flight packets
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), portID, channelID, 1, []byte("hash"))
},
types.ErrPendingInflightPackets,
},
{
"flush status is not FLUSHCOMPLETE",
func() {
channel := path.EndpointB.GetChannel()
channel := path.EndpointA.GetChannel()
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
// Set in acceptable state
channel.State = types.TRYUPGRADE
Copy link
Contributor Author

@DimitrisJim DimitrisJim Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

don't particularly like having to set up the channel in a state we want to check but I think it's a trade-off between having the handshake be performed in the malleate function vs inside the loop body.

OTOH I can add a typical handshake dance here to get them in the right states as Colin mentioned in another PR. Lmk!

Copy link
Contributor

Choose a reason for hiding this comment

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

My experience is that it is more maintainable to use setup handlers rather than hardcoded expectations


channel.FlushStatus = types.FLUSHING
path.EndpointB.SetChannel(channel)
path.EndpointA.SetChannel(channel)
},
types.ErrInvalidFlushStatus,
},
{
"connection not found",
func() {
channel := path.EndpointA.GetChannel()
// Set in acceptable state
channel.State = types.TRYUPGRADE
channel.FlushStatus = types.FLUSHCOMPLETE

channel.ConnectionHops = []string{"connection-100"}
path.EndpointA.SetChannel(channel)
},
Expand All @@ -536,6 +606,12 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() {
{
"invalid connection state",
func() {
channel := path.EndpointA.GetChannel()
// Set in acceptable state
channel.State = types.TRYUPGRADE
channel.FlushStatus = types.FLUSHCOMPLETE
path.EndpointA.SetChannel(channel)

connectionEnd := path.EndpointA.GetConnection()
connectionEnd.State = connectiontypes.UNINITIALIZED
path.EndpointA.SetConnection(connectionEnd)
Expand All @@ -556,24 +632,10 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() {
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion

err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointB.ChanUpgradeTry()
suite.Require().NoError(err)

// Pending implementation of ack on msg_server to move channel state for A.
// Currently fails on validation.
err = path.EndpointA.ChanUpgradeAck()
suite.Require().NoError(err)

tc.malleate()

// ensure clients are up to date to receive valid proofs
suite.Require().NoError(path.EndpointA.UpdateClient())
proofCounterpartyChannel, _, proofHeight := path.EndpointB.QueryChannelUpgradeProof()

err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen(
proofCounterpartyChannel, _, proofHeight := path.EndpointA.QueryChannelUpgradeProof()
err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen(
suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID,
path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight,
)
Expand Down