Poll for defaultAccount to update dapp & overlay subscriptions #4417

jacogr · 2017-02-03T11:22:56Z

ngotchac · 2017-02-03T11:36:18Z

js/src/api/subscriptions/personal.js

+
+        nextTimeout();
+      })
+      .catch(nextTimeout);


The error will be the first argument of nextTimeout right ? So I guess setTimeout would break

100%. Actually a bug in eth_blockNumber then as well. Should fix in both places.

ngotchac · 2017-02-03T11:38:30Z

js/src/api/subscriptions/personal.js

  _defaultAccount = () => {
+    const nextTimeout = (timeout = 1000) => {


It doesn't feel right to make a call every second whereas this shouldn't really change... It's understandable for eth_blockNumber because it's more or less 14 useless calls for 1 useful one. But here it's thousands I'd guess.
Not sure what better approach we could have though (except having a proper WS push)...

Agreed. As said, not perfect, but beats each dapp having to do exactly this. (Or worse poll eth_accounts)

What about using local storage events then ?

As a bugfix & stop-gap before push, I don't believe it is time well invested. In addition, all the instances don't actually share the same localStorage.

UI is served from 8180

Local dapps are served via :8080

That's right

ngotchac · 2017-02-03T11:38:56Z

js/src/api/subscriptions/personal.js

  _defaultAccount = () => {
+    const nextTimeout = (timeout = 1000) => {
+      setTimeout(() => {


Shouldn't we have a way of unsubscribing/cancelling this polling ?

The default subscription cancel takes care of that. An optimisation may be to look at the number of subscribers, however the individual module doesn't actually manage that nor have access ot it.

How can it cancel the timeouts if we don't store the timeout ID ?

The polling gets done once for each subscription instance - which matches up with the API instances. (So while an API is loaded, it will poll.) Only part being managed is the subscriber.

Yes ok, so the _defaultAccount polling gets cancelled when the API gets nuked, right ? We might want to just keep track of it in case we case we want to stop the polling ; just by adding this._defaultAccountTimeoutId = setTimeout would do I guess. Would be easier to then move to a proper logic of stopping the polling when the Transport is disconnected, starting when it's connected ; stopping when there are no subscribers, etc..

Stored in both case now. (Unused)

ngotchac · 2017-02-03T11:39:49Z

js/src/api/subscriptions/personal.js

+      }, timeout);
+    };
+
+    if (!this._api.transport.isConnected) {


Instead of polling every 500ms when the transport is disconnected, we could use the transport events system to subscribe when connected, and unsubscribe when disconnected.

This matches up with what we do in eth_blockNumber. I really don't want to go off the rails when we haven't done it for the rest of the subscription mechanism. (Especially not as a bugfix)

Well, ok, true. However it's not because it's wrongly done in eth_blockNumber that we should stay on this track

As a bugfix, I'm not going to rewrite the full subscriptions module.

"Wrong" is not correct - it is the original implementation, which works and does all the heavy lifting for subscriptions as it stands in the app.

Wrong might indeed be the wrong word (ehe). Inefficient then. But ok as a bug fix. Would really need to move to push from the WS ; currently we are making at least 4 requests per second

Agreed. Not optimal to say the least.

ngotchac · 2017-02-03T11:46:16Z

We might want to use Web Storage Event so that every instance of the API would update when the default account changes

ngotchac · 2017-02-03T12:25:57Z

By migrating to polling, the default account icon is changed only after the next polling request is executed. There might be a delay of 1 second (or more if the node is choking) before the icon gets updated. Couldn't we have a way of sending the request as soon as the default account is changed ?

jacogr · 2017-02-03T12:31:40Z

For that case, can put back the poll updates (the lines removed for the listening) - however that only works when using secure. (Not in dapps nor extension). Probably better than nothing though.

It will need to wait though, I need to get the condition tested. (Along with this one, it needs to go in for 1.5.1 - and in that case I'm building the updated Parity, which means a stop to editing for the time being)

EDIT: re-added

ngotchac

All ok, until we get proper push

ngotchac · 2017-02-03T12:54:44Z

js/src/api/subscriptions/eth.js

@@ -37,7 +38,7 @@ export default class Eth {

  _blockNumber = () => {
    const nextTimeout = (timeout = 1000) => {
-      setTimeout(() => {
+      this._pollTimerId = setTimeout(() => {


jacogr · 2017-02-03T12:58:52Z

Glad to see you hate this approach as much as me :)

* Poll for defaultAccount (Fixes #4413) * Fix nextTimeout on catch * Store timers * Re-enable default updates on change detection

* s/Delete Contract/Forget Contract/ (#4237) * Adjust the location of the signer snippet (#4155) * Additional building-block UI components (#4239) * Currency WIP * Expand tests * Pass className * Add QrCode * Export new components in ~/ui * s/this.props.netSymbol/netSymbol/ * Fix import case * ui/SectionList component (#4292) * array chunking utility * add SectionList component * Add TODOs to indicate possible future work * Add missing overlay style (as used in dapps at present) * Add a Playground for the UI Components (#4301) * Playground // WIP * Linting * Add Examples with code * CSS Linting * Linting * Add Connected Currency Symbol * 2015-2017 * 2015-2017 * 2015-2017 * 2015-2017 * 2015-2017 * 2015-2017 * 2015-2017 * Added `renderSymbol` tests * PR grumbles * Add Eth and Btc QRCode examples * 2015-2017 * Add tests for playground * Fixing tests * Split Dapp icon into ui/DappIcon (#4308) * Add QrCode & Copy to ShapeShift (#4322) * Extract CopyIcon to ~/ui/Icons * Add copy & QrCode address * Default size 4 * Add bitcoin: link * use protocol links applicable to coin exchanged * Remove .only * Display QrCode for accounts, addresses & contracts (#4329) * Allow Portal to be used as top-level modal (#4338) * Portal * Allow Portal to be used in as both top-level and popover * modal/popover variable naming * export Portal in ~/ui * Properly handle optional onKeyDown * Add simple Playground Example * Add proper event listener to Portal (#4359) * Display AccountCard name via IdentityName (#4235) * Fix signing (#4363) * Dapp Account Selection & Defaults (#4355) * Add parity_defaultAccount RPC (with subscription) (#4383) * Default Account selector in Signer overlay (#4375) * Typo, fixes #4271 (#4391) * Fix ParityBar account selection overflows (#4405) * Available Dapp selection alignment with Permissions (Portal) (#4374) * registry dapp: make lookup use lower case (#4409) * Dapps use defaultAccount instead of own selectors (#4386) * Poll for defaultAccount to update dapp & overlay subscriptions (#4417) * Poll for defaultAccount (Fixes #4413) * Fix nextTimeout on catch * Store timers * Re-enable default updates on change detection * Add block & timestamp conditions to Signer (#4411) * Extension installation overlay (#4423) * Extension installation overlay * Pr gumbles * Spelling * Update Chrome URL * Fix for non-included jsonrpc * Extend Portal component (as per Modal) #4392

Poll for defaultAccount (Fixes #4413)

8c2be26

jacogr added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Feb 3, 2017

ngotchac suggested changes Feb 3, 2017

View reviewed changes

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

Fix nextTimeout on catch

2ecdd4a

jacogr added the B0-patch label Feb 3, 2017

jacogr added 2 commits February 3, 2017 13:47

Store timers

bf0db9d

Re-enable default updates on change detection

f4489fb

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

ngotchac approved these changes Feb 3, 2017

View reviewed changes

ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 3, 2017

jacogr merged commit f48a2df into master Feb 3, 2017

jacogr deleted the jg-defaultAccount-poll branch February 3, 2017 13:24

jacogr added a commit that referenced this pull request Feb 3, 2017

Poll for defaultAccount to update dapp & overlay subscriptions (#4417)

1e2c126

* Poll for defaultAccount (Fixes #4413) * Fix nextTimeout on catch * Store timers * Re-enable default updates on change detection

jacogr added a commit that referenced this pull request Feb 3, 2017

Poll for defaultAccount to update dapp & overlay subscriptions (#4417)

69a07f3

* Poll for defaultAccount (Fixes #4413) * Fix nextTimeout on catch * Store timers * Re-enable default updates on change detection

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Poll for defaultAccount to update dapp & overlay subscriptions #4417

Poll for defaultAccount to update dapp & overlay subscriptions #4417

jacogr commented Feb 3, 2017

ngotchac Feb 3, 2017

jacogr Feb 3, 2017

ngotchac Feb 3, 2017

jacogr Feb 3, 2017

ngotchac Feb 3, 2017 •

edited

Loading

jacogr Feb 3, 2017 •

edited

Loading

ngotchac Feb 3, 2017

ngotchac Feb 3, 2017

jacogr Feb 3, 2017

ngotchac Feb 3, 2017

jacogr Feb 3, 2017

ngotchac Feb 3, 2017

jacogr Feb 3, 2017

ngotchac Feb 3, 2017

jacogr Feb 3, 2017 •

edited

Loading

ngotchac Feb 3, 2017

jacogr Feb 3, 2017 •

edited

Loading

ngotchac Feb 3, 2017

jacogr Feb 3, 2017

ngotchac commented Feb 3, 2017

ngotchac commented Feb 3, 2017

jacogr commented Feb 3, 2017 •

edited

Loading

ngotchac left a comment

ngotchac Feb 3, 2017

jacogr commented Feb 3, 2017

		_defaultAccount = () => {
		const nextTimeout = (timeout = 1000) => {

Poll for defaultAccount to update dapp & overlay subscriptions #4417

Poll for defaultAccount to update dapp & overlay subscriptions #4417

Conversation

jacogr commented Feb 3, 2017

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

ngotchac Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

jacogr Feb 3, 2017 • edited Loading

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

jacogr Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac commented Feb 3, 2017

ngotchac commented Feb 3, 2017

jacogr commented Feb 3, 2017 • edited Loading

ngotchac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr commented Feb 3, 2017

ngotchac Feb 3, 2017 •

edited

Loading

jacogr Feb 3, 2017 •

edited

Loading

jacogr Feb 3, 2017 •

edited

Loading

jacogr Feb 3, 2017 •

edited

Loading

jacogr commented Feb 3, 2017 •

edited

Loading