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 3 commits
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
3 changes: 2 additions & 1 deletion modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,8 @@ func (im IBCModule) OnAcknowledgementPacket(
return err
}

events.EmitOnAcknowledgementPacketEvent(ctx, data, ack)
recvSuccess := ack.Success()
events.EmitOnAcknowledgementPacketEvent(ctx, data, recvSuccess, ack)

return nil
}
Expand Down
3 changes: 2 additions & 1 deletion modules/apps/transfer/internal/events/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func EmitOnRecvPacketEvent(ctx context.Context, packetData types.FungibleTokenPa
}

// EmitOnAcknowledgementPacketEvent emits a fungible token packet event in the OnAcknowledgementPacket callback
func EmitOnAcknowledgementPacketEvent(ctx context.Context, packetData types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) {
func EmitOnAcknowledgementPacketEvent(ctx context.Context, packetData types.FungibleTokenPacketDataV2, recvSuccess bool, ack channeltypes.Acknowledgement) {
tokensStr := mustMarshalJSON(packetData.Tokens)
forwardingHopsStr := mustMarshalJSON(packetData.Forwarding.Hops)
sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: https://github.com/cosmos/ibc-go/issues/5917
Expand All @@ -78,6 +78,7 @@ func EmitOnAcknowledgementPacketEvent(ctx context.Context, packetData types.Fung
sdk.NewAttribute(types.AttributeKeyTokens, tokensStr),
sdk.NewAttribute(types.AttributeKeyMemo, packetData.Memo),
sdk.NewAttribute(types.AttributeKeyForwardingHops, forwardingHopsStr),
sdk.NewAttribute(types.AttributeKeyRecvSuccess, strconv.FormatBool(recvSuccess)),
sdk.NewAttribute(types.AttributeKeyAck, ack.String()),
),
sdk.NewEvent(
Expand Down
1 change: 1 addition & 0 deletions modules/apps/transfer/types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const (
AttributeKeyRefundReceiver = "refund_receiver"
AttributeKeyRefundTokens = "refund_tokens"
AttributeKeyAckSuccess = "success"
AttributeKeyRecvSuccess = "recv_success"
AttributeKeyAck = "acknowledgement"
AttributeKeyAckError = "error"
AttributeKeyMemo = "memo"
Expand Down
6 changes: 3 additions & 3 deletions modules/apps/transfer/v2/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (im *IBCModule) OnTimeoutPacket(ctx context.Context, sourceChannel string,
return nil
}

func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, acknowledgement []byte, payload types.Payload, relayer sdk.AccAddress) error {
func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, recvSuccess bool, acknowledgement []byte, payload types.Payload, relayer sdk.AccAddress) error {
var ack channeltypes.Acknowledgement
if err := transfertypes.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
Expand All @@ -124,10 +124,10 @@ func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel
return err
}

if err := im.keeper.OnAcknowledgementPacket(ctx, payload.SourcePort, sourceChannel, data, ack); err != nil {
if err := im.keeper.OnAcknowledgementPacket(ctx, payload.SourcePort, sourceChannel, data, recvSuccess); err != nil {
return err
}

events.EmitOnAcknowledgementPacketEvent(ctx, data, ack)
events.EmitOnAcknowledgementPacketEvent(ctx, data, recvSuccess, ack)
return nil
}
15 changes: 4 additions & 11 deletions modules/apps/transfer/v2/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/cosmos/ibc-go/v9/modules/apps/transfer/internal/events"
transferkeeper "github.com/cosmos/ibc-go/v9/modules/apps/transfer/keeper"
"github.com/cosmos/ibc-go/v9/modules/apps/transfer/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
channelkeeperv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/keeper"
channeltypesv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
Expand Down Expand Up @@ -186,18 +185,12 @@ func (k *Keeper) OnRecvPacket(ctx context.Context, sourceChannel, destChannel st
return nil
}

func (k *Keeper) OnAcknowledgementPacket(ctx context.Context, sourcePort, sourceChannel string, data types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) error {
switch ack.Response.(type) {
case *channeltypes.Acknowledgement_Result:
// the acknowledgement succeeded on the receiving chain so nothing
// needs to be executed and no error needs to be returned
return nil
case *channeltypes.Acknowledgement_Error:
// We refund the tokens from the escrow address to the sender
func (k *Keeper) OnAcknowledgementPacket(ctx context.Context, sourcePort, sourceChannel string, data types.FungibleTokenPacketDataV2, recvSuccess bool) error {
if !recvSuccess {
// refund the tokens back to the sender
return k.refundPacketTokens(ctx, sourcePort, sourceChannel, data)
default:
return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response)
}
return nil
}

func (k *Keeper) OnTimeoutPacket(ctx context.Context, sourcePort, sourceChannel string, data types.FungibleTokenPacketDataV2) error {
Expand Down
11 changes: 7 additions & 4 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 All @@ -362,7 +365,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
if expPass {
suite.Require().NoError(err)

if bytes.Equal(expectedAck.AppAcknowledgements[0], ackBytes) {
if expectedAck.RecvSuccess {
// tokens remain escrowed
for _, t := range tokens {
escrowedAmount := suite.chainA.GetSimApp().TransferKeeperV2.GetTotalEscrowForDenom(suite.chainA.GetContext(), t.Denom.IBCDenom())
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
17 changes: 11 additions & 6 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 Expand Up @@ -212,7 +217,7 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem
for i, pd := range msg.Packet.Payloads {
cbs := k.Router.Route(pd.SourcePort)
ack := msg.Acknowledgement.AppAcknowledgements[i]
err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, ack, pd, relayer)
err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, msg.Acknowledgement.RecvSuccess, ack, pd, relayer)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: We will need to tweak this eventually since it would be weird to call ack callback with a successful ack bytes but a false recvSuccess but not necessary for single payload change

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to add a TODO comment here for that

if err != nil {
return nil, errorsmod.Wrapf(err, "failed OnAcknowledgementPacket for source port %s, source channel %s, destination channel %s", pd.SourcePort, msg.Packet.SourceChannel, msg.Packet.DestinationChannel)
}
Expand Down
11 changes: 7 additions & 4 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 @@ -377,15 +380,15 @@ func (suite *KeeperTestSuite) TestMsgAcknowledgement() {

// Modify the callback to return an error.
// This way, we can verify that the callback is not executed in a No-op case.
path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, []byte, sdk.AccAddress) error {
path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, bool, []byte, sdk.AccAddress) error {
return mock.MockApplicationCallbackError
}
},
},
{
name: "failure: callback fails",
malleate: func() {
path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, []byte, sdk.AccAddress) error {
path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, bool, []byte, sdk.AccAddress) error {
return mock.MockApplicationCallbackError
}
},
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
7 changes: 6 additions & 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 @@ -84,11 +84,16 @@ func TestCommitPacket(t *testing.T) {
// same commitment output. But it is also useful to catch any changes in the commitment.
func TestCommitAcknowledgement(t *testing.T) {
ack := types.Acknowledgement{
RecvSuccess: true,
AppAcknowledgements: [][]byte{
[]byte("some bytes"),
},
}

commitment := types.CommitAcknowledgement(ack)
require.Equal(t, "f03b4667413e56aaf086663267913e525c442b56fa1af4fa3f3dab9f37044c5b", hex.EncodeToString(commitment))
require.Equal(t, "fc02a4453c297c9b65189ec354f4fc7f0c1327b72f6044a20d4dd1fac8fda9f7", hex.EncodeToString(commitment))

ack.RecvSuccess = false
commitment = types.CommitAcknowledgement(ack)
require.Equal(t, "47a3b131712a356465258d5a9f50340f990a37b14e665b49ea5afa170f5e7aac", hex.EncodeToString(commitment))
}
Loading
Loading