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

remove builder event types #97

Merged
merged 6 commits into from
Jul 17, 2024
Merged

Conversation

imabdulbasit
Copy link
Contributor

@imabdulbasit imabdulbasit commented Jul 16, 2024

This PR:

  • removes BuilderEvent and BuilderEventTypes
  • adds a filter parameter to EventStreamer

@imabdulbasit imabdulbasit force-pushed the ab/remove-builder-event-types branch from 60fcfa9 to d202e8f Compare July 16, 2024 20:26
Copy link
Member

@elliedavidson elliedavidson left a comment

Choose a reason for hiding this comment

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

LGTM but I'll defer approval to someone more familiar with this repo.

src/events_source.rs Outdated Show resolved Hide resolved

// required for sending startup info
known_nodes_with_stake: Vec<PeerConfig<Types::SignatureKey>>,
non_staked_node_count: usize,
filter: Option<EventFilterSet<Types>>,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should not be a property of the EventsStreamer but of the connection, so different clients can have different filters. E.g. it could be a parameter of get_event_stream, which would then apply a filter(...) before returning the stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm my initial idea was to broadcast only events needed but this is better.

#[async_trait]
pub trait EventsSource<Types>
where
Types: NodeType,
{
type EventStream: Stream<Item = Arc<Event<Types>>> + Unpin + Send + 'static;
async fn get_event_stream(&self) -> Self::EventStream;

async fn subscribe_events(&self) -> BoxStream<'static, Arc<Event<Types>>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looked unnecessary

@imabdulbasit imabdulbasit merged commit 925a8b4 into main Jul 17, 2024
5 checks passed
@imabdulbasit imabdulbasit deleted the ab/remove-builder-event-types branch July 17, 2024 18:41
@move47
Copy link
Contributor

move47 commented Jul 18, 2024

Hi @nyospe @imabdulbasit , Earlier we defined BuilderEvents to suppress the unnecessary stuff embedded in HotShot events which builder don't care about as it used to affect builder performance. For e.g . we removed something from Decide event here: #53. I am not sure whether that went away with this change.

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.

4 participants