-
Notifications
You must be signed in to change notification settings - Fork 610
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
Emit an event to indicate a successful acknowledgement in the ICA module #1466
Merged
chatton
merged 10 commits into
main
from
cian/issue#1452-ica-module-does-not-emit-an-event-on-success-or-error-of-executing-ica-transactions
Jun 7, 2022
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5f7aba1
feat: Emit an event to indicate a successful acknowledgement in the I…
chatton 4707987
Merge branch 'main' into cian/issue#1452-ica-module-does-not-emit-an-…
chatton 6702d96
feat: Handle both event types in a single function. Added entry to CH…
chatton b52209b
Merge remote-tracking branch 'origin/cian/issue#1452-ica-module-does-…
chatton 94360ae
Merge branch 'main' into cian/issue#1452-ica-module-does-not-emit-an-…
chatton def3a0d
Merge branch 'main' into cian/issue#1452-ica-module-does-not-emit-an-…
chatton 2987233
Merge branch 'main' into cian/issue#1452-ica-module-does-not-emit-an-…
chatton c77f3c7
creating icatypes AttributeKeyAckSuccess, changing errorMsg to var de…
chatton c8ffcb4
Merge remote-tracking branch 'origin/cian/issue#1452-ica-module-does-…
chatton 997ba57
Merge branch 'main' into cian/issue#1452-ica-module-does-not-emit-an-…
chatton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,27 @@ | ||
package keeper | ||
|
||
import ( | ||
"fmt" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
||
icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" | ||
"github.com/cosmos/ibc-go/v3/modules/core/exported" | ||
) | ||
|
||
// EmitWriteErrorAcknowledgementEvent emits an event signalling an error acknowledgement and including the error details | ||
func EmitWriteErrorAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, err error) { | ||
// EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error | ||
// details if any. | ||
func EmitAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, ack exported.Acknowledgement, err error) { | ||
var errorMsg string | ||
if err != nil { | ||
errorMsg = err.Error() | ||
} | ||
|
||
ctx.EventManager().EmitEvent( | ||
sdk.NewEvent( | ||
icatypes.EventTypePacket, | ||
sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName), | ||
sdk.NewAttribute(icatypes.AttributeKeyAckError, err.Error()), | ||
sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg), | ||
sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()), | ||
sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), | ||
), | ||
) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like input on whether or not there are additional fields/attributes that should be included in this event. Please let me know if I've missed anything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine, maybe we could add a
AttributeKeyAckSuccess
like for transfer.But I am also now wondering if we could merge both event functions into one and have something like this:
with the
icatypes.AttributeKeyAckError
empty if it was successful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking something very similar with regards to merging the functions.