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

Poll for defaultAccount to update dapp & overlay subscriptions #4417

Merged
merged 4 commits into from
Feb 3, 2017
Merged

Poll for defaultAccount to update dapp & overlay subscriptions #4417

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c2be26
Poll for defaultAccount (Fixes #4413)
jacogr Feb 3, 2017
2ecdd4a
Fix nextTimeout on catch
jacogr Feb 3, 2017
bf0db9d
Store timers
jacogr Feb 3, 2017
f4489fb
Re-enable default updates on change detection
jacogr Feb 3, 2017
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
3 changes: 2 additions & 1 deletion js/src/api/subscriptions/eth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export default class Eth {
this._started = false;

this._lastBlock = new BigNumber(-1);
this._pollTimerId = null;
}

get isStarted () {
Expand All @@ -37,7 +38,7 @@ export default class Eth {

_blockNumber = () => {
const nextTimeout = (timeout = 1000) => {
setTimeout(() => {
this._pollTimerId = setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

this._blockNumber();
}, timeout);
};
Expand Down
16 changes: 12 additions & 4 deletions js/src/api/subscriptions/personal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export default class Personal {
this._started = false;

this._lastDefaultAccount = '0x0';
this._pollTimerId = null;
}

get isStarted () {
Expand All @@ -43,11 +44,13 @@ export default class Personal {
// doesn't work. Since the defaultAccount is critical to operation, we poll in exactly
// same way we do in ../eth (ala same as eth_blockNumber) and update. This should be moved
// to pub-sub as it becomes available
_defaultAccount = () => {
_defaultAccount = (timerDisabled = false) => {
const nextTimeout = (timeout = 1000) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ngotchac ngotchac Feb 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What about using local storage events then ?

Copy link
Contributor Author

@jacogr jacogr Feb 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

  1. UI is served from 8180
  2. Local dapps are served via :8080

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right

setTimeout(() => {
this._defaultAccount();
}, timeout);
if (!timerDisabled) {
this._pollTimerId = setTimeout(() => {
this._defaultAccount();
}, timeout);
}
};

if (!this._api.transport.isConnected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jacogr jacogr Feb 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jacogr jacogr Feb 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Not optimal to say the least.

Expand Down Expand Up @@ -109,6 +112,11 @@ export default class Personal {
case 'parity_setAccountMeta':
this._accountsInfo();
return;

case 'parity_setDappsAddresses':
case 'parity_setNewDappsWhitelist':
this._defaultAccount(true);
return;
}
});
}
Expand Down