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 1 commit
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
32 changes: 25 additions & 7 deletions js/src/api/subscriptions/personal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ export default class Personal {
this._api = api;
this._updateSubscriptions = updateSubscriptions;
this._started = false;

this._lastDefaultAccount = '0x0';
}

get isStarted () {
Expand All @@ -37,12 +39,33 @@ export default class Personal {
]);
}

// FIXME: Because of the different API instances, the "wait for valid changes" approach
// 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 = () => {
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(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stored in both case now. (Unused)

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.

nextTimeout(500);
return;
}

return this._api.parity
.defaultAccount()
.then((defaultAccount) => {
this._updateSubscriptions('parity_defaultAccount', null, defaultAccount);
});
if (this._lastDefaultAccount !== defaultAccount) {
this._lastDefaultAccount = defaultAccount;
this._updateSubscriptions('parity_defaultAccount', null, defaultAccount);
}

nextTimeout();
})
.catch(nextTimeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

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

case 'parity_setDappsAddresses':
case 'parity_setNewDappsWhitelist':
this._defaultAccount();
return;
}
});
}
Expand Down
3 changes: 3 additions & 0 deletions js/src/api/subscriptions/personal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ function stubApi (accounts, info) {

return {
_calls,
transport: {
isConnected: true
},
parity: {
accountsInfo: () => {
const stub = sinon.stub().resolves(info || TEST_INFO)();
Expand Down