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: gas limit parameter for MetaMask #389

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

Neurone
Copy link
Contributor

@Neurone Neurone commented Jan 14, 2022

Fixed Metamask parameter for gasLimit.

Metamask and other web3 libraries (i.e. geth console, web3.js, etc.) use transaction.gas property for transaction's gas limit, instead of the canonical transaction.gasLimit as mentioned in the yellow paper. This implies Metamask does not read any value for gas limit from the JSON object, and so it chooses one on its own. This behavior can lead to inconsistencies between clients and the wallet, for example when forecasting the transaction's cost (see shapeshift/web#526).

@vercel
Copy link

vercel bot commented Jan 14, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shapeshift/hdwallet/9HsThuFppYodyZh1oDu2ZAjpjsqW
✅ Preview: https://hdwallet-git-fork-internetofpeers-master-shapeshift.vercel.app

@Neurone
Copy link
Contributor Author

Neurone commented Jan 14, 2022

Please note I ran the test suite before any modification and after the fix, and the result is the following in both cases:

Test Suites: 3 failed, 24 passed, 27 total
Tests:       20 failed, 302 passed, 322 total
Snapshots:   24 passed, 24 total
Time:        432.043 s

@mrnerdhair
Copy link
Contributor

Just fyi, there shouldn't be any failing tests -- something might be busted with your setup?

@Neurone
Copy link
Contributor Author

Neurone commented Jan 14, 2022

I just did yarn, yarn build, yarn test. I think the integration tests are failing.

Documentation says:

The integration tests have been set up to run either against a physical KeepKey
with debug firmware on it, or in CI pointed at a dockerized version of the
emulator.

I don't have a KeepKey (sorry ^^) and I don't know how startup an emulator. CI via Github have all checks passed, so I suppose this is the reason.

@Neurone
Copy link
Contributor Author

Neurone commented Jan 14, 2022

Just noticed the docker:run:emulator in the package.json ^^ Anyway, all problems seem related to argonBenchmark and the require("crypto").webcrypto object that seems undefined, I'll try to scout more as soon as I have more time to dedicate:

  ● argonBenchmark › argonBenchmark › should run correctly

    TypeError: Cannot read property 'subtle' of undefined

      13 |     const realCrypto = require("crypto").webcrypto as Crypto;
      14 |     setCrypto({
    > 15 |       subtle: realCrypto.subtle,
         |                          ^
      16 |       getRandomValues: await deterministicGetRandomValues(realCrypto),
      17 |     });

@Neurone
Copy link
Contributor Author

Neurone commented Jan 14, 2022

The problem was with my Nodejs v14.18.3 (Latest LTS: Fermium). I switched to v16.13.2 (Latest LTS: Gallium) and now all tests work just fine!

Test Suites: 27 passed, 27 total
Tests:       322 passed, 322 total
Snapshots:   52 passed, 52 total
Time:        25.198 s
Ran all test suites.
Done in 25.99s.

@0xean 0xean requested a review from pastaghost January 17, 2022 15:15
@mrnerdhair
Copy link
Contributor

That makes sense; hdwallet supports being used under older node versions but does require node 16 for tests. As far as the KK emulator goes, you can

git clone https://github.com/keepkey/keepkey-firmware
cd keepkey-firmware
git submodule update --init --recursive
cd scripts/emulator
docker-compose up kkemu

to build/run it; I think the docker:run:emulator version is a bit old at this point.

@Neurone Neurone changed the title Fixed gas limit parameter for Metamask fix: gas limit parameter for MetaMask Jan 18, 2022
@0xean 0xean merged commit 9d1e0b2 into shapeshift:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants