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

Ensure that addresses are properly hex-prefixed in the generate ERC token transfer functions #15064

Merged
merged 3 commits into from
Jun 28, 2022
Merged

Ensure that addresses are properly hex-prefixed in the generate ERC token transfer functions #15064

merged 3 commits into from
Jun 28, 2022

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Jun 28, 2022 •
edited
Loading

Fixes #15058

This PR is currently comparing to the branch for #15063, we need to merge that PR first, and then rebase this PR's branch onto master.

we used to do this to create a token transfer transaction from the send screen:
https://github.com/MetaMask/metamask-extension/pull/14465/files#diff-bad76d7279418f3520a0ca93cfaeeb6fcbb1f0d4782f6ff942d422632a1c2780L1762-L1766

        token.transfer(address, value, {
          ...txParams,
          to: undefined,
          data: undefined,
        });

we now do this:
https://github.com/MetaMask/metamask-extension/pull/14465/files#diff-3c58b916cefff81e4927a9afd392acadbf4515f99f41783964a9851b7774791bR815-R833

const txMeta = await promisifiedBackground.addUnapprovedTransaction(
        txParams,
        ORIGIN_METAMASK,
        type,
      );
    });

previously we would create the token transfer via libraries that depend on this from ethjs-abi https://github.com/ethjs/ethjs-abi/blob/master/src/utils/index.js#L166, so validation of the hex string would fail there, and the user wouldn't be able to send the transaction, with the change referenced above, that validation stopped

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

@danjm danjm mentioned this pull request Jun 28, 2022
Closed
6 tasks
Base automatically changed from revert-v10.16.0 to master June 28, 2022 14:14
@danjm danjm force-pushed the fix-token-address-bug branch from 8d0c269 to 70efd4b Compare June 28, 2022 14:16
@danjm danjm marked this pull request as ready for review June 28, 2022 14:17
@danjm danjm requested a review from a team as a code owner June 28, 2022 14:17
@danjm danjm requested a review from darkwing June 28, 2022 14:17
Gudahtt
Gudahtt previously approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

We should follow up with additional unit tests, an e2e test, and inline docs for these functions.

ui/pages/send/send.utils.js
@@ -142,7 +142,7 @@ function generateERC20TransferData({
.call(
abi.rawEncode(
['address', 'uint256'],
[toAddress, addHexPrefix(amount)],
[addHexPrefix(toAddress), addHexPrefix(amount)],
Copy link
Member

Choose a reason for hiding this comment

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

Generally I'd prefer us blowing up for invalid inputs rather than normalizing them, to make the code simpler and easier to trace. I can see why this would be easier for now but longer term this adds unnecessary complexity.

@danjm danjm changed the base branch from master to Version-v10.15.2 June 28, 2022 14:25
@Gudahtt
Copy link
Member

Gudahtt commented Jun 28, 2022

Actually, maybe better to target this at develop instead of master.

The unit test failure appears unrelated, it's one that intermittently fails.

@danjm danjm force-pushed the fix-token-address-bug branch from 70efd4b to 783a72d Compare June 28, 2022 14:54
@danjm danjm requested a review from kumavis as a code owner June 28, 2022 14:54
@danjm danjm changed the base branch from Version-v10.15.2 to develop June 28, 2022 14:54
@danjm danjm dismissed Gudahtt’s stale review June 28, 2022 14:54

The base branch was changed.

Gudahtt
Gudahtt previously approved these changes Jun 28, 2022
View reviewed changes
brad-decker
brad-decker previously approved these changes Jun 28, 2022
View reviewed changes
adonesky1
adonesky1 previously approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@darkwing
Copy link
Contributor

Needs unit test update, but I sent USDC on MainNet to an address without the 0x prefix and it made it to the appropriate address.

@danjm danjm dismissed stale reviews from adonesky1, brad-decker, and Gudahtt via eaa7599 June 28, 2022 16:07
@danjm danjm merged commit cb77f94 into develop Jun 28, 2022
@danjm danjm deleted the fix-token-address-bug branch June 28, 2022 16:51
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2022
@metamaskbot
Copy link
Collaborator

Builds ready [eaa7599]
Page Load Metrics (1973 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint93157124209
domContentLoaded16952150195911053
load17842150197310651
domInteractive16952150195911053

highlights:

storybook

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

@adonesky1 adonesky1 adonesky1 approved these changes

@Gudahtt Gudahtt Gudahtt approved these changes

@brad-decker brad-decker brad-decker left review comments

@darkwing darkwing Awaiting requested review from darkwing darkwing is a code owner automatically assigned from MetaMask/extension-devs

@kumavis kumavis Awaiting requested review from kumavis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Metamask sending TX to random wallet addresses - repeatable bug
7 participants
@danjm @Gudahtt @darkwing @metamaskbot @brad-decker @adonesky1 @FrederikBolding