Loosened restrictions on parameters to newHead subscription#7732
Loosened restrictions on parameters to newHead subscription#7732tjade273 wants to merge 1 commit into
Conversation
|
It looks like @tjade273 hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io Once you've signed, plesae reply to this thread with Many thanks, Parity Technologies CLA Bot |
|
[clabot:check]. |
|
It looks like @tjade273 signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
5chdn
added
A0-pleasereview 🤓
|
|
||
| let error = match (kind, params.into()) { | ||
| (pubsub::Kind::NewHeads, None) => { | ||
| (pubsub::Kind::NewHeads, _) => { |
There was a problem hiding this comment.
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?
tomusdrw
added
A5-grumble 🔥
|
BTW: would be good to add tests for both behaviours. |
|
Let me know when you are ready, @tjade273 :) |
4 participants
Addresses #7731
Currently the patch simply allows any parameter that is parseable as a log, then ignores it.
We could instead restrict it to only the empty log, but this is more general.