PubSub for parity-js #5830

kaikun213 · 2017-06-13T08:33:09Z

In progress. Still needs testing and documentation

'parity' instead of 'parity_subscribe' calls params with empty array as options. If eth_subscribe includes empty array parity-core will send invalid request (eth api doesn't have options)

tomusdrw

Overall looks quite nice, couple of grumbles regarding style, code quality and usability of the pubsub() namespace.

tomusdrw · 2017-06-28T15:31:58Z

js/src/api/pubsub/pubsub.js

+  // }
+
+  // parity API
+  accountsInfo (callback) {


How about splitting this APIs into separate namespaces, like:
api.pubsub().parity().accountsInfo() and api.pubsub().eth().newHeads() ?

We could, I had them split before but that makes it more difficult for the user again to keep track which subscriptionId belongs to which api for unsubscribing.
We could split them again but keep that the Id's are non-correlating.
Also eth only supports one method so far so the extra work for the user seemed unnecessary.

Actually prefer @tomusdrw approach in this case - it also makes sure that we never overlap, and aligns better with teh way the APIs are structured?

Ok, api().pubsub.eth , api().pubsub.parity and api().pubsub.net are supported now. Subscription IDs are namespace unrelated, thus unsubscribing can be done either by calling api().pubsub.unsubscribe(id) or one of the modules e.g. api().pubsub.eth.unsubscribe(id)

tomusdrw · 2017-06-28T15:32:23Z

js/src/api/pubsub/pubsub.js

+  }
+
+  sendRawTransaction (callback, data) {
+    return this.addListener(this._api, 'eth_sendRawTransaction', callback, [inData(data)]);


Doesn't really make sense to subscribe to that.

tomusdrw · 2017-06-28T15:32:38Z

js/src/api/pubsub/pubsub.js

+    return this.addListener(this._api, 'eth_sendRawTransaction', callback, [inData(data)]);
+  }
+
+  submitTransaction (callback, data) {


Doesn't really make sense to subscribe to that.

tomusdrw · 2017-06-28T15:33:47Z

js/src/api/pubsub/pubsub.js

+    return this.addListener(this._api, 'eth_getWork', callback);
+  }
+
+  submitWork (callback, nonce, powHash, mixDigest) {


Doesn't really make sense to subscribe to that.

tomusdrw · 2017-06-28T15:33:50Z

js/src/api/pubsub/pubsub.js

+    return this.addListener(this._api, 'eth_submitWork', callback, [inNumber16(nonce), powHash, mixDigest]);
+  }
+
+  submitHashrate (callback, hashrate, clientId) {


Doesn't really make sense to subscribe to that.

tomusdrw · 2017-06-28T15:42:55Z

js/src/api/transport/ws/ws.js

        // Don't print error if request rejected or not is not yet up...
        if (!/(rejected|not yet up)/.test(result.error.message)) {
          console.error(`${method}(${JSON.stringify(params)}): ${result.error.code}: ${result.error.message}`);
        }

        const error = new TransportError(method, result.error.code, result.error.message);

-        reject(error);
+        result.id ? reject(error) : callback(error);


Imho better to write an if-else instead.

tomusdrw · 2017-06-28T15:44:29Z

js/src/api/transport/ws/ws.js

+      const json = this.encode(uMethod, params);
+
+      this._messages[id] = { id, 'method': uMethod, params, json,
+        resolve: _ => {


I would extract those methods to const resolve = () => {...} and then leave this.messages[id] as simple as in subscribe.

tomusdrw · 2017-06-28T15:45:47Z

js/src/api/transport/ws/ws.js

+  }
+
+  unsubscribe (messageId) {
+    return new Promise(() => {


Why this promise is never resolved? I think it should be resolved in this._messages[id], no?

@kaikun213 That one is still valid too.

Had some missunderstanding there. Fixxed now!

tomusdrw · 2017-06-28T15:46:12Z

js/src/api/transport/ws/ws.js

+          delete this._messages[messageId];
+          method === 'eth_subscribe' ? delete this._subscriptions['eth_subscription'][subId] : delete this._subscriptions['parity_subscription'][subId];
+        }, reject: e => {
+          throw Error('Error unsubscribing.' + e);


Instead of crashing when usubscribe fails you should rather reject the upper promise.

tomusdrw · 2017-06-28T15:47:59Z

js/src/api/transport/ws/ws.js

+      this._messages[id] = { id, 'method': uMethod, params, json,
+        resolve: _ => {
+          delete this._messages[messageId];
+          method === 'eth_subscribe' ? delete this._subscriptions['eth_subscription'][subId] : delete this._subscriptions['parity_subscription'][subId];


delete this._subscriptions[method === 'eth_subscribe' ? ...][subId];

Also I think this check is done more often than necessary, can't we store the subscription names and unsubscribe method names in this_messages? I think it would simplify a lot of code.

Yes thought about it, now I realized it makes it much cleaner.

https://github.com/paritytech/parity/wiki/JSONRPC-trace-module#trace_call

tomusdrw

Almost there, few more grumbles.

tomusdrw · 2017-07-05T12:15:55Z

js/src/api/pubsub/eth/eth.js

+    return this.addListener('eth', 'newHeads', callback);
+  }
+  //
+  // logs (callback) {


Logs are now available: #5705 but this can come in a separate PR.

Imho we should either uncomment the code or remove it.

tomusdrw · 2017-07-05T12:20:15Z

js/src/api/rpc/trace/trace.js

@@ -28,9 +28,9 @@ export default class Trace {
      .then(outTraces);
  }

-  call (options, blockNumber = 'latest', whatTrace = ['trace']) {
+  call (options, whatTrace = ['trace'], blockNumber = 'latest') {


Good catch 👍

tomusdrw · 2017-07-05T12:21:20Z

js/src/api/transport/ws/ws.js

        this.error(event.data);

+        if (result.params.error) {
+          result[error] = result.params.error;


@kaikun213 This comment is still valid.

tomusdrw · 2017-07-05T12:21:58Z

js/src/api/transport/ws/ws.js

+  }
+
+  unsubscribe (messageId) {
+    return new Promise(() => {


@kaikun213 That one is still valid too.

tomusdrw · 2017-07-05T12:22:23Z

js/src/api/transport/ws/ws.js

-          throw Error('Error unsubscribing.' + e);
-        } };
-
+      this._messages[id] = { id, 'method': uMethod, params, json, resolve, reject };


' around method are not required

tomusdrw · 2017-07-05T16:51:06Z

Looks good. @jacogr Could you have a look and merge if you consider it ready?

jacogr

Brilliant, happy as it stands (and looking forward to actually cleaning up the polling using this).

kaikun213 added 2 commits June 13, 2017 10:25

PubSub Integration WebSocket

66fc121

PubSub Provider API

f38fb4d

kaikun213 added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Jun 13, 2017

kaikun213 added 8 commits June 13, 2017 10:49

Parity License and fix switch statement

1f6fd73

Minor fix: use parameter api

4df1057

Exclude subscriptionId return

fcffa39

Unsubscribe parameters as array

65f8091

secureProvider API added

e5ad5b0

isSecure check

1f065c5

Refractor: Formatting in callback (no Promise)

82134b7

Tests for parityProvider

87ad489

kaikun213 added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 14, 2017

kaikun213 added 8 commits June 14, 2017 13:42

Refractor: Formatting in callback (secure API)

60d7afd

Updated transaction documentation

e2b3759

Module instead of API-Names, Options always as array (e.g. empty)

6a0b33b

'parity' instead of 'parity_subscribe' calls params with empty array as options. If eth_subscribe includes empty array parity-core will send invalid request (eth api doesn't have options)

Removed isSecure transport check, because APIs are configurable

1eb138a

Refractor Provider API to single Pubsub

de0f0a0

Modify transport layer to have single identifier for subscriptions

0476901

FIX: Display pubsub errors

db5af2e

Discard Messages after unsubscribing

95de48c

kaikun213 requested review from jacogr and tomusdrw June 26, 2017 10:56

tomusdrw reviewed Jun 28, 2017

View reviewed changes

kaikun213 added 5 commits June 28, 2017 18:21

Fix: display error normal messages correctly

0da4dce

Simplified code, removed unnecessary pubsub methods

0103a7f

trace_call API 2nd argument blockNumber, first whatTrace

5a5ad4a

https://github.com/paritytech/parity/wiki/JSONRPC-trace-module#trace_call

Separate namespaces pubsub. eth, parity, net

864cf06

Keep error for messages from unsubscribed topics.

951d0cc

tomusdrw reviewed Jul 5, 2017

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 5, 2017

kaikun213 added 3 commits July 5, 2017 14:50

Fix: Unsubscribe Promise

c9c25ef

Add Test: Unsubscribe promise resolved

a873d56

Fix: 'error' in params

26d5043

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 5, 2017

jacogr approved these changes Jul 6, 2017

View reviewed changes

jacogr merged commit 349316f into master Jul 6, 2017

jacogr deleted the kaikun branch July 6, 2017 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PubSub for parity-js #5830

PubSub for parity-js #5830

kaikun213 commented Jun 13, 2017

tomusdrw left a comment

tomusdrw Jun 28, 2017

kaikun213 Jun 29, 2017

jacogr Jul 3, 2017

kaikun213 Jul 4, 2017

tomusdrw Jun 28, 2017

tomusdrw Jun 28, 2017

tomusdrw Jun 28, 2017

tomusdrw Jun 28, 2017

tomusdrw Jun 28, 2017

tomusdrw Jun 28, 2017

tomusdrw Jun 28, 2017

tomusdrw Jul 5, 2017

kaikun213 Jul 5, 2017

tomusdrw Jun 28, 2017

tomusdrw Jun 28, 2017

kaikun213 Jun 29, 2017

tomusdrw left a comment

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

tomusdrw commented Jul 5, 2017

jacogr left a comment

PubSub for parity-js #5830

PubSub for parity-js #5830

Conversation

kaikun213 commented Jun 13, 2017

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Jul 5, 2017

jacogr left a comment

Choose a reason for hiding this comment