Skip to content

feat: Add PPOM Validation to normal send.#22321

Merged
segun merged 12 commits intodevelopfrom
olu/ppom-validation-normal-send
Jan 25, 2024
Merged

feat: Add PPOM Validation to normal send.#22321
segun merged 12 commits intodevelopfrom
olu/ppom-validation-normal-send

Conversation

@segun
Copy link
Copy Markdown
Contributor

@segun segun commented Dec 18, 2023 •
edited
Loading

Description

Currently we send ppom validation requests only for incoming requests, this PR adds ppom validation for send requests from wallets too.

Related issues

Fixes: #1657

Manual testing steps

  1. Build and launch MM
  2. Send ETH on mainnet to this address 0x5FbDB2315678afecb367f032d93F642f64180aa3
  3. See that Blockaid banner is not shown
  4. Checkout this branch, build and launch MM
  5. Send ETH on mainnet to the address 0x5FbDB2315678afecb367f032d93F642f64180aa3
  6. See that Blockaid banner is shown.

Screenshots/Recordings

Before

old_ppom.mov

After

new_ppom.mov

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@segun segun self-assigned this Dec 18, 2023
@segun segun requested a review from a team as a code owner December 18, 2023 15:03
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 18, 2023

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.

@segun segun added team-confirmations-secure-ux-PR PRs from the confirmations team needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 18, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 18, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (722d8fe) 68.13% compared to head (06e07a4) 68.14%.
Report is 3 commits behind head on develop.

❗ Current head 06e07a4 differs from pull request most recent head 91ec78a. Consider uploading reports for the commit 91ec78a to get more accurate results

Files Patch % Lines
app/scripts/lib/transaction/util.ts 84.62% 2 Missing ⚠️
app/scripts/metamask-controller.js 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #22321      +/-   ##
===========================================
+ Coverage    68.13%   68.14%   +0.01%     
===========================================
  Files         1087     1087              
  Lines        42665    42676      +11     
  Branches     11349    11351       +2     
===========================================
+ Hits         29066    29079      +13     
+ Misses       13599    13597       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 19, 2023

Builds ready [e487671]
Page Load Metrics (1350 ± 156 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint983631909244
domContentLoaded15240677134
load89620701350326156
domInteractive15240677134
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 291 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

blackdevelopa
blackdevelopa reviewed Dec 20, 2023
View reviewed changes
@segun segun force-pushed the olu/ppom-validation-normal-send branch from e487671 to 874869f Compare December 20, 2023 12:52
@jpuri
Copy link
Copy Markdown
Contributor

jpuri commented Dec 20, 2023

It will be nice to also have some test coverage in the PR.

blackdevelopa
blackdevelopa previously approved these changes Dec 20, 2023
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 20, 2023

Builds ready [874869f]
Page Load Metrics (1607 ± 144 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1003061765527
domContentLoaded9207425225
load93621491607300144
domInteractive9207425225
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 292 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

legobeat
legobeat suggested changes Dec 22, 2023
View reviewed changes
@segun segun force-pushed the olu/ppom-validation-normal-send branch from 874869f to dafa308 Compare January 11, 2024 20:02
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 12, 2024

Builds ready [dafa308]
Page Load Metrics (1202 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint852711576933
domContentLoaded9184576933
load9021516120219393
domInteractive9184576933

@segun segun force-pushed the olu/ppom-validation-normal-send branch from dafa308 to c66ee3d Compare January 12, 2024 14:34
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 12, 2024

Builds ready [c66ee3d]
Page Load Metrics (1227 ± 109 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint852851717235
domContentLoaded8185777636
load88516231227228109
domInteractive8185777636
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 262 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

jpuri
jpuri previously approved these changes Jan 15, 2024
View reviewed changes
blackdevelopa
blackdevelopa previously approved these changes Jan 15, 2024
View reviewed changes
@digiwand
Copy link
Copy Markdown
Contributor

digiwand commented Jan 15, 2024

I think the screen recording in the description's "After" needs to be updated. not seeing the banner alert in it cc: @segun

digiwand
digiwand reviewed Jan 15, 2024
View reviewed changes
digiwand
digiwand previously approved these changes Jan 15, 2024
View reviewed changes
legobeat
legobeat previously approved these changes Jan 16, 2024
View reviewed changes
@legobeat legobeat self-requested a review January 16, 2024 10:30
jpuri
jpuri previously approved these changes Jan 24, 2024
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 24, 2024

Builds ready [91ec78a]
Page Load Metrics (778 ± 26 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint951831152110
domContentLoaded11371863
load6979587785526
domInteractive11371863
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 968 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

digiwand
digiwand reviewed Jan 24, 2024
View reviewed changes
@segun segun force-pushed the olu/ppom-validation-normal-send branch from 91ec78a to cd3bc5d Compare January 24, 2024 16:40
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 24, 2024

Builds ready [8eac590]
Page Load Metrics (789 ± 30 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint92158109168
domContentLoaded9301653
load73210027896330
domInteractive9301653
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 960 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 24, 2024

Builds ready [7cfe269]
Page Load Metrics (807 ± 24 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint91137112116
domContentLoaded10522094
load7289208075024
domInteractive10522094
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 960 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

digiwand
digiwand previously approved these changes Jan 24, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

nice work. LGTM!

@segun segun force-pushed the olu/ppom-validation-normal-send branch from 61cee15 to 33375bb Compare January 25, 2024 09:13
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Jan 25, 2024

Builds ready [33375bb]
Page Load Metrics (810 ± 26 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97166116157
domContentLoaded11321852
load7419638105326
domInteractive11321852
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 881 Bytes (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@segun segun merged commit c525579 into develop Jan 25, 2024
@segun segun deleted the olu/ppom-validation-normal-send branch January 25, 2024 13:02
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 25, 2024
@metamaskbot metamaskbot added the release-11.10.0 Issue or pull request that will be included in release 11.10.0 label Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jpuri jpuri jpuri approved these changes

@cryptotavares cryptotavares Awaiting requested review from cryptotavares

@legobeat legobeat Awaiting requested review from legobeat

+2 more reviewers

@blackdevelopa blackdevelopa blackdevelopa approved these changes

@digiwand digiwand digiwand left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@segun segun

Labels

release-11.10.0 Issue or pull request that will be included in release 11.10.0 team-confirmations-secure-ux-PR PRs from the confirmations team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@segun @metamaskbot @jpuri @digiwand @cryptotavares @blackdevelopa @legobeat