Refactor send page state management #10965

brad-decker · 2021-04-30T20:49:18Z

Refactor Send Page State Management

Problem Statements

The "state" of the send page is not centralized making it near impossible to understand the logic because each component owns it's own impact on the state. The result was the same overall effect being applied in multiple places (ex: updating the max amount when the max button is clicked, AND anytime the gas field is modified AND anytime the asset was changed, each implementation in it's respective component's componentDidUpdate/lifecycle methods)
The Components that make up the UI are too complex due to being the sole managers of state, making it near impossible to refactor or update.
Changes required for future logic updates would need to be carried through many different places

Design goals of refactor

No functional change to the UX.
Centralize all state update logic as it pertains to the send page into a send slice using redux-toolkit.
keep metamask/background state and send page state clearly separated.
capture the intent of the send page, which is to create a draft transaction.
eliminate any dead code found or created by way of this refactor
adhere to the redux-toolkit design guide as much as possible

jsconfig.json

app/scripts/controllers/transactions/index.js

ui/ducks/app/app.js

ui/ducks/app/app.test.js

ui/ducks/metamask/metamask.js

ui/pages/send/send-footer/send-footer.component.test.js

ui/pages/send/send.container.js

ui/pages/send/send.utils.js

ui/selectors/send.js

ui/store/actions.js

metamaskbot · 2021-05-03T16:32:46Z

Builds ready [d9d759a]

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 (776 ± 51 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	60	107	73	11	5
		domContentLoaded	487	897	774	106	51
		load	488	898	776	106	51
		domInteractive	486	896	774	106	51

ui/ducks/send/send.js

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.container.js

test/e2e/metamask-ui.spec.js

ui/ducks/send/send.js

ui/store/store.js

ui/ducks/ens.js

ui/pages/send/send.component.js

ui/pages/send/send.js

ui/pages/send/send.test.js

ui/pages/send/send-footer/send-footer.container.js

ui/ducks/send/send.js

ui/pages/send/send-content/send-amount-row/send-amount-row.component.js

ui/pages/send/send-content/send-amount-row/amount-max-button/amount-max-button.component.js

ui/pages/send/send-content/add-recipient/ens-input.component.js

ui/pages/send/send-content/add-recipient/add-recipient.component.js

ui/pages/send/send-content/add-recipient/add-recipient.component.test.js

ui/helpers/utils/token-util.js

ui/ducks/send/send.js

metamaskbot · 2021-06-22T17:34:52Z

Builds ready [8ef1d5e]

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 (601 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	72	60	9	4
		domContentLoaded	398	717	599	88	42
		load	399	718	601	88	42
		domInteractive	398	717	599	88	42

ui/pages/send/send-footer/send-footer.component.js

ui/ducks/send/send.js

metamaskbot · 2021-06-22T19:14:34Z

Builds ready [7209ec6]

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 (613 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	52	76	63	7	3
		domContentLoaded	410	736	612	109	52
		load	411	737	613	109	52
		domInteractive	409	735	612	109	52

Gudahtt · 2021-06-22T21:41:02Z

Everything looks good to me! I can re-review after the rebase

metamaskbot · 2021-06-22T23:02:26Z

Builds ready [849ca6a]

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 (673 ± 56 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	52	85	65	9	4
		domContentLoaded	419	819	672	117	56
		load	421	821	673	117	56
		domInteractive	419	819	671	117	56

adonesky1 · 2021-06-23T16:23:38Z

ui/store/actions.js

@@ -2738,6 +2723,16 @@ export function estimateGas(params) {
  return promisifiedBackground.estimateGas(params);
 }

+export async function updateTokenType(tokenAddress) {
+  let token = {};
+  try {


Guess you felt loading indicator wasn't required here?

its handled at the callsite now, the loading indicator is shown anytime we updateAsset to a token asset.

metamaskbot · 2021-06-23T16:35:08Z

Builds ready [f7bb6c3]

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 (569 ± 46 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	67	58	8	4
		domContentLoaded	380	647	568	96	46
		load	382	649	569	96	46
		domInteractive	380	647	567	96	46

adonesky1 · 2021-06-23T17:16:23Z

ui/ducks/send/send.js

@@ -44,6 +44,7 @@ import {
  hideLoadingIndication,
  showConfTxPage,
  showLoadingIndication,
+  updateTokenType,


So while trying to test this I re-found an issue similar to one I had been having while trying to tack a PR on top of the broad send-slice refactor.
Here it is, so entering the send flow the form status is immediately set to valid:

once, however, I click the dropdown to select another token, the form becomes invalid:

That is expected, but then I cannot get the form status to be valid again:

even if I go back and select Eth as the send asset:

I believe it has something to do with sequencing of actions and the gas loading thunk. The last call to validateSendState sets us to invalid here.

can you try 3d8ca2d out for a spin? see if the issue persists?

@brad-decker That commit seems to have fixed the issue, 🚀

adonesky1 · 2021-06-23T17:25:10Z

ui/ducks/send/send.js

@@ -843,6 +845,10 @@ const slice = createSlice({
        ):
          state.status = SEND_STATUSES.INVALID;
          break;
+        case state.asset.type === ASSET_TYPES.TOKEN &&


not req but wondering if it makes sense to separate this out so that asset has an error field, as we do with amount and gas?

i was thinking that when I was working through erc721 updates, i think its a definite candidate for an improvement but not to be handled here.

metamaskbot · 2021-06-23T17:35:39Z

Builds ready [3d8ca2d]

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 (614 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	93	63	11	5
		domContentLoaded	365	758	612	108	52
		load	366	759	614	108	52
		domInteractive	365	758	612	108	52

PeterYinusa · 2021-06-23T18:09:23Z

LGTM!

metamaskbot · 2021-06-23T18:19:40Z

Builds ready [0320dff]

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 (560 ± 33 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	69	61	6	3
		domContentLoaded	377	634	558	69	33
		load	379	636	560	69	33
		domInteractive	377	634	558	69	33

Gudahtt

LGTM!

adonesky1 · 2021-06-23T20:40:22Z

ui/pages/confirm-send-ether/confirm-send-ether.container.js

@@ -1,7 +1,7 @@
 import { connect } from 'react-redux';


At some point - not in this PR - we should consider renaming some of these files/components/containers that refer to ether towards more chain-agnostic language, like you do throughout this PR

darkwing

Tested this a bunch and the code looks good! Let's gooooooo!

adonesky1

Amazing work Brad! LGTM

brad-decker commented Apr 30, 2021

View reviewed changes

jsconfig.json Outdated Show resolved Hide resolved

brad-decker force-pushed the improve-send-slice branch 3 times, most recently from 07015e4 to d9d759a Compare May 3, 2021 15:36

brad-decker commented May 3, 2021

View reviewed changes

brad-decker marked this pull request as ready for review May 3, 2021 16:45

brad-decker requested a review from a team as a code owner May 3, 2021 16:45

brad-decker requested review from NiranjanaBinoy and Gudahtt May 3, 2021 16:45

Gudahtt reviewed May 3, 2021

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

Gudahtt reviewed May 3, 2021

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

Gudahtt reviewed May 3, 2021

View reviewed changes

ui/ducks/send/send.js Show resolved Hide resolved

adonesky1 reviewed May 3, 2021

View reviewed changes

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.container.js Show resolved Hide resolved

adonesky1 reviewed May 3, 2021

View reviewed changes

...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.container.js Outdated Show resolved Hide resolved

brad-decker force-pushed the improve-send-slice branch from 0aac00b to e19e5d9 Compare May 12, 2021 19:40

brad-decker changed the title ~~unravel send slice from metamask~~ Refactor send page state management May 12, 2021

brad-decker force-pushed the improve-send-slice branch 4 times, most recently from b284d46 to 294b8c6 Compare May 12, 2021 22:05

brad-decker mentioned this pull request May 12, 2021

Use hexToDecimal to parse gasLimit in send footer #10969

Closed

Gudahtt reviewed May 13, 2021

View reviewed changes

test/e2e/metamask-ui.spec.js Outdated Show resolved Hide resolved

brad-decker commented May 13, 2021

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

brad-decker commented May 13, 2021

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

brad-decker commented May 13, 2021

View reviewed changes

ui/ducks/send/send.js Show resolved Hide resolved

brad-decker commented May 13, 2021

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

Gudahtt reviewed May 13, 2021

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

ui/ducks/send/send.js Outdated Show resolved Hide resolved

ui/store/store.js Outdated Show resolved Hide resolved

ui/ducks/ens.js Show resolved Hide resolved

tmashuang mentioned this pull request May 13, 2021

Improve send slice tests #11091

Merged

brad-decker force-pushed the improve-send-slice branch from 294b8c6 to 60a4eee Compare May 17, 2021 21:54

Gudahtt reviewed Jun 22, 2021

View reviewed changes

ui/pages/send/send-footer/send-footer.component.js Outdated Show resolved Hide resolved

Gudahtt reviewed Jun 22, 2021

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

improve send slice using redux-toolkit

849ca6a

brad-decker force-pushed the improve-send-slice branch from 581597d to 849ca6a Compare June 22, 2021 21:48

brad-decker requested a review from Gudahtt June 22, 2021 21:49

Gudahtt previously approved these changes Jun 23, 2021

View reviewed changes

fixes for ERC721 token detection

f7bb6c3

brad-decker dismissed Gudahtt’s stale review via f7bb6c3 June 23, 2021 16:16

adonesky1 reviewed Jun 23, 2021

View reviewed changes

fix gasLimit loading invalidation

3d8ca2d

adonesky1 reviewed Jun 23, 2021

View reviewed changes

validate gas field after amount changes when sending native asset

0320dff

Gudahtt approved these changes Jun 23, 2021

View reviewed changes

adonesky1 reviewed Jun 23, 2021

View reviewed changes

darkwing approved these changes Jun 23, 2021

View reviewed changes

adonesky1 approved these changes Jun 23, 2021

View reviewed changes

brad-decker merged commit c30cb7d into develop Jun 23, 2021

brad-decker deleted the improve-send-slice branch June 23, 2021 21:35

github-actions bot locked and limited conversation to collaborators Jun 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor send page state management #10965

Refactor send page state management #10965

brad-decker commented Apr 30, 2021 •

edited

Loading

metamaskbot commented May 3, 2021

metamaskbot commented Jun 22, 2021

metamaskbot commented Jun 22, 2021

Gudahtt commented Jun 22, 2021

metamaskbot commented Jun 22, 2021

adonesky1 Jun 23, 2021

brad-decker Jun 23, 2021

metamaskbot commented Jun 23, 2021

adonesky1 Jun 23, 2021

brad-decker Jun 23, 2021

adonesky1 Jun 23, 2021

adonesky1 Jun 23, 2021

brad-decker Jun 23, 2021

metamaskbot commented Jun 23, 2021

PeterYinusa commented Jun 23, 2021

metamaskbot commented Jun 23, 2021

Gudahtt left a comment

adonesky1 Jun 23, 2021 •

edited

Loading

darkwing left a comment

adonesky1 left a comment

Refactor send page state management #10965

Refactor send page state management #10965

Conversation

brad-decker commented Apr 30, 2021 • edited Loading