Use Identity instead of index for ACCOUNT_SWITCH and ACCOUNT_REMOVE

[chrisbobbe]

This turned out to be independent of #5633.

[gnprice]

Thanks for spotting this issue and the fix!

I've read through the first two commits:
469eb65 accounts reducer tests: Make ACCOUNT_REMOVE test a bit stronger
bc8d444 accountsReducer: Invariant that ACCOUNT_REMOVE's index is in bounds

and partway through the next:
f196eb4 account actions: Have removeAccount take an Identity, not an index

Just one comment so far, below. I'll return to read the remainder of the branch when I'm back at work next week.

[gnprice]

This doesn't benefit from memoization (i.e. from createSelector) — the callback we pass to createSelector is not doing any work, just immediately returning a function expression.

Removing that will simplify things. Then I think let's simplify further by uncurrying, so that this is just a function on two arguments rather than a function on one argument that returns a function on the other argument.

[chrisbobbe]

Makes sense. I think I was looking at this other selector when writing this one:

/**
 * All known accounts, indexed by identity.
 */
export const getAccountsByIdentity: GlobalSelector<(Identity) => Account | void> = createSelector(
  getAccounts,
  accounts => {
    const map = new Map(
      accounts.map(account => [keyOfIdentity(identityOfAccount(account)), account]),
    );
    return identity => map.get(keyOfIdentity(identity));
  },
);

That one is "curried" too, right? I wonder if there's a way to simplify that one, while still enforcing that callers pass the right kind of thing to identify the account (whether an Identity or a keyOfIdentity string). Anyway, not worth spending too much time on.

[gnprice]

That one has the type of a curried function, in that it's of the form A => (B => C) (whereas the uncurried counterpart of that type would be (A, B) => C.

But the implementation has more going on. An implementation that's purely curried without more happening would look like:

export const getAccountsByIdentity: GlobalState => ((Identity) => Account | void) = state => identity => {
  const accounts = getAccounts(state);
  const map = new Map(
    accounts.map(account => [keyOfIdentity(identityOfAccount(account)), account]),
  );
  return map.get(keyOfIdentity(identity));
};

The actual implementation using createSelector, though, does a cache lookup before the new Map step. Because the new Map represents doing some work (constructing a new data structure, vs. just looking up properties in existing objects and doing simple boolean tests), that caching is potentially useful.

[chrisbobbe]

Thanks for that review comment! Revision pushed, addressing it by rewriting getIsActiveAccount as you described.

[gnprice]

Thanks for the revision!

Finished reading the branch. Generally all looks good — I appreciate the careful reasoning in the commits that add invariants. One comment below, on the NFC last commit.

[gnprice]

I think the ideal version of this component wouldn't use any Redux selectors, so that it's just a plain piece of UI that takes the data it's given via props and presents a UI for it.

That way an ancestor component, here AccountPickScreen, is responsible for putting together the needed data — basically a view-model — and passing it down. This is the arrangement the existing code in main has.

It'd be a little bit annoying to refactor things so that the new version of this component can be pure in that sense. Really what's happening is that the job of getAccountStatuses is to prepare the view-model for this screen, and so we'd want to make a renamed version of that selector that's more explicitly aimed at that, and then cheerfully add to it whatever other data this screen comes to need, like identifying which account is active. (This is a bit annoying because getAccountStatuses has grown one other call site. That call site doesn't actually use isLoggedIn, though; so it should just call getIdentities instead.)

At a minimum, though, we can refrain from refactoring to give this new dependencies on Redux for data it already had. I.e., we can let this component keep taking an AccountStatus (and AccountList keep taking an array of those), even if we add a useGlobalSelector call to get isActiveAccount.

[gnprice]

Sent a quick PR #5649 to do part of this refactoring, reframing getAccountStatuses as that view-model.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks for the revision! Looks good; merging.