Skip to content

Commit

Permalink
chore: add authority to function params, test flow for checking msg s…
Browse files Browse the repository at this point in the history
…ender against authority
  • Loading branch information
charleenfei committed Nov 13, 2023
1 parent 658b00f commit 9e1c847
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 20 deletions.
11 changes: 2 additions & 9 deletions modules/core/04-channel/keeper/upgrade.go
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, signer, authority string) 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 signer == authority && 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
47 changes: 37 additions & 10 deletions modules/core/04-channel/keeper/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1215,6 +1215,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
errorReceiptProof []byte
proofHeight clienttypes.Height
sender string
authority string
)

tests := []struct {
Expand All @@ -1236,6 +1237,23 @@ 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 = "different message"

Check failure on line 1251 in modules/core/04-channel/keeper/upgrade_test.go

View workflow job for this annotation

GitHub Actions / lint

string `different message` has 3 occurrences, make it a constant (goconst)

Check failure on line 1251 in modules/core/04-channel/keeper/upgrade_test.go

View workflow job for this annotation

GitHub Actions / lint

string `different message` has 3 occurrences, make it a constant (goconst)
suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)
suite.coordinator.CommitBlock(suite.chainB)
},
expError: nil,
},
{
name: "channel not found",
malleate: func() {
Expand Down Expand Up @@ -1280,7 +1298,7 @@ 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)
Expand All @@ -1294,14 +1312,21 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
},
expError: commitmenttypes.ErrInvalidProof,
},
// TODO: add test case for invalid sender once it is implemented.
// {
// name: "invalid sender",
// malleate: func() {
//
// },
// expError: nil,
// },
{
name: "sender is not authority, error verification failed",
malleate: func() {
authority = ibctesting.TestAccAddress

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"
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,
},
}

for _, tc := range tests {
Expand Down Expand Up @@ -1347,9 +1372,11 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
channel.State = types.FLUSHCOMPLETE
path.EndpointA.SetChannel(channel)

authority = sender

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, sender, authority)

expPass := tc.expError == nil
if expPass {
Expand Down
3 changes: 2 additions & 1 deletion modules/core/keeper/msg_server.go
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 {
authority := k.GetAuthority()
if err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight, msg.Signer, authority); 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 9e1c847

Please sign in to comment.