Skip to content

Convert ConfirmTxScreen to an ES6 class#7790

Merged
whymarrh merged 4 commits intoMetaMask:developfrom
whymarrh:es6-classes-conf-tx
Jan 13, 2020
Merged

Convert ConfirmTxScreen to an ES6 class#7790
whymarrh merged 4 commits intoMetaMask:developfrom
whymarrh:es6-classes-conf-tx

Conversation

@whymarrh
Copy link
Copy Markdown
Contributor

@whymarrh whymarrh commented Jan 9, 2020

Refs #7766

This PR converts the ConfirmTxScreen component to an ES6 class.

@whymarrh whymarrh requested review from Gudahtt and danjm as code owners January 9, 2020 17:02
@whymarrh whymarrh mentioned this pull request Jan 9, 2020
Closed
19 tasks
Gudahtt
Gudahtt requested changes Jan 13, 2020
View reviewed changes
ui/app/pages/confirm-transaction/conf-tx.js Outdated
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt Jan 13, 2020

Choose a reason for hiding this comment

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

Hmm, this prop is never passed in 🤔
I guess it's correct to add it here, since it is used, but maybe we should delete it instead.

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt Jan 13, 2020

Choose a reason for hiding this comment

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

After further investigation, this and probably other props passed into SignatureRequest should be omitted, because they're retrieved separate in the SignatureRequest container component.
This can wait for a separate PR I suppose

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt Jan 13, 2020

Choose a reason for hiding this comment

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

I see that the same applies for accounts, and selectedAddress

Copy link
Copy Markdown
Contributor Author

@whymarrh whymarrh Jan 13, 2020

Choose a reason for hiding this comment

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

accounts is only pulled from state for one of the two components returned from signatureSelect. I'll remove selectedAddress and conversionRate though, those are both clearly redundant (and not actually passed in, as you noticed 🤦‍♂).

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt Jan 13, 2020

Choose a reason for hiding this comment

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

accounts isn't passed in either though, so if it is used in one of the signatureSelect components, it must not be very important, because it ain't there.

Copy link
Copy Markdown
Contributor Author

@whymarrh whymarrh Jan 13, 2020

Choose a reason for hiding this comment

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

Line 236 rn is accounts={this.props.accounts}, which passes it in?

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt Jan 13, 2020

Choose a reason for hiding this comment

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

I mean it's not passed into this component; it's not retrieved in mapStateToProps, nor is it passed in from above.

Copy link
Copy Markdown
Contributor Author

@whymarrh whymarrh Jan 13, 2020

Choose a reason for hiding this comment

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

Oh, heck. Good point.

Gudahtt
Gudahtt previously approved these changes Jan 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@whymarrh whymarrh force-pushed the es6-classes-conf-tx branch from 5736364 to 105407d Compare January 13, 2020 14:51
Gudahtt
Gudahtt previously approved these changes Jan 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

accounts does seem just as deserving of deletion to me as the other two properties, but this is still a step forward.

@whymarrh whymarrh force-pushed the es6-classes-conf-tx branch from 105407d to 27a2b90 Compare January 13, 2020 16:35
@whymarrh whymarrh merged commit 2c3b1a0 into MetaMask:develop Jan 13, 2020
@whymarrh whymarrh deleted the es6-classes-conf-tx branch January 13, 2020 17:02
yqrashawn pushed a commit to yqrashawn/conflux-portal that referenced this pull request Jan 15, 2020
@whymarrh @Gudahtt @yqrashawn
Convert ConfirmTxScreen to an ES6 class (MetaMask#7790)
15bd7c0 
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
yqrashawn pushed a commit to Conflux-Chain/conflux-portal that referenced this pull request Jan 15, 2020
@whymarrh @Gudahtt @yqrashawn
Convert ConfirmTxScreen to an ES6 class (MetaMask#7790)
f3f5797 
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2026
@mikesposito @Prithpal-Sooriya
bump: @metamask/assets-controllers from ^99.0.0 to ^99.3.1 cp-1…
d9dbb7c 
…3.18.0 (#39887)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
[skip-e2e]
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->
This PR bump `@metamask/assets-controllers` from `^99.0.0` to `^99.3.1`.
The current resolved version in the `yarn.lock` is `99.1.0`, which
includes a change that introduces a bug with Tempo native balances. A
fix has been released in `99.3.1`, so we need to bump to that version to
get the fix.

The cherry-pick into `13.18.0` is necessary as it includes the affected
version of `@metamask/assets-controllers`, and we don't want it to reach
production.

```markdown
## [99.3.1]

### Fixed

- Remove `Tempo Testnet` multicall address ([#7858](MetaMask/core#7858)).

## [99.3.0]

### Added

- Add optional `rwaData` support when adding tokens in `TokensController` ([#7804](MetaMask/core#7804)).

### Changed

- Lock `@metamask/core-backend` to `5.0.0` ([#7852](MetaMask/core#7852))
- Bump `@metamask/profile-sync-controller` from `^27.0.0` to `^27.1.0` ([#7849](MetaMask/core#7849))
- Fix trending tokens showing incorrect market cap values by adding `usePriceApiData` parameter (defaults to `true`) to use price API data for accurate market data ([#7829](MetaMask/core#7829))
- Bump `@metamask/transaction-controller` from `^62.13.0` to `^62.15.0` ([#7832](MetaMask/core#7832), [#7854](MetaMask/core#7854))

## [99.2.0]

### Added

- Add `HYPEREVM` support ([#7790](MetaMask/core#7790))
  - Add `HYPEREVM` in `SupportedTokenDetectionNetworks`
  - Add `HYPEREVM` in `SUPPORTED_NETWORKS_ACCOUNTS_API_V4`

### Changed

- Simplify TokenListController initialization ([#7740](MetaMask/core#7740))
- Bump `@metamask/storage-service` from `^0.0.1` to `^1.0.0` ([#7797](MetaMask/core#7797))
- Bump `@metamask/transaction-controller` from `^62.11.0` to `^62.13.0` ([#7775](MetaMask/core#7775), [#7802](MetaMask/core#7802))
- Bump `@metamask/preferences-controller` from `^22.0.0` to `^22.1.0` ([#7802](MetaMask/core#7802))

### Removed

- Removed token-search-discovery-controller package ([#7789](MetaMask/core#7789))
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/39887?quickstart=1)

## **Changelog**

<!--
If this PR is not End-User-Facing and should not show up in the
CHANGELOG, you can choose to either:
1. Write `CHANGELOG entry: null`
2. Label with `no-changelog`

If this PR is End-User-Facing, please write a short User-Facing
description in the past tense like:
`CHANGELOG entry: Added a new tab for users to see their NFTs`
`CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker`

(This helps the Release Engineer do their job more quickly and
accurately)
-->

CHANGELOG entry: null

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Dependency upgrade in a core controllers package can change
token/balance behavior and introduces transitive version shifts; risk is
mitigated by being a targeted bump intended to pick up a bugfix release.
> 
> **Overview**
> Bumps `@metamask/assets-controllers` from `^99.0.0` to `^99.3.1` and
refreshes `yarn.lock` to resolve to `99.3.1`.
> 
> Removes the Yarn patch previously applied to
`@metamask/assets-controllers@93.1.0` (and its patched exchange-rate
precision changes), relying on upstream package fixes instead; lockfile
also reflects updated transitive constraints (e.g., pinning
`@metamask/core-backend` to `5.0.0` and newer controller deps).
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
7457931. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: Prithpal Sooriya <prithpal.sooriya@consensys.net>
runway-github bot added a commit that referenced this pull request Feb 11, 2026
@runway-github @Prithpal-Sooriya
temp+bump: @metamask/assets-controllers from ^99.0.0 to ^99.3.1…
b79fa5d 
… cp-13.18.0 (#39887)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
[skip-e2e]
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->
This PR bump `@metamask/assets-controllers` from `^99.0.0` to `^99.3.1`.
The current resolved version in the `yarn.lock` is `99.1.0`, which
includes a change that introduces a bug with Tempo native balances. A
fix has been released in `99.3.1`, so we need to bump to that version to
get the fix.

The cherry-pick into `13.18.0` is necessary as it includes the affected
version of `@metamask/assets-controllers`, and we don't want it to reach
production.

```markdown
## [99.3.1]

### Fixed

- Remove `Tempo Testnet` multicall address ([#7858](MetaMask/core#7858)).

## [99.3.0]

### Added

- Add optional `rwaData` support when adding tokens in `TokensController` ([#7804](MetaMask/core#7804)).

### Changed

- Lock `@metamask/core-backend` to `5.0.0` ([#7852](MetaMask/core#7852))
- Bump `@metamask/profile-sync-controller` from `^27.0.0` to `^27.1.0` ([#7849](MetaMask/core#7849))
- Fix trending tokens showing incorrect market cap values by adding `usePriceApiData` parameter (defaults to `true`) to use price API data for accurate market data ([#7829](MetaMask/core#7829))
- Bump `@metamask/transaction-controller` from `^62.13.0` to `^62.15.0` ([#7832](MetaMask/core#7832), [#7854](MetaMask/core#7854))

## [99.2.0]

### Added

- Add `HYPEREVM` support ([#7790](MetaMask/core#7790))
  - Add `HYPEREVM` in `SupportedTokenDetectionNetworks`
  - Add `HYPEREVM` in `SUPPORTED_NETWORKS_ACCOUNTS_API_V4`

### Changed

- Simplify TokenListController initialization ([#7740](MetaMask/core#7740))
- Bump `@metamask/storage-service` from `^0.0.1` to `^1.0.0` ([#7797](MetaMask/core#7797))
- Bump `@metamask/transaction-controller` from `^62.11.0` to `^62.13.0` ([#7775](MetaMask/core#7775), [#7802](MetaMask/core#7802))
- Bump `@metamask/preferences-controller` from `^22.0.0` to `^22.1.0` ([#7802](MetaMask/core#7802))

### Removed

- Removed token-search-discovery-controller package ([#7789](MetaMask/core#7789))
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/39887?quickstart=1)

## **Changelog**

<!--
If this PR is not End-User-Facing and should not show up in the
CHANGELOG, you can choose to either:
1. Write `CHANGELOG entry: null`
2. Label with `no-changelog`

If this PR is End-User-Facing, please write a short User-Facing
description in the past tense like:
`CHANGELOG entry: Added a new tab for users to see their NFTs`
`CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker`

(This helps the Release Engineer do their job more quickly and
accurately)
-->

CHANGELOG entry: null

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Dependency upgrade in a core controllers package can change
token/balance behavior and introduces transitive version shifts; risk is
mitigated by being a targeted bump intended to pick up a bugfix release.
> 
> **Overview**
> Bumps `@metamask/assets-controllers` from `^99.0.0` to `^99.3.1` and
refreshes `yarn.lock` to resolve to `99.3.1`.
> 
> Removes the Yarn patch previously applied to
`@metamask/assets-controllers@93.1.0` (and its patched exchange-rate
precision changes), relying on upstream package fixes instead; lockfile
also reflects updated transitive constraints (e.g., pinning
`@metamask/core-backend` to `5.0.0` and newer controller deps).
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
7457931. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: Prithpal Sooriya <prithpal.sooriya@consensys.net>
Prithpal-Sooriya added a commit that referenced this pull request Feb 11, 2026
@mikesposito @Prithpal-Sooriya
bump: @metamask/assets-controllers from ^99.0.0 to ^99.3.1 cp-1…
bea88d9 
…3.18.0 (#39887)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
[skip-e2e]
-->

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->
This PR bump `@metamask/assets-controllers` from `^99.0.0` to `^99.3.1`.
The current resolved version in the `yarn.lock` is `99.1.0`, which
includes a change that introduces a bug with Tempo native balances. A
fix has been released in `99.3.1`, so we need to bump to that version to
get the fix.

The cherry-pick into `13.18.0` is necessary as it includes the affected
version of `@metamask/assets-controllers`, and we don't want it to reach
production.

```markdown

- Remove `Tempo Testnet` multicall address ([#7858](MetaMask/core#7858)).

- Add optional `rwaData` support when adding tokens in `TokensController` ([#7804](MetaMask/core#7804)).

- Lock `@metamask/core-backend` to `5.0.0` ([#7852](MetaMask/core#7852))
- Bump `@metamask/profile-sync-controller` from `^27.0.0` to `^27.1.0` ([#7849](MetaMask/core#7849))
- Fix trending tokens showing incorrect market cap values by adding `usePriceApiData` parameter (defaults to `true`) to use price API data for accurate market data ([#7829](MetaMask/core#7829))
- Bump `@metamask/transaction-controller` from `^62.13.0` to `^62.15.0` ([#7832](MetaMask/core#7832), [#7854](MetaMask/core#7854))

- Add `HYPEREVM` support ([#7790](MetaMask/core#7790))
  - Add `HYPEREVM` in `SupportedTokenDetectionNetworks`
  - Add `HYPEREVM` in `SUPPORTED_NETWORKS_ACCOUNTS_API_V4`

- Simplify TokenListController initialization ([#7740](MetaMask/core#7740))
- Bump `@metamask/storage-service` from `^0.0.1` to `^1.0.0` ([#7797](MetaMask/core#7797))
- Bump `@metamask/transaction-controller` from `^62.11.0` to `^62.13.0` ([#7775](MetaMask/core#7775), [#7802](MetaMask/core#7802))
- Bump `@metamask/preferences-controller` from `^22.0.0` to `^22.1.0` ([#7802](MetaMask/core#7802))

- Removed token-search-discovery-controller package ([#7789](MetaMask/core#7789))
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/39887?quickstart=1)

<!--
If this PR is not End-User-Facing and should not show up in the
CHANGELOG, you can choose to either:
1. Write `CHANGELOG entry: null`
2. Label with `no-changelog`

If this PR is End-User-Facing, please write a short User-Facing
description in the past tense like:
`CHANGELOG entry: Added a new tab for users to see their NFTs`
`CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker`

(This helps the Release Engineer do their job more quickly and
accurately)
-->

CHANGELOG entry: null

Fixes:

1. Go to this page...
2.
3.

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<!-- [screenshots/recordings] -->

<!-- [screenshots/recordings] -->

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] 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.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Dependency upgrade in a core controllers package can change
token/balance behavior and introduces transitive version shifts; risk is
mitigated by being a targeted bump intended to pick up a bugfix release.
>
> **Overview**
> Bumps `@metamask/assets-controllers` from `^99.0.0` to `^99.3.1` and
refreshes `yarn.lock` to resolve to `99.3.1`.
>
> Removes the Yarn patch previously applied to
`@metamask/assets-controllers@93.1.0` (and its patched exchange-rate
precision changes), relying on upstream package fixes instead; lockfile
also reflects updated transitive constraints (e.g., pinning
`@metamask/core-backend` to `5.0.0` and newer controller deps).
>
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
7457931. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: Prithpal Sooriya <prithpal.sooriya@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@danjm danjm Awaiting requested review from danjm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@whymarrh @Gudahtt