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

Gas fee estimate for trade different than Metamask #526

Closed
SS-FOX-McCloud opened this issue Dec 1, 2021 · 27 comments · Fixed by #1101
Closed

Gas fee estimate for trade different than Metamask #526

SS-FOX-McCloud opened this issue Dec 1, 2021 · 27 comments · Fixed by #1101
Assignees
@Neurone
Labels
bounty Issue is posted in dework.xyz as a bounty

Comments

@SS-FOX-McCloud
Copy link

SS-FOX-McCloud commented Dec 1, 2021 •
edited by 0xean
Loading

When a user previews a trade on V2 using Metamask, the gas fee estimate given is significantly different than the gas fee given by the Metamask extension when signing the trade.

  • visit https://app.shapeshift.com/dashboard
  • connect metamask
  • preview trade ETH/ERC-20-ETH/ERC-20
  • take note of the gas fee estimate
  • click "confirm and trade"
  • see the fee that Metamask is having you use to sign the TX

Screen Shot 2021-12-01 at 12 06 13 PM

Screen Shot 2021-12-01 at 12 06 33 PM

Screen Shot 2021-12-01 at 12 06 40 PM

A/C:

  • The estimated gas fee should be much closer to the actual gas fee that Metamask pulls when completing a trade on V2.
  • Update and extend any tests associated with this flow
@0xean
Copy link
Contributor

0xean commented Dec 2, 2021

@SS-FOX-McCloud - any concerns to bounty this one out? Seems like it should be a straightforward fix for a new contributor.

@0xean 0xean added needs bounty Issues that should be bountied. bounty Issue is posted in dework.xyz as a bounty labels Dec 2, 2021
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 250.0 FOX (171.47 USD @ $0.69/FOX) attached to it as part of the ShapeShift fund.

@0xean 0xean added good first issue Good for newcomers and removed needs bounty Issues that should be bountied. labels Dec 3, 2021
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Workers have applied to start work.

These users each claimed they can complete the work by 1 week, 6 days from now.
Please review their action plans below:

1) hyuze has applied to start work (Funders only: approve worker | reject worker).

Make sure that I am carrying out tasks as stated to ensure full completion.

Learn more on the Gitcoin Issue Details page.

@armr-dev
Copy link

armr-dev commented Dec 4, 2021

Hey, I just took a look at this issue. Is there any chance that the problem is with the gas value sent by the API? I haven't checked the API that Metamask uses yet, but it might be sending a different value.

@0xean
Copy link
Contributor

0xean commented Dec 5, 2021

Hey @armr-dev - I just saw your comment on the other GH issue I had approved you for work on. I didn't realize someone had already submitted a PR for that since I didn't approve their work. Anyways, I appreciate you bringing that up.

Would you want to pick this one up instead? Happy to have you dive in and see if you can figure out a fix and if the fix is more complex than originally expected, I am very willing to make sure we can get more FOX heading your way ;)

Thanks for the interest in ShapeShift and the great communication so far.

@armr-dev
Copy link

armr-dev commented Dec 5, 2021

Hey! I already took a look at this one. It does look complex, but that won't keep me from trying 😎.

I will be working on this one and will keep you updated on the progress.

@0xean
Copy link
Contributor

0xean commented Dec 5, 2021

@armr-dev - awesome, can you apply for the bounty on gitcoin here and i will approve you?

https://gitcoin.co/issue/shapeshift/web/526/100027238

Also - on Monday the rest of the team will be around, and we can happily answer any questions in our discord

https://discord.gg/shapeshift

@armr-dev
Copy link

armr-dev commented Dec 5, 2021

@armr-dev - awesome, can you apply for the bounty on gitcoin here and i will approve you?

https://gitcoin.co/issue/shapeshift/web/526/100027238

Also - on Monday the rest of the team will be around, and we can happily answer any questions in our discord

https://discord.gg/shapeshift

Done! I'll join the Discord server. Ty! 😊

@gitcoinbot
Copy link

⚡️ A tip worth 200.00000 FOX (138.18 USD @ $0.69/FOX) has been granted to @armr-dev for this issue from @0xean. ⚡️

Nice work @armr-dev! To redeem your tip, login to Gitcoin at https://gitcoin.co/explorer and select 'Claim Tip' from dropdown menu in the top right, or check your email for a link to the tip redemption page.

@gitcoinbot
Copy link

gitcoinbot commented Jan 11, 2022 •
edited
Loading

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Workers have applied to start work.

These users each claimed they can complete the work by 1 month, 1 week ago.
Please review their action plans below:

1) sdmg15 has applied to start work (Funders only: approve worker | reject worker).

I want to take a look at this when it's rebountied ... :)
2) charleslee94 has applied to start work (Funders only: approve worker | reject worker).

I want to take a look at this task.

Learn more on the Gitcoin Issue Details page.

@0xean
Copy link
Contributor

0xean commented Jan 11, 2022 •
edited
Loading

Adding some more info from @pastaghost on where to look,

Okay, it looks like the problem has something to do with two different sources being used for the fee estimates being displayed vs the fee estimates passed to hdwallet-metamask. In one of those cases, the gas estimate is coming from lib/packages/swapper/src/swappers/zrx/ZrxExecuteQuote/ZrxExecuteQuoteTx.ts:130-158. In the second, the gas estimate is coming from lib/packages/swapper/src/swappers/zrx/getZrxQuote/getZrxQuote.ts:97-100. You'll see that in getZrxQuote.ts, the gas estimate is for some reason being multiplied by 1.5 where it isn't in the other case. I did play around with this a few weeks ago, and unfortunately it wasn't just a matter of removing/adding the factor of 1.5 in either place, but that brought the gas fee estimates into much closer agreement.

@charleslee94
Copy link

Hello,

I am a software engineer with 6 years of experience in JS/TS and React. I am interested in dipping my toes into web3 development. This looks like something that might be a good place to start.

I have already applied on gitcoin. Thanks!

@Neurone
Copy link
Contributor

Neurone commented Jan 12, 2022

I have the fix for this issue, even though the culprit is not in this repo ;) Let me know if I can send a PR. I applied to gitcoin also.

@0xean
Copy link
Contributor

0xean commented Jan 12, 2022

@Neurone - Will approve you now. Yea, just saw your comment in gitcoin and looking at the screenshots, makes sense! That MaxPriorityFee is way off. Anyways, will approve you now and yea, the PR is probably in lib.

TY.

@Neurone
Copy link
Contributor

Neurone commented Jan 12, 2022

The bug is in the hdwallet library. It seems safe to disclose it but, just to be extra sure, I already sent an inquiry to the security team following the responsible disclosure program, I used the ticket form via zendesk. As soon as they give me green light I'll publish the PR.

@0xean
Copy link
Contributor

0xean commented Jan 12, 2022

Appreciate the caution. (CC @reidrankin please see above)

@0xean
Copy link
Contributor

0xean commented Jan 14, 2022

@Neurone - confirmed with our Security workstream that you are fine to publish.

@Neurone Neurone mentioned this issue Jan 14, 2022
Merged
@Neurone
Copy link
Contributor

Neurone commented Jan 14, 2022

Sent the PR, I summarize also here the problem.

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 leads to inconsistencies between clients and the wallet, and this is why the price changes when Metamask asks the user to sign the tx.

@0xean
Copy link
Contributor

0xean commented Jan 18, 2022

fixed in shapeshift/hdwallet#389 - will need to up hd wallet version in web

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 600.0 FOX (244.6 USD @ $0.41/FOX) has been submitted by:

  1. @neurone

@0xean please take a look at the submitted work:

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 600.0 FOX (178.9 USD @ $0.3/FOX) attached to this issue has been approved & issued to @Neurone.

Additional Tips for this Bounty:

  • 0xean tipped 200.0000 FOX worth 59.63 USD to armr-dev.

@gomesalexandre gomesalexandre mentioned this issue Jan 30, 2022
Closed
@Neurone
Copy link
Contributor

Neurone commented Feb 1, 2022 •
edited
Loading

@0xdef1cafe the latest published shapeshift/hdwallet is still 1.18.4, currently about two months old. We cannot change the reference in the package.json until the new library is deployed on yarnpkg.com.

Side note: there is also some misalignment about the tagging on that repo; the last tag was v1.16.2, created in September 2021. Maybe it's an occasion to fix that also. Just opened an issue there for this.

@0xdef1cafe 0xdef1cafe changed the title (Alpha) V2: Gas fee estimate for trade different than Metamask Gas fee estimate for trade different than Metamask Feb 11, 2022
@0xdef1cafe 0xdef1cafe added blocked and removed good first issue Good for newcomers labels Feb 11, 2022
@0xdef1cafe
Copy link
Collaborator

comment above still valid as of writing, requires new hdwallet version to publish, likely to come week of 2/21

@0xean
Copy link
Contributor

0xean commented Feb 25, 2022

@0xdef1cafe - to check with highlander on the release of HD wallet

@0xdef1cafe
Copy link
Collaborator

@Neurone hdwallet finally got released - are you interested in picking this back up?

@BitHighlander
Copy link
Contributor

BitHighlander commented Feb 25, 2022 •
edited
Loading

`Successfully published:

version 1.19.0 is published :shipit: should be gtg @Neurone

@Neurone
Copy link
Contributor

Neurone commented Feb 25, 2022

@Neurone hdwallet finally got released - are you interested in picking this back up?

Sure, no problem. It should be just a matter of an upgrade in package.json. I can send a PR for that.

@Neurone Neurone mentioned this issue Feb 26, 2022
Closed
5 tasks
@0xdef1cafe 0xdef1cafe removed the blocked label Mar 2, 2022
@0xdef1cafe 0xdef1cafe mentioned this issue Mar 2, 2022
Merged
5 tasks
Repository owner moved this from Todo to Done in ShapeShift Dashboard Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Neurone Neurone

Labels
bounty Issue is posted in dework.xyz as a bounty
Projects
ShapeShift Dashboard
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: upgrade hdwallet shapeshift/web
8 participants
@Neurone @BitHighlander @charleslee94 @0xean @armr-dev @gitcoinbot @SS-FOX-McCloud @0xdef1cafe