Skip to content

Commit

Permalink
chore: propagate IBC authority to 04-channel keeper to be used within…
Browse files Browse the repository at this point in the history 
… upgrade cancellation (#5093)

* chore: add authority to function params, test flow for checking msg sender against authority

* linter

* refactor: use isAuthority bool in favour of passing signer and authority to 04-channel

---------

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
  • Loading branch information
@charleenfei @damiannolan
charleenfei and damiannolan authored Nov 22, 2023
1 parent fa3f92f commit 081646c
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 22 deletions.
11 changes: 2 additions & 9 deletions modules/core/04-channel/keeper/upgrade.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ func (k Keeper) WriteUpgradeOpenChannel(ctx sdk.Context, portID, channelID strin
}

// ChanUpgradeCancel is called by a module to cancel a channel upgrade that is in progress.
func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, errorReceipt types.ErrorReceipt, errorReceiptProof []byte, proofHeight clienttypes.Height, signer string) error {
func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, errorReceipt types.ErrorReceipt, errorReceiptProof []byte, proofHeight clienttypes.Height, isAuthority bool) error {
channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
Expand All @@ -563,7 +563,7 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err
// if the msgSender is authorized to make and cancel upgrades AND the current channel has not already reached FLUSHCOMPLETE
// then we can restore immediately without any additional checks
// otherwise, we can only cancel if the counterparty wrote an error receipt during the upgrade handshake
if k.isAuthorizedUpgrader(signer) && channel.State != types.FLUSHCOMPLETE {
if isAuthority && channel.State != types.FLUSHCOMPLETE {
return nil
}

Expand Down Expand Up @@ -595,13 +595,6 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err
return nil
}

// isAuthorizedUpgrader checks if the sender is authorized to cancel the upgrade.
func (Keeper) isAuthorizedUpgrader(signer string) bool {
// TODO: the authority is only available on the core ibc keeper at the moment.
// return k.GetAuthority() == signer
return true
}

// WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process.Auxiliary upgrade state is
// also deleted.
func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string, errorReceipt types.ErrorReceipt) {
Expand Down
49 changes: 37 additions & 12 deletions modules/core/04-channel/keeper/upgrade_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
errorReceipt types.ErrorReceipt
errorReceiptProof []byte
proofHeight clienttypes.Height
sender string
isAuthority bool
)

tests := []struct {
Expand All @@ -1224,6 +1224,25 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
},
expError: nil,
},
{
name: "sender is authority, channel in flushing, upgrade can be cancelled even with invalid error receipt",
malleate: func() {
channel := path.EndpointA.GetChannel()
channel.State = types.FLUSHING
path.EndpointA.SetChannel(channel)

var ok bool
errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

errorReceipt.Message = ibctesting.InvalidID
suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)
suite.coordinator.CommitBlock(suite.chainB)

isAuthority = true
},
expError: nil,
},
{
name: "channel not found",
malleate: func() {
Expand Down Expand Up @@ -1268,28 +1287,34 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
expError: connectiontypes.ErrConnectionNotFound,
},
{
name: "error verification failed",
name: "sender is authority, channel is in flush complete, error verification failed",
malleate: func() {
var ok bool
errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

errorReceipt.Message = "different message"
errorReceipt.Message = ibctesting.InvalidID

suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)
suite.coordinator.CommitBlock(suite.chainB)

isAuthority = true
},
expError: commitmenttypes.ErrInvalidProof,
},
{
name: "sender is not authority, error verification failed",
malleate: func() {
var ok bool
errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

errorReceipt.Message = ibctesting.InvalidID
suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)
suite.coordinator.CommitBlock(suite.chainB)
},
expError: commitmenttypes.ErrInvalidProof,
},
// TODO: add test case for invalid sender once it is implemented.
// {
// name: "invalid sender",
// malleate: func() {
//
// },
// expError: nil,
// },
}

for _, tc := range tests {
Expand Down Expand Up @@ -1337,7 +1362,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {

tc.malleate()

err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight, sender)
err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight, isAuthority)

expPass := tc.expError == nil
if expPass {
Expand Down
3 changes: 2 additions & 1 deletion modules/core/keeper/msg_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1018,7 +1018,8 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms
return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)
}

if err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight, msg.Signer); err != nil {
isAuthority := k.GetAuthority() == msg.Signer
if err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight, isAuthority); err != nil {
ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error())
return nil, errorsmod.Wrap(err, "channel upgrade cancel failed")
}
Expand Down

0 comments on commit 081646c

Please sign in to comment.