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.

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
30 changes: 25 additions & 5 deletions packages/controller-utils/src/util.test.ts
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
33 changes: 29 additions & 4 deletions packages/controller-utils/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,20 +251,45 @@ 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.
*/
export function toChecksumHexAddress(address: string) {
export function toChecksumHexAddress(address: string): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mcmire mcmire Mar 14, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@legobeat legobeat Mar 14, 2024

Choose a reason for hiding this comment

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

How is narrowing the return type breaking?

As opposed to:

  • Widening return type would/could be breaking
  • Narrowing input parameter type is generally breaking

Copy link
Contributor

Choose a reason for hiding this comment

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

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


/**
* Convert an address to a checksummed hexadecimal address.
*
* Note that this particular overload does nothing.
*
* @param address - A value that is not a string (e.g. `undefined` or `null`).
* @returns The `address` untouched.
* @deprecated This overload is designed to gracefully handle an invalid input
* and is only present for backward compatibility. It may be removed in a future
* major version. Please pass a string to `toChecksumHexAddress` instead.
*/
export function toChecksumHexAddress<T>(address: T): T;

// Tools only see JSDocs for overloads and ignore them for the implementation.
// eslint-disable-next-line jsdoc/require-jsdoc
export function toChecksumHexAddress(address: unknown) {
legobeat marked this conversation as resolved.
Show resolved Hide resolved
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
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading