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

Jump to bottom

feat(controller-utils): support bn.js v4 input to BN functions (re-introduces #4844) #4886

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(controller-utils): support bn.js v4 input to BN functions (re-introduces #4844) #4886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions packages/controller-utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,10 @@
"@metamask/utils": "^10.0.0",
"@spruceid/siwe-parser": "2.1.0",
"@types/bn.js": "^5.1.5",
"@types/bnjs4": "npm:@types/bn.js@^4.11.6",
"bignumber.js": "^9.1.2",
"bn.js": "^5.2.1",
"bnjs4": "npm:bn.js@^4.12.0",
"eth-ens-namehash": "^2.0.8",
"fast-deep-equal": "^3.1.3"
},
Expand Down
17 changes: 17 additions & 0 deletions packages/controller-utils/src/util.test.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem the changes to fromHex or toHex are being tested. Can we test those? (Happy to add them to this branch.)

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import EthQuery from '@metamask/eth-query';
import BigNumber from 'bignumber.js';
import BN from 'bn.js';
import BN4 from 'bnjs4';
import nock from 'nock';

import { FakeProvider } from '../../../tests/fake-provider';
Expand Down Expand Up @@ -32,11 +33,27 @@ describe('util', () => {

it('bNToHex', () => {
expect(util.BNToHex(new BN('1337'))).toBe('0x539');
expect(util.BNToHex(new BN4('1337'))).toBe('0x539');
expect(util.BNToHex(new BigNumber('1337'))).toBe('0x539');
});

it('fractionBN', () => {
expect(util.fractionBN(new BN('1337'), 9, 10).toNumber()).toBe(1203);
expect(util.fractionBN(new BN4('1337'), 9, 10).toNumber()).toBe(1203);
// Ensure return values use the same bn.js implementation as input by detection using non-typed API
/* eslint-disable @typescript-eslint/no-explicit-any */
expect(
(util.fractionBN(new BN4('1337'), 9, 10) as any)._strip,
).toBeUndefined();
expect(
(util.fractionBN(new BN4('1337'), 9, 10) as any).strip,
).toBeDefined();
expect(
(util.fractionBN(new BN('1337'), 9, 10) as any)._strip,
).toBeDefined();
expect(
(util.fractionBN(new BN('1337'), 9, 10) as any).strip,
).toBeUndefined();
});
Comment on lines 40 to 57
Copy link
Contributor

@mcmire mcmire Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we split all of these tests out into separate tests so we can give them proper names? Also, can we just test that the right BN instance is returned by using the toBeInstanceOf matcher?

Suggested change
it('fractionBN', () => {
expect(util.fractionBN(new BN('1337'), 9, 10).toNumber()).toBe(1203);
expect(util.fractionBN(new BN4('1337'), 9, 10).toNumber()).toBe(1203);
// Ensure return values use the same bn.js implementation as input by detection using non-typed API
/* eslint-disable @typescript-eslint/no-explicit-any */
expect(
(util.fractionBN(new BN4('1337'), 9, 10) as any)._strip,
).toBeUndefined();
expect(
(util.fractionBN(new BN4('1337'), 9, 10) as any).strip,
).toBeDefined();
expect(
(util.fractionBN(new BN('1337'), 9, 10) as any)._strip,
).toBeDefined();
expect(
(util.fractionBN(new BN('1337'), 9, 10) as any).strip,
).toBeUndefined();
});
describe('fractionBN', () => {
describe('when the first argument is an instance of BN from modern bn.js', () => {
it('computes floor(A * B / C), where B and C are numbers', () => {
const result = util.fractionBN(new BN('1337'), 9, 10);
expect(result.toNumber()).toBe(1203);
});
it('computes floor(A * B / C), where B is a string and C is a number', () => {
const result = util.fractionBN(new BN('1337'), '9', 10);
expect(result.toNumber()).toBe(1203);
});
it('computes floor(A * B / C), where B is a number and C is a string', () => {
const result = util.fractionBN(new BN('1337'), 9, '10');
expect(result.toNumber()).toBe(1203);
});
it('returns the same version of BN as the input', () => {
const result = util.fractionBN(new BN('1337'), 9, 10);
expect(result).toBeInstanceOf(BN);
});
});
describe('when the first argument is an instance of BN from bn.js 4', () => {
it('computes floor(A * B / C), where B and C are numbers', () => {
const result = util.fractionBN(new BN4('1337'), 9, 10);
expect(result.toNumber()).toBe(1203);
});
it('computes floor(A * B / C), where B is a string and C is a number', () => {
const result = util.fractionBN(new BN4('1337'), '9', 10);
expect(result.toNumber()).toBe(1203);
});
it('computes floor(A * B / C), where B is a number and C is a string', () => {
const result = util.fractionBN(new BN4('1337'), 9, '10');
expect(result.toNumber()).toBe(1203);
});
it('returns the same version of BN as the input', () => {
const result = util.fractionBN(new BN4('1337'), 9, 10);
expect(result).toBeInstanceOf(BN4);
});
});
});


it('getBuyURL', () => {
Expand Down
46 changes: 37 additions & 9 deletions packages/controller-utils/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from '@metamask/utils';
import type { BigNumber } from 'bignumber.js';
import BN from 'bn.js';
import BN4 from 'bnjs4';
import ensNamehash from 'eth-ens-namehash';
import deepEqual from 'fast-deep-equal';

Expand Down Expand Up @@ -69,10 +70,31 @@ export function isSafeChainId(chainId: Hex): boolean {
*/
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/naming-convention
export function BNToHex(inputBn: BN | BigNumber) {
export function BNToHex(inputBn: BN | BN4 | BigNumber): string {
return add0x(inputBn.toString(16));
}

function getBNImplementation(targetBN: BN4): typeof BN4;
function getBNImplementation(targetBN: BN): typeof BN;
/**
* Return the bn.js library responsible for the BN in question
* @param targetBN - A BN instance
* @returns A bn.js instance
*/
function getBNImplementation(targetBN: BN | BN4): typeof BN4 | typeof BN {
return Object.keys(targetBN).includes('_strip') ? BN4 : BN;
}
Comment on lines +77 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

The JSDoc is in the wrong place here. It should go on each overload, not the implementation:

Suggested change
function getBNImplementation(targetBN: BN4): typeof BN4;
function getBNImplementation(targetBN: BN): typeof BN;
/**
* Return the bn.js library responsible for the BN in question
* @param targetBN - A BN instance
* @returns A bn.js instance
*/
function getBNImplementation(targetBN: BN | BN4): typeof BN4 | typeof BN {
return Object.keys(targetBN).includes('_strip') ? BN4 : BN;
}
/**
* Return the constructor of the given BN instance, which in this case is the BN
* constructor from bn.js 4.
* @param targetBN - A instance of BN from bn.js 4.
* @returns The BN constructor from bn.js 4.
*/
function getBNImplementation(targetBN: BN4): typeof BN4;
/**
* Return the constructor of the given BN instance, which in this case is the BN
* constructor from bn.js 5+.
* @param targetBN - A instance of BN from bn.js 5+.
* @returns The BN constructor from bn.js 5+.
*/
function getBNImplementation(targetBN: BN): typeof BN;
function getBNImplementation(targetBN: BN | BN4): typeof BN4 | typeof BN {
return Object.keys(targetBN).includes('_strip') ? BN4 : BN;
}


export function fractionBN(
targetBN: BN,
numerator: number | string,
denominator: number | string,
): BN;
export function fractionBN(
targetBN: BN4,
numerator: number | string,
denominator: number | string,
): BN4;
Comment on lines +88 to +97
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar as above. The JSDoc should go above each overload.

Suggested change
export function fractionBN(
targetBN: BN,
numerator: number | string,
denominator: number | string,
): BN;
export function fractionBN(
targetBN: BN4,
numerator: number | string,
denominator: number | string,
): BN4;
/**
* Multiply a BN by a fraction and get a BN back.
*
* This overload takes an instance of BN from bn.js 5+ and returns another
* instance of the same class.
*
* @param targetBN - Number to multiply by a fraction.
* @param numerator - Numerator of the fraction multiplier.
* @param denominator - Denominator of the fraction multiplier.
* @returns Product of the multiplication.
*/
export function fractionBN(
targetBN: BN,
numerator: number | string,
denominator: number | string,
): BN;
/**
* Multiply a BN by a fraction and get a BN back.
*
* This overload takes an instance of BN from bn.js 4 and returns another
* instance of the same class.
*
* @param targetBN - Number to multiply by a fraction.
* @param numerator - Numerator of the fraction multiplier.
* @param denominator - Denominator of the fraction multiplier.
* @returns Product of the multiplication.
*/
export function fractionBN(
targetBN: BN4,
numerator: number | string,
denominator: number | string,
): BN4;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This JSDoc is not necessary (but I can't suggest removing it because the diff crosses over into unchanged lines).

* Used to multiply a BN by a fraction.
*
Expand All @@ -82,12 +104,16 @@ export function BNToHex(inputBn: BN | BigNumber) {
* @returns Product of the multiplication.
*/
export function fractionBN(
targetBN: BN,
targetBN: BN | BN4,
numerator: number | string,
denominator: number | string,
) {
const numBN = new BN(numerator);
const denomBN = new BN(denominator);
): BN | BN4 {
// @ts-expect-error - Signature overload confusion
const BNImplementation = getBNImplementation(targetBN);

const numBN = new BNImplementation(numerator);
const denomBN = new BNImplementation(denominator);
// @ts-expect-error - BNImplementation gets unexpected typed
return targetBN.mul(numBN).div(denomBN);
Comment on lines +111 to 117
Copy link
Contributor

@mcmire mcmire Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure we need the ts-expect-errors. There are two code paths here. We need to tell TypeScript which one to go down, it cannot figure that out itself.

Also, looking at this more closely, I am unsure why we need the getBNImplementation function. Why can't we just use instanceof? It seems like this would work just as well:

Suggested change
// @ts-expect-error - Signature overload confusion
const BNImplementation = getBNImplementation(targetBN);
const numBN = new BNImplementation(numerator);
const denomBN = new BNImplementation(denominator);
// @ts-expect-error - BNImplementation gets unexpected typed
return targetBN.mul(numBN).div(denomBN);
if (targetBN instanceof BN4) {
const numBN = new BN4(numerator);
const denomBN = new BN4(denominator);
return targetBN.mul(numBN).div(denomBN);
}
const numBN = new BN(numerator);
const denomBN = new BN(denominator);
return targetBN.mul(numBN).div(denomBN);

}

Expand Down Expand Up @@ -192,18 +218,20 @@ export function hexToText(hex: string) {
}
}

export function fromHex(value: string | BN): BN;
export function fromHex(value: BN4): BN4;
/**
* Parses a hex string and converts it into a number that can be operated on in a bignum-safe,
* base-10 way.
*
* @param value - A base-16 number encoded as a string.
* @returns The number as a BN object in base-16 mode.
*/
export function fromHex(value: string | BN): BN {
export function fromHex(value: string | BN | BN4): BN | BN4 {
Comment on lines +221 to +230
Copy link
Contributor

Choose a reason for hiding this comment

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

Same story here with the JSDoc. How about:

Suggested change
export function fromHex(value: string | BN): BN;
export function fromHex(value: BN4): BN4;
/**
* Parses a hex string and converts it into a number that can be operated on in a bignum-safe,
* base-10 way.
*
* @param value - A base-16 number encoded as a string.
* @returns The number as a BN object in base-16 mode.
*/
export function fromHex(value: string | BN): BN {
export function fromHex(value: string | BN | BN4): BN | BN4 {
/**
* Converts a number into a representation that can be safely operated on in a
* bignum-safe way.
*
* In this case the number is already in that representation.
*
* @param value - A BN instance from modern bn.js.
* @returns The same BN instance.
*/
export function fromHex(value: BN): BN;
/**
* Converts a base-16 number encoded as a hex string into a base-10
* representation that can be safely operated on in a bignum-safe way.
*
* @param value - A hex string, or a BN instance from bn.js.
* @returns The number as a BN instance from modern bn.js in base-10 mode.
*/
export function fromHex(value: string): BN;
/**
* Converts a number into a representation that can be safely operated on in a
* bignum-safe way.
*
* In this case the number is already in that representation.
*
* @param value - A BN instance from bn.js 4.
* @returns The same BN instance.
*/
export function fromHex(value: BN4): BN4;

if (BN.isBN(value)) {
return value;
}
return new BN(hexToBN(value).toString(10));
return new BN(hexToBN(value as string).toString(10), 10);
Comment on lines 231 to +234
Copy link
Contributor

Choose a reason for hiding this comment

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

The type-cast here doesn't seem right. TypeScript thinks that value is a string | BN4.

BN.isBN is a type guard that coerces value to BN. It seems like we need a comparable check for BN4:

Suggested change
if (BN.isBN(value)) {
return value;
}
return new BN(hexToBN(value).toString(10));
return new BN(hexToBN(value as string).toString(10), 10);
if (BN.isBN(value)) {
return value;
}
if (BN4.isBN(value)) {
return value;
}
return new BN(hexToBN(value).toString(10));

}

/**
Expand All @@ -212,14 +240,14 @@ export function fromHex(value: string | BN): BN {
* @param value - An integer, an integer encoded as a base-10 string, or a BN.
* @returns The integer encoded as a hex string.
*/
export function toHex(value: number | bigint | string | BN): Hex {
export function toHex(value: number | bigint | string | BN | BN4): Hex {
if (typeof value === 'string' && isStrictHexString(value)) {
return value;
}
const hexString =
BN.isBN(value) || typeof value === 'bigint'
? value.toString(16)
: new BN(value.toString(), 10).toString(16);
: new BN(value.toString(10), 10).toString(16);
return `0x${hexString}`;
}

Expand Down
13 changes: 12 additions & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2338,9 +2338,11 @@ __metadata:
"@metamask/utils": "npm:^10.0.0"
"@spruceid/siwe-parser": "npm:2.1.0"
"@types/bn.js": "npm:^5.1.5"
"@types/bnjs4": "npm:@types/bn.js@^4.11.6"
"@types/jest": "npm:^27.4.1"
bignumber.js: "npm:^9.1.2"
bn.js: "npm:^5.2.1"
bnjs4: "npm:bn.js@^4.12.0"
deepmerge: "npm:^4.2.2"
eth-ens-namehash: "npm:^2.0.8"
fast-deep-equal: "npm:^3.1.3"
Expand Down Expand Up @@ -4333,6 +4335,15 @@ __metadata:
languageName: node
linkType: hard

"@types/bnjs4@npm:@types/bn.js@^4.11.6":
version: 4.11.6
resolution: "@types/bn.js@npm:4.11.6"
dependencies:
"@types/node": "npm:*"
checksum: 10/9ff3e7a1539a953c381c0d30ea2049162e3cab894cda91ee10f3a84d603f9afa2b2bc2a38fe9b427de94b6e2b7b77aefd217c1c7b07a10ae8d7499f9d6697a41
languageName: node
linkType: hard

"@types/debug@npm:^4.1.7":
version: 4.1.12
resolution: "@types/debug@npm:4.1.12"
Expand Down Expand Up @@ -5421,7 +5432,7 @@ __metadata:
languageName: node
linkType: hard

"bn.js@npm:^4.11.9":
"bn.js@npm:^4.11.9, bnjs4@npm:bn.js@^4.12.0":
version: 4.12.0
resolution: "bn.js@npm:4.12.0"
checksum: 10/10f8db196d3da5adfc3207d35d0a42aa29033eb33685f20ba2c36cadfe2de63dad05df0a20ab5aae01b418d1c4b3d4d205273085262fa020d17e93ff32b67527
Expand Down
Loading