fix: get correct Gas Limit

[ErvinDimitri]

Description

There was an issue in getting the correct gas Limit for a transaction, because it was calculating while the user was still entering the amount. So to get a more accurate estimation of gas Fee, it will re-estimate the gas Fee right after the user clicks the Next button (in the send amount modal) It is a continuation from PR: https://github.com/shapeshift/web/pull/1215

Notice

• [x] Have you followed the guidelines in our Contributing guide?
• [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Pull Request Type

• [x] :bug: Bug fix (Non-breaking Change: Fixes an issue)
• [ ] :hammer_and_wrench: Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
• [ ] :nail_care: New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

Sending ETH to (contract address) will give an out of gas error closes #1025

Risk

If the Gas Limit is set incorrectly, or this change fails, in this case, estimate a lower gas limit, the user will end up with a failed transaction; On the other hand, if the Gas Limit is overestimated, some users will end up paying more for the fees than expected (but will still be refunded), and others who do not have sufficient funds will not be able to make the transaction.

Testing

To test the gas limit re-estimation, I used the debugging technique to analyse if the re-estimated gas limit was the necessary gas limit to send ETH to a smart contract

How to replicate the issue

https://github.com/shapeshift/web/pull/1215#issuecomment-1073962042

Screenshots (if applicable)

[0xApotheosis]

Hey @ErvinDimitri - could you please ensure that the PR template is used and filled out?

Preferably the PR is linked to an issue that describes what this PR closes, how to replicate the issue etc.

[ErvinDimitri]

Yes, I will follow the PR's template

[0xdef1cafe]

@ErvinDimitri as per @0xApotheosis comments - please update the PR description as per our template before we review this.

can you also please provide an ETA for completion here?

[ErvinDimitri]

I already made the updates on PR description to follow with the template

[0xdef1cafe]

@ErvinDimitri please check again - go through the steps of opening a new PR, but don't actually create one. you are missing the risk and testing sections.

[ErvinDimitri]

Added the missing sections - Risk and Testing

[0xean]

Re-opening this PR. @ErvinDimitri reached out asking for some help understanding what these sections are for. Maybe something we should add to the docs in the future.

[elmutt]

lgtm

[ErvinDimitri]

@0xApotheosis, the estimatedFormFees() is currently being invoked in handleInputChange which means that every time the user enters a digit it will estimate the Gas Limit, but this method can lead to an issue because the Gas Limit to transfer 0 ETH to a smart contract is completely different from transferring a non-zero value to a smart contract, ending up with a failed transaction.

Here is an example of what is happening:

This type of situation happens when the user is typing a value that has several zeros at the beginning (Ex: 0.0007)

So to make sure we estimate the gas limit for the correct amount entered, re-estimate the gas limit right after the user clicks the Next button (in the send amount modal). I'm using this approach because in the moment the user clicks the Next button, we know that the value entered will not change, which means we can trust in the gas limit re-estimated.

[0xApotheosis]

Thanks for the explanation @ErvinDimitri.

This PR only adds an extra computation, which potentially makes the gas estimate more timely, but does not make the response any more "correct" (if it is "incorrect" to begin with).

I agree that sending zero and non-zero values to a smart contract requires different gas values, and this does appear to be captured correctly in the responses we are receiving - again, this PR does not change that in any way.

I'll close this PR for now.

[ErvinDimitri]

Thanks for the explanation @ErvinDimitri.

Welcome @0xApotheosis

I made a demo here, that shows that this is a valid approach, and is essential for those with a slow internet connection

[0xApotheosis]

GM @ErvinDimitri, I appreciate your persistence on this one, and for the excellent replication video!

You've convinced me - there does seem to be a bug here, but it's not the issue linked in the PR. The bug you've found looks like an issue with the order of the responses that are returned from getFeeData in lib'sEthereumChainAdapter.ts, and how they are handled by the client.

From the example you've provided, my understanding is that the bug is caused by the responses returning out of order, and the value from the last received response (rather than the last requested response) is used in the Confirm component. Is that how you see it?

A better solution to this would address the root cause of the bug, which might look something like using Axios's AbortController to drop any existing requests when a new request is made, which would prevent requests from being received out of order.

Finally, we recently merged a PR which fixed a debouncing issue with the send input, which may have actually mitigated the issue. Are you able to confirm if it has been fixed by that PR?

[0xApotheosis]

@ErvinDimitri a quick update of this - I replicated your original issue, and it does seem to be fixed by https://github.com/shapeshift/web/pull/1399, as I couldn't replicate the issue once the debouncing fix was applied.

I suspect that with the debouncing now working correctly, we send at most one request per second, which mitigates the risk of jitter muddling the order of our returned responses.

Your PR here was the only reason I stumbled across the debouncing issue in the first place, so thank you for that!

[ErvinDimitri]

From the example you've provided, my understanding is that the bug is caused by the responses returning out of order, and the value from the last received response (rather than the last requested response) is used in the Confirm component. Is that how you see it?

Yes, exactly

Are you able to confirm if it has been fixed by that PR?

I tested the code using the PR https://github.com/shapeshift/web/pull/1399/ and the bug was mitigated

Your PR here was the only reason I stumbled across the debouncing issue in the first place, so thank you for that!

My pleasure, I learned interesting things during this PR, thanks

[0xean]

@ErvinDimitri - tip sent your way for the efforts.

[ErvinDimitri]

Thank you so much 💪