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

chore: upgrade transaction controller to 35.0.0 #10263

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Jul 6, 2024

Description

Upgrade the TransactionController from version 13.0.0 to 35.0.0.

Upgrade the ApprovalController from version 3.5.2 to 7.0.1.

Since both controllers are now at the latest version, the patches contain only the changes that are unique to mobile and not yet added to the main core branch.

See TransactionController patch here.

See ApprovalController patch here.

The following changes are included, organised by TransactionController version:

33.0.0

  • Added the AccountsController:getSelectedAccount action to the TransactionController messenger.
  • Removed the getSelectedAddress option in the TransactionController constructor.

29.0.0

  • Removed the gasFeeControllerEstimateType option from call to mergeGasFeeEstimates.

27.0.0

  • Disable resubmit via pendingTransactions.isResubmitEnabled callback.

25.0.0

  • Replaced all hub event listeners with subscriptions via the controller messenger.
  • Added ExtendedControllerMessenger with helper methods:
    • subscribeOnceIf
    • tryUnsubscribe
  • Replaced references to TransactionState with TransactionControllerState.
  • Removed use of config object in TransactionController constructor.
  • Removed direct update of TransactionMeta from events as they are now read only.
    • Replaced with cloneDeep and updateTransaction.
  • Prevent duplicate calls to onUnapprovedTransaction in RootRPCMethodsUI.js due to unstable dependencies.
    • Add useSwapsTransactions hook.
    • Use useCallback in useMetrics.
  • Remove imports from dist to access CHAIN_IDS and ETHERSCAN_SUPPORTED_NETWORKS.

23.0.0

  • Added getNetworkClientRegistry callback to TransactionController constructor.
  • Added NetworkController:stateChange event to controller messenger.

17.0.0

  • Disabled direct swaps integration with disableSwaps option in TransactionController constructor.
  • Removed usage of legacy type clashing with gas fee & type validation:
    • SET_INDIVIDUAL_TOKEN_TRANSACTION
    • ETHER_TRANSACTION'
  • Cleared overlapping gas properties in transaction parameters to satisfy new gas fee validations.

14.0.0

  • Added getPermittedAccounts option to TransactionController constructor.
  • Updated getPermittedAccounts implementation to always allow internal origins:
    • process.env.MM_FOX_CODE
    • TransactionTypes.MMM

Related issues

Fixes: [Pending]

Manual testing steps

Full regression of all transaction flows, including:

  • Standard Transactions
  • Swaps
  • Smart Transactions
  • Gas Fee Update
  • Speed Up / Cancel
  • Layer 1 Fees

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.

Copy link

socket-security bot commented Jul 10, 2024

Copy link

socket-security bot commented Jul 10, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@keystonehq/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/@metamask/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@matthewwalsh0 matthewwalsh0 added the team-confirmations Push issues to confirmations team label Jul 10, 2024
@matthewwalsh0
Copy link
Member Author

@SocketSecurity ignore-all

@matthewwalsh0 matthewwalsh0 added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jul 11, 2024
@matthewwalsh0 matthewwalsh0 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 11, 2024
@matthewwalsh0 matthewwalsh0 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 11, 2024
@matthewwalsh0 matthewwalsh0 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 15, 2024
@matthewwalsh0 matthewwalsh0 changed the title chore: upgrade transaction controller to 33.0.1 chore: upgrade transaction controller to 35.0.0 Jul 16, 2024
@matthewwalsh0 matthewwalsh0 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 16, 2024
Copy link
Contributor

github-actions bot commented Jul 16, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 387304a
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/179e64d4-6e93-40b7-a34d-0e2360da32d0

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@MetaMask MetaMask deleted a comment from github-actions bot Jul 16, 2024
@MetaMask MetaMask deleted a comment from github-actions bot Jul 16, 2024
@MetaMask MetaMask deleted a comment from github-actions bot Jul 16, 2024
@MetaMask MetaMask deleted a comment from github-actions bot Jul 16, 2024
@MetaMask MetaMask deleted a comment from github-actions bot Jul 16, 2024
@matthewwalsh0 matthewwalsh0 marked this pull request as ready for review July 16, 2024 12:41
@matthewwalsh0 matthewwalsh0 requested review from a team as code owners July 16, 2024 12:41
@matthewwalsh0
Copy link
Member Author

Regression Tests

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

This PR is amazing!! Thanks @matthewwalsh0
Left just one comment about the anys in the Engine file!

I have also a question, since we are on the latest version of transaction controller and approval controller, when do you think it will be possible to remove the patches completely?

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
getExternalPendingTransactions: (address: string) =>
this.smartTransactionsController.getTransactions({
addressFrom: address,
status: SmartTransactionStatuses.PENDING,
}),

sign: (transaction, from, transactionMeta) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of any, should we type it as Transaction instead? Also I see TransactionController as any as well, wondering if we could type as TransactionController (if that makes sense)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, just did a bind to the method directly and cast to the correct type.

Enable simulation.
@matthewwalsh0
Copy link
Member Author

This PR is amazing!! Thanks @matthewwalsh0 Left just one comment about the anys in the Engine file!

I have also a question, since we are on the latest version of transaction controller and approval controller, when do you think it will be possible to remove the patches completely?

Thanks Tomas! I'd love to remove both patches as soon as possible once this branch is merged, since I didn't want to delay this work or introduce any more risk and change that would need to be tested.

Copy link
Contributor

@pedronfigueiredo pedronfigueiredo left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

sonarcloud bot commented Jul 17, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
35.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

@sleepytanya
Copy link
Contributor

@matthewwalsh0
Error when confirming test dapp transactions - Invalid transaction envelope type: specified type "0x0" but including maxFeePerGas and maxPriorityFeePerGas requires type: "0x2"

iphone.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run Smoke E2E Triggers smoke e2e on Bitrise team-confirmations Push issues to confirmations team
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

None yet

5 participants