Add subscription subprovider to handle websocket pub/sub #207
Conversation
| @@ -3,6 +3,7 @@ const inherits = require('util').inherits | |||
| const ethUtil = require('ethereumjs-util') | |||
| const Subprovider = require('./subprovider.js') | |||
| const Stoplight = require('../util/stoplight.js') | |||
| const EventEmitter = require('events').EventEmitter | |||
There was a problem hiding this comment.
this is correct and fine but you can also just do
const EventEmitter = require('events')
subproviders/subscriptions.js
Outdated
| method = self.newLogFilter.bind(self, options) | ||
| break | ||
| case 'newPendingTransactions': | ||
| method = self.newPendingTransactionFilter |
There was a problem hiding this comment.
I'm not sure why it would, but I'll add it for symmetry w/ the one above it
| self.allResults.push(log) | ||
| }) | ||
| if (validLogs.length > 0) { | ||
| self.emit('data', validLogs) |
There was a problem hiding this comment.
does this also need the null error event?
There was a problem hiding this comment.
no, the only consumer of the event is the SubscriptionSubprovider - would rather emit error in that case.
subproviders/subscriptions.js
Outdated
| return | ||
| } | ||
|
|
||
| method(function(err, id) { |
There was a problem hiding this comment.
i found this part least clear
maybe renaming to createFilter would be better
subproviders/subscriptions.js
Outdated
| } | ||
|
|
||
| method(function(err, id) { | ||
| if (!err) { |
There was a problem hiding this comment.
abort early on err
if (err) return cb(err)There was a problem hiding this comment.
good catch - no idea why I wrote it that way :-/
|
|
||
| // a cheap crack at multiple inheritance | ||
| // I don't really care if `instanceof EventEmitter` passes... | ||
| Object.assign(SubscriptionSubprovider.prototype, EventEmitter.prototype) |
There was a problem hiding this comment.
I think instead of SubscriptionSubprovider being an EE, we can just make engine an EE and require that to be passed into the constructor -- and lol directly emit events on it /shrug
There was a problem hiding this comment.
I'd rather leave it like it is - I see no reason why this class needs to be tightly coupled to engine like that. Further, in ganache-core I make the provider an EventEmitter (it must be, per web3 requirements), and I proxy the EE methods for the data event directly to the SubscriptionSubprovider. I like it this way over proxying to the engine because it's more explicit that I'm subscribing to pub/sub events.
There was a problem hiding this comment.
Now that I've thought about it a bit more, I wonder if there shouldn't be an event middleware stack. As pub/sub grows in scope, I could imagine wanting to write middleware that would squelch or mutate notifications.
Maybe that's just YAGNI, though...
subproviders/subscriptions.js
Outdated
| SubscriptionSubprovider.prototype._notificationHandler = function (id, subscriptionType, result) { | ||
| const self = this | ||
| if (subscriptionType === 'newHeads') { | ||
| result = { |
There was a problem hiding this comment.
should break this off as a utility method
subproviders/subscriptions.js
Outdated
|
|
||
| SubscriptionSubprovider.prototype.unsubscribe = function(connection, payload, cb) { | ||
| let subscriptionId = payload.params[0] | ||
| if (!this.subscriptions.hasOwnProperty(subscriptionId.toLowerCase())) { |
There was a problem hiding this comment.
this.subscriptions[subscriptionId] should be fine, eh?
There was a problem hiding this comment.
the id is a hex string - I wanted it to be case insensitive
There was a problem hiding this comment.
but ew, I forgot to write it in that way over in subscribe - I'll fix it there
Edit: and also below the line you commented on here... ugly.
There was a problem hiding this comment.
alternatively I could normalize it so that 0x0001 matches 0x1, etc.
There was a problem hiding this comment.
ok, forget all that noise - I'm going to do it your way :-D
subproviders/subscriptions.js
Outdated
| break | ||
| case 'newHeads': | ||
| method = self.newBlockFilter | ||
| cb(null, id) |
There was a problem hiding this comment.
Wtf? That shouldn't have been there... I definitely need to write some tests in here - have been relying too heavily on ganache integration tests
Needs a test for the subscription subprovider itself, but otherwise I think this is ready.