Resolve local names for each network

[jagodarybacka]

Resolves #2448

What

Local names of addresses should be global so they should be the same regardless of the selected network.

• make custom address change global, regardless of network
• select "edit name" input when the slideup opens
• fix placement of error message on the "edit name" input

Screen.Recording.2022-11-07.at.17.02.30.mov

Testing

• add account and change it's name
• check if this custom name is visible regardless what network is selected
• check if you can send tokens to this address using custom name
• check if custom name is visible in the UI instead of ENS/UNS/address

Latest build: extension-builds-2555 (as of Thu, 06 Apr 2023 14:55:09 GMT).

[Shadowfiend]

Hmmm… If I'm not mistaken, the same address on RSK is not controlled by the same keys, as far as I understand it. So we can't treat them as the same from a naming perspective either, otherwise there are potentially ways for a user to be tricked into sending something to a known address that isn't actually a known address on a given network.

I feel like we could address this with a little extra safety and a little less overall complexity by changing sameAddressBookEntry to look something like:

const sameAddressBookEntry = (a: AddressOnNetwork, b: AddressOnNetwork) =>
   normalizeEVMAddress(a.address) === normalizeEVMAddress(b.address) &&
   networksShareAddresses(a.network, b.network)

Where we define a new function, networksShareAddresses , alongside sameNetwork, which might roughly look like:

/**
 * Tests whether two networks share addresses; that is, if the same address on
 * both networks is controlled by the same keys.
 */
export function networksShareAddresses(
  network1: AnyNetwork,
  network2: AnyNetwork
): boolean {
  if (sameNetwork(network1, network2)) {
    return true
  }

  return network1.family === network2.family &&
    ADDRESS_EQUIVALENT_NETWORK_GROUPS.some((group) =>
      group.has(network1.chainID) && group.has(network2.chainID))
}

Then we have the last piece, in constants/networks.ts:

/**
 * An array of groups of address-equivalent networks, represented by their chain
 * IDs. Each set of chain IDs represents a group of networks on which the same
 * address is controlled by the same keys. For example, the same address on Polygon
 * and on Ethereum mainnet is controlled by the same keys in the wallet. The same
 * address on Ethereum mainnet and RSK might not be.
 */
export const ADDRESS_EQUIVALENT_NETWORK_GROUPS: Set<string>[] = [
  new Set(ETHEREUM.chainID, POLYGON.chainID, ...)
]

Since we already resolve names in a network-aware way, this would let name resolution proceed without any modifications for local networks. Since we already resolve addresses in a network-aware way, name resolution could again continue to work as expected. Finally, if we added a network with an equivalent address in the future, I believe this should allow the name resolution to function completely normally as well. In general I think it'll allow fewer special cases across the wallet by encapsulating the shared nature of addresses very close to the network concept, instead of applying specialized code in name resolution to handle it.

Thoughts?

[mhluongo]

Hmmm… If I'm not mistaken, the same address on RSK is not controlled by the same keys, as far as I understand it.

This can be true on account abstraction chains, but I'm not sure it's true on RSK... instead, the same recovery phrase in their ecosystem will yield different addresses due to the different default derivation path? Would be good to get confirmation here.

[Shadowfiend]

the same recovery phrase in their ecosystem will yield different addresses due to the different default derivation path

This ostensibly means the same thing though, right? Though I suppose the attack vector here requires brute forcing an attack address.

[jagodarybacka]

@Shadowfiend can you point to their docs where you found this? I'm digging and I can't find anything about it.

If this is the case with keys/addresses then I think we have a much bigger problem - adding new addresses for the same imported seed phrase is adding addresses that are valid for ETH but not for RSK?

[Shadowfiend]

Possible I'm being philosophical instead of practical here; I think the thing to do here is check in with the RSK folks and see if it makes sense to them that if I add an Ethereum seed phrase I can then switch to RSK and use it as-is, without generating a different address.

The flip possibility here is that you add an RSK address using the RSK derivation path, and then switch to Ethereum and use that. This will "work", but if you try to import that same seed phrase or key in another Ethereum wallet, you most likely won't be able to generate that address without putting in a custom derivation path. Not sure how big of a concern this is, but it's one of the edge cases of multi-network support when different derivation paths are “normal” for different networks.

[Shadowfiend]

As a side note, if we don't care about the RSK distinction, can't we just redefine sameAddressBookEntry to ignore network and drop all the other logic changes in the name service?

[jagodarybacka]

sameAddressBookEntry is one part but then we had names service's cache and it was not working correctly (case: change local name twice) - I've dropped saving local names here because they are "cached" anyway in the address book which contains most recent names set by user so it's better to keep one source of data IMO.

[jagodarybacka]

@ahsan-javaiid can you please take a look at the conversation we have here? Do we need to be worried about different derivation path on RSK?

[Shadowfiend]

That concern may be more real than I expected depending on how the ethereum app and RSK app on ledger interact with these addresses, I realize now 😅 Lot of interactions we need to test.

[alepc253]

@Shadowfiend the possibility of using the derivation path of another chain is a feature in many wallets and a useful one under certain contexts, example: you added RSK as custom rpc in Metamask and you got an address after applying the ETH derivation path by default (and only option, in fact). This user will only get the same account in a wallet that integrates correctly RSK if this feature is possible.
MyCrypto as example:

[alepc253]

@Shadowfiend with Ledger is different because the apps only work with the right derivation path (so the only way to get an ETH address is by connecting to the ETH app and sending the ETH dpath as parameter)

[alepc253]

@jagodarybacka @Shadowfiend in summary, I don't think this is something to worry or a blocker. cc @ahsan-javaiid

[Shadowfiend]

in summary, I don't think this is something to worry or a blocker.

Out of office rn so can't go into detail, but my conclusion is the opposite here: we do need to handle accounts that have different characteristics on different networks, and that may need to be listed on one network but not another, because of the Ledger limitation. If we do that we should probably apply it to generated/imported accounts as well to avoid unexpected behavior if eg a user chooses to move a Tally seed phrase to a ledger (even though this isn't necessarily recommended).

Another option would be deciding which Ledger app to request the user opens based on derivation path, but then we could end up with a user being prompted to open the RSK app to sign an ethereum transaction because they got confused.

Cc @mhluongo here for more thoughts while I'm out.

[alepc253]

@Shadowfiend I read the whole thread again and the possibility of restoring the seed into Ledger (I missed that scenario) and now I see more clearly your concerns.
I focused on an edge case -and use it as a feature- but not on the more frequent one (the video at the beggining of the thread) so, yes, I agree that the approach that would get less frictions would be deriving the seed with the right network derivation path.
This approach (deriving with the right dpaths when changing network) is the one we applied with Nifty wallet (now discontinued) and it worked.
About the naming -and if it helps-, it could be the same account name if we associate it with the address index of the dpaths, what do you think?

[alepc253]

@Shadowfiend if the user goes with the "import recovery phrase" option, he/she consciously selects the derivation path that wants to apply so this is a flow where the impact is minimum. Maybe it's more concerning for the "create new wallet" flow.

[ahsan-javaiid]

Related: #2673

[hyphenized]

Code looks good. I think the concern related to the derivation path is valid but we should track it in a different issue as it is currently only a problem for RSK, and, also seems more of a product decision that's being partially addressed in #2673. Approving but leaving up in case @greg-nagy wants to give it another pass.

[hyphenized]

Suggested change

	// if there is no loacal name then look deeper
	// If there is no local name then look deeper

[hyphenized]

We can omit this because of the previous check right?

[hyphenized]

Suggested change

	// emmit local names without a network and update all address networks paird in the redux?
	// Emit local names without a network and update all address network pairs in Redux

[Shadowfiend]

Latest state of my thinking on this PR:

• My concerns still largely stand. I still think my original proposed path is a cleaner approach, though it's ultimately a product decision whether we want to take that path or not. The fact that a Ledger using the RSK app cannot control the same address as the same Ledger on the ETH app is the biggest thing that makes me twitchy about the current approach—but that points to a deeper need to reconsider Ledger and indeed account display across networks, and reaches beyond this very specific change.
• The current approach feels like it isn't enough to handle local names on newly-added networks, which I assume the custom networks work is going to introduce—could be wrong as I'm less familiar with that side of the codebase at this point.
• The modified sameAddressBook approach I suggested feels like it would also allow us to handle custom networks more or less transparently, by assuming that they are all effectively equivalent addresses.

Basically I'm still in “we should modify the approach here”, but with weak conviction, and if we land this as-is I think that's fine. I think there's still some stuff lurking around account handling for networks with non-Ethereum derivation paths (Bitcoin, RSK, etc) that will trigger further revisiting this stuff if and when more work is put in on that front.

[alepc253]

@hyphenized @jagodarybacka what do you think? would you move forward with this approach? Any further changes that we can help with?

[Shadowfiend]

Picking this up hopefully this week based on the approach outlined in RFB 5.