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

Complete level 36 migration of TypeScript by converting conversion.utils.js and deprecate old conversion util #17262

Merged
merged 3 commits into from
Jan 24, 2023
Merged

Complete level 36 migration of TypeScript by converting conversion.utils.js and deprecate old conversion util #17262

merged 3 commits into from
Jan 24, 2023

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented Jan 17, 2023 •
edited
Loading

Explanation

This Pull request aims to complete the level 36 block of the ts-migration dashboard https://metamask.github.io/metamask-extension-ts-migration-dashboard/

This one was a large undertaking because conversions.util.js contained the dreaded 'conversionUtil' that had prolific use throughout the codebase. Usage was in many different places and forms:

  1. Directly imports conversionUtil (which is a wrapper around the 'converter' method)
  2. Uses one of the many conversions utils such as hexWEIToDecGWEI, addCurrencies, etc that composed the conversionUtil
  3. Uses conversionUtil by way of any number of different helper files that made their own conversion helpers throughout the codebase.

Problems with the converter method.

Instantiation of the value is not guaranteed to yield a BigNumber

  let convertedValue = fromNumericBase
    ? toBigNumber[fromNumericBase](value)
    : value;

  if (fromDenomination) {
    convertedValue = toNormalizedDenomination[fromDenomination](convertedValue);
  }

If you do not supply fromNumericBase to this function, which has no default value, the base value that you provided will be used. A few lines below this the 'convertedValue' is used as if it is guaranteed to be a BigNumber.

The value returned from this method is not guaranteed to be any specific type

  if (toNumericBase) {
    convertedValue = baseChange[toNumericBase](convertedValue);
  }
  return convertedValue;

If you do not supply 'toNumericBase' you will get a BigNumber value returned. This isn't bad, except that your return value type is dependent on a prop passed to the method, and if you attempt to use a non BigNumber as a BigNumber we get the "Not a BigNumber" error.

fromCurrency and toCurrency have no impact on the operation except to check if they are the same

  if (fromCurrency !== toCurrency) {
    if (conversionRate === null || conversionRate === undefined) {
      throw new Error(
        `Converting from ${fromCurrency} to ${toCurrency} requires a conversionRate, but one was not provided`,
      );
    }
    let rate = toBigNumber.dec(conversionRate);
    if (invertConversionRate) {
      rate = new BigNumber(1.0).div(conversionRate);
    }
    convertedValue = convertedValue.times(rate);
  }

In many parts of our codebase I have seen conversionUtil called like this:

const currency = conversionUtil(totalInHexWei, {
  fromNumericBase: 'hex',
  toNumericBase: 'dec',
  fromDenomination: 'WEI',
  fromCurrency: 'ETH',
  toCurrency: 'USD',
  conversionRate: 457.65,
});

Because I was never confident in my understanding of conversions of ETH to Fiat I assumed there was some magic here. NOPE!

On the first line of the snippet at the beginning of this section, we check if fromCurrency and toCurrency are the same, if they are it does nothing, otherwise it checks that conversionRate was supplied and throws an error if not. It then applies the conversion rate. The currency markers do nothing else at all. Instead we could have conditionally passed conversionRate if it made sense and use the existence of the conversionRate to determine if it should be applied.

There's also another fun tidbit....

fromDenomination and toDenomination make sense, but what happens if you omit one or the other?

There's nothing majorly wrong with these:

// Big Number Constants
const BIG_NUMBER_WEI_MULTIPLIER = new BigNumber('1000000000000000000');
const BIG_NUMBER_GWEI_MULTIPLIER = new BigNumber('1000000000');
const BIG_NUMBER_ETH_MULTIPLIER = new BigNumber('1');

// Setter Maps
const toBigNumber = {
  hex: (n) => new BigNumber(stripHexPrefix(n), 16),
  dec: (n) => new BigNumber(String(n), 10),
  BN: (n) => new BigNumber(n.toString(16), 16),
};
const toNormalizedDenomination = {
  WEI: (bigNumber) => bigNumber.div(BIG_NUMBER_WEI_MULTIPLIER),
  GWEI: (bigNumber) => bigNumber.div(BIG_NUMBER_GWEI_MULTIPLIER),
  ETH: (bigNumber) => bigNumber.div(BIG_NUMBER_ETH_MULTIPLIER),
};

Here, however,

}) => {
 //Very first line of function
  let convertedValue = fromNumericBase
    ? toBigNumber[fromNumericBase](value)
    : value;

  if (fromDenomination) {
    convertedValue = toNormalizedDenomination[fromDenomination](convertedValue);
  }
// [.....apply the conversionRate as shown in the above snippet]
  if (toDenomination) {
    convertedValue = toSpecifiedDenomination[toDenomination](convertedValue);
  }

This method converts the currency value to ETH by default if you supply fromDenomination. SO:

const v = conversionUtil(valueInHexWEI, { fromNumericBase: 'hex', fromDenomination: 'WEI', toNumericBase: 'dec'});

v is going to be the value in base 10 (decimal) of ETH. The problem with this is that the contract of it switching to ETH is baked into the code itself and not the props, which would be fine if the name of the method implied this. We do have methods that end in the name 'ToDecETH' which are more widely used for this operation, but under the hood they use conversionUtil. What makes worse is the existence of toDenomination. The existence of toDenomination should imply that not including it would not convert anything. That isn't the case. Furthermore if you use toDenomination without fromDenomination its assumed that the value you were using was in ETH already. How handy :)

Two different rounding operations occur

  if (numberOfDecimals !== undefined && numberOfDecimals !== null) {
    convertedValue = convertedValue.round(
      numberOfDecimals,
      BigNumber.ROUND_HALF_DOWN,
    );
  }

  if (roundDown) {
    convertedValue = convertedValue.round(roundDown, BigNumber.ROUND_DOWN);
  }

I do not know why these options are different, but I assume there is a reason. I'm hoping someone will be able to point that out to me in code review.

Problems with the helper methods that use converter/conversionUtil:

They are unnecessarily complex to use/understand

const addCurrencies = (a, b, options = {}) => {
  const { aBase, bBase, ...conversionOptions } = options;

  if (!isValidBase(aBase) || !isValidBase(bBase)) {
    throw new Error('Must specify valid aBase and bBase');
  }

  const value = getBigNumber(a, aBase).add(getBigNumber(b, bBase));

  return converter({
    value,
    ...conversionOptions,
  });
};

Usage:

const value = addCurrencies(
  currencyA,
  currencyB,
  { 
    aBase: 10, // Notably not 'dec' like the converter method bc who needs consistency 
    bBase: 16, // Same but for 'hex'
    fromNumericBase: :shrug: // Do you know? It takes all options from conversionUtil. More in details
    toNumericBase: 'dec',
  }
);

So the first thing this method does is check that you supplied a valid 10 or 16 value for aBase and bBase, which uses the same values as the parseInt, toString, etc methods (10, 16) to determine if the string is decimal or hexadecimal. I like that, it makes sense and encourages the understanding of these parameters of those methods. 'hex' and 'dec' I think were used for onboarding/understanding purposes but they just map to 10 and 16 respectfully and you can't use the knowledge of them anywhere else (you'd need to use 10/16 bases for the other methods).

Then it does something peculiar, it uses a helper method 'getBigNumber' and calls it to get the BigNumber representation of the a value then calls '.add' with the BigNumber representation of the B value.

Then it supplies the value to the converter method with all the params you want to pass it that the converter method operates on. So compound all of the issues I wrote about the converter method with the following:

  1. What happens if you supply 'fromNumericBase' to this method? Fun fact: 'BN' and undefined are valid values for fromNumericBase. 'BN' is for instances of BN or BigNumbers and undefined assumes its already a BigNumber. undefined is the correct value for fromNumericBase in the above example.
  2. If you look only at the usage of addCurrencies can you tell what the order of operations is? Does it convert each value then add it together, or does it add it together then convert it. If I supply a conversionRate and fromCurrency/toCurrency what should I expect?

Here's another fun example:

const conversionGreaterThan = ({ ...firstProps }, { ...secondProps }) => {
  const firstValue = converter({ ...firstProps });
  const secondValue = converter({ ...secondProps });

  return firstValue.gt(secondValue);
};

And here is the most complex usage I can find in real code:

  const customPriceSafe = conversionGreaterThan(
    {
      value: customGasPrice,
      fromNumericBase: 'hex',
      fromDenomination: 'WEI',
      toDenomination: 'GWEI',
    },
    { value: estimatedPrice, fromNumericBase: 'dec' },
  );

  return customPriceSafe;
}

Here we are attempting to compare a decimal value representing an amount in GWEI to a hex value representing an amount in WEI. The usage here is correct, but we can pass literally any propertiers of the converter to either A or B operand and it just becomes extremely confusing.

TO BE CONTINUED

Solution

This Pull Request implements a new class in TypeScript called 'Numeric' which has a lot of similarities with BigNumber except that it has denomination manipulation built in as well as a helper method for applyConverionRate. Each chaining operator of this new utility returns a new instance of Numeric. So for example:

const value = addCurrencies(
  currencyA,
  currencyB,
  { 
    aBase: 10, // Notably not 'dec' like the converter method bc who needs consistency 
    bBase: 16, // Same but for 'hex'
    fromNumericBase: :shrug: // Do you know? It takes all options from conversionUtil. More in details
    toNumericBase: 'dec',
  }
);

In this previous example from above the way you'd write the same thing using Numeric:

const value = new Numeric(currencyA, 10)
  .add(new Numeric(currencyB, 16))
  .toBase(10)
  .toString();
  const customPriceSafe = conversionGreaterThan(
    {
      value: customGasPrice,
      fromNumericBase: 'hex',
      fromDenomination: 'WEI',
      toDenomination: 'GWEI',
    },
    { value: estimatedPrice, fromNumericBase: 'dec' },
  );

  return customPriceSafe;
}

And this one would be expressed like:

const customPriceSafe = new Numeric(customGasPrice, 16, EtherDenomination.WEI)
  .toDenomination(EtherDenomination.ETH)
  .greaterThan(new Numeric(estimatedPrice, 10));

As you can see its much easier to understand because each chain in the operation expresses the order of operations instead of being baked into the application code.

Fixes #16211
Fixes #15463
Fixes #17211
Progresses #6290

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@brad-decker brad-decker changed the base branch from develop to convert-transaction-constants January 17, 2023 16:06
@brad-decker brad-decker force-pushed the convert-transaction-constants branch from 9160788 to 8260271 Compare January 17, 2023 17:56
Base automatically changed from convert-transaction-constants to develop January 18, 2023 14:47
@brad-decker brad-decker force-pushed the level-36-ts branch 2 times, most recently from 9db6610 to 8efb871 Compare January 19, 2023 15:09
@brad-decker brad-decker marked this pull request as ready for review January 19, 2023 17:43
@brad-decker brad-decker requested a review from a team as a code owner January 19, 2023 17:43
@brad-decker brad-decker requested a review from mcmire January 19, 2023 17:43
pedronfigueiredo
pedronfigueiredo reviewed Jan 19, 2023
View reviewed changes
shared/modules/Numeric.ts Outdated
if (parts.length === 1) {
return false;
}
return parts.every((part) => isHexStringOrNegatedHexString(part));
Copy link
Contributor

Choose a reason for hiding this comment

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

this implementation will pattern match -<hex num>.-<hex num> as a decimal hex. Flagging to ensure it's intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a safety net. I don't think its actually valid hex to do hex.hex but BigNumber uses that notation internally for hexadecimal containing a decimal point when stringified.... it seems... There should be no other cases where we get 'hex'.'hex' other than as a result of toString on a BigNumber which will always be re-interpretable by BigNumber during instantiation. We should add tests for this soon but as of right now I am of the mind that this is okay.

@brad-decker brad-decker requested a review from kumavis as a code owner January 19, 2023 20:32
@metamaskbot
Copy link
Collaborator

Builds ready [7ed213f]
Page Load Metrics (1314 ± 122 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint843271265024
domContentLoaded91118271298260125
load92118271314254122
domInteractive91118271298260125
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -32331 bytes
  • ui: 51837 bytes
  • common: 29601 bytes

highlights:

storybook

Gudahtt
Gudahtt reviewed Jan 20, 2023
View reviewed changes
shared/modules/Numeric.ts Outdated
@@ -0,0 +1,621 @@
import { BigNumber } from 'bignumber.js';
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to break this PR up into stages, to make it easier to review?

e.g.

  • Add this new Numeric module
  • Migrating code from conversion.utils.js to Numeric in chunks
  • Delete conversion.utils.js

That might make this less intimidating to review. And we can more effectively assign each chunk to the people most familiar with that code (e.g. we could ask the swaps team to just review the swaps changes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gudahtt I will break this down some but I don't know that I can break it down to the level of granularity that would be the easiest to review. I say this for the sake of expediency with hitting our goals for Q1 within the allotted time we have for TS as well because there are some order of operations issues. @DDDDDanica also asked for this, so cc'ing her for acknowledgement and thanking her for pushing back as well.

Add this new Numeric module

This would be easy to split out, considering it wouldn't be used anywhere yet.

Migrating code from conversion.utils.js to Numeric in chunks

So to do this I would need to redo some things because I converted conversion.utils.js to typescript first to get type feedback. To be able to merge this in chunks I would need to migrate to Numeric in chunks in JavaScript because if i attempt to do this in TypeScript it will not be mergeable due to lint / compile errors. So this would involve more rework than I think if sensible with the time constraints mentioned. However...

There's the fact that I relocated a bunch of methods that use conversionUtil from other parts of the codebase to the conversion.utils.js file. That would be a more meaningful break-up strategy and would mitigate a lot of LOC from this PR. I already did this PR breakdown here: #17319

I will see what else can be shaved off into separate PRs.

Copy link
Member

@Gudahtt Gudahtt Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh sorry, I explained that poorly. I meant to say:

Migrating code that uses conversion.utils.js to use Numeric instead, in chunks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gudahtt I think i've got it fairly granular now. Some things have already merged, namely Numeric module and the relocation of various helper methods to the conversion.utils.js shared file. I will rename this PR and move from draft when it comes up but it now only contains the changes from the conversion.utils.js -> conversion.utils.ts conversion.

@metamaskbot
Copy link
Collaborator

Builds ready [f5ec0cc]
Page Load Metrics (1280 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint98173122199
domContentLoaded9461562125418388
load9891562128019091
domInteractive9461562125418388
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -32331 bytes
  • ui: 51837 bytes
  • common: 29601 bytes

highlights:

storybook

@codecov-commenter
Copy link

Codecov Report

Merging #17262 (f5ec0cc) into develop (5c087d6) will decrease coverage by 0.10%.
The diff coverage is 59.00%.

@@             Coverage Diff             @@
##           develop   #17262      +/-   ##
===========================================
- Coverage    59.79%   59.69%   -0.10%     
===========================================
  Files          936      937       +1     
  Lines        36142    35982     -160     
  Branches      9286     9232      -54     
===========================================
- Hits         21611    21478     -133     
+ Misses       14531    14504      -27
Impacted Files Coverage Δ
app/scripts/controllers/incoming-transactions.js 89.38% <ø> (+0.09%) ⬆️
app/scripts/lib/local-store.js 55.10% <0.00%> (ø)
app/scripts/metamask-controller.js 59.95% <ø> (+0.03%) ⬆️
shared/constants/transaction.ts 100.00% <ø> (ø)
shared/lib/metamask-controller-utils.js 66.67% <ø> (-13.33%) ⬇️
shared/lib/swaps-utils.js 66.67% <ø> (ø)
...ed-gas-fee-inputs/base-fee-input/base-fee-input.js 100.00% <ø> (ø)
...ee-inputs/priority-fee-input/priority-fee-input.js 97.30% <ø> (ø)
...vanced-gas-inputs/advanced-gas-inputs.container.js 100.00% <ø> (ø)
...ion/confirm-page-container-navigation.component.js 75.00% <ø> (ø)
... and 75 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

DDDDDanica
DDDDDanica reviewed Jan 20, 2023
View reviewed changes
@brad-decker brad-decker mentioned this pull request Jan 20, 2023
Merged
8 tasks
@brad-decker
Copy link
Contributor Author

Converting to draft for now. Thanks @pedronfigueiredo and @DDDDDanica for initial reviews

@brad-decker brad-decker marked this pull request as draft January 20, 2023 15:16
@metamaskbot
Copy link
Collaborator

Builds ready [c9cadaf]
Page Load Metrics (2853 ± 527 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint11927989991051505
domContentLoaded1413490328341108532
load1414490328531097527
domInteractive1413490328341108532
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -32331 bytes
  • ui: 51837 bytes
  • common: 29601 bytes

highlights:

storybook

@brad-decker brad-decker mentioned this pull request Jan 20, 2023
Merged
5 tasks
@brad-decker brad-decker force-pushed the level-36-ts branch 2 times, most recently from f937859 to 78a2bf9 Compare January 20, 2023 17:32
@brad-decker brad-decker changed the base branch from develop to add-numeric-module January 20, 2023 17:41
@metamaskbot
Copy link
Collaborator

Builds ready [78a2bf9]
Page Load Metrics (1482 ± 102 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97157125178
domContentLoaded113418081451219105
load113418421482212102
domInteractive113418081451219105
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -32627 bytes
  • ui: -4150 bytes
  • common: 92478 bytes

highlights:

storybook

@brad-decker brad-decker mentioned this pull request Jan 20, 2023
Merged
8 tasks
Base automatically changed from add-numeric-module to develop January 20, 2023 21:16
@brad-decker brad-decker changed the base branch from develop to convert-app-utils-to-ts January 20, 2023 21:22
@brad-decker brad-decker changed the base branch from convert-app-utils-to-ts to conversion-util-replacement-ui January 20, 2023 21:36
@metamaskbot
Copy link
Collaborator

Builds ready [731d4b0]
Page Load Metrics (1467 ± 115 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint108171128178
domContentLoaded104718361439243117
load106418361467239115
domInteractive104618351439243117
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -32627 bytes
  • ui: -4150 bytes
  • common: 92478 bytes

highlights:

storybook

@brad-decker brad-decker force-pushed the conversion-util-replacement-ui branch from 20c29cd to d22468e Compare January 23, 2023 19:32
Base automatically changed from conversion-util-replacement-ui to develop January 24, 2023 14:44
@brad-decker brad-decker changed the title Level 36 TS dashboard migration (conversion.utils.js and app/scripts/lib/utils.js) Complete level 36 migration of TypeScript by converting conversion.utils.js and deprecate old conversion util Jan 24, 2023
@brad-decker brad-decker marked this pull request as ready for review January 24, 2023 14:47
DDDDDanica
DDDDDanica previously approved these changes Jan 24, 2023
View reviewed changes
@metamaskbot
Copy link
Collaborator

Builds ready [287650f]
Page Load Metrics (1309 ± 128 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint941801282211
domContentLoaded100419161281258124
load100419161309267128
domInteractive100419161281258124
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 bytes
  • ui: 802 bytes
  • common: -3567 bytes

mcmire
mcmire reviewed Jan 24, 2023
View reviewed changes
jpuri
jpuri previously approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

Great work 👍

jpuri
jpuri previously approved these changes Jan 24, 2023
View reviewed changes
mcmire
mcmire approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I assume that we plan on adding more tests in a future PR? Either way, big kudos for tackling this. I really like the improvements here, and the Numeric class seems like a much better design than before.

@metamaskbot
Copy link
Collaborator

Builds ready [1959a6d]
Page Load Metrics (1259 ± 132 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint911931243014
domContentLoaded97620521256274132
load97820621259275132
domInteractive97620521256274132
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 bytes
  • ui: 802 bytes
  • common: -3567 bytes

@brad-decker brad-decker merged commit f29683c into develop Jan 24, 2023
@brad-decker brad-decker deleted the level-36-ts branch January 24, 2023 18:49
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Gudahtt Gudahtt Gudahtt left review comments

@mcmire mcmire mcmire approved these changes

@DDDDDanica DDDDDanica DDDDDanica approved these changes

@jpuri jpuri jpuri left review comments

@pedronfigueiredo pedronfigueiredo pedronfigueiredo left review comments

@kumavis kumavis Awaiting requested review from kumavis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@brad-decker @metamaskbot @codecov-commenter @mcmire @jpuri @Gudahtt @DDDDDanica @pedronfigueiredo