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 NFT transfer with big token id #4636

Merged
merged 8 commits into from
Jul 28, 2022

Conversation

wdv4758h
Copy link
Contributor

@wdv4758h wdv4758h commented Jul 7, 2022

Description

Current implementation does not work with big token id. This will cause NFT transfer failure, since the id will be converted to something not correct.

For example:

> // the hex is 0x159ffe6f22fd5cc42c524df6fd5e28d0de38f34e
> // and this big number is 
> decimal = '123456789012345678901234567890123456789012345678'

> // original implementation
> parseInt(decimal).toString(16)
'159ffe6f22fd5d00000000000000000000000000'

> // new implementation
> toBigNumber.dec(decimal).toString(16)
'159ffe6f22fd5cc42c524df6fd5e28d0de38f34e'

This PR simply change the conversion to use BigNumber, which is already in the codebase.

Screenshots/Recordings

Issue

Hex string conversion failure cause NFT transfer failure.

Checklist

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

JavaScript's parseInt does not handle big number
@wdv4758h wdv4758h requested a review from a team as a code owner July 7, 2022 03:22
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2022

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.

@wdv4758h
Copy link
Contributor Author

wdv4758h commented Jul 7, 2022

I have read the CLA Document and I hereby sign the CLA

@gantunesr
Copy link
Member

gantunesr commented Jul 21, 2022

Thanks for the contribution @wdv4758h, do you mind adding unit tests cases to the toHexadecimal method to verify that this issue is fixed

@gantunesr gantunesr added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 21, 2022
@wdv4758h
Copy link
Contributor Author

Test for big number is added.
Here is the screenshot of test failure before the patch:
without-patch
The test will pass after the patch.

@gantunesr
Copy link
Member

Thanks @wdv4758h. The PR LGTM, I'll move forward for our QA team to test it. They'll point out in this PR any issues they find.

@gantunesr gantunesr 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 Jul 22, 2022
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM

@gantunesr
Copy link
Member

gantunesr commented Jul 22, 2022

@wdv4758h I see a linting error in the code, can you fix it please

/home/runner/work/metamask-mobile/metamask-mobile/app/util/number/index.test.ts
Error:   4[8](https://github.com/MetaMask/metamask-mobile/runs/7463846736?check_suite_focus=true#step:5:9)3:12  

error  Replace `toHexadecimal('12345678[9](https://github.com/MetaMask/metamask-mobile/runs/7463846736?check_suite_focus=true#step:5:10)0[12](https://github.com/MetaMask/metamask-mobile/runs/7463846736?check_suite_focus=true#step:5:13)345678901234567890123456789012345678')` with `⏎······toHexadecimal('123456789012345678901234567890123456789012345678'),⏎····`  prettier/prettier

✖ 2 problems (1 error, 1 warning)

You can use yarn lint:fix

@wdv4758h
Copy link
Contributor Author

@gantunesr done, the yarn lint:fix is applied.

@Fatxx
Copy link
Contributor

Fatxx commented Jul 27, 2022

Hey @wdv4758h!

Thank you very much for your contribution.

I was testing the issue #4092 and I can confirm that these changes solve the problem reported.

Please check the following:

Current bug:

VIDEO-2022-07-27-19-27-54.mp4

Fix running the same flow on this PR:

@gantunesr gantunesr added the release-5.6.0 Issue or pull request that will be included in release 5.6.0 label Jul 27, 2022
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 27, 2022
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

I have tested on polygon, BSC and main net: swaps, sending ERC-721's, Sending ENS ERC-721's, sending ERC-20's and native currencies

this is 🌮 🌮 🌮 🌮

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Jul 27, 2022
@gantunesr gantunesr merged commit 3e542b8 into MetaMask:main Jul 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jul 28, 2022
@wdv4758h wdv4758h deleted the fix-nft-transfer-with-big-id branch July 29, 2022 01:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-5.6.0 Issue or pull request that will be included in release 5.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants