Skip to content

fix: display hash as hex in personal signature confirmation#8157

Merged
matthewwalsh0 merged 2 commits into
mainfrom
fix/display-hash-as-hex-personal-signature
Jan 31, 2024
Merged

fix: display hash as hex in personal signature confirmation#8157
matthewwalsh0 merged 2 commits into
mainfrom
fix/display-hash-as-hex-personal-signature

Conversation

@matthewwalsh0

@matthewwalsh0 matthewwalsh0 commented Dec 19, 2023

Copy link
Copy Markdown
Member

Description

When displaying a personal sign confirmation, the extension displays data with a length of 32 bytes as the original hexadecimal string, rather than the utf8 equivalent. This is likely to acommodate cases where a hash is to be signed rather than a human readable message.

Mobile always displays the utf8 equivalent which results in invalid characters if a hash was provided.

See extension code here.

Related issues

Fixes:

Manual testing steps

  1. Perform personal_sign request with the first argument (data) as a 32 byte hexadecimal string.
  2. Ensure same string is visible in the confirmation.
  3. Perform personal_sign with any other buffer size.
  4. Ensure equivalent UTF8 is displayed.

Screenshots/Recordings

Before

before

After

after

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.

@matthewwalsh0 matthewwalsh0 requested a review from a team as a code owner December 19, 2023 13:09
@matthewwalsh0 matthewwalsh0 added the team-confirmations-system-deprecated DEPRECATED: please use "team-confirmations" instead label Dec 19, 2023
@matthewwalsh0 matthewwalsh0 requested a review from jpuri December 19, 2023 13:17
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

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

Comparison is base (db8b71d) 39.44% compared to head (a607653) 39.47%.
Report is 9 commits behind head on main.

Files Patch % Lines
app/components/UI/PersonalSign/PersonalSign.tsx 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8157      +/-   ##
==========================================
+ Coverage   39.44%   39.47%   +0.03%     
==========================================
  Files        1220     1227       +7     
  Lines       29748    29802      +54     
  Branches     2823     2834      +11     
==========================================
+ Hits        11734    11765      +31     
- Misses      17325    17343      +18     
- Partials      689      694       +5     

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

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5a0f0b91-ef56-4aed-b123-64b91c5dd500
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4ee7c321-28dc-43e9-99a7-26c06627105d
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@sleepytanya

Copy link
Copy Markdown
Contributor

32 byte hexadecimal string is shown in the Confirmation for Personal sign:
Android build
Screenshot 2024-01-31 at 9 56 56 AM
iOS build
Screenshot 2024-01-31 at 10 13 33 AM
Test-dapp generated signature
Screenshot 2024-01-31 at 9 58 18 AM

@sleepytanya sleepytanya added the QA Passed QA testing has been completed and passed label Jan 31, 2024
@matthewwalsh0 matthewwalsh0 merged commit a201472 into main Jan 31, 2024
@matthewwalsh0 matthewwalsh0 deleted the fix/display-hash-as-hex-personal-signature branch January 31, 2024 16:06
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 31, 2024
@metamaskbot metamaskbot added release-7.17.0 Issue or pull request that will be included in release 7.17.0 release-7.16.0 Issue or pull request that will be included in release 7.16.0 and removed release-7.17.0 Issue or pull request that will be included in release 7.17.0 labels Jan 31, 2024
@metamaskbot

Copy link
Copy Markdown
Collaborator

Missing release label release-7.16.0 on PR. Adding release label release-7.16.0 on PR and removing other release labels(release-7.17.0), as PR was cherry-picked in branch 7.16.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed QA testing has been completed and passed release-7.16.0 Issue or pull request that will be included in release 7.16.0 team-confirmations-system-deprecated DEPRECATED: please use "team-confirmations" instead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants