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.

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

Use send state for send flow token #8695

Merged
merged 2 commits into from
May 29, 2020
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 29, 2020

The chosen token in the send flow was set from one of two places: metamask.selectedTokenAddress or metamask.send.token. The former is used most of the time, but the latter is used for the 'Edit' button shown in the upper-left of the confirmation UI.

The send flow will now exclusively use metamask.send.token for the token state during the send flow. metamask.selectedTokenAddress is now only used for the selected token state on the Home screen. This simplifies the Redux state, as the send token is now in one place instead of two, and metamask.selectedTokenAddress has only one purpose.

@metamaskbot
Copy link
Collaborator

Builds ready [c177e0e]
Page Load Metrics (911 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint35125682814
domContentLoaded495102290910751
load497102891110751
domInteractive494102190910751

@Gudahtt Gudahtt force-pushed the use-send-token-in-send-flow branch 2 times, most recently from a04c9cc to 2d3f2ea Compare May 29, 2020 02:38
@Gudahtt Gudahtt marked this pull request as ready for review May 29, 2020 02:42
@Gudahtt Gudahtt requested a review from a team as a code owner May 29, 2020 02:42
@metamaskbot
Copy link
Collaborator

Builds ready [2d3f2ea]
Page Load Metrics (716 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29124572813
domContentLoaded42984071515374
load43184271615374
domInteractive42983971415374

The chosen token in the `send` flow was set from one of two places:
`metamask.selectedTokenAddress` or `metamask.send.token`. The former is
used most of the time, but the latter is used for the 'Edit' button
shown in the upper-left of the confirmation UI.

The send flow will now exclusively use `metamask.send.token` for the
token state during the send flow. `metamask.selectedTokenAddress` is
now only used for the selected token state on the Home screen. This
simplifies the Redux state, as the send token is now in one place
instead of two, and `metamask.selectedTokenAddress` has only one
purpose.
@Gudahtt Gudahtt force-pushed the use-send-token-in-send-flow branch from 2d3f2ea to 091e782 Compare May 29, 2020 03:07
@Gudahtt Gudahtt mentioned this pull request May 29, 2020
@metamaskbot
Copy link
Collaborator

Builds ready [091e782]
Page Load Metrics (615 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31443542
domContentLoaded3347066138239
load3357086158239
domInteractive3347066138239

brad-decker
brad-decker previously approved these changes May 29, 2020
rekmarks
rekmarks previously approved these changes May 29, 2020
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

The changes herein look great, but we should probably take the opportunity to undo a grievous injury to the English language.

@@ -55,7 +55,7 @@ describe('add-recipient utils', function () {
})
})

it('should null if to is truthy part of tokens but selectedToken falsy', function () {
it('should null if to is truthy part of tokens but sendToken falsy', function () {
Copy link
Member

Choose a reason for hiding this comment

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

This PR did not commit the original sin here, but what the hell is this test description?

Ditto for the other cases in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

/giphy say what?

Copy link
Member Author

Choose a reason for hiding this comment

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

I started addressing this, but it got bigger than I had anticipated. I'll address this separately

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been addressed in #8705

ui/app/selectors/send.js Show resolved Hide resolved
@@ -38,7 +38,6 @@ const mapDispatchToProps = (dispatch) => {
toNumericBase: 'hex',
})

dispatch(setSelectedToken(tokenAddress))
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this was removed because its purpose was to set the token correctly on the edit screen, and that's handled now by the token below being passed to updateSend.

@Gudahtt Gudahtt dismissed stale reviews from rekmarks and brad-decker via f62d43c May 29, 2020 17:13
@metamaskbot
Copy link
Collaborator

Builds ready [f62d43c]
Page Load Metrics (612 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint319641178
domContentLoaded34475461012761
load34575661212761
domInteractive34475461012761

@Gudahtt Gudahtt merged commit ddaa492 into develop May 29, 2020
@Gudahtt Gudahtt deleted the use-send-token-in-send-flow branch May 29, 2020 17:46
Gudahtt added a commit that referenced this pull request Jun 1, 2020
* origin/develop: (689 commits)
  Implement asset page (#8696)
  fix crash on signature request (#8709)
  Fix accounts menu styling (#8707)
  Delete docs/porting_to_new_environment.md (#8704)
  Remove unused `getToErrorObject` parameters (#8705)
  hide connected-status on metamask ext (#8703)
  Stop adding permissions middleware to trusted connections (#8701)
  Use `send` state for send flow token (#8695)
  do not display extension id in connection modal (#8699)
  Fix tab content disappearing during scrolling on macOS Firefox (#8702)
  close details when button is pressed (#8694)
  Refactor token selectors (#8671)
  Update eth_accounts permission description (#8693)
  Extract selected token from token input (#8692)
  Fix propType for Home defaultHomeActiveTabName (#8683)
  Fix create account form styling (#8689)
  Remove unused `getSelectedTokenAssetImage` selector (#8691)
  Remove `getTxParams` (#8676)
  do not show account mismatch alert on details (#8678)
  Fix connect hardware styling (#8680)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants