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

refactor: remove FlushStatus from ack handler and msg #4364

Merged
merged 8 commits into from
Aug 17, 2023
Merged

refactor: remove FlushStatus from ack handler and msg #4364

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
50bb359
chore: remove restore logic in try handler, fast forward upgrade sequ…
chatton Aug 16, 2023
bf8f276
chore: fix linter
chatton Aug 16, 2023
729d0d6
refactor: remove channel flush status from ack msg and handler
damiannolan Aug 16, 2023
a480386
fix: address test acknowledgement failing testcase, replace flush sta…
damiannolan Aug 16, 2023
fa111f1
fix: linter crying
damiannolan Aug 16, 2023
c82beaa
adding back failure testcase for AcknowledgePacket
damiannolan Aug 16, 2023
1c335d1
updating testcase name to be more reflective of channel state
damiannolan Aug 16, 2023
fd14263
Merge branch '04-channel-upgrades' into damian/remove-flush-status-fr…
damiannolan Aug 17, 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
250 changes: 125 additions & 125 deletions modules/apps/29-fee/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,131 +1121,131 @@ func (suite *FeeTestSuite) TestOnChanUpgradeInit() {
}
}

func (suite *FeeTestSuite) TestOnChanUpgradeTry() {
var (
expFeeEnabled bool
path *ibctesting.Path
)

testCases := []struct {
name string
malleate func()
expError error
}{
{
"success",
func() {},
nil,
},
{
"success disable fees",
func() {
// create a new path using a fee enabled channel and downgrade it to disable fees
expFeeEnabled = false
path = ibctesting.NewPath(suite.chainA, suite.chainB)

mockFeeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointA.ChannelConfig.Version = mockFeeVersion
path.EndpointB.ChannelConfig.Version = mockFeeVersion

upgradeVersion := ibcmock.Version
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion

suite.coordinator.Setup(path)
err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)
},
nil,
},
{
"invalid upgrade version",
func() {
expFeeEnabled = false
counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
counterpartyUpgrade.Fields.Version = ibctesting.InvalidID
path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)

suite.coordinator.CommitBlock(suite.chainA)

// intentionally force the error here so we can assert that a passthrough occurs when fees should not be enabled for this channel
suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
return "", ibcmock.MockApplicationCallbackError
}
},
channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
},
{
"invalid fee version",
func() {
expFeeEnabled = false
upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: ibctesting.InvalidID, AppVersion: ibcmock.Version}))

counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
counterpartyUpgrade.Fields.Version = upgradeVersion
path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)

suite.coordinator.CommitBlock(suite.chainA)
},
channeltypes.NewUpgradeError(1, types.ErrInvalidVersion),
},
{
"underlying app callback returns error",
func() {
expFeeEnabled = false
suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
return "", ibcmock.MockApplicationCallbackError
}
},
channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()

path = ibctesting.NewPath(suite.chainA, suite.chainB)

// configure the initial path to create an unincentivized mock channel
path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointA.ChannelConfig.Version = ibcmock.Version
path.EndpointB.ChannelConfig.Version = ibcmock.Version

suite.coordinator.Setup(path)

// configure the channel upgrade version to enabled ics29 fee middleware
expFeeEnabled = true
upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion

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

tc.malleate()

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

isFeeEnabled := suite.chainB.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().Equal(expFeeEnabled, isFeeEnabled)

if tc.expError != nil {
// NOTE: application callback failure in OnChanUpgradeTry results in an ErrorReceipt being written to state signaling for cancellation
if expUpgradeError, ok := tc.expError.(*channeltypes.UpgradeError); ok {
errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(found)
suite.Require().Equal(expUpgradeError.GetErrorReceipt(), errorReceipt)
}
}
})
}
}
// func (suite *FeeTestSuite) TestOnChanUpgradeTry() {
// var (
// expFeeEnabled bool
// path *ibctesting.Path
// )
//
// testCases := []struct {
// name string
// malleate func()
// expError error
// }{
// {
// "success",
// func() {},
// nil,
// },
// {
// "success disable fees",
// func() {
// // create a new path using a fee enabled channel and downgrade it to disable fees
// expFeeEnabled = false
// path = ibctesting.NewPath(suite.chainA, suite.chainB)
//
// mockFeeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
// path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointA.ChannelConfig.Version = mockFeeVersion
// path.EndpointB.ChannelConfig.Version = mockFeeVersion
//
// upgradeVersion := ibcmock.Version
// path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
// path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
//
// suite.coordinator.Setup(path)
// err := path.EndpointA.ChanUpgradeInit()
// suite.Require().NoError(err)
// },
// nil,
// },
// {
// "invalid upgrade version",
// func() {
// expFeeEnabled = false
// counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
// counterpartyUpgrade.Fields.Version = ibctesting.InvalidID
// path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)
//
// suite.coordinator.CommitBlock(suite.chainA)
//
// // intentionally force the error here so we can assert that a passthrough occurs when fees should not be enabled for this channel
// suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
// return "", ibcmock.MockApplicationCallbackError
// }
// },
// channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
// },
// {
// "invalid fee version",
// func() {
// expFeeEnabled = false
// upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: ibctesting.InvalidID, AppVersion: ibcmock.Version}))
//
// counterpartyUpgrade := path.EndpointA.GetChannelUpgrade()
// counterpartyUpgrade.Fields.Version = upgradeVersion
// path.EndpointA.SetChannelUpgrade(counterpartyUpgrade)
//
// suite.coordinator.CommitBlock(suite.chainA)
// },
// channeltypes.NewUpgradeError(1, types.ErrInvalidVersion),
// },
// {
// "underlying app callback returns error",
// func() {
// expFeeEnabled = false
// suite.chainB.GetSimApp().FeeMockModule.IBCApp.OnChanUpgradeTry = func(_ sdk.Context, _, _ string, _ channeltypes.Order, _ []string, _ string) (string, error) {
// return "", ibcmock.MockApplicationCallbackError
// }
// },
// channeltypes.NewUpgradeError(1, ibcmock.MockApplicationCallbackError),
// },
// }
//
// for _, tc := range testCases {
// tc := tc
// suite.Run(tc.name, func() {
// suite.SetupTest()
//
// path = ibctesting.NewPath(suite.chainA, suite.chainB)
//
// // configure the initial path to create an unincentivized mock channel
// path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
// path.EndpointA.ChannelConfig.Version = ibcmock.Version
// path.EndpointB.ChannelConfig.Version = ibcmock.Version
//
// suite.coordinator.Setup(path)
//
// // configure the channel upgrade version to enabled ics29 fee middleware
// expFeeEnabled = true
// upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
// path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
// path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = upgradeVersion
//
// err := path.EndpointA.ChanUpgradeInit()
// suite.Require().NoError(err)
//
// tc.malleate()
//
// err = path.EndpointB.ChanUpgradeTry()
// suite.Require().NoError(err)
//
// isFeeEnabled := suite.chainB.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
// suite.Require().Equal(expFeeEnabled, isFeeEnabled)
//
// if tc.expError != nil {
// // NOTE: application callback failure in OnChanUpgradeTry results in an ErrorReceipt being written to state signaling for cancellation
// if expUpgradeError, ok := tc.expError.(*channeltypes.UpgradeError); ok {
// errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
// suite.Require().True(found)
// suite.Require().Equal(expUpgradeError.GetErrorReceipt(), errorReceipt)
// }
// }
// })
// }
// }

// TODO: Revisit these testcases when core refactor is completed
// func (suite *FeeTestSuite) TestOnChanUpgradeAck() {
Expand Down
8 changes: 4 additions & 4 deletions modules/core/04-channel/keeper/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,17 +405,17 @@ func emitChannelUpgradeTimeoutEvent(ctx sdk.Context, portID string, channelID st
}

// emitErrorReceiptEvent emits an error receipt event
func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade, err error) {
func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgradeFields types.UpgradeFields, err error) {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeChannelUpgradeInit,
sdk.NewAttribute(types.AttributeKeyPortID, portID),
sdk.NewAttribute(types.AttributeKeyChannelID, channelID),
sdk.NewAttribute(types.AttributeCounterpartyPortID, currentChannel.Counterparty.PortId),
sdk.NewAttribute(types.AttributeCounterpartyChannelID, currentChannel.Counterparty.ChannelId),
sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgrade.Fields.ConnectionHops[0]),
sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgrade.Fields.Version),
sdk.NewAttribute(types.AttributeKeyUpgradeOrdering, upgrade.Fields.Ordering.String()),
sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgradeFields.ConnectionHops[0]),
sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgradeFields.Version),
sdk.NewAttribute(types.AttributeKeyUpgradeOrdering, upgradeFields.Ordering.String()),
sdk.NewAttribute(types.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", currentChannel.UpgradeSequence)),
sdk.NewAttribute(types.AttributeKeyUpgradeErrorReceipt, err.Error()),
),
Expand Down
8 changes: 3 additions & 5 deletions modules/core/04-channel/keeper/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,9 @@ func (k Keeper) AcknowledgePacket(
)
}

if channel.State != types.OPEN && channel.FlushStatus != types.FLUSHING {
return errorsmod.Wrapf(
types.ErrInvalidChannelState,
"packets cannot be acknowledged on channel with state (%s) and flush status (%s)", channel.State, channel.FlushStatus,
)
// TODO(damian): update TRYUPGRADE to FLUSHING following https://github.com/cosmos/ibc-go/issues/4243
if !collections.Contains(channel.State, []types.State{types.OPEN, types.TRYUPGRADE}) {
return errorsmod.Wrapf(types.ErrInvalidChannelState, "packets cannot be acknowledged on channel with state (%s)", channel.State)
}

// Authenticate capability to ensure caller has authority to receive packet on this channel
Expand Down
2 changes: 1 addition & 1 deletion modules/core/04-channel/keeper/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, true},
{"success on channel in tryupgrade and flush status in flushing", func() {
{"success on channel in flushing state", func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment this is functioning with "tryupgrade" rather than the flushing state, but I made the change to the test string because this will still pass successfully when #4243 is completed

// setup uses an UNORDERED channel
suite.coordinator.Setup(path)

Expand Down
36 changes: 16 additions & 20 deletions modules/core/04-channel/keeper/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,13 @@ func (k Keeper) ChanUpgradeTry(
if found {
proposedUpgradeFields = upgrade.Fields
} else {
// TODO: add fast forward feature
// https://github.com/cosmos/ibc-go/issues/3794
// if the counterparty sequence is greater than the current sequence, we fast-forward to the counterparty sequence.
if counterpartyUpgradeSequence > channel.UpgradeSequence {
// using -1 as WriteUpgradeInitChannel increments the sequence
channel.UpgradeSequence = counterpartyUpgradeSequence - 1
k.SetChannel(ctx, portID, channelID, channel)
}

// NOTE: OnChanUpgradeInit will not be executed by the application
upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields)
if err != nil {
Expand Down Expand Up @@ -153,9 +158,10 @@ func (k Keeper) ChanUpgradeTry(
); err != nil {
return types.Upgrade{}, errorsmod.Wrap(err, "failed to verify counterparty channel state")
}

if err := k.syncUpgradeSequence(ctx, portID, channelID, channel, counterpartyUpgradeSequence); err != nil {
return types.Upgrade{}, err
if counterpartyUpgradeSequence < channel.UpgradeSequence {
return upgrade, types.NewUpgradeError(channel.UpgradeSequence-1, errorsmod.Wrapf(
types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence < current upgrade sequence (%d < %d)", counterpartyUpgradeSequence, channel.UpgradeSequence,
))
}

// verifies the proof that a particular proposed upgrade has been stored in the upgrade path of the counterparty
Expand Down Expand Up @@ -189,11 +195,6 @@ func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string

previousState := channel.State
channel.State = types.TRYUPGRADE
channel.FlushStatus = types.FLUSHING

if !k.HasInflightPackets(ctx, portID, channelID) {
channel.FlushStatus = types.FLUSHCOMPLETE
}

upgrade.Fields.Version = upgradeVersion

Expand All @@ -215,7 +216,6 @@ func (k Keeper) ChanUpgradeAck(
ctx sdk.Context,
portID,
channelID string,
counterpartyFlushStatus types.FlushStatus,
counterpartyUpgrade types.Upgrade,
proofChannel,
proofUpgrade []byte,
Expand All @@ -230,10 +230,6 @@ func (k Keeper) ChanUpgradeAck(
return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.OPEN, types.STATE_FLUSHING, channel.State)
}

if !collections.Contains(counterpartyFlushStatus, []types.FlushStatus{types.FLUSHING, types.FLUSHCOMPLETE}) {
return errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected one of [%s, %s], got %s", types.FLUSHING, types.FLUSHCOMPLETE, counterpartyFlushStatus)
}

connection, found := k.connectionKeeper.GetConnection(ctx, channel.ConnectionHops[0])
if !found {
return errorsmod.Wrap(connectiontypes.ErrConnectionNotFound, channel.ConnectionHops[0])
Expand All @@ -251,7 +247,7 @@ func (k Keeper) ChanUpgradeAck(
Counterparty: types.NewCounterparty(portID, channelID),
Version: channel.Version,
UpgradeSequence: channel.UpgradeSequence,
FlushStatus: counterpartyFlushStatus, // provided by the relayer
FlushStatus: types.NOTINFLUSH, // TODO: remove flush status from channel end
Copy link
Member Author

Choose a reason for hiding this comment

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

temporarily hard code the flush status to the default value so proofs can succeed

}

// verify the counterparty channel state containing the upgrade sequence
Expand Down Expand Up @@ -892,7 +888,7 @@ func (k Keeper) abortUpgrade(ctx sdk.Context, portID, channelID string, err erro
upgradeError = types.NewUpgradeError(channel.UpgradeSequence, err)
}

if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil {
if err := k.WriteErrorReceipt(ctx, portID, channelID, upgrade.Fields, upgradeError); err != nil {
return err
}

Expand All @@ -911,14 +907,14 @@ func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string, upgrad
k.deleteUpgradeInfo(ctx, portID, channelID)
}

// writeErrorReceipt will write an error receipt from the provided UpgradeError.
func (k Keeper) writeErrorReceipt(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeError *types.UpgradeError) error {
// WriteErrorReceipt will write an error receipt from the provided UpgradeError.
func (k Keeper) WriteErrorReceipt(ctx sdk.Context, portID, channelID string, upgradeFields types.UpgradeFields, upgradeError *types.UpgradeError) error {
channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

k.SetUpgradeErrorReceipt(ctx, portID, channelID, upgradeError.GetErrorReceipt())
emitErrorReceiptEvent(ctx, portID, channelID, channel, upgrade, upgradeError)
emitErrorReceiptEvent(ctx, portID, channelID, channel, upgradeFields, upgradeError)
return nil
}
Loading
Loading