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: WriteAcknowledgement API #882

Merged
merged 8 commits into from
Feb 9, 2022
Merged

refactor: WriteAcknowledgement API #882

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
95b7e8e
refactor: WriteAcknowledgement takes exported.Acknowledgement instead…
seantking Feb 7, 2022
9f44d36
fix: adding check for empty byte string
seantking Feb 8, 2022
b16f85e
chore: update changelog
seantking Feb 8, 2022
37ba44f
fixing test case + adding migration docs
seantking Feb 8, 2022
8d5c66d
testing: Adding MockEmptyAcknowledgement to testing library
seantking Feb 8, 2022
fea6321
docs: fix version
seantking Feb 9, 2022
d3b7625
test: add check for ack is nil
seantking Feb 9, 2022
fca9daf
Merge branch 'main' into sean/issue#854-update-ics4-write-ack
seantking Feb 9, 2022
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
9 changes: 5 additions & 4 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 @@ -312,7 +312,7 @@ func (k Keeper) WriteAcknowledgement(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
acknowledgement []byte,
acknowledgement exported.Acknowledgement,
) error {
channel, found := k.GetChannel(ctx, packet.GetDestPort(), packet.GetDestChannel())
if !found {
Expand Down Expand Up @@ -342,14 +342,15 @@ func (k Keeper) WriteAcknowledgement(
return types.ErrAcknowledgementExists
}

if len(acknowledgement) == 0 {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
bz := acknowledgement.Acknowledgement()
Copy link
Member

Choose a reason for hiding this comment

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

You should still check if acknowledgement is nil first or it will panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, thank you

if len(bz) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if len(bz) == 0 {
if acknowledgement == nil || len(acknowledgement.Acknowledgement()) == 0 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the check as a separate if statement. Just my personal preference.

return sdkerrors.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be empty")
Comment on lines +349 to 351
Copy link
Member

Choose a reason for hiding this comment

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

Just change this check to if acknowledgement == nil

}

// set the acknowledgement so that it can be verified on the other side
k.SetPacketAcknowledgement(
ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
types.CommitAcknowledgement(acknowledgement),
types.CommitAcknowledgement(bz),
)

// log that a packet acknowledgement has been written
Expand All @@ -362,7 +363,7 @@ func (k Keeper) WriteAcknowledgement(
"dst_channel", packet.GetDestChannel(),
)

EmitWriteAcknowledgementEvent(ctx, packet, channel, acknowledgement)
EmitWriteAcknowledgementEvent(ctx, packet, channel, bz)

return nil
}
Expand Down
17 changes: 9 additions & 8 deletions 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 @@ -493,7 +493,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
var (
path *ibctesting.Path
ack []byte
ack exported.Acknowledgement
packet exported.PacketI
channelCap *capabilitytypes.Capability
)
Expand All @@ -504,7 +504,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
func() {
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
ack = ibctesting.MockAcknowledgement
ack = ibcmock.MockAcknowledgement
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
},
true,
Expand All @@ -513,13 +513,13 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
// use wrong channel naming
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, ibctesting.InvalidID, ibctesting.InvalidID, timeoutHeight, disabledTimeoutTimestamp)
ack = ibctesting.MockAcknowledgement
ack = ibcmock.MockAcknowledgement
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
}, false},
{"channel not open", func() {
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
ack = ibctesting.MockAcknowledgement
ack = ibcmock.MockAcknowledgement

err := path.EndpointB.SetChannelClosed()
suite.Require().NoError(err)
Expand All @@ -530,7 +530,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
func() {
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
ack = ibctesting.MockAcknowledgement
ack = ibcmock.MockAcknowledgement
channelCap = capabilitytypes.NewCapability(3)
},
false,
Expand All @@ -540,8 +540,8 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
func() {
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
ack = ibctesting.MockAcknowledgement
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack)
ack = ibcmock.MockAcknowledgement
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack.Acknowledgement())
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
},
false,
Expand All @@ -551,7 +551,8 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
func() {
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
ack = nil
ack = types.NewResultAcknowledgement([]byte{})
suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack.Acknowledgement())
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
},
false,
Expand Down
2 changes: 1 addition & 1 deletion modules/core/05-port/types/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ type ICS4Wrapper interface {
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
packet exported.PacketI,
ack []byte,
ack exported.Acknowledgement,
) error
}

Expand Down
2 changes: 1 addition & 1 deletion modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke
// NOTE: IBC applications modules may call the WriteAcknowledgement asynchronously if the
// acknowledgement is nil.
if ack != nil {
if err := k.ChannelKeeper.WriteAcknowledgement(ctx, cap, msg.Packet, ack.Acknowledgement()); err != nil {
if err := k.ChannelKeeper.WriteAcknowledgement(ctx, cap, msg.Packet, ack); err != nil {
return nil, err
}
}
Expand Down
2 changes: 1 addition & 1 deletion testing/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ func (endpoint *Endpoint) WriteAcknowledgement(ack exported.Acknowledgement, pac
channelCap := endpoint.Chain.GetChannelCapability(packet.GetDestPort(), packet.GetDestChannel())

// no need to send message, acting as a handler
err := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.WriteAcknowledgement(endpoint.Chain.GetContext(), channelCap, packet, ack.Acknowledgement())
err := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.WriteAcknowledgement(endpoint.Chain.GetContext(), channelCap, packet, ack)
if err != nil {
return err
}
Expand Down