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.

Sign up for GitHub

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

Jump to bottom

Clear Hex data when Token Transfer reverts ETH #5839

Merged
merged 3 commits into from
Mar 21, 2023
Merged

Clear Hex data when Token Transfer reverts ETH #5839

merged 3 commits into from
Mar 21, 2023

Conversation

blackdevelopa
Copy link
Contributor

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

@blackdevelopa blackdevelopa self-assigned this Feb 27, 2023
@blackdevelopa blackdevelopa added team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label 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: please use "team-confirmations" label 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
Contributor

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
Contributor Author

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

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

app/components/Views/SendFlow/Amount/index.js Outdated Show resolved Hide resolved
@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
Contributor

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 A successful QA run through has been done 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 PR for 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 A successful QA run through has been done release-6.3.0 PR for 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.

None yet
7 participants
@blackdevelopa @seaona @jpuri @bschorchit @tommasini @cortisiko @hilvmason