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.

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

[Do Not Merge] Async Ack #1876

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[Do Not Merge] Async Ack #1876

wants to merge 11 commits into from

Conversation

chipshort
Copy link
Contributor

@chipshort chipshort commented May 3, 2024

also fixes #1891
Needs wasmvm release with new message type.

I had to do changes in quite a few places

@chipshort chipshort changed the title [Do Not Merge] Async Ack prototype [Do Not Merge] Async Ack May 23, 2024
@chipshort chipshort marked this pull request as ready for review May 23, 2024 11:26
Copy link

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Lgtm. Added some comments :)

Comment on lines +283 to +292
} else if ack == nil || ack.Acknowledgement() == nil {
// emit all contract and submessage events on success
// nil ack is a success case, see: https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/keeper/msg_server.go#L453
ctx.EventManager().EmitEvents(em.Events())
// contract wants async acknowledgement, so store the packet for later
i.keeper.StoreAsyncAckPacket(ctx, packet)
ack = nil
} else if ack.Success() {
// emit all contract and submessage events on success
ctx.EventManager().EmitEvents(em.Events())
Copy link

Choose a reason for hiding this comment

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

This might be easier to read if it is not chained after the error handling, for example something like:

Suggested change
} else if ack == nil || ack.Acknowledgement() == nil {
// emit all contract and submessage events on success
// nil ack is a success case, see: https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/keeper/msg_server.go#L453
ctx.EventManager().EmitEvents(em.Events())
// contract wants async acknowledgement, so store the packet for later
i.keeper.StoreAsyncAckPacket(ctx, packet)
ack = nil
} else if ack.Success() {
// emit all contract and submessage events on success
ctx.EventManager().EmitEvents(em.Events())
types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err)
return ack
}
switch {
case ack == nil || ack.Acknowledgement() == nil:
// emit all contract and submessage events on success
// nil ack is a success case, see: https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/keeper/msg_server.go#L453
ctx.EventManager().EmitEvents(em.Events())
// contract wants async acknowledgement, so store the packet for later
i.keeper.StoreAsyncAckPacket(ctx, packet)
ack = nil
case ack.Success():
// emit all contract and submessage events on success
ctx.EventManager().EmitEvents(em.Events())
default:
panic("unreachable")
}

it also has the added benefit of panicing in an unexpected scenario

msgResponses := [][]*codectypes.Any{{any}}

return nil, [][]byte{val}, msgResponses, nil
case msg.IBC.WriteAcknowledgement != nil:
Copy link

Choose a reason for hiding this comment

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

I'm assuming only the contract itself can send this message

Comment on lines +268 to +269
// We don't give the contract to supply a success flag because it's not part of the IBC spec.
// Currently, ibc-go also just ignores this flag for `WriteAcknowledgement`.
Copy link

Choose a reason for hiding this comment

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

I believe this is ignored when handling an acknowledgement asynchronously, which is the case here. However, this is used if the acknowledgement is synchronous, see here. We discard the state changes due to application callbacks if this is false. Since the entire contract state is in the application callback, even if the ack was synchronous, you'd want to always return true. Right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible problem with the nil ack
2 participants