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: Provider::subscribe_logs #339

Merged
merged 13 commits into from
Mar 22, 2024
Merged

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Mar 19, 2024

Motivation

Adds Provider::subscribe_logs and Event::subscribe methods

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm, I would want Event::subscribe too

@onbjerg onbjerg added the enhancement New feature or request label Mar 19, 2024
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

pending @DaniPopes

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

@DaniPopes can we somehow unify the usage of Subscription<T> and PollChannel<T> to have a single front-end that works for both polling and pubsub?

using channels should allow us to erase the type of the backing stream/coroutine

crates/contract/src/event.rs Show resolved Hide resolved
@DaniPopes
Copy link
Member

@prestwich I think so, but I feel that would complicate things quite a bit for not much benefit?

@prestwich
Copy link
Member

it means that users would follow the same API regardless of provider and get automatically upgraded to pubsub subscriptions when they change endpoints (unless they specifically want to downgrade to polling for some reason)

@onbjerg
Copy link
Member

onbjerg commented Mar 20, 2024

i agree w prestwich here, this would most likely also give us transport stacking back? although i'd leave that for a follow up since it's somewhat unrelated to this pr

crates/contract/src/event.rs Outdated Show resolved Hide resolved
Self { sub, _phantom: PhantomData }
}

/// Converts the subscription into a stream.
Copy link
Member

Choose a reason for hiding this comment

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

This results in an unnameable type. Is there any situation in which a user would want to name the resulting stream?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we want a similar thing as SubscriptionStream here with the type we decode on poll_next?

Copy link
Member

Choose a reason for hiding this comment

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

ideally we have these all work the same way. can convert this to issue?

@prestwich
Copy link
Member

Regardless, that's not in scope for this PR. Opened #346 to capture.

@prestwich
Copy link
Member

re-running CI before merge

@prestwich prestwich merged commit 98e9012 into alloy-rs:main Mar 22, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants