chore: ics29 fee payee event emission#1568
Conversation
damiannolan
requested review from
AdityaSripal,
charleenfei,
colin-axner and
seantking
as code owners
crodriguezvega
left a comment
crodriguezvega
left a comment
There was a problem hiding this comment.
I just left a nit...
Thank you, @damiannolan, for picking up this so quickly!
modules/apps/29-fee/keeper/events.go
Outdated
| } | ||
|
|
||
| // EmitRegisteredPayeeEvent emits an event containing information of a registered payee for a relayer on a particular channel | ||
| func EmitRegisteredPayeeEvent(ctx sdk.Context, relayer, payee, channelID string) { |
There was a problem hiding this comment.
Nit: currently the Emit... functions we have in the code base use a verb in the present tense, therefore should we rename this to EmitRegisterPayeeEvent? I agree with you that event names that signal that something has occurred should have a verb in the past tense, but maybe we can stay consistent for now with the rest of the codebase and refactor the names of all the functions in a separate PR...
There was a problem hiding this comment.
I actually thought of this when I added these functions here 😅
The main reason I opted for past tense was because of the existing EmitIncentivizedPacketEvent at the top of the file. I've no problem to update these tho, I'll push an update! 👍
There was a problem hiding this comment.
Ah, true, there was already one Emit... function here like that... I should have caught that one back then; shame on me! ;)
modules/apps/29-fee/keeper/events.go
Outdated
| } | ||
|
|
||
| // EmitRegisteredCounterpartyPayeeEvent emits an event containing information of a registered counterparty payee for a relayer on a particular channel | ||
| func EmitRegisteredCounterpartyPayeeEvent(ctx sdk.Context, relayer, counterpartyPayee, channelID string) { |
| const ( | ||
| EventTypeIncentivizedPacket = "incentivized_ibc_packet" | ||
| EventTypeIncentivizedPacket = "incentivized_ibc_packet" | ||
| EventTypeRegisterPayee = "register_payee" |
There was a problem hiding this comment.
just thinking aloud would there be a benefit to prefixing these events with say ics29_register_payee. In the future, there may be similar events from other modules it might become harder to distinguish?
There was a problem hiding this comment.
When we emit events we should always be including the sdk.AttributeKeyModule with the module name as well so it can be correlated like that pretty easily too I guess!
4 participants
Description
RegisterPayeeRegisterCounterpartyPayeecloses: #1566
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes