Restore previous behavior of toChecksumHexAddress #4046

mcmire · 2024-03-12T22:19:50Z

Explanation

Prior to 40acc6c, toChecksumHexAddress in @metamask/controller-utils would not throw when given undefined or null. Now it does, because addHexPrefix from ethereumjs-util has been replaced with add0x from @metamask/utils, and the latter is more strict when it comes to input.

This change is causing some tests on the extension side to fail. Granted, these tests are likely creating an incomplete state object, so they ought to be fixed so that they pass a string to toChecksumHexAddress. However, these test failures serve as a reminder that there may be other parts of the extension codebase not covered by tests which are using toChecksumHexAddress incorrectly. If the extension were using TypeScript throughout, finding these problem areas would be trivial, because we would have seen type errors already. But because the extension codebase is still primarily written in JavaScript, we cannot guarantee that toChecksumHexAddress won't throw for some particular use case even after we fix the obvious usages.

Therefore, to prevent unexpected runtime errors, this commit restores the existing behavior of toChecksumHexAddress.

References

We've been talking about this in our company chat but there are also a couple of PRs open on the extension side which get around the issue described above. This is a more direct fix.

Changelog

`@metamask/controller-utils`

FIXED: Allow toChecksumHexAddress to take and handle non-string inputs again, which was removed in 8.0.4.

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate

Prior to 40acc6c, `toChecksumHexAddress` in `@metamask/controller-utils` would not throw when given `undefined` or `null`. Now it does, because `addHexPrefix` from `ethereumjs-util` has been replaced with `add0x` from `@metamask/utils`, and the latter is more strict when it comes to input. This change is causing some tests on the extension side to fail. Granted, these tests are likely creating an incomplete state object, so they ought to be fixed so that they pass a string to `toChecksumHexAddress`. However, these test failures serve as a reminder that there may be other parts of the extension codebase not covered by tests which are using `toChecksumHexAddress` incorrectly. If the extension were using TypeScript throughout, finding these problem areas would be trivial, because we would have seen type errors already. But because the extension codebase is still primarily written in JavaScript, we cannot guarantee that `toChecksumHexAddress` won't throw for some particular use case even after we fix the obvious usages. Therefore, to prevent unexpected runtime errors, this commit restores the existing behavior of `toChecksumHexAddress`.

packages/controller-utils/src/util.ts

legobeat · 2024-03-13T01:19:44Z

packages/controller-utils/src/util.ts

 * @returns A 0x-prefixed hexidecimal checksummed address, if address is valid. Otherwise original input 0x-prefixe, if address is valid. Otherwise original input 0x-prefixed.
 */
 export function toChecksumHexAddress(address: string) {


I do think that if undefined input is to be considered as part of implementation, it should be reflected in types as well? At the same time I see using them to steer towards intended usage (perhaps at some point in the future, it's a usage that can be phased out?)

So what do you think about adding undefined (should that be made wider? any, even?) as an overload but marking it as deprecated?

Suggested change

* @returns A 0x-prefixed hexidecimal checksummed address, if address is valid. Otherwise original input 0x-prefixe, if address is valid. Otherwise original input 0x-prefixed.

*/

export function toChecksumHexAddress(address: string) {

* @returns A 0x-prefixed hexadecimal checksummed address, if address is valid. Otherwise returns input 0x-prefixed.

*/

export function toChecksumHexAddress(address: string): Hex;

/**

* Returns input unmodified.

* @deprecated This function should only be called with string input.

* @param address - The address to convert.

* @returns Input unmodified

*/

export function toChecksumHexAddress<T extends null | undefined>(address: T): T;

/**

* Convert an address to a checksummed hexadecimal address.

*

* @param address - The address to convert.

* @returns A 0x-prefixed hexadecimal checksummed address, if address is valid. Otherwise if input is string, returns it 0x-prefixed. If input is not string, returns it unmodified.

*/

export function toChecksumHexAddress<T extends null | string | undefined>(

address: T,

) {

I agree with @legobeat. | null | undefined should be part of the address param's type signature.

Especially with exactOptionalPropertyTypes enabled, the only clear use case for | undefined over ?: is when undefined is explicitly being assigned or passed in, not just an uninstantiated or omitted value. And that's exactly the case here.

I don't think overloading is necessary, especially since type signatures for overloaded functions are not inferred properly by TypeScript anyway.

See Inconsistent type inference on overloaded function types microsoft/TypeScript#57351

We see this with the getNetworkClientById method in NetworkController. Its return type is always inferred as AutoManagedNetworkClient<CustomNetworkClientConfiguration>.

We also don't need to narrow address beyond string, so there might not be a strong need to make this generic.

I think we could do this the simple way:

Suggested change

* @returns A 0x-prefixed hexidecimal checksummed address, if address is valid. Otherwise original input 0x-prefixe, if address is valid. Otherwise original input 0x-prefixed.

*/

export function toChecksumHexAddress(address: string) {

* @returns A 0x-prefixed hexidecimal checksummed address, if address is valid. Otherwise original input 0x-prefixe, if address is valid. Otherwise original input 0x-prefixed.

*/

export function toChecksumHexAddress(address: string | null | undefined) {

While I agree with @legobeat and @MajorLift, the logic added by @mcmire is easing the function for all types of input other than string, so we are "supporting" many more types than just null or undefined - with this in mind, perhaps a string | unknown type would make more sense?

Hmm. I see the point being made here, but the problem with changing the argument type is that this also changes the return type. So I think we would need overloads in order to communicate that:

When address is not a string, the return type is the same

When address is a string, the return type is a string (and really, it's a Hex)

Otherwise, if the argument type is unknown, then the return type is inferred to be unknown. This causes problems with how it's used in e.g. AddressBookController (and any other place), and the resulting address would need to be checked for whether it is really a hex string. It seems like we should avoid this.

Thoughts on the best approach?

@mikesposito That's a great point! @mcmire In that case, using overloads and generics the way @legobeat originally suggested seems like the best way (with nullable types not handled explicitly).

Here's the overloaded function signature:

/** Overload signatures */ export function toChecksumHexAddress(address: string): `0x${string}`; export function toChecksumHexAddress<T>(address: T): T; /** Implementation signature */ export function toChecksumHexAddress(address: unknown): unknown { ... }

And here's a Playground link showing that the correct overload signature (and return type) is inferred for string vs. non-string input.

unknown sounds reasonable! Generics do seem necessary to get the expected return type, though: https://github.com/MetaMask/core/actions/runs/8268140049/job/22620308101?pr=4046

My overloaded version (#4046 (comment)) runs into this error:

packages/preferences-controller/src/PreferencesController.ts:261:24 - error TS2538: Type 'unknown' cannot be used as an index type. 261 if (identities[address]) { ~~~~~~~ packages/preferences-controller/src/PreferencesController.ts:266:20 - error TS2538: Type 'unknown' cannot be used as an index type. 266 identities[address] = { ~~~~~~~

This is fixed with the following:

diff --git a/packages/preferences-controller/src/PreferencesController.ts b/packages/preferences-controller/src/PreferencesController.ts index 2bf761950..b7451b2c2 100644 --- a/packages/preferences-controller/src/PreferencesController.ts +++ b/packages/preferences-controller/src/PreferencesController.ts @@ -254,7 +254,9 @@ export class PreferencesController extends BaseController< * @param addresses - List of addresses to use to generate new identities. */ addIdentities(addresses: string[]) { - const checksummedAddresses = addresses.map(toChecksumHexAddress); + const checksummedAddresses = addresses.map((address) => + toChecksumHexAddress(address), + ); this.update((state) => { const { identities } = state; for (const address of checksummedAddresses) {

This is interesting. I didn't know passing a callback into map can interfere with type inference like this.

packages/controller-utils/src/util.ts

MajorLift · 2024-03-14T15:17:02Z

packages/controller-utils/src/util.ts

 */
-export function toChecksumHexAddress(address: string) {
+export function toChecksumHexAddress<T>(address: T) {


This relies on the return type being string | T. Setting it to T causes the following error:

Type 'string' is not assignable to type 'T'. 'T' could be instantiated with an arbitrary type which could be unrelated to 'string'.ts(2322)

IMO string | T defeats the purpose of using the generic param T.

How do you feel about adapting the overload you suggested earlier (#4046 (comment)), so that it distinctly handles string vs. non-string input, inferring the return types for each, and narrows the non-generic return type to Hex instead of string (playground link)?

Here's the diff for this change, including an error fix (see #4046 (comment)). This passes yarn build, yarn test.

diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index f06cf6391..e5da29a81 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -250,13 +250,27 @@ export async function safelyExecuteWithTimeout<Result>( } } +/** + * Convert an address to a checksummed hexadecimal address. + * + * @param address - The address to convert. + * @returns A 0x-prefixed hexadecimal checksummed address, if address is valid. Otherwise returns it 0x-prefixed. + */ +export function toChecksumHexAddress(address: string): Hex; +/** + * Returns input unmodified. + * @deprecated This function should only be called with string input. + * @param address - The address to convert. + * @returns Unmodified input + */ +export function toChecksumHexAddress<T>(address: T): T; /** * Convert an address to a checksummed hexadecimal address. * * @param address - The address to convert. * @returns The address in 0x-prefixed hexadecimal checksummed form if it is valid, or untouched otherwise. */ -export function toChecksumHexAddress<T>(address: T) { +export function toChecksumHexAddress(address: unknown): unknown { if (typeof address !== 'string') { // Mimic behavior of `addHexPrefix` from `ethereumjs-util` (which this // function was previously using) for backward compatibility. diff --git a/packages/preferences-controller/src/PreferencesController.ts b/packages/preferences-controller/src/PreferencesController.ts index 2bf761950..b7451b2c2 100644 --- a/packages/preferences-controller/src/PreferencesController.ts +++ b/packages/preferences-controller/src/PreferencesController.ts @@ -254,7 +254,9 @@ export class PreferencesController extends BaseController< * @param addresses - List of addresses to use to generate new identities. */ addIdentities(addresses: string[]) { - const checksummedAddresses = addresses.map(toChecksumHexAddress); + const checksummedAddresses = addresses.map((address) => + toChecksumHexAddress(address), + ); this.update((state) => { const { identities } = state; for (const address of checksummedAddresses) {

mcmire · 2024-03-14T17:06:24Z

@MajorLift @legobeat I've incorporated the feedback above. Let me know what you think!

…vious type)

MajorLift

LGTM!

MajorLift · 2024-03-14T17:12:23Z

packages/controller-utils/src/util.ts

 */
-export function toChecksumHexAddress(address: string) {
+export function toChecksumHexAddress(address: string): string;


Good catch!

Why not return Hex? 🤔 This does not have to overlap with the generic one just below, if that's the idea?

After I made that suggestion I looked back and realized that it didn't return Hex before, it always returned string before, regardless of the input. Hex does make sense, but if we changed that, it would be breaking — it would not be a regression fix. So I wanted to avoid that for now. Does that make sense / sound right?

How is narrowing the return type breaking?

As opposed to:

Widening return type would/could be breaking

Narrowing input parameter type is generally breaking

mcmire · 2024-03-14T17:50:44Z

We've temporarily gotten around this problem in the extension with MetaMask/metamask-extension#22928, so, while this is still important to land, we have time to make sure that @legobeat is on board.

legobeat

Seems more semantic to constrain the return type to Hex for string input but otherwise looks good to go.

legobeat · 2024-03-14T22:55:49Z

packages/controller-utils/src/util.ts

 */
-export function toChecksumHexAddress(address: string) {
+export function toChecksumHexAddress(address: string): string;


Suggested change

export function toChecksumHexAddress(address: string): string;

export function toChecksumHexAddress(address: string): Hex;

mcmire · 2024-03-15T15:29:30Z

I am going to merge this out of an abundance of caution, but the question about whether narrowing the return type causes a breaking change is a good one. I will continue the conversation above.

## Explanation ## References ## Changelog ## [13.0.0] Accounts Controller ### Changed - Fix update setSelectedAccount to throw if the id is not found ([#4167](#4167)) - Fix normal account indexing naming with index gap ([#4089](#4089)) - **BREAKING** Bump peer dependency `@metamask/snaps-controllers` to `^6.0.3` and dependencies `@metamask/snaps-sdk` to `^3.1.1`, `@metamask/eth-snap-keyring` to `^3.0.0`([#4090](#4090)) ## [28.0.0] Assets Controller ### Added - Add reservoir migration ([#4030](#4030)) ### Changed - Fix getting nft tokenURI ([#4136](#4136)) - **BREAKING** Bump peer dependency on `@metamask/keyring-controller` ([#4090](#4090)) - Fix token detection during account change ([#4133](#4133)) - Fix update nft metadata when toggles off ([#4096](#4096)) - Adds `tokenMethodIncreaseAllowance` ([#4069](#4069)) - Fix mantle token mispriced ([#4045](#4045)) ## [15.0.0] Keyring Controller ### Changed - **BREAKING** use getAccounts on HD Keyring when calling addNewAccount ([#4158](#4158)) - Pass CAIP-2 scope to execution context ([#4090](#4090)) - Allow gas limits to be changed during #addPaymasterData ([#3942](#3942)) ## [10.0.0] Preferences Controller ### Changed - **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to `^15.0.0` ([#4090](#4090)) - Restore previous behavior of toChecksumHexAddress ([#4046](#4046)) ## [15.0.0] Signature Controller ### Changed - **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to `^15.0.0` ([#4090](#4090)) ## [8.0.0] User Operation Controller ### Changed - **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to `^15.0.0` and Pass CAIP-2 scope to execution context ([#4090](#4090)) - Allow gas limits to be changed during #addPaymasterData ([#3942](#3942)) ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate

legobeat reviewed Mar 13, 2024

View reviewed changes

packages/controller-utils/src/util.ts Outdated Show resolved Hide resolved

legobeat reviewed Mar 13, 2024

View reviewed changes

mcmire added 2 commits March 13, 2024 10:19

Use unknown for type

2c548fe

Update JSDoc

1312126

legobeat suggested changes Mar 14, 2024

View reviewed changes

packages/controller-utils/src/util.ts Show resolved Hide resolved

legobeat and others added 2 commits March 14, 2024 21:47

Make toCheckSumAddress signature generic

ca5dc82

fix toChecksumAddress return type

cbb1acd

legobeat previously approved these changes Mar 14, 2024

View reviewed changes

MajorLift reviewed Mar 14, 2024

View reviewed changes

mcmire added 2 commits March 14, 2024 11:04

Add an overload to handle non-string input

3eae5bf

Merge branch 'main' into fix-to-checksum-hex-address

8fa1fa2

mcmire dismissed legobeat’s stale review via 8fa1fa2 March 14, 2024 17:05

mcmire marked this pull request as ready for review March 14, 2024 17:06

mcmire requested a review from a team as a code owner March 14, 2024 17:06

Change 'normal' overload back to string from Hex (as this was the pre…

3d13053

…vious type)

MajorLift approved these changes Mar 14, 2024

View reviewed changes

legobeat approved these changes Mar 14, 2024

View reviewed changes

legobeat reviewed Mar 14, 2024

View reviewed changes

Merge branch 'main' into fix-to-checksum-hex-address

3e32b20

mcmire merged commit 7da8ef7 into main Mar 15, 2024
139 checks passed

mcmire deleted the fix-to-checksum-hex-address branch March 15, 2024 15:37

montelaidev mentioned this pull request Apr 17, 2024

Release/140.0.0 #4177

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore previous behavior of toChecksumHexAddress #4046

Restore previous behavior of toChecksumHexAddress #4046

mcmire commented Mar 12, 2024 •

edited

Loading

legobeat Mar 13, 2024 •

edited

Loading

MajorLift Mar 13, 2024

mikesposito Mar 13, 2024

mcmire Mar 13, 2024 •

edited

Loading

MajorLift Mar 13, 2024 •

edited

Loading

legobeat Mar 14, 2024

MajorLift Mar 14, 2024

MajorLift Mar 14, 2024 •

edited

Loading

mcmire commented Mar 14, 2024

MajorLift left a comment

MajorLift Mar 14, 2024

legobeat Mar 14, 2024

mcmire Mar 14, 2024 •

edited

Loading

legobeat Mar 14, 2024 •

edited

Loading

mcmire commented Mar 14, 2024

legobeat left a comment •

edited

Loading

legobeat Mar 14, 2024

mcmire commented Mar 15, 2024

- * @returns A 0x-prefixed hexidecimal checksummed address, if address is valid. Otherwise original input 0x-prefixe, if address is valid. Otherwise original input 0x-prefixed.
- */
-export function toChecksumHexAddress(address: string) {
+ * @returns A 0x-prefixed hexadecimal checksummed address, if address is valid. Otherwise returns input 0x-prefixed.
+ */
+export function toChecksumHexAddress(address: string): Hex;
+/**
+ * Returns input unmodified.
+ * @deprecated This function should only be called with string input.
+ * @param address - The address to convert.
+ * @returns Input unmodified
+ */
+export function toChecksumHexAddress<T extends null | undefined>(address: T): T;
+/**
+ * Convert an address to a checksummed hexadecimal address.
+ *
+ * @param address - The address to convert.
+ * @returns A 0x-prefixed hexadecimal checksummed address, if address is valid. Otherwise if input is string, returns it 0x-prefixed. If input is not string, returns it unmodified.
+ */
+export function toChecksumHexAddress<T extends null | string | undefined>(
+  address: T,
+) {

	export function toChecksumHexAddress(address: string): string;
	export function toChecksumHexAddress(address: string): Hex;

Restore previous behavior of toChecksumHexAddress #4046

Restore previous behavior of toChecksumHexAddress #4046

Conversation

mcmire commented Mar 12, 2024 • edited Loading

Explanation

References

Changelog

@metamask/controller-utils

Checklist

legobeat Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

MajorLift Mar 13, 2024

Choose a reason for hiding this comment

mikesposito Mar 13, 2024

Choose a reason for hiding this comment

mcmire Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

MajorLift Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

legobeat Mar 14, 2024

Choose a reason for hiding this comment

MajorLift Mar 14, 2024

Choose a reason for hiding this comment

MajorLift Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

mcmire commented Mar 14, 2024

MajorLift left a comment

Choose a reason for hiding this comment

MajorLift Mar 14, 2024

Choose a reason for hiding this comment

legobeat Mar 14, 2024

Choose a reason for hiding this comment

mcmire Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

legobeat Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

mcmire commented Mar 14, 2024

legobeat left a comment • edited Loading

Choose a reason for hiding this comment

legobeat Mar 14, 2024

Choose a reason for hiding this comment

mcmire commented Mar 15, 2024

mcmire commented Mar 12, 2024 •

edited

Loading

`@metamask/controller-utils`

legobeat Mar 13, 2024 •

edited

Loading

mcmire Mar 13, 2024 •

edited

Loading

MajorLift Mar 13, 2024 •

edited

Loading

MajorLift Mar 14, 2024 •

edited

Loading

mcmire Mar 14, 2024 •

edited

Loading

legobeat Mar 14, 2024 •

edited

Loading

legobeat left a comment •

edited

Loading