Fixing incorrectly typed token decimal attribute by ryanml · Pull Request #10666 · MetaMask/metamask-extension

ryanml · 2021-03-17T06:31:06Z

Essentially if you are to add an NFT, or something with a 0 decimal value that does not allow for override, that gets saved to the state as "0" as opposed to 0, so we causing some component issues. Migrations added to back-fix the issue.

Manual testing steps:
Defined in issue

metamaskbot · 2021-03-17T06:46:44Z

Builds ready [c8b5794]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (1277 ± 101 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	1038	1851	1273	208	100
		load	1040	1866	1277	209	101
		domInteractive	1037	1850	1272	208	100

danjm · 2021-03-17T13:10:51Z

app/scripts/migrations/054.js

+
+function transformState(state) {
+  const newState = state;
+


A migration of accountTokens will also be needed. Check out this PR which is also about to migrate tokens and accountTokens: https://github.com/MetaMask/metamask-extension/pull/10593/files#diff-0ee44923cc3a183d3803fea2022e1b17f127940f603909b954588cc7c7aa2439R47

And for reference on what those are, see this thread: https://consensys.slack.com/archives/GTQAGKY5V/p1615395521395500?thread_ts=1615332992.380200&cid=GTQAGKY5V

Good catch here 👍 I believe having these incorrectly typed in part of accountTokens does not produce the UI error in question, but for structural parity they should absolutely be the same.

I see that this is addressed now, but for future reference: it does end up indirectly causing the problem, because tokens gets derived from accountTokens whenever the user switches accounts or networks.

@Gudahtt - ah, thank you for pointing this out

danjm

I left one comment that needs to be addressed

ryanml · 2021-03-17T18:45:18Z

app/scripts/migrations/054.js

+  }
+  newState.PreferencesController.tokens = tokens;
+
+  const { accountTokens } = newState.PreferencesController || {};


I tried to clean up this pyramid but the linter wasn't having it ;)

metamaskbot · 2021-03-17T19:01:42Z

Builds ready [84facd5]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (807 ± 80 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	585	1074	805	165	79
		load	586	1075	807	166	80
		domInteractive	584	1074	804	165	79

danjm

LGTM

app/scripts/migrations/054.js

Gudahtt · 2021-03-18T17:54:39Z

app/scripts/migrations/054.js

+  const { tokens } = newState.PreferencesController || [];
+  if (Array.isArray(tokens)) {
+    for (const token of tokens) {
+      if (token.decimals === '0') {


Are you sure that 0 was the only possible string? 🤔 We could check the type here instead.

The same goes for this comparison in the accountTokens loop as well of course

@Gudahtt - 0 was the only default decimal value I had seen occur during testing of numerous NFT's, but I think you're right that things should extend to check for strings in general. I can make needed updates here

per our conversation, we are now doing general string primitive checks, as well as looking for an potential corruption that could result in being unable to perform a numerical conversion.

app/scripts/migrations/054.js

metamaskbot · 2021-03-18T22:03:21Z

Builds ready [d2af4f2]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (897 ± 100 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	50	141	81	21	10
		domContentLoaded	563	1174	895	209	100
		load	569	1175	897	209	100
		domInteractive	563	1174	895	209	100

ryanml · 2021-03-19T03:44:57Z

app/scripts/migrations/054.js

+  for (const token of validTokens) {
+    // In the case of a decimal value type string, set to 0.
+    if (typeof token.decimals === 'string') {
+      // eslint-disable-next-line radix


note, per discussion with @Gudahtt we'll let the radix be inferred

metamaskbot · 2021-03-19T04:01:13Z

Builds ready [4e3d223]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (591 ± 11 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	68	54	8	4
		domContentLoaded	541	638	589	23	11
		load	542	639	591	23	11
		domInteractive	540	637	589	23	11

Gudahtt

LGTM! Couple of comment issues and one suggestion, but looks good otherwise.

app/scripts/migrations/054.js

Gudahtt

LGTM!

metamaskbot · 2021-03-19T17:08:32Z

Builds ready [2d32d6f]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (600 ± 14 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	48	84	55	9	4
		domContentLoaded	544	661	597	29	14
		load	545	663	600	29	14
		domInteractive	543	661	597	29	14

ryanml requested a review from a team as a code owner March 17, 2021 06:31

ryanml requested a review from danjm March 17, 2021 06:31

danjm reviewed Mar 17, 2021

View reviewed changes

danjm suggested changes Mar 17, 2021

View reviewed changes

ryanml force-pushed the token-demical-type-fix branch from c8b5794 to 84facd5 Compare March 17, 2021 18:44

ryanml requested a review from danjm March 17, 2021 18:45

ryanml commented Mar 17, 2021

View reviewed changes

ryanml requested a review from Gudahtt March 17, 2021 22:10

danjm previously approved these changes Mar 18, 2021

View reviewed changes

Gudahtt reviewed Mar 18, 2021

View reviewed changes

ryanml dismissed danjm’s stale review via d2af4f2 March 18, 2021 21:47

ryanml force-pushed the token-demical-type-fix branch from 84facd5 to d2af4f2 Compare March 18, 2021 21:47

ryanml requested a review from Gudahtt March 18, 2021 21:49

ryanml force-pushed the token-demical-type-fix branch from d2af4f2 to 4e3d223 Compare March 19, 2021 03:44

ryanml commented Mar 19, 2021

View reviewed changes

Gudahtt previously approved these changes Mar 19, 2021

View reviewed changes

app/scripts/migrations/054.js Outdated Show resolved Hide resolved

app/scripts/migrations/054.js Outdated Show resolved Hide resolved

app/scripts/migrations/054.js Outdated Show resolved Hide resolved

ryanml dismissed Gudahtt’s stale review via 6040195 March 19, 2021 16:41

ryanml force-pushed the token-demical-type-fix branch from 4e3d223 to 6040195 Compare March 19, 2021 16:41

ryanml requested a review from Gudahtt March 19, 2021 16:42

Gudahtt reviewed Mar 19, 2021

View reviewed changes

app/scripts/migrations/054.js Outdated Show resolved Hide resolved

Fixing incorrectly typed token decimal attribute

2d32d6f

ryanml force-pushed the token-demical-type-fix branch from 6040195 to 2d32d6f Compare March 19, 2021 16:49

ryanml requested a review from Gudahtt March 19, 2021 16:51

Gudahtt approved these changes Mar 19, 2021

View reviewed changes

ryanml merged commit 530e8c1 into develop Mar 19, 2021

ryanml deleted the token-demical-type-fix branch March 19, 2021 17:17

github-actions bot locked and limited conversation to collaborators Mar 19, 2021

Conversation

ryanml commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented Mar 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 17, 2021

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gudahtt Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metamaskbot commented Mar 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 19, 2021

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryanml commented Mar 17, 2021 •

edited

Loading

Gudahtt Mar 18, 2021 •

edited

Loading