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

ics29:feat: emit event escrow #914

Merged
merged 6 commits into from
Feb 17, 2022
Merged

ics29:feat: emit event escrow #914

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd31079
feat: emit EventTypeSendIncentivizedPacket event on EscrowPacket
seantking Feb 11, 2022
1c5e749
fix: string conversion
seantking Feb 11, 2022
0f5ffe5
refactor: add helper fn for emit event
seantking Feb 15, 2022
d41c83f
chore: godoc
seantking Feb 17, 2022
d394406
nit: use .String())
seantking Feb 17, 2022
0064c61
Merge branch 'ics29-fee-middleware' into sean/issue#762-emit-event-es…
seantking Feb 17, 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
12 changes: 12 additions & 0 deletions modules/apps/29-fee/keeper/escrow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types"
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
)

// EscrowPacketFee sends the packet fee to the 29-fee module account to hold in escrow
Expand Down Expand Up @@ -38,6 +39,17 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee types.IdentifiedP

// Store fee in state for reference later
k.SetFeeInEscrow(ctx, identifiedFee)

// Emit event so that relayers know an incentivized packet is ready to be relayed
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeSendIncentivizedPacket,
sdk.NewAttribute(channeltypes.AttributeKeyPortID, identifiedFee.PacketId.PortId),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should maybe put the relayer address here as well?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea.
Should we add an events.go and move the event emissions funcs there? Similarly to 04-channel

Copy link
Member

Choose a reason for hiding this comment

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

I actually think its unnecessary. Right now, relayer list is guaranteed to be nil, so no point in emitting it.

And once it does become non-nil, we can add it to events in a non-breaking manner

Copy link
Contributor

Choose a reason for hiding this comment

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

we can add it to events in a non-breaking manner

this won't necessarily be true by the time the change is made (as it does seem there is desire to eventually include events in consensus), but I agree we can leave it off for now

sdk.NewAttribute(channeltypes.AttributeKeyChannelID, identifiedFee.PacketId.ChannelId),
sdk.NewAttribute(channeltypes.AttributeKeySequence, fmt.Sprint(identifiedFee.PacketId.Sequence)),
Copy link
Member

Choose a reason for hiding this comment

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

We need to emit the fee for each relayer msg:

RecvFee => recv_coins
AckFee => ack_coins
TimeoutFee => timeout_coins

),
)

return nil
}

Expand Down
4 changes: 3 additions & 1 deletion modules/apps/29-fee/types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
package types

// 29-fee events
const ()
const (
EventTypeSendIncentivizedPacket = "send_incentivized_packet"
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
)