Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Loosened restrictions on parameters to newHead subscription #7732

Closed
tjade273 wants to merge 1 commit into from
Closed

Loosened restrictions on parameters to newHead subscription #7732

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions rpc/src/v1/impls/eth_pubsub.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,18 +251,16 @@ impl<C: Send + Sync + 'static> EthPubSub for EthPubSubClient<C> {
kind: pubsub::Kind,
params: Trailing<pubsub::Params>,
) {

let error = match (kind, params.into()) {
(pubsub::Kind::NewHeads, None) => {
(pubsub::Kind::NewHeads, _) => {
Copy link
Copy Markdown
Collaborator

@tomusdrw tomusdrw Jan 30, 2018

Choose a reason for hiding this comment

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

I don't like loosening it completely. I think the RPC and the spec should be as specific as possible, we'll run into plenty of issues if we allow leniency of parameters (like developers wondering why a parameter doesn't work if they make a typo or the parameter is not supported).

IMHO The spec just not got properly updated after newBlocks has been renamed to newHeads, includeTransactions paramteter in examples doesn't make much sense in case of headers-only subscription.

Having said that, I'm ok with allowing:
eth_subscribe('newHeads') and eth_subscribe('newHeads', {}) call (optional options argument that we might add in the future), but not eth_subscribe('newHeads', {'anything': true}).

@tjade273 what do you think?

Copy link
Copy Markdown
Author

@tjade273 tjade273 Jan 30, 2018

Choose a reason for hiding this comment

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

Makes sense, I'll tweak it

self.heads_subscribers.write().push(subscriber);
return;
},
(pubsub::Kind::Logs, Some(pubsub::Params::Logs(filter))) => {
self.logs_subscribers.write().push(subscriber, filter.into());
return;
},
(pubsub::Kind::NewHeads, _) => {
errors::invalid_params("newHeads", "Expected no parameters.")
},
(pubsub::Kind::Logs, _) => {
errors::invalid_params("logs", "Expected a filter object.")
},
Expand Down