Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore previous behavior of toChecksumHexAddress #4046

Merged
merged 9 commits into from
Mar 15, 2024
Merged

Restore previous behavior of toChecksumHexAddress #4046

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
47d1cbb
Restore previous behavior of toChecksumHexAddress
mcmire Mar 12, 2024
2c548fe
Use unknown for type
mcmire Mar 13, 2024
1312126
Update JSDoc
mcmire Mar 13, 2024
ca5dc82
Make toCheckSumAddress signature generic
legobeat Mar 14, 2024
cbb1acd
fix toChecksumAddress return type
legobeat Mar 14, 2024
3eae5bf
Add an overload to handle non-string input
mcmire Mar 14, 2024
8fa1fa2
Merge branch 'main' into fix-to-checksum-hex-address
mcmire Mar 14, 2024
3d13053
Change 'normal' overload back to string from Hex (as this was the pre…
mcmire Mar 14, 2024
3e32b20
Merge branch 'main' into fix-to-checksum-hex-address
mcmire Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions packages/controller-utils/src/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,13 +277,33 @@ describe('util', () => {
});

describe('toChecksumHexAddress', () => {
const fullAddress = `0x${VALID}`;
it('should return address for valid address', () => {
expect(util.toChecksumHexAddress(fullAddress)).toBe(fullAddress);
it('should return an 0x-prefixed checksum address untouched', () => {
const address = '0x4e1fF7229BDdAf0A73DF183a88d9c3a04cc975e0';
expect(util.toChecksumHexAddress(address)).toBe(address);
});

it('should return address for non prefix address', () => {
expect(util.toChecksumHexAddress(VALID)).toBe(fullAddress);
it('should prefix a non-0x-prefixed checksum address with 0x', () => {
expect(
util.toChecksumHexAddress('4e1fF7229BDdAf0A73DF183a88d9c3a04cc975e0'),
).toBe('0x4e1fF7229BDdAf0A73DF183a88d9c3a04cc975e0');
});

it('should convert a non-checksum address to a checksum address', () => {
expect(
util.toChecksumHexAddress('0x4e1ff7229bddaf0a73df183a88d9c3a04cc975e0'),
).toBe('0x4e1fF7229BDdAf0A73DF183a88d9c3a04cc975e0');
});

it('should return "0x" if given an empty string', () => {
expect(util.toChecksumHexAddress('')).toBe('0x');
});

it('should return the input untouched if it is undefined', () => {
expect(util.toChecksumHexAddress(undefined)).toBeUndefined();
});

it('should return the input untouched if it is null', () => {
expect(util.toChecksumHexAddress(null)).toBeNull();
});
});

Expand Down
16 changes: 12 additions & 4 deletions packages/controller-utils/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,20 +251,28 @@ export async function safelyExecuteWithTimeout<Result>(
}

/**
* Convert an address to a checksummed hexidecimal address.
* Convert an address to a checksummed hexadecimal address.
*
* @param address - The address to convert.
* @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.
* @returns The address in 0x-prefixed hexadecimal checksummed form if it is valid, or untouched otherwise.
*/
export function toChecksumHexAddress(address: string) {
export function toChecksumHexAddress<T>(address: T) {
Copy link
Contributor

@MajorLift MajorLift Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

if (typeof address !== 'string') {
// Mimic behavior of `addHexPrefix` from `ethereumjs-util` (which this
// function was previously using) for backward compatibility.
return address;
}

const hexPrefixed = add0x(address);

if (!isHexString(hexPrefixed)) {
// Version 5.1 of ethereumjs-utils would have returned '0xY' for input 'y'
// Version 5.1 of ethereumjs-util would have returned '0xY' for input 'y'
// but we shouldn't waste effort trying to change case on a clearly invalid
// string. Instead just return the hex prefixed original string which most
// closely mimics the original behavior.
return hexPrefixed;
}

return toChecksumAddress(hexPrefixed);
}

Expand Down
Loading