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

feat: simplify event service interface #14859

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Conversation

robert-zaremba
Copy link
Collaborator

Description

Simplify the core/event service interface, by making the method names shorter. Some methods were rather long, and we can simplify by removing some part from the composed names which are 'obvious' from the context (package name, type name).

Related to: #14686
cc: @aaronc


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@robert-zaremba robert-zaremba requested a review from a team as a code owner January 31, 2023 21:00
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

lgtm, could you update the ADR as well please?

core/event/service.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member

aaronc commented Feb 1, 2023

Are we still thinking of having a bundle service with all the interfaces combined? In that case the long method names would be useful

@tac0turtle
Copy link
Member

Are we still thinking of having a bundle service with all the interfaces combined? In that case the long method names would be useful

I think an optional one could be good, but not required. Agree maybe we keep the names for a future where we have bundled service.

@tac0turtle
Copy link
Member

closing this because of the point Aaron rose on bundled services

@tac0turtle tac0turtle closed this Feb 1, 2023
@aaronc
Copy link
Member

aaronc commented Feb 1, 2023

I'm not a huge fan of the bundled service personally but I can see why people would want it to reduce verbosity. An alternative to the long names is going back to using an event manager and then we have a service method GetEventManager

@tac0turtle
Copy link
Member

oh sorry I thought you were for it. I don't have a strong preference, happy to reopen and merge this

@aaronc
Copy link
Member

aaronc commented Feb 1, 2023

Let's discuss in the next call. I think the bundle service might be preferred by some users

@robert-zaremba
Copy link
Collaborator Author

Let's reopen and close once we have decision.

@robert-zaremba robert-zaremba reopened this Feb 1, 2023
@robert-zaremba
Copy link
Collaborator Author

@aaronc , what is a bundle service? I don't see any Github issue about it.

Co-authored-by: Aleksandr Bezobchuk <[email protected]>
@kocubinski
Copy link
Member

@aaronc , what is a bundle service? I don't see any Github issue about it.

I think the conversation about that concept may be tracked back to this PR comment

@aaronc
Copy link
Member

aaronc commented Feb 6, 2023

Based on our call today, I think we want to do something like this:

package event

type Service interface {
        EventManager(context.Context) Manager
}

type Manager interface {
  	Emit(event protoiface.MessageV1) error
	EmitKV(eventType string, attrs ...Attribute) error
	EmitNonConsensus(event protoiface.MessageV1) error
}

This will allow for a bundle service and shorter names. Usage would be k.svc.EventManager(ctx).Emit(evt) as opposed to k.eventSvc.EmitProtoEvent(ctx, evt).

How does that sound?

@robert-zaremba
Copy link
Collaborator Author

Why introducing the Manager, rather than having all methods in the Service?

@aaronc
Copy link
Member

aaronc commented Feb 8, 2023

To support a service bundle as described in #12972 (comment), i.e.

type Service interface {
  event.Service
  store.KVStoreService
  store.MemoryStoreService
  gas.Service
  ...
}

@tac0turtle
Copy link
Member

@robert-zaremba is it okay if I push this over the finish line?

@robert-zaremba
Copy link
Collaborator Author

@tac0turtle , as you prefer
LMK

@tac0turtle tac0turtle enabled auto-merge (squash) February 9, 2023 20:12
@tac0turtle tac0turtle merged commit da5db35 into main Feb 9, 2023
@tac0turtle tac0turtle deleted the robert/event-service branch February 9, 2023 20:30
tsenart pushed a commit to meka-dev/cosmos-sdk that referenced this pull request Apr 12, 2023
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: marbar3778 <[email protected]>
@coderabbitai coderabbitai bot mentioned this pull request May 3, 2024
12 tasks
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.

5 participants