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

fix: Correct preferences controller usage for isOnPhishingList hook #28803

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Nov 29, 2024

Description

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:05
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 29, 2024
@FrederikBolding FrederikBolding force-pushed the fb/fix-snaps-notification-links branch from d19eef9 to 11006e6 Compare November 29, 2024 10:09
@FrederikBolding FrederikBolding added this pull request to the merge queue Nov 29, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [11006e6]
Page Load Metrics (1921 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39422021835365175
domContentLoaded15822196188617484
load16192203192116278
domInteractive248639189
backgroundConnect791352512
firstReactRender158921168
getState43211963316
initialActions01000
loadScripts11761722145415072
setupStore66110126
uiStartup18672521217820096
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -11 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Merged via the queue into develop with commit db4386f Nov 29, 2024
75 checks passed
@FrederikBolding FrederikBolding deleted the fb/fix-snaps-notification-links branch November 29, 2024 10:52
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2024
@metamaskbot metamaskbot added the release-12.10.0 Issue or pull request that will be included in release 12.10.0 label 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 release-12.10.0 Issue or pull request that will be included in release 12.10.0 team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants