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

[Gudahtt]

This PR re-introduces the changes that were originally merged as part of #4844. They were reverted due to an incompatibility with @metamask/create-release-branch v3, which was resolved with the update to v3.1.

References:

• Revert PR: Revert "feat(controller-utils): support bn.js v4 input to BN functions" #4858
• @metamask/create-release-branch support for npm: aliases: feat: allow npm:name@version dependency redirections in manifest create-release-branch#158
• @metamask/create-release-branch v3.1 release with backported alias support: 3.1.0 create-release-branch#160
• @metamask/create-release-branch bump to v3.1: chore: Bump @metamask/create-release-branch from v3.0 to v3.1 #4884

Original description of #4844:

Explanation

• Extend types to indicate support for passing in bn.js v4 instances to BN.js utility functions
  • Affected functions:
    • BNToHex
    • fractionBN
    • fromHex
    • toHex
  • If input is v4 instance, return as v4. Otherwise use v5, as previously.
  • fractionBN now uses the original BN implementation when passed a v4 BN instance
• Use overload signatures to more specifically type BN-related util functions

References

• Context: fix(deps): unpin ethereumjs-abi@0.6,x ethereumjs-util@6.x metamask-mobile#11972
  • Note that these functions are already used with v4 input but this was not caught due to the way bn.js is imported downstream. Making this support explicit facilitates resolving the type issue without requiring a prior full migration off bn.js v4 or resorting to peppering ts-expect-errors all over.
• feat(controller-utils): support BigNumber as input to BNToHex #4827
• [Util v7| Tracking Issue for BN.js v4 and v5 interoperability issues ethereumjs/ethereumjs-util#250

Changelog

@metamask/controller-utils

• CHANGED: The following functions now accept input from bn.js v4 library:
  • BNToHex
  • fractionBN
  • fromHex
  • toHex
• FIXED: fractionToBN now returns output using the same bn.js library version that created the input

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
• I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

[socket-security]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@types/bn.js@4.11.6	None	0	13.9 kB	types

View full report↗︎

[mcmire]

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);
	});
	});
	});

[mcmire]

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;
	}

[mcmire]

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;

[mcmire]

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

[mcmire]

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

[mcmire]

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;

[mcmire]

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

[mcmire]

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