search

shapeshift / web

ShapeShift Web
https://app.shapeshift.com
MIT License
169 stars 179 forks source link

Gas fee estimate for trade different than Metamask #526

Closed SS-FOX-McCloud closed 2 years ago

SS-FOX-McCloud commented 2 years ago

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.

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:

0xean commented 2 years ago

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

gitcoinbot commented 2 years ago

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.

gitcoinbot commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

⚡️ 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

0xean commented 2 years ago

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

Neurone commented 2 years ago

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 commented 2 years ago

fixed in https://github.com/shapeshift/hdwallet/pull/389 - will need to up hd wallet version in web

gitcoinbot commented 2 years ago

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 commented 2 years ago

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:

Neurone commented 2 years ago

@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 commented 2 years ago

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

0xean commented 2 years ago

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

0xdef1cafe commented 2 years ago

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

BitHighlander commented 2 years ago

`Successfully published:

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

Neurone commented 2 years ago

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