Skip to content

Clear Hex data when Token Transfer reverts ETH#5839

Merged
seaona merged 3 commits intomainfrom
bgfix/hex_data
Mar 21, 2023
Merged

Clear Hex data when Token Transfer reverts ETH#5839
seaona merged 3 commits intomainfrom
bgfix/hex_data

Conversation

@blackdevelopa
Copy link
Copy Markdown
Contributor

@blackdevelopa blackdevelopa commented Feb 27, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
When you send ETH the hex data should be empty by default (0x). For token transfers, we are interacting with contacts, so the Hex Data is then filled with the long values (representing the method, and the arguments passed). If we switch back to ETH, it should clear the hex data to default (0x)

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
2. What is the improvement/solution?

Screenshots/Recordings
http://recordit.co/Alt9uBAOz0

Issue

Progresses #5759

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@blackdevelopa blackdevelopa requested a review from a team as a code owner February 27, 2023 08:36
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 27, 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.

@blackdevelopa blackdevelopa self-assigned this Feb 27, 2023
@blackdevelopa blackdevelopa added team-confirmations-secure-ux-deprecated DEPRECATED: please use "team-confirmations" instead team-confirmations-secure-ux-PR PR from the confirmations team needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed team-confirmations-secure-ux-deprecated DEPRECATED: please use "team-confirmations" instead labels Feb 27, 2023
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Feb 27, 2023
@seaona
Copy link
Copy Markdown
Member

seaona commented Mar 1, 2023

QAd, comments below:

  • I've noticed that when we try to Deploy a Contract, now we get the error to address:undefined and cannot proceed
invalid-to-address.mp4
  • I think there is a problem with gas calculation when we switch from one type of Send to the other. It looks like gas is not re-calculated resulting in non-desired behaviors for both cases. These issues are in production so I think we can treat them separately. I have opened a dedicated ticket for that, so we don't have to fix this in this PR @blackdevelopa @bschorchit .

@seaona seaona added QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 1, 2023
@blackdevelopa
Copy link
Copy Markdown
Contributor Author

blackdevelopa commented Mar 9, 2023

Thanks @seaona. I made an update and here's a screen recording.
http://recordit.co/3iHCiaZLVg

@seaona seaona requested a review from jpuri March 9, 2023 13:40
@jpuri
Copy link
Copy Markdown
Contributor

jpuri commented Mar 9, 2023

Changes look good 👍

@bschorchit bschorchit added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 10, 2023
@bschorchit
Copy link
Copy Markdown
Contributor

bschorchit commented Mar 10, 2023

Following our new best practices, this needs a second review before going back to QA and being merged.

tommasini
tommasini approved these changes Mar 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

@blackdevelopa blackdevelopa removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 10, 2023
@bschorchit bschorchit added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 14, 2023
@seaona
Copy link
Copy Markdown
Member

seaona commented Mar 21, 2023

QA'd again and I can see that the issue is fixed.
The Hex Data is cleared successfully after updating transactions.

mobile-hex-data-clear.mp4

@seaona seaona merged commit fc8f610 into main Mar 21, 2023
@seaona seaona deleted the bgfix/hex_data branch March 21, 2023 10:59
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2023
@seaona seaona added QA Passed QA testing has been completed and passed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA'd but questions A QA run through has been done but you need clarification on minor issues you found labels Mar 21, 2023
@bschorchit bschorchit added the release-6.3.0 Issue or pull request that will be included in release 6.3.0 label Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jpuri jpuri jpuri approved these changes

@tommasini tommasini tommasini approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels

QA Passed QA testing has been completed and passed release-6.3.0 Issue or pull request that will be included in release 6.3.0 team-confirmations-secure-ux-PR PR from the confirmations team unit test coverage confirmed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@blackdevelopa @seaona @jpuri @bschorchit @tommasini @cortisiko @hilvmason