feat(23012): convert 2 level 19 files to ts #23276

DDDDDanica · 2024-03-01T19:29:40Z

Description

In order to contribute to ts conversion of metamask-controller.js, we started the conversion of related files.
shared/modules/transaction.utils.ts and shared/notifications/index.ts will be 2 lower level files we included here.

Related issues

Fixes:

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-03-01T19:29:52Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

shared/modules/transaction.utils.ts

metamaskbot · 2024-03-05T16:37:01Z

Builds ready [2d08eed]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (968 ± 428 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	74	209	121	37	18
		domContentLoaded	12	72	29	16	8
		load	60	2072	968	892	428
		domInteractive	12	72	29	16	8

Bundle size diffs [🚀 Bundle size reduced!]

background: -5 Bytes (-0.00%)
ui: 0 Bytes (0.00%)
common: -296 Bytes (-0.01%)

codecov · 2024-03-05T16:37:14Z

Codecov Report

Attention: Patch coverage is 75.00000% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 68.74%. Comparing base (318a8ce) to head (00694ce).
Report is 7 commits behind head on develop.

❗ Current head 00694ce differs from pull request most recent head 5d8c134. Consider uploading reports for the commit 5d8c134 to get more accurate results

Files	Patch %	Lines
shared/notifications/index.ts	44.44%	10 Missing ⚠️
shared/modules/transaction.utils.ts	86.11%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #23276      +/-   ##
===========================================
+ Coverage    68.67%   68.74%   +0.07%     
===========================================
  Files         1106     1106              
  Lines        43356    43307      -49     
  Branches     11591    11573      -18     
===========================================
- Hits         29773    29769       -4     
+ Misses       13583    13538      -45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shared/modules/contract-utils.ts

app/scripts/lib/transaction/metrics.ts

shared/modules/transaction.utils.ts

shared/notifications/index.ts

metamaskbot · 2024-03-06T10:37:31Z

Builds ready [4245085]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1188 ± 397 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	80	178	122	30	15
		domContentLoaded	14	66	30	14	7
		load	67	2124	1188	826	397
		domInteractive	14	66	30	14	7

shared/modules/transaction.utils.ts

zone-live

LGTM

shared/modules/transaction.utils.ts

zone-live · 2024-03-08T09:31:34Z

shared/modules/transaction.utils.ts

      try {
-        ({ name } = data && parseStandardTokenTransactionData(data));
+        const parsedData = data && parseStandardTokenTransactionData(data);
+        if (parsedData && parsedData.name) {


a lot better for readability 👍🏼

metamaskbot · 2024-03-08T09:57:56Z

Builds ready [00694ce]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1521 ± 396 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	83	213	144	34	16
		domContentLoaded	10	88	39	23	11
		load	68	2308	1521	824	396
		domInteractive	10	88	39	23	11

Bundle size diffs [🚀 Bundle size reduced!]

background: -5 Bytes (-0.00%)
ui: 0 Bytes (0.00%)
common: -296 Bytes (-0.01%)

MajorLift · 2024-03-11T02:29:17Z

shared/modules/transaction.utils.ts

  // If the transaction type is already one of the inferrable types, then we do
  // not need to re-establish the type.
  let inferrableType = txMeta.type;
-  if (INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type) === false) {
+  if (!INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type as TransactionType)) {


Let's resolve this by letting the compiler narrow the type (+ adding a runtime check) instead of relying on a type assertion.

Suggested change

if (!INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type as TransactionType)) {

if (txMeta.type && !INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type)) {

This added null check removes the | undefined from the type with runtime guarantees.

The type assertion is dangerous because it fixes the entire type as TransactionType, rather than only removing a portion of the type signature. For example, if we defined a TransactionMetaType somewhere down the road, the as TransactionType assertion would prevent the compiler from inferring that type, while a null check or non-nullable assertion would not.

Thanks for the advice, will add ! see 5d8c13

MajorLift · 2024-03-11T02:38:52Z

shared/modules/transaction.utils.ts

+    isHexString(transactionMeta?.txParams?.maxFeePerGas as string) &&
+    isHexString(transactionMeta?.txParams?.maxPriorityFeePerGas as string)


We could guarantee that undefined is never passed into isHexString by providing fallback values that won't pollute the arguments' type signatures. This provides runtime safety while the type assertion does not.

Suggested change

isHexString(transactionMeta?.txParams?.maxFeePerGas as string) &&

isHexString(transactionMeta?.txParams?.maxPriorityFeePerGas as string)

isHexString(transactionMeta?.txParams?.maxFeePerGas ?? '') &&

isHexString(transactionMeta?.txParams?.maxPriorityFeePerGas ?? '')

shared/modules/transaction.utils.ts

MajorLift · 2024-03-11T13:37:28Z

shared/modules/transaction.utils.ts

+        const parsedData = data && parseStandardTokenTransactionData(data);
+        if (parsedData && parsedData.name) {


Using && to assert non-nullability like this is not a good pattern, since it pollutes the type signature of the assignee.

In this case parsedData gets stuck with a falsy "" type which is neither useful nor actually representative of its expected behavior.

Could we narrow parsedData down to TransactionDescription | undefined with a ternary expression? That way we can just use optional chaining for the null check.

Suggested change

const parsedData = data && parseStandardTokenTransactionData(data);

if (parsedData && parsedData.name) {

const parsedData = data

? parseStandardTokenTransactionData(data)

: undefined;

if (parsedData?.name) {

Yeah of course ! it is definitely a better type check !

shared/notifications/index.ts

DDDDDanica · 2024-03-13T01:48:45Z

Reopened here to be able to add changes
#23445

DDDDDanica added the team-extension-platform Extension Platform team label Mar 1, 2024

DDDDDanica self-assigned this Mar 1, 2024

DDDDDanica requested a review from a team as a code owner March 1, 2024 19:29

DDDDDanica force-pushed the feature/23012 branch 13 times, most recently from e75f733 to 5ffd231 Compare March 5, 2024 02:34

legobeat reviewed Mar 5, 2024

View reviewed changes

shared/modules/transaction.utils.ts Outdated Show resolved Hide resolved

DDDDDanica force-pushed the feature/23012 branch from 5ffd231 to 2d08eed Compare March 5, 2024 15:50

DDDDDanica commented Mar 5, 2024

View reviewed changes

shared/modules/contract-utils.ts Show resolved Hide resolved

DDDDDanica commented Mar 5, 2024

View reviewed changes

app/scripts/lib/transaction/metrics.ts Show resolved Hide resolved

DDDDDanica commented Mar 5, 2024

View reviewed changes

shared/modules/transaction.utils.ts Show resolved Hide resolved

DDDDDanica commented Mar 5, 2024

View reviewed changes

shared/notifications/index.ts Show resolved Hide resolved

feat(23012): convert 2 level 19 files to ts

4245085

DDDDDanica force-pushed the feature/23012 branch from 2d08eed to 4245085 Compare March 6, 2024 10:12

metamaskbot added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 6, 2024

legobeat reviewed Mar 6, 2024

View reviewed changes

shared/modules/transaction.utils.ts Outdated Show resolved Hide resolved

legobeat requested a review from a team March 7, 2024 22:35

zone-live previously approved these changes Mar 8, 2024

View reviewed changes

Merge branch 'develop' into feature/23012

00694ce

MajorLift reviewed Mar 11, 2024

View reviewed changes

shared/modules/transaction.utils.ts Outdated Show resolved Hide resolved

MajorLift reviewed Mar 11, 2024

View reviewed changes

shared/notifications/index.ts Show resolved Hide resolved

DDDDDanica dismissed zone-live’s stale review via 5d8c134 March 12, 2024 13:34

DDDDDanica force-pushed the feature/23012 branch 2 times, most recently from f0a044d to 0a94eac Compare March 12, 2024 22:20

feat(23012): change types based on comments

0a94eac

DDDDDanica closed this Mar 13, 2024

github-actions bot locked and limited conversation to collaborators Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(23012): convert 2 level 19 files to ts #23276

feat(23012): convert 2 level 19 files to ts #23276

DDDDDanica commented Mar 1, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Mar 1, 2024

metamaskbot commented Mar 5, 2024

codecov bot commented Mar 5, 2024 •

edited

Loading

metamaskbot commented Mar 6, 2024

zone-live left a comment

zone-live Mar 8, 2024

metamaskbot commented Mar 8, 2024

MajorLift Mar 11, 2024 •

edited

Loading

DDDDDanica Mar 12, 2024 •

edited

Loading

MajorLift Mar 11, 2024 •

edited

Loading

DDDDDanica Mar 12, 2024

MajorLift Mar 11, 2024 •

edited

Loading

DDDDDanica Mar 12, 2024

DDDDDanica Mar 12, 2024

DDDDDanica commented Mar 13, 2024

	if (!INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type as TransactionType)) {
	if (txMeta.type && !INFERRABLE_TRANSACTION_TYPES.includes(txMeta.type)) {

		isHexString(transactionMeta?.txParams?.maxFeePerGas as string) &&
		isHexString(transactionMeta?.txParams?.maxPriorityFeePerGas as string)

		const parsedData = data && parseStandardTokenTransactionData(data);
		if (parsedData && parsedData.name) {

feat(23012): convert 2 level 19 files to ts #23276

feat(23012): convert 2 level 19 files to ts #23276

Conversation

DDDDDanica commented Mar 1, 2024 • edited by github-actions bot Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Mar 1, 2024

metamaskbot commented Mar 5, 2024

codecov bot commented Mar 5, 2024 • edited Loading

Codecov Report

metamaskbot commented Mar 6, 2024

zone-live left a comment

Choose a reason for hiding this comment

zone-live Mar 8, 2024

Choose a reason for hiding this comment

metamaskbot commented Mar 8, 2024

MajorLift Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

DDDDDanica Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

MajorLift Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

DDDDDanica Mar 12, 2024

Choose a reason for hiding this comment

MajorLift Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

DDDDDanica Mar 12, 2024

Choose a reason for hiding this comment

DDDDDanica Mar 12, 2024

Choose a reason for hiding this comment

DDDDDanica commented Mar 13, 2024

DDDDDanica commented Mar 1, 2024 •

edited by github-actions bot

Loading

codecov bot commented Mar 5, 2024 •

edited

Loading

MajorLift Mar 11, 2024 •

edited

Loading

DDDDDanica Mar 12, 2024 •

edited

Loading

MajorLift Mar 11, 2024 •

edited

Loading

MajorLift Mar 11, 2024 •

edited

Loading