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

feat: use networkClientId to resolve chainId in PPOM Middleware #27263

Merged
merged 5 commits into from
Sep 19, 2024

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Sep 18, 2024

Description

Updates the PPOM Middleware to use the chainId for the dapp selected network instead of the globally selected network when processing a request

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

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
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@jiexi jiexi changed the title Use networkClientId to resolve chainId in PPOM Middleware feat: use networkClientId to resolve chainId in PPOM Middleware Sep 18, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [d30b4a5]
Page Load Metrics (1854 ± 121 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25827511782427205
domContentLoaded155526431818228110
load156427591854253121
domInteractive206638146
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -124 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@jiexi jiexi marked this pull request as ready for review September 18, 2024 22:18
@jiexi jiexi requested a review from a team as a code owner September 18, 2024 22:18
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

Seems correct to me but we should definitely get an approval from @jpuri or someone else on confirmations before merging!

@jiexi
Copy link
Contributor Author

jiexi commented Sep 19, 2024

tested this by trying to send a malicious tx on a different chain from the globally selected by setting QueuedRequestController.shouldRequestSwitchNetwork to always return false. Verified in the PPOM controller that the request's chainId was different from the globally selected chain Id. Got the PPOM warning in the confirmation as expected

To be extra safe, the globally selected network was set to Linea Sepolia, a chain that PPOM does not support. This implies that the warning shown must have used the request's chainId

Copy link

sonarcloud bot commented Sep 19, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [47ba18d]
Page Load Metrics (1857 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint38721491784348167
domContentLoaded15522141183313464
load15642161185713565
domInteractive14186535024
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -124 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@jiexi jiexi merged commit 178c95b into develop Sep 19, 2024
77 checks passed
@jiexi jiexi deleted the jl/ppom-middleware-use-chainId-from-networkClientId branch September 19, 2024 17:58
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-wallet-api-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants