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

Use global success value in acknowledgment #7621

Open
wants to merge 4 commits into
base: feat/ibc-eureka
Choose a base branch
from
Open

Use global success value in acknowledgment #7621

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57e80c5
add global success to acknowledgements
AdityaSripal Dec 4, 2024
6b18589
make changes to ack callback
AdityaSripal Dec 9, 2024
0468e3c
remove unnecessary code
AdityaSripal Dec 9, 2024
4418c78
ack validation
AdityaSripal Dec 9, 2024
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: 6 additions & 3 deletions modules/apps/transfer/v2/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacketTransfer() {
"failure: receive is disabled",
func() {},
func() {
expectedAck.RecvSuccess = false
expectedAck.AppAcknowledgements[0] = channeltypes.NewErrorAcknowledgement(transfertypes.ErrReceiveDisabled).Acknowledgement()
suite.chainB.GetSimApp().TransferKeeperV2.SetParams(suite.chainB.GetContext(),
transfertypes.Params{
Expand Down Expand Up @@ -232,7 +233,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacketTransfer() {

// by default, we assume a successful acknowledgement will be written.
ackBytes := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
expectedAck = channeltypesv2.Acknowledgement{AppAcknowledgements: [][]byte{ackBytes}}
expectedAck = channeltypesv2.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{ackBytes}}
tc.malleate()

err = path.EndpointB.MsgRecvPacket(packet)
Expand Down Expand Up @@ -296,6 +297,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
{
"failure: proof verification failure",
func() {
expectedAck.RecvSuccess = false
expectedAck.AppAcknowledgements[0] = mockv2.MockFailRecvPacketResult.Acknowledgement
},
commitmenttypes.ErrInvalidProof,
Expand All @@ -304,6 +306,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
{
"failure: escrowed tokens are refunded",
func() {
expectedAck.RecvSuccess = false
expectedAck.AppAcknowledgements[0] = channeltypes.NewErrorAcknowledgement(transfertypes.ErrReceiveDisabled).Acknowledgement()
},
nil,
Expand Down Expand Up @@ -353,7 +356,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
suite.Require().NoError(err)

ackBytes := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
expectedAck = channeltypesv2.Acknowledgement{AppAcknowledgements: [][]byte{ackBytes}}
expectedAck = channeltypesv2.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{ackBytes}}
tc.malleate()

err = path.EndpointA.MsgAcknowledgePacket(packet, expectedAck)
Expand Down Expand Up @@ -505,7 +508,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() {
}

ackBytes := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
successfulAck := channeltypesv2.Acknowledgement{AppAcknowledgements: [][]byte{ackBytes}}
successfulAck := channeltypesv2.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{ackBytes}}

originalAmount, ok := sdkmath.NewIntFromString(ibctesting.DefaultGenesisAccBalance)
suite.Require().True(ok)
Expand Down
15 changes: 10 additions & 5 deletions modules/core/04-channel/v2/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,20 +131,20 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ

// build up the recv results for each application callback.
ack := types.Acknowledgement{
RecvSuccess: true,
AppAcknowledgements: [][]byte{},
}

var isAsync bool
cacheCtx, writeFn = sdkCtx.CacheContext()
for _, pd := range msg.Packet.Payloads {
// Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful.
cacheCtx, writeFn = sdkCtx.CacheContext()
cb := k.Router.Route(pd.DestinationPort)
res := cb.OnRecvPacket(cacheCtx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, pd, signer)

if res.Status != types.PacketStatus_Failure {
// write application state changes for asynchronous and successful acknowledgements
writeFn()
} else {
if res.Status == types.PacketStatus_Failure {
// Set RecvSuccess to false
ack.RecvSuccess = false
// Modify events in cached context to reflect unsuccessful acknowledgement
sdkCtx.EventManager().EmitEvents(internalerrors.ConvertToErrorEvents(cacheCtx.EventManager().Events()))
Copy link
Member Author

Choose a reason for hiding this comment

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

We can break here and just have no AppAcknowledgements on an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked into doing this. It became too complicated because it caused empty acks to be valid which I didn't want to do. Otherwise I need to create a sentinel ack and fill it in for the acks that didn't error which was more complicated than necessary especially since we only support single payload for now

}
Expand All @@ -163,6 +163,11 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ
ack.AppAcknowledgements = append(ack.AppAcknowledgements, res.Acknowledgement)
}

if ack.RecvSuccess {
// write application state changes for successful acknowledgements
writeFn()
}

// note this should never happen as the payload would have had to be empty.
if len(ack.AppAcknowledgements) == 0 {
sdkCtx.Logger().Error("receive packet failed", "source-channel", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "invalid acknowledgement results"))
Expand Down
7 changes: 5 additions & 2 deletions modules/core/04-channel/v2/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,10 @@ func (suite *KeeperTestSuite) TestMsgRecvPacket() {
tc.malleate()

// expectedAck is derived from the expected recv result.
expectedAck := types.Acknowledgement{AppAcknowledgements: [][]byte{expRecvRes.Acknowledgement}}
expectedAck := types.Acknowledgement{
RecvSuccess: expRecvRes.Status == types.PacketStatus_Success,
AppAcknowledgements: [][]byte{expRecvRes.Acknowledgement},
}

// modify the callback to return the expected recv result.
path.EndpointB.Chain.GetSimApp().MockModuleV2B.IBCApp.OnRecvPacket = func(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, data types.Payload, relayer sdk.AccAddress) types.RecvPacketResult {
Expand Down Expand Up @@ -432,7 +435,7 @@ func (suite *KeeperTestSuite) TestMsgAcknowledgement() {
suite.Require().NoError(err)

// Construct expected acknowledgement
ack = types.Acknowledgement{AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement}}
ack = types.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement}}

tc.malleate()

Expand Down
2 changes: 2 additions & 0 deletions modules/core/04-channel/v2/keeper/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {

// create standard ack that can be malleated
ack = types.Acknowledgement{
RecvSuccess: true,
AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement},
}

Expand Down Expand Up @@ -335,6 +336,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
packet types.Packet
err error
ack = types.Acknowledgement{
RecvSuccess: true,
AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement},
}
freezeClient bool
Expand Down
5 changes: 5 additions & 0 deletions modules/core/04-channel/v2/types/commitment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ func CommitAcknowledgement(acknowledgement Acknowledgement) []byte {
buf = append(buf, hash[:]...)
}

if acknowledgement.RecvSuccess {
buf = append([]byte{byte(1)}, buf...)
} else {
buf = append([]byte{byte(0)}, buf...)
}
buf = append([]byte{byte(2)}, buf...)

hash := sha256.Sum256(buf)
Expand Down
2 changes: 1 addition & 1 deletion modules/core/04-channel/v2/types/commitment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,5 @@ func TestCommitAcknowledgement(t *testing.T) {
}

commitment := types.CommitAcknowledgement(ack)
require.Equal(t, "f03b4667413e56aaf086663267913e525c442b56fa1af4fa3f3dab9f37044c5b", hex.EncodeToString(commitment))
require.Equal(t, "47a3b131712a356465258d5a9f50340f990a37b14e665b49ea5afa170f5e7aac", hex.EncodeToString(commitment))
}
120 changes: 81 additions & 39 deletions modules/core/04-channel/v2/types/packet.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion modules/core/ante/ante_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func (suite *AnteTestSuite) createAcknowledgementMessageV2(isRedundant bool) *ch
err = suite.path.EndpointA.MsgRecvPacket(packet)
suite.Require().NoError(err)

ack := channeltypesv2.Acknowledgement{AppAcknowledgements: [][]byte{mock.MockRecvPacketResult.Acknowledgement}}
ack := channeltypesv2.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{mock.MockRecvPacketResult.Acknowledgement}}
if isRedundant {
err = suite.path.EndpointB.MsgAcknowledgePacket(packet, ack)
suite.Require().NoError(err)
Expand Down
3 changes: 2 additions & 1 deletion proto/ibc/core/channel/v2/packet.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ message Payload {

// Acknowledgement contains a list of all ack results associated with a single packet.
message Acknowledgement {
repeated bytes app_acknowledgements = 1;
bool recv_success = 1;
repeated bytes app_acknowledgements = 2;
}

// PacketStatus specifies the status of a RecvPacketResult.
Expand Down
Loading