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

add event emissions to provider and consumer #427

Merged
merged 12 commits into from
Nov 11, 2022
Merged

Conversation

MSalopek
Copy link
Contributor

@MSalopek MSalopek commented Nov 2, 2022

Events added:

  • create_client (provider -> during CreateConsumerClient call)
  • consumer_added (provider)
  • execute_consumer_chain_slash (provider)
  • fee_channel_opened (consumer)
  • first_vsc_packet (consumer -> mainly for debugging so we know that first vsc was received)
  • fee_distribution (consumer)
  • send_slash_packet (consumer)
  • send_matured_vsc_packet (consumer)

@MSalopek MSalopek added type: feature-request New feature or request improvement scope: UI Addressing UX changes and improvements to user interface labels Nov 2, 2022
@MSalopek MSalopek self-assigned this Nov 2, 2022
x/ccv/consumer/module.go Outdated Show resolved Hide resolved
@MSalopek MSalopek force-pushed the masa/356-add-events branch from 44fa19e to 8ebb7bc Compare November 7, 2022 18:00
@MSalopek MSalopek marked this pull request as ready for review November 7, 2022 18:02
@MSalopek MSalopek force-pushed the masa/356-add-events branch from 4ec552a to 5cd18e7 Compare November 7, 2022 18:23
@MSalopek MSalopek changed the title add todos for event emissions add event emissions to provider and consumer Nov 8, 2022
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Nice work @MSalopek. See my comments below. It could be useful to ask some front-end devs for feedback.

x/ccv/provider/keeper/keeper.go Show resolved Hide resolved
x/ccv/provider/keeper/proposal.go Outdated Show resolved Hide resolved
x/ccv/consumer/keeper/relay.go Outdated Show resolved Hide resolved
x/ccv/provider/keeper/keeper.go Outdated Show resolved Hide resolved
x/ccv/consumer/keeper/relay.go Outdated Show resolved Hide resolved
x/ccv/consumer/keeper/relay.go Show resolved Hide resolved
@MSalopek MSalopek force-pushed the masa/356-add-events branch from 2af0044 to 40f122b Compare November 10, 2022 12:30
@danwt danwt requested a review from shaspitz as a code owner November 11, 2022 12:11
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

Nice work! I don't see a reason not to approve as this clearly cannot introduce bugs. Improvements and more events can be added later if necessary.

EventTypeFeeTransferChannelOpened = "fee_channel_opened"
EventTypeConsumerClientCreated = "consumer_client_created"

EventExecuteConsumerChainSlash = "execute_consumer_chain_slash"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should line 12 have EventType as a prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Comment on lines 5 to 15
EventTypeTimeout = "timeout"
EventTypePacket = "ccv_packet"
EventTypeChannelClose = "channel_closed"
EventTypeChannelEstablished = "channel_established"
EventTypeFeeTransferChannelOpened = "fee_channel_opened"
EventTypeConsumerClientCreated = "consumer_client_created"

EventExecuteConsumerChainSlash = "execute_consumer_chain_slash"
EventTypeFeeDistribution = "fee_distribution"
EventTypeSendSlashPacket = "send_slash_packet"
EventTypeSendMaturedVSCPacket = "send_matured_vsc_packet"
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these are in past tense and some are in present tense, could it be more consistent?

By the way there is a nice vscode extension change-case which lets you easily change Camel to snake ect. Some of these dont match (ChannelClose = "channel_closed")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed cases that did not match.

x/ccv/types/events.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

Just a couple small things but great stuff!

Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

LGTM

@shaspitz shaspitz merged commit 5dc2bbf into main Nov 11, 2022
@shaspitz shaspitz deleted the masa/356-add-events branch November 11, 2022 19:03
danwt pushed a commit that referenced this pull request Nov 14, 2022
commit 5dc2bbf
Author: MSalopek <[email protected]>
Date:   Fri Nov 11 20:03:22 2022 +0100

    add event emissions to provider and consumer (#427)

    * add todos for event emissions

    * introduce ccv_consumer_added, ccv_fee_channel_opened events

    * add vsc_first_packet event in consumer

    * add fee distribution event; small refactor

    * add slash request events

    * add pending slash request clearing events

    * move all events to ccv; add CreateClient event on provi

    * remove event addition TODO tags

    * update after reviews

    * small refactors

    * address review comments and refactor

    Co-authored-by: Daniel T <[email protected]>

commit 8d158d9
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Fri Nov 11 01:27:22 2022 -0800

    Update CODEOWNERS (#466)
danwt added a commit that referenced this pull request Nov 14, 2022
commit 5dc2bbf
Author: MSalopek <[email protected]>
Date:   Fri Nov 11 20:03:22 2022 +0100

    add event emissions to provider and consumer (#427)

    * add todos for event emissions

    * introduce ccv_consumer_added, ccv_fee_channel_opened events

    * add vsc_first_packet event in consumer

    * add fee distribution event; small refactor

    * add slash request events

    * add pending slash request clearing events

    * move all events to ccv; add CreateClient event on provi

    * remove event addition TODO tags

    * update after reviews

    * small refactors

    * address review comments and refactor

    Co-authored-by: Daniel T <[email protected]>

commit 8d158d9
Author: Shawn Marshall-Spitzbart <[email protected]>
Date:   Fri Nov 11 01:27:22 2022 -0800

    Update CODEOWNERS (#466)

Co-authored-by: Daniel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: UI Addressing UX changes and improvements to user interface type: feature-request New feature or request improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants