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

[cherry-pick] fix: Correct preferences controller usage for isOnPhishingList hook #28806

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

FrederikBolding
Copy link
Member

Description

This is a cherry-pick to the RC for the following commit: db4386f

In this commit the preferences controller was converted to BaseControllerV2, however the isOnPhishingList hook was not corrected to reference the state properly. The hook will currently always throw which means that link validation fails for Snaps notifications, making them unable to display. This PR corrects that mistake.

Note: This is an edge-case of the Snaps API that doesn't have good E2E coverage yet. We should prioritize that.

Open in GitHub Codespaces

Manual testing steps

The following Snap should work correctly and display a notification:

export const onRpcRequest: OnRpcRequestHandler = async ({
  origin,
  request,
}) => {
  switch (request.method) {
    case 'hello':
      return snap.request({
        method: 'snap_notify',
        params: {
          type: 'inApp',
          message: 'Hello! [metamask.io](https://metamask.io)',
        },
      });
    default:
      throw new Error('Method not found.');
  }
};

@FrederikBolding FrederikBolding added the team-snaps-platform Snaps Platform team label Nov 29, 2024
@FrederikBolding FrederikBolding requested a review from a team as a code owner November 29, 2024 10:55
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 29, 2024
@FrederikBolding FrederikBolding changed the base branch from Version-v12.8.0 to Version-v12.9.0 November 29, 2024 12:48
@FrederikBolding FrederikBolding requested review from kumavis and a team as code owners November 29, 2024 12:48
…#28803)

<!--
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.
-->

## **Description**

In [this
commit](cedabc6)
the preferences controller was converted to `BaseControllerV2`, however
the `isOnPhishingList` hook was not corrected to reference the state
properly. The hook will currently always throw which means that link
validation fails for Snaps notifications, making them unable to display.
This PR corrects that mistake.

Note: This is an edge-case of the Snaps API that doesn't have good E2E
coverage yet. We should prioritize that.

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

## **Manual testing steps**

The following Snap should work correctly and display a notification:
```
export const onRpcRequest: OnRpcRequestHandler = async ({
  origin,
  request,
}) => {
  switch (request.method) {
    case 'hello':
      return snap.request({
        method: 'snap_notify',
        params: {
          type: 'inApp',
          message: 'Hello! [metamask.io](https://metamask.io)',
        },
      });
    default:
      throw new Error('Method not found.');
  }
};

```
@FrederikBolding FrederikBolding force-pushed the fb/snaps-notification-hotfix-rc branch from b11855b to c7d6e4d Compare November 29, 2024 12:50
@metamaskbot
Copy link
Collaborator

Builds ready [c7d6e4d]
Page Load Metrics (1811 ± 81 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14822145181816680
domContentLoaded14762033178116177
load14802139181116881
domInteractive25166393115
backgroundConnect7105332512
firstReactRender1594362713
getState453081036230
initialActions00000
loadScripts11041570136114168
setupStore68011168
uiStartup169429392125339163
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -11 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@danjm danjm merged commit 7177452 into Version-v12.9.0 Nov 29, 2024
70 of 71 checks passed
@danjm danjm deleted the fb/snaps-notification-hotfix-rc branch November 29, 2024 13:57
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants