Don't pop-up notifications after network switch #4076

ngotchac · 2017-01-06T17:22:06Z

Closes #4049

Also close notifications when clicked, and focus on the UI window/tab.
Better network switching handling.

derhuerst · 2017-01-06T17:26:53Z

There's duplicated logic with chainMiddleware.js now. Would be nice to get this unified into a newChain action.

ngotchac · 2017-01-06T17:56:33Z

@derhuerst Ok will try to hook up there after #4077 gets merged

ngotchac · 2017-01-09T18:16:40Z

Hooked up to the chain middleware now.

Rewrite a part of the status and connection logic. Better handling of the disconnect and connect events (don't poll anymore when disconnected, but wait for the api transport layer to emit an open event).

jacogr · 2017-01-10T10:18:54Z

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

@@ -131,6 +131,8 @@ export default class Ws extends JsonRpcBase {
    this._autoConnect = true;
    this._retries = 0;

+    this.emit('open');


Only ws? We have 2 transports, needs to be in-sync.

Additionally, since it is done on the API layer, we need to have tests that prove that is does what it is supposed to for all the transports.

Well the HTTP layer will never close nor open right (or at each request)?

It still has the isConnected & connected property. Basically when connected is set to true it should emit open, when set to false (errors), it should be emit false.

(Since we actually have the polling to say connected/not-connected the emitter will actually add value)

jacogr · 2017-01-10T10:20:20Z

js/src/redux/providers/balancesActions.js

            const account = accounts[address];
            const txValue = value.minus(oldValue);

            const redirectToAccount = () => {
-              const route = `/account/${account.address}`;
+              const route = `/accounts/${account.address}`;


Picked this up last night as well. I guess we actually need to externalise the route definitions somehow so we don't miss these again in the future.

Might be worth it yes. Note that this route is still working

jacogr · 2017-01-10T10:22:20Z

js/src/redux/providers/status.js

    this._subscribeBlockNumber();
-    this._pollStatus();
-    this._pollLongStatus();
+    this._pollLongStatus(true);


In theory (before I run this), we should now detect the chain change basically immediately? (Instead of the long delay as it is in current master.)

Yep it should be immediate, and the connecting overlay should stay until the node is really ready.

jacogr · 2017-01-10T10:24:06Z

js/src/redux/providers/status.js

    const gotConnected = !this._apiStatus.isConnected && apiStatus.isConnected;

    if (gotConnected) {
-      this._pollLongStatus();
+      this._pollLongStatus(gotConnected);


Tiny grumble - not convinced I like the got naming here. Would prefer a is or has especially since this one had me spinning a bit (just not well-known/consistent)

jacogr · 2017-01-10T10:24:38Z

js/src/redux/providers/status.js

@@ -173,23 +220,23 @@ export default class Status {
   * fetched every 30s just in case, and whenever
   * the client got reconnected.
   */
-  _pollLongStatus = () => {
+  _pollLongStatus = (gotConnected = false) => {


Same here with the got naming.

jacogr · 2017-01-10T10:27:26Z

js/src/secureApi.js

@@ -40,6 +42,10 @@ export default class SecureApi extends Api {
    this._tryNextToken();
  }

+  get transport () {


Is there any reason we are re-defining this here? Api has both this._transport as well as get transport already and SecureApi only extends Api.

Nope, will delete

coveralls · 2017-01-10T11:33:36Z

Changes Unknown when pulling 316da79 on ng-notifications into ** on master**.

ngotchac · 2017-01-10T12:16:59Z

Added events to HTTP and the corresponding tests

jacogr · 2017-01-10T12:29:24Z

js/src/api/transport/http/http.js

@@ -46,19 +46,33 @@ export default class Http extends JsonRpcBase {
    };
  }

+  _setConnected () {


I think these should have been in the JsonRpc base - so both implementations are consistent with the flags they set and the events they emit (at least the base flags, not the internal extensions).

ngotchac · 2017-01-10T16:43:26Z

Ok so for clarifications : the PR is cleaning a bit how balances are fetched when the API is disconnected. The behavior needed for this PR to actually work (ie. not showing notifications on network change) is that nothing gets fetched when the node is disconnected, and the first fetch on reconnection should be initiated by the chainMiddleware that is aware of a network change and thus can fetch balances without showing notifications.

So, in order to be able to do that, the Redux status logic needed some change, ie. stop polling when the connection is closed. That's why I had to introduce close (and thus open) events to the API transport layer, so that we can stop polling after the close event is emitted.

Now, without polling all the time the API status (which is no more needed, and is actually better), the signer needs a cleanup so that it can show the proper overlay on disconnect, etc. That's why all these 3 parts of the UI have been touched.

coveralls · 2017-01-10T19:14:45Z

Changes Unknown when pulling fccec02 on ng-notifications into ** on master**.

coveralls · 2017-01-10T21:07:29Z

Changes Unknown when pulling fccec02 on ng-notifications into ** on master**.

jacogr · 2017-01-11T16:13:46Z

js/src/secureApi.js

+   */
+  _waitUntilNodeReady () {
+    return this
+      .parity.enode()


My big concern with this is that I have seen cases where this never gets a valid value. Parity appears to be running, however enode is not set. (Parity core issue no doubt, may have been fixed, but still a concerns)

Ok, true. What about a mac response time (eg. 10s) after which it's get validated anyhow...

It is a tough one, since from the UI perspective, everything works even without the enode. Well, it appears working...

I suppose my preference would have been an "enode not ready" status warning, instead of waiting for it - not to be done here and now though, that check just has me slightly uncomfortable...

jacogr · 2017-01-11T16:16:15Z

js/src/secureApi.js

 const sysuiToken = window.localStorage.getItem('sysuiToken');

 export default class SecureApi extends Api {
+


Extra blank line.

jacogr · 2017-01-11T16:16:23Z

js/src/secureApi.js

+  _isConnecting = false;
+  _needsToken = false;
+  _tokens = [];
+


extra blank line.

ngotchac · 2017-01-12T00:18:38Z

Finally got through with this. Had to rewrite some of the Tokens Balances fetching logic, which was a bit messy and inefficient (called multiple times on load, sometimes it would take a new block for the tokens to be shown). Now everything seems to work fine, no notifications on network switch, correct updating of the tokens. We might want to tackle a performance issue that can be seen on the main net (in another PR). We could implement a way to only fetch the tokens balances for which the UI addresses (accounts, address book, contracts) have received a Transfer() event (one call thanks to eth_getLogs).

* Better notifications * Don't pollute with notifs if switched networks * Better connection close/open events / No more notifs on change network * PR Grumbles * Add close and open events to HTTP // Add tests * Fix tests * WIP Signer Fix * Fix Signer // Better reconnection handling * PR Grumbles * PR Grumbles * Fixes wrong fetching of balances + Notifications * Secure API WIP * Updated Secure API Connection + Status * Linting * Linting * Updated Secure API Logic * Proper handling of token updates // Fixing poping notifications * PR Grumbles * PR Grumbles * Fixing tests

* Fix broken transfer total balance (#4127) * Add proper label to method decoding inputs (#4136) * Another minor estimation fix (#4133) * Return 0 instead of error with out of gas on estimate_gas * Fix stuff up. * Another estimate gas fix. * Alter balance to maximum possible rather than GP=0. * Only increase to amount strictly necessary. * Get rid of unsafe code in ethkey, propagate incorrect Secret errors. (#4119) * Implementing secret * Fixing tests * Refactor VoteCollector (#4101) * dir * simple validator list * stub validator contract * make the engine hold Weak<Client> instead of IoChannel * validator set factory * register weak client with ValidatorContract * check chain security * add address array to generator * register provider contract * update validator set on notify * add validator contract spec * simple list test * split update and contract test * contract change * use client in tendermint * fix deadlock * step duration in params * adapt tendermint tests * add storage fields to test spec * constructor spec * execute under wrong address * create under correct address * revert * validator contract constructor * move genesis block lookup * add removal ability to contract * validator contract adding validators * fix basic authority * validator changing test * more docs * update sync tests * remove env_logger * another env_logger * cameltoe * hold EngineClient instead of Client * return error on misbehaviour * nicer return * sprinkle docs * Reenable mainnet update server. (#4137) * basic tests for subscribeToEvents (#4115) * subscribeToEvent fixtures ✅ * subscribeToEvent tests ✅ * temporarily skip failing test (#4138) * Improvements and optimisations to estimate_gas (#4142) * Return 0 instead of error with out of gas on estimate_gas * Fix stuff up. * Another estimate gas fix. * Alter balance to maximum possible rather than GP=0. * Only increase to amount strictly necessary. * Improvements and optimisations to estimate_gas. - Introduce proper error type - Avoid building costly traces * Fix tests. * Actually fix testsActually fix tests * Use estimateGas error (as per updated implementation) (#4131) * Use estimateGas error (as per updated implementation) * EXCEPTION_ERROR as per #4142 * Better error log reporting & handling (#4128) * Don't pop-up notifications after network switch (#4076) * Better notifications * Don't pollute with notifs if switched networks * Better connection close/open events / No more notifs on change network * PR Grumbles * Add close and open events to HTTP // Add tests * Fix tests * WIP Signer Fix * Fix Signer // Better reconnection handling * PR Grumbles * PR Grumbles * Fixes wrong fetching of balances + Notifications * Secure API WIP * Updated Secure API Connection + Status * Linting * Linting * Updated Secure API Logic * Proper handling of token updates // Fixing poping notifications * PR Grumbles * PR Grumbles * Fixing tests * Trim spaces from InputAddress (#4126) * Trim spaces for addresses * onSubmit has only value, not event * onSubmit (again) * Length check on trimmed value * Remove bindActionCreators({}, dispatch) (empty) (#4135)

ngotchac added 2 commits January 6, 2017 17:55

Better notifications

719a1fa

Don't pollute with notifs if switched networks

c6f18f4

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 6, 2017

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

ngotchac added 2 commits January 9, 2017 18:06

Merge branch 'master' into ng-notifications

983dcf1

Better connection close/open events / No more notifs on change network

cce9225

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

jacogr reviewed Jan 10, 2017

View reviewed changes

PR Grumbles

316da79

ngotchac added 2 commits January 10, 2017 13:15

Add close and open events to HTTP // Add tests

bdeea95

Fix tests

8f71a02

jacogr reviewed Jan 10, 2017

View reviewed changes

jacogr added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 10, 2017

ngotchac added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jan 10, 2017

jacogr removed the A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. label Jan 10, 2017

WIP Signer Fix

bf28bcb

PR Grumbles

e088a43

ngotchac added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 10, 2017

Fixes wrong fetching of balances + Notifications

fccec02

ngotchac added 6 commits January 11, 2017 12:18

Secure API WIP

42554de

Merge branch 'master' into ng-notifications

101ee24

Updated Secure API Connection + Status

1489544

Linting

1dbc434

Linting

580782d

Updated Secure API Logic

9eda90c

jacogr reviewed Jan 11, 2017

View reviewed changes

js/src/secureApi.js

_isConnecting = false;

_needsToken = false;

_tokens = [];

Copy link

Contributor

jacogr Jan 11, 2017

Choose a reason for hiding this comment

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

extra blank line.

ngotchac added 4 commits January 12, 2017 01:04

Proper handling of token updates // Fixing poping notifications

f589df6

PR Grumbles

7c5b8e1

PR Grumbles

5c097ff

Merge branch 'master' into ng-notifications

61637d2

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

Fixing tests

db67df5

jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 12, 2017

jacogr merged commit 81beec1 into master Jan 12, 2017

jacogr deleted the ng-notifications branch January 12, 2017 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't pop-up notifications after network switch #4076

Don't pop-up notifications after network switch #4076

ngotchac commented Jan 6, 2017

derhuerst commented Jan 6, 2017

ngotchac commented Jan 6, 2017

ngotchac commented Jan 9, 2017

jacogr Jan 10, 2017

ngotchac Jan 10, 2017 •

edited

jacogr Jan 10, 2017 •

edited

jacogr Jan 10, 2017

ngotchac Jan 10, 2017

jacogr Jan 10, 2017

ngotchac Jan 10, 2017

jacogr Jan 10, 2017

jacogr Jan 10, 2017

jacogr Jan 10, 2017

ngotchac Jan 10, 2017

coveralls commented Jan 10, 2017

ngotchac commented Jan 10, 2017

jacogr Jan 10, 2017

ngotchac commented Jan 10, 2017

coveralls commented Jan 10, 2017

coveralls commented Jan 10, 2017

jacogr Jan 11, 2017

ngotchac Jan 11, 2017

jacogr Jan 11, 2017 •

edited

jacogr Jan 11, 2017

jacogr Jan 11, 2017

ngotchac commented Jan 12, 2017

		const sysuiToken = window.localStorage.getItem('sysuiToken');

		export default class SecureApi extends Api {

Don't pop-up notifications after network switch #4076

Don't pop-up notifications after network switch #4076

Conversation

ngotchac commented Jan 6, 2017

derhuerst commented Jan 6, 2017

ngotchac commented Jan 6, 2017

ngotchac commented Jan 9, 2017

Choose a reason for hiding this comment

ngotchac Jan 10, 2017 • edited

Choose a reason for hiding this comment

jacogr Jan 10, 2017 • edited

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

coveralls commented Jan 10, 2017

ngotchac commented Jan 10, 2017

Choose a reason for hiding this comment

ngotchac commented Jan 10, 2017

coveralls commented Jan 10, 2017

coveralls commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Jan 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac commented Jan 12, 2017

ngotchac Jan 10, 2017 •

edited

jacogr Jan 10, 2017 •

edited

jacogr Jan 11, 2017 •

edited