Fix slow balances

[ngotchac]

Better fetching of the token balances.

1. The UI now makes one call for about 64 balances updates, thanks to this https://github.com/paritytech/contracts/pull/76
2. It caches the tokens info it got from the registry (only the META data can be changed, ie. the images)
3. A bunch of fixes on how the balances are retrieved.

[tomusdrw]

contacts[to] = contracts[to] || [];

[tomusdrw]

I think I would prefer reduce to altering the promise

[tomusdrw]

methods[log.method] = methods[log.method] || [];

[ngotchac]

I'm not sure how this would be better?

[tomusdrw]

Single line instead of 3 = less visual clutter and it's also a common pattern in JS

[tomusdrw]

Can be just returned directly.

[tomusdrw]

Should be removed.

[tomusdrw]

Fetching all images and all info looks a bit heavy.

I think we should further optimize it and only fetch the info/images of tokens that are actually displayed (but in a separate PR)

[ngotchac]

That's actually what's implemented ! :)

[tomusdrw]

Sorry, can't see where do we have it.
we take allTokens from state and then we divide them between the ones that are fully fetched and the ones that need partially fetched. And we get the images for the latter ones.
What I miss is where we only take tokens that user has in her account (i.e are actually displayed).

[ngotchac]

So it's in /src/redux/providers/balancesActions.js > fetchTokensBalances : the fetchTokens method is called only for the tokens we don't have meta-data/name yet.

loadTokensBasics in tokensActions is the method that is called on load, and that fetches only the tokens addresses. When the token cache is already filled, it actually only updates the images that might have changed since the last load.

[tomusdrw]

map/reduce here seems overcomplicated, tokenIds will always have either 0 or 1 element if I'm not mistaken:

const ethUpdates = accountAddresses
   // only accounts fetching eth 
  .filter(address => updates[address].find(tokenId => ETH_TOKEN.id))
  .reduce((nextUpdates, address) => {
     nextUpdates[address] = [ETH_TOKEN.id];
   }, {});

[tomusdrw]

This could be simplified as well:

const tokenUpdates = Object.keys(updates).reduce((nextUpdates, address) => {
   const tokens = updates[address].filter(tokenId => tokenId !== ETH_TOKEN.id);
   if (tokens.length) {
      nextUpdates[address] = tokens;
   }
   return nextUpdates;
}, {});

[tomusdrw]

const ethPromise = fetchEthBalances(api, ...);
const tokenPromise = Object.keys(tokenUpdates).reduce((promise, address) => {
  return promise.then(balances => {
     return fetchTokensBalances(api, updateTokens, ...).then(newBalances => {
       balances[address] = newBalances[address];
       return balances;
     });
  });
}, Promise.resolve({}));

return Promise.all([ethPromise, tokenPromise]).then(([ethBalances, tokensBalances]) => {
 ...
});

[tomusdrw]

Now that I wrote the code I think there is a bit of redundancy in calling fetchTokensBalances:

Why can't we do a single query for all tokens seen in any account and all accounts at once?
Something like:

const updateTokens = uniq(Object.values(tokenUpdates).reduce((all, ids) => all.concat(ids), []));
const accounts = Object.keys(tokenUpdates);
const tokenPromise = fetchTokensBalances(api, updateTokens, accounts);

?

[ngotchac]

So it's because not all accounts need to update the same tokens.

[tomusdrw]

Yes, but I'm wondering if a single call would not be more performant for users that more than one account and cause less convoluted code - I don't like that we use some scope variables to pass asynchronous results, it's really hard to read.

[ngotchac]

Yeah I agree that it's hard to read, and the whole thing could be simplified if we didn't really care about having a fine-tuned fetching. But I guess the scope of the PR was to be able to make just the right number of calls.

[tomusdrw]

In the future it might actually be better to display balances as they come instead of waiting for all of them.

[ngotchac]

Yeah, right now it's first the ETH balances, then tokens. But with Redux, the more we dispatch a new state, the more it re-renders a lot of Components...

[tomusdrw]

Why is that? It should only re-render the components that use the updated state, no? Maybe we should focus on improving performance of re-renders?

[ngotchac]

So I spoke/wrote too fast. It shouldn't re-render components, but will run the dispatcher methods and compare the results for every rendered components, so it's anyway better to do it once than multiple times.

[gavofyork]

is this still polling on the JS side? what about using filters?

[tomusdrw]

1. It uses filter for transfer even on any non-zero address token in the registry.
2. When the event is received we check if to or from is one of our accounts
3. If it is we refresh a balance of this particular token for affected accounts.

Another filter is set up to monitor the token registry and update the other filters if there are any changes there.

The main issue was with initial fetch that now is working much better:

1. The token ids are cached (so we don't need to query the token registry 600 times just to get the addresses)
2. Initial balances are fetched in chunks for all accounts - for each chunk of 64 tokens we produce eth_call that is able to fetch balances for all accounts. (Fetching all tokens at once takes too long and causes noticable lag).
3. The rest of polling methods is now using parity_pubsub, so we don't need to periodicaly check for:

• best block
• pending transactions (signer)
• nodeHealth
• peers
• syncStatus

That's from the top of my head, @ngotchac correct me if I'm wrong on anything.

[ngotchac]

Grumbles should be addressed!

[tomusdrw]

Some additional questions, but LGTM.

And the UX difference is yuge.

[tomusdrw]

I see. Fair enough.

[tomusdrw]

Sorry, can't see where do we have it.
we take allTokens from state and then we divide them between the ones that are fully fetched and the ones that need partially fetched. And we get the images for the latter ones.
What I miss is where we only take tokens that user has in her account (i.e are actually displayed).

[tomusdrw]

Yes, but I'm wondering if a single call would not be more performant for users that more than one account and cause less convoluted code - I don't like that we use some scope variables to pass asynchronous results, it's really hard to read.

[tomusdrw]

Why is that? It should only re-render the components that use the updated state, no? Maybe we should focus on improving performance of re-renders?