Skip to content

Conversation

@0xKarl98
Copy link
Contributor

@0xKarl98 0xKarl98 commented Jan 6, 2026

This closes #20750

Changes

  • Parse the subscription kind and raw params at the callsite: if the kind maps to a known SubscriptionKind (newHeads/logs/newPendingTransactions/syncing), we validate and normalize params upfront and then invoke the standard handlers.

  • If the kind is unknown, we try a per‑kind custom handler (registered via register_handler) first, then fall back to a global fallback_handler if configured; otherwise we return invalid_params.

    • For known kinds, invalid params are rejected immediately with invalid_params to preserve strict RPC semantics and avoid masking client errors. Custom subscriptions are only accepted when explicitly registered (per‑kind handler or global fallback), and they must provide their own subscription stream; otherwise the request is rejected.
  • Invalid paths are explicitly:

    • Known kind + invalid params (e.g., logs expects Filter/none; newPendingTransactions expects bool/none; deserialization/type mismatch) → invalid_params.
    • Unknown kind + no registered handler/fallback → invalid_params.
    • Handler/fallback returns Err(ErrorObject) → forwarded as‑is (e.g., invalid_params or other error).

cc @mattsse

Copy link
Collaborator

@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.

great start

Comment on lines +389 to +390
/// Fallback handler for unknown subscription kinds or params.
fallback_handler: Option<FallbackHandler>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need a way to inject additional handlers in here, so I think this could be like an Arc<Rwlock<HashMap<String, Handler> for example

Comment on lines 60 to +74
pub fn with_spawner(eth_api: Eth, subscription_task_spawner: Box<dyn TaskSpawner>) -> Self {
let inner = EthPubSubInner { eth_api, subscription_task_spawner };
Self::with_spawner_and_fallback(eth_api, subscription_task_spawner, None)
}

/// Creates a new, shareable instance with a fallback handler for unknown subscription kinds or
/// params.
pub fn with_spawner_and_fallback_handler<F>(
eth_api: Eth,
subscription_task_spawner: Box<dyn TaskSpawner>,
fallback_handler: F,
) -> Self
where
F: Fn(String, Option<Box<JsonRawValue>>) -> Result<PubSubStream, ErrorObject<'static>>
+ Send
+ Sync
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can keep all of this as is, and instead add smth like register_handler() that inserts a new handler

this is similar to the Methods type

pub struct Methods {
	callbacks: Arc<FxHashMap<&'static str, MethodCallback>>,

/// Merge the given [`Methods`] in all configured methods.
///
/// Fails if any of the methods in other is present already.
pub fn merge_configured(
&mut self,
other: impl Into<Methods>,

basically, we want a way to inject additional handlers after the fact, like:

ctx.modules.merge_configured(StorageWatcherApiServer::into_rpc(rpc))?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah should do it

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker Jan 6, 2026
@0xKarl98 0xKarl98 marked this pull request as draft January 7, 2026 01:46
@0xKarl98 0xKarl98 requested review from mattsse January 7, 2026 04:48
@0xKarl98 0xKarl98 marked this pull request as ready for review January 7, 2026 05:13
@0xKarl98 0xKarl98 marked this pull request as draft January 7, 2026 08:09
@0xKarl98 0xKarl98 marked this pull request as ready for review January 7, 2026 09:00
@0xKarl98
Copy link
Contributor Author

0xKarl98 commented Jan 7, 2026

pending @mattsse

Copy link
Collaborator

@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.

cool, a few more style suggestions

Comment on lines 43 to 64
type PubSubStream = Box<dyn Stream<Item = Box<JsonRawValue>> + Send + Unpin>;
type SubscriptionHandler = Arc<
dyn Fn(Option<Box<JsonRawValue>>) -> Result<PubSubStream, ErrorObject<'static>> + Send + Sync,
>;
type FallbackHandler = Arc<
dyn Fn(String, Option<Box<JsonRawValue>>) -> Result<PubSubStream, ErrorObject<'static>>
+ Send
+ Sync,
>;

struct ParsedSubscription {
kind: SubscriptionKind,
params: Option<Params>,
}

#[derive(Debug)]
enum ParseSubscriptionError {
UnsupportedKind,
InvalidParams(&'static str),
}

impl ParseSubscriptionError {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move these private types below the ethpubinner impl block so that the order is

pub
private

Comment on lines +88 to +96
/// Creates a new, shareable instance with a fallback handler for unknown subscription kinds or
/// params.
pub fn with_spawner_and_fallback_handler<F>(
eth_api: Eth,
subscription_task_spawner: Box<dyn TaskSpawner>,
fallback_handler: F,
) -> Self
where
F: Fn(String, Option<Box<JsonRawValue>>) -> Result<PubSubStream, ErrorObject<'static>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can do without this fn

Comment on lines +202 to 207
/// Dispatches validated subscription kinds to their concrete stream implementations.
async fn handle_parsed(
&self,
accepted_sink: SubscriptionSink,
parsed: ParsedSubscription,
) -> Result<(), ErrorObject<'static>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should remain unchanged entirely, easier to review that way if we dont alter existing code too much, so this should remain

 pub async fn handle_accepted(
        &self,
        accepted_sink: SubscriptionSink,
        kind: SubscriptionKind,
        params: Option<Params>,

with the same body

Comment on lines +465 to +466
/// Fallback handler for unknown subscription kinds or params.
fallback_handler: Option<FallbackHandler>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this we don't need because if no additional_handlers match then we return an error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Support additional eth_subscribe handlers

2 participants