feat/AddressBookIdentityProvider

[topocount]

This will allow identity badges to show the names stored for addresses in the address book.

One wrinkle:

• With the LocalIdentityProvider component users are able to easily add a preferred "custom label" on top of the label resolved from the address book provider. However, the address book labels still show as "Custom Label" in the LocalIdentityProvider When they should show as something like "Address Book Entry"

[chadoh]

A couple notes; I need to look through the rest of the new AddressBookIdentityProvider still

[chadoh]

Hard-coding known Address Book-type appIds seems like a good, quick short-term solution. In the long-term, if we kept that architecture, I guess the process for a new team to develop a full-featured new Address Book-type app would be:

1. Build their app
2. Publish it
3. Send a PR to aragon.js, adding their app's ID to this array

Has there been a decision around whether Aragon should have this sort of centralized review process, or whether it should be more of a permissionless system? If we want to make this permissionless someday, we will need a different architecture.

[topocount]

I think Aragon eventually wants to develop a way to detect third-party ID providers that don't need to be merged into their repo, so this would improve that process significantly

[chadoh]

Should we add a TODO for now, noting that this should eventually become a permissionless system?

[topocount]

This is the kind of thing that won't get developed until there's demand for it, either internally or externally due to this maintenance process becoming unsustainable. I'll update the note and if Brett wants it out, we'll remove it

[chadoh]

Some questions and ocmments on AddressBookIdentityProvider.

[chadoh]

Since calling modify is not supported for AddressBookIdentityProvider, let's update this comment. Maybe we can use it to explain how modify could end up being called, and why it's useful to have this error-throwing function as a safeguard.

[chadoh]

search and getAll look pretty nice! Here are some quick nitpicks to make them even nicer.

[chadoh]

If we use reduce, we can do the filter and map in one pass instead of two:

return Object.entries(await this.getAll()).reduce(
  (filtered, [address, { name }]) => {
    const found = (
      isAddressSearch &&
      searchTerm.length > 3 &&
      address.toLowerCase().indexOf(searchTerm.toLowerCase()) === 0
    ) || name.toLowerCase().indexOf(searchTerm.toLowerCase()) > -1
    return found ? [ ...filtered, { name, address } ] : filtered
  },
  []
)

Feel free to get rid of aspects of this proposed solution which are maybe too clever, and feel free to come up with better names for variables!

[topocount]

I think this is more of a style point, rather than of of efficiency. I'm actually a big fan of splitting these tasks up between filter and map because it's easier to read imo

[chadoh]

Given that we return an object ({ ...entriesObject, ...allEntriesResolved }), why do we need this? If we pass in a plain object to start, then we can avoid this, right?

[topocount]

We need this because the anonymous function is asynchronous and returns a Promise that needs to be resolved, even if we start with a plain object as the initial accumulator value

[chadoh]

I really like implicit returns for things like this:

const entriesObject = entries.reduce(
  (obj, entry) => ({ ...obj, [entry.addr.toLowerCase()]: entry.data }),
  {}
)

Separate question: do we want toLowerCase here?

[topocount]

I actually try to avoid implicit returns if the function is going to take up more than one line. I think they can lead to some unnecessary confusion.

[chadoh]

It's a little strange that getAll starts with an array and works to turn it into an object, just so search can do some work to turn it back into an array. Do we expect getAll to be used somewhere other than within search? That is, is it part of the external API of AddressBookIdentityProvider? If not, if it's only used in search, then we could keep it as an array here and do less work in search.

[chadoh]

I think it might be nice to also show a search of 0x3, which should return all of the entries.

[topocount]

This isn't possible because an address search won't return unless a full byte is entered in the search

Addressed applicable changes, and justified other decisions