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: centralize redesigned confirmation decision logic #28445

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

cryptotavares
Copy link
Contributor

@cryptotavares cryptotavares commented Nov 13, 2024

Description

This PR centralizes the redesigned confirmation decision logic to improve code organization and reduce duplication across the codebase. Currently, redesign confirmation decision logic is scattered across multiple files, making it harder to maintain and more prone to inconsistencies.

Motivation

The existing implementation has several issues:

  1. Duplicate logic for handling confirmation decisions across different transaction types
  2. Prevent inconsistent handling of redesigned confirmation flows

Key changes:

  • Move supported redesigned confirmation decision logic to the shared dir (to be used both in the ui and app bundles)
  • Updated signature metrics tracking to support developer mode enabled

Types of changes:

  • Code style update (changes that do not affect the meaning of the code)

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Ensure that when either the experimental confirmations redesign toggle, developer option confirmations redesign toggle or the env ENABLE_CONFIRMATIONS_REDESIGN is enabled the we are presenting the new redesign confirmations.

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.

@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Nov 13, 2024
Copy link

sentry-io bot commented Nov 13, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: app/scripts/lib/createRPCMethodTrackingMiddleware.js

Function Unhandled Issue
finalizeSignatureFragment Error: Event fragment with id signature-911707255 does not exist. /sc...
Event Count: 1 Affected Users: 0
📄 File: app/scripts/lib/transaction/metrics.ts (Click to Expand)
Function Unhandled Issue
buildEventFragmentProperties TypeError: null has no properties /background.html
Event Count: 20 Affected Users: 0
buildEventFragmentProperties SyntaxError: Invalid or unexpected token /scripts...
Event Count: 7 Affected Users: 0
buildEventFragmentProperties TypeError: Cannot destructure property 'name' of '(intermediate value)' as it is null. ...
Event Count: 5 Affected Users: 0
buildEventFragmentProperties TypeError: e.ᅦ槌ᅯ榫ᅤ槝iceModel is not a function /sc...
Event Count: 4 Affected Users: 0
---

Did you find this useful? React with a 👍 or 👎

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 14, 2024
@cryptotavares cryptotavares force-pushed the cryptotavares/centralize-redesigned-confirmation-decision-logic branch from 58fce0b to 7702e54 Compare November 14, 2024 22:23
@cryptotavares cryptotavares marked this pull request as ready for review November 14, 2024 22:41
@cryptotavares cryptotavares requested review from a team as code owners November 14, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants