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

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

Merged
merged 24 commits into from
Jan 12, 2017
Merged

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Jan 6, 2017

Closes #4049

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

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 6, 2017
@derhuerst
Copy link
Contributor

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

@ngotchac 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
Copy link
Contributor Author

ngotchac commented Jan 6, 2017

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

@ngotchac 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
@ngotchac
Copy link
Contributor Author

ngotchac commented Jan 9, 2017

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

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

this.emit('open');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ngotchac ngotchac Jan 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jacogr jacogr Jan 10, 2017

Choose a reason for hiding this comment

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

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)

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

const redirectToAccount = () => {
const route = `/account/${account.address}`;
const route = `/accounts/${account.address}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this._subscribeBlockNumber();
this._pollStatus();
this._pollLongStatus();
this._pollLongStatus(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

if (gotConnected) {
this._pollLongStatus();
this._pollLongStatus(gotConnected);
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

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

Choose a reason for hiding this comment

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

Same here with the got naming.

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

get transport () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, will delete

@coveralls
Copy link

Coverage Status

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

@ngotchac
Copy link
Contributor Author

Added events to HTTP and the corresponding tests

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

_setConnected () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jacogr 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 ngotchac added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jan 10, 2017
@jacogr jacogr removed the A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. label Jan 10, 2017
@ngotchac
Copy link
Contributor Author

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.

@ngotchac 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
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

*/
_waitUntilNodeReady () {
return this
.parity.enode()
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jacogr jacogr Jan 11, 2017

Choose a reason for hiding this comment

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

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

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

export default class SecureApi extends Api {

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra blank line.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

extra blank line.

@ngotchac 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
@ngotchac
Copy link
Contributor Author

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

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 12, 2017
@jacogr jacogr merged commit 81beec1 into master Jan 12, 2017
@jacogr jacogr deleted the ng-notifications branch January 12, 2017 13:25
arkpar pushed a commit that referenced this pull request Jan 12, 2017
* 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
arkpar added a commit that referenced this pull request Jan 12, 2017
* 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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After switching networks balance notifications pop up
4 participants