Normalize and require keyholder address

[rekmarks]

Terminology

What I call the "keyholder" address has previously been referred to as the "from" address and "sender" address. "from" and "sender" are misnomers, because the address simply needs to refer to an account controlled by MetaMask, whose private key is needed to perform some operation. Sometimes, this address is the recipient of a message. Other times, it is the sender/signer of a message.

Thus, "keyholder."

Summary of Changes

• Require keyholder addresses wherever they are required in practice
• Validate and normalize keyholder addresses whenever they are provided

Changes in Detail

• Require keyholder addresses wherever they are required in practice
• Validate and normalize keyholder addresses whenever they are provided
  • validateSender has been renamed to validateAndNormalizeKeyholder, and fulfills both purposes
  • Previously, the address would only be validated if provided. A valid address is now required.
  • Previously, the address would be normalized (i.e. lowercased) during validation without being overwritten. We now pass the normalized address to the extension if validation succeeds.
    • Note that we don't overwrite the parameters themselves; we only normalize the address value we use internally to identify the keyholder account.
• Update tests to reflect the above changes. All tests pass.

This PR will be followed by standardization PRs and a major version bump.

[Gudahtt]

This seems to no longer be allowed. I'm not entirely sure yet which methods had allowed the "sender" address to be omitted, but requiring it all of a sudden seems like a breaking change.

[rekmarks]

I was trying to figure out whether we actually inserted a "default" as the comment suggests but couldn't find any examples. I'm going to run some experiments and see if you can request any of these RPC methods without specifying a keyholder address.

[rekmarks]

After testing develop and prod, I can confirm that we reject any request without a keyholder address for the wallet middleware RPC methods that require a keyholder address.

This includes eth_sendTransaction, where a default from address is injected by web3 in our test dapp. We do not fallback to a default address in prod or develop.

Similarly, all signature methods fail in production if a keyholder address is not specified. Our UI handles this very poorly, so rejecting them in this middleware layer will circumvent that problem entirely.

[Gudahtt]

There are a lot of distinct, functional changes in here. In addition to the two in the description, there is also the change in the from address to now be required (as mentioned in a comment), and the removal of the unused processTypedMessageV0, and other unrelated refactoring. These should be in separate PRs.

We need to fix this bug ASAP to get this release out, so we need to both understand why the bug is happening in the first place (i.e. who was responsible for normalizing the address in prod? Why does it work there?), and fixing that specific problem. These other changes are introducing further uncertainty, making review more difficult, and degrading the quality of the git history.

[Gudahtt]

LGTM!

I have tested and traced through each of these methods, to check whether they supported a missing from address in prod.

The typed messages all verify at least that from is a string, and this middleware was validating that any truthy string passed in was a valid address. Passing in an empty string gets past this validation, but it still blows up with the message "No keyring found for the requested account" in the keyring controller signing method. The other signing methods lack similar validation, but also blow up at the same spot in the keyring controller. signTransaction blows up in the transaction controller with the message "Transaction from address isn't valid for this account". So all of these methods blow up without an address, in various different ways. The impact of this PR is to give a consistent error message across all of them.

Normalizing the from address seems sound as well. For the signing methods, the from address not being normalized wasn't an issue because it was only used in the keyring controller (which handles normalization itself). The UI wasn't using it - it would always show the selectedAddress (which @rekmarks fixed in MetaMask/metamask-extension#8079 ). The normalization now ensures that the UI doesn't crash in attempting to find the identity associated with the given address.

For transactions, the from address is already normalized in the extension. So that normalization is now redundant. We may want to consider following this up with a PR in the extension to remove the now-redundant normalization when this package is bumped.

While both these changes seem fine, this would still be better split into two separate PRs. Making the from address required is not a release-blocker, and does have a mixed impact (it results in better error handling in some cases, but arguably slightly worse in some others). It's functionally distinct from the normalization, relies upon different assumptions, and could fail in wholly different ways. The normalization is all that we strictly need for v8.