client/asset/eth: don't fail in swapGas on estimateInitGas RPC failure #2118

chappjc · 2023-02-07T19:32:55Z

Testing a USDC swap, my maker failed (at first) to create the first swap in the sequence with the following error:

[DBG] CORE: Swappable match 6cdfd20fe1b2bbb168f1c0071ef625d71c1bfc04b4afb6c653a06d02de094788 for order a595439a976bbc4681297efbbbf306e33020fea9e763eec8b90d1ab0137ae2f5 (Maker)
[DBG] CORE: Using stored change coin address: ..., amount:47320000, fees:43560000 (usdc.eth) for order a595439a976bbc4681297efbbbf306e33020fea9e763eec8b90d1ab0137ae2f5 matches
[TRC] CORE[eth][ETH][RPC]: using cached header from "eth-mainnet.blastapi.io"
[ERR] CORE[eth][USDC.ETH]: (60001) error estimating swap gas: execution reverted: transfer from failed
[ERR] CORE: notify: |ERROR| (order) Swap send error - Error encountered sending a swap output(s) worth 47.320000 USDC on order a595439a - Order: a595439a976bbc4681297efbbbf306e33020fea9e763eec8b90d1ab0137ae2f5
[TRC] CORE: notify: |DATA| (balance) balance updated
[ERR] CORE: Route 'match' request handler error (DEX ...): runMatches - {tick of order a595439a976bbc4681297efbbbf306e33020fea9e763eec8b90d1ab0137ae2f5 failed: ... tick: {swapMatches order a595439a976bbc4681297efbbbf306e33020fea9e763eec8b90d1ab0137ae2f5 - {error sending swap transaction: error getting gas fees: execution reverted: transfer from failed}}}

This was not fatal because it retried on the next tick about a minute later, but for some reason it appears that c.cb.EstimateGas for the init tx payload returned execution reverted: transfer from failed. May just be a provider error, but I don't think we need to fail to swap because we can use the known gas cap values (just like we do in redeemGas).

JoeGruffins

Can you make the log under this 10%% so the 10% prints correctly?

chappjc · 2023-02-09T15:24:59Z

client/asset/eth/eth.go

+		w.log.Errorf("(%d) error estimating swap gas (using expected gas cap instead): %v", w.assetID, err)
 		// TODO: investigate "gas required exceeds allowance".
-		return 0, 0, false, err
+		// TODO: investigate "error getting gas fees: execution reverted: transfer from failed"
+		// return 0, 0, false, err


I believe whether we fail depends on the caller. FundOrder should probably fail, whilst Swap should not.
This will take some refactoring unfortunately.

Proposed solution in 6101a6c

chappjc · 2023-02-09T16:27:18Z

Pushed two new commits. 0bea901 uses the batched gas limits as a fallback if for some reason reserves are insufficient for the naive oneSwap * n limit that does not consider batch savings.

6101a6c lets the live estimate remain fatal for order funding and pre-order estimates, but not for the swap when negotiation is already underway.

Need to re-test, but putting back to ready-for-review.

JoeGruffins · 2023-02-10T06:48:36Z

Will still fail in swap if we did not reserve enough, but it is sure to get an estimate or fail in FundOrder so less likely the second estimate is higher.

What if there was one swap, and the second live estimate was, for whatever reason, one gas higher than the initial. Would that fail?

chappjc · 2023-02-13T17:29:45Z

What if there was one swap, and the second live estimate was, for whatever reason, one gas higher than the initial. Would that fail?

Really makes me question the utility of doing a live estimate at swap time (for n=1) at all. This would be an especially frustrating failure mode if the wallet has plenty of available balance and it's just the reserved amts in the funding coins that seem insufficient.

chappjc · 2023-02-13T17:35:39Z

Sorry for changing this one so much after your reviews @martonp and @buck54321. #2118 (comment)

The question @JoeGruffins posed remains: if it's a single lot swap (n=1), and order funding got a live estimate of X and used that for reserves, then the live estimate at time of swap got X+1, we'd fail to swap because it exceeds reserves. Options:

that's correct, block the swap to avoid a failed transaction
Swap anyway with the "third fallback that only uses our known gases" (see code comments, but apply that in the n=1 case too).

martonp · 2023-02-14T17:16:57Z

client/asset/eth/eth.go

-	oneSwap, _, _, err := w.swapGas(1, swaps.Version)
+	// Set the gas limit as high as reserves will allow.
+	n := len(swaps.Contracts)
+	oneSwap, nSwap, _, err := w.swapGas(n, swaps.Version, false) // allow live est to fail, using our own gas values


If the live estimate fails here, and our hardcoded estimates are higher than the actual live estimates, then wouldn't swapVal+fees > reservedVal always be true?

I thought they would be equal. reservedVal should turn out to be swapVal + g.Swap * maxFeeRate * n. Basing that on my understanding on how the reserves (via the fundingCoin) are initially set.

It's generally a sign of major problems coming if the live estimates are higher than the hard-coded estimates. In other words, it is (and has been) normal to have the hard coded estimates be what comes out of swapGas.
Consider the code on master, the checks below haven't changed, only added a fallback to the more realistic batched gas limits.

chappjc · 2023-02-14T23:52:51Z

I don't think I have the bandwidth to follow through on this swapGas PR, but there are some issues that need to get resolved for 0.6. Maybe these change (or the gist) can be absorbed into #2129?

buck54321 · 2023-02-16T11:57:49Z

client/asset/eth/eth.go

 		// TODO: investigate "gas required exceeds allowance".
-		return 0, 0, false, err
+		// TODO: investigate "error getting gas fees: execution reverted: transfer from failed" (seems to be approval not actually ready)


Did you see it maybe during MaxOrder or PreSwap? swapGas is used in paths that are called before FundOrder. maybeApproveTokenSwapContract is only called in FundOrder. I think maybe token wallets need to provide requireLiveEst = false in the MaxOrder and PreSwap paths when allowance is required.

No, got this from Swap. It was a maker order that was just placed. Taker placed shortly after. Matched and got the error.

Oh, it was also testnet or mainnet, cannot recall. I may have commented on the mainnet eth PR. Almost positive it was the very first trade with the token and it did the approval on the fly when order was created.

I probably had to ignore any preswap error, but again, don't recall.

buck54321 · 2023-02-16T12:13:47Z

client/asset/eth/eth.go

-	est.Refund = g.Refund
-
-	est.Swap, est.nSwap, _, err = w.swapGas(n, initVer)
+	est.Swap, est.nSwap, _, err = w.swapGas(n, initVer, true) // fail if live gas estimate fails


While you're here, a few lines down, it says

est.oneGas = est.Swap + est.Redeem est.nGas = est.nSwap + est.nRedeem

Adding the redeem value there is wrong and pointless. They're both zero. I think those lines needs to come after the redeem wallet check.

Good catch. This would only affect ETH+TOKEN, right?

Like this? ba8b9fa

I'm now wondering if maxOrder needs to include refund in it's oneFee value.

I'm now wondering if maxOrder needs to include refund in it's oneFee value.

Oh, @JoeGruffins is doing this in 7222c01. Good.

Before failing to Swap on account of insufficient reserves, use the the realistic gas limit when it is a batch swap.

chappjc · 2023-02-16T18:01:32Z

Swapped on mainnet with this rebased on top of the mainnet PR. Branch: https://github.com/chappjc/dcrdex/tree/eth-mainnet%2B%2B
That branch also included @martonp's PR #2125 with 3 providers for the server

chappjc · 2023-02-17T18:08:53Z

Closing in favor of #2143

martonp approved these changes Feb 7, 2023

View reviewed changes

JoeGruffins approved these changes Feb 8, 2023

View reviewed changes

chappjc added this to the 0.6 milestone Feb 8, 2023

chappjc force-pushed the dont-fail-estinitgas branch from ce11336 to d958f58 Compare February 8, 2023 15:59

buck54321 approved these changes Feb 8, 2023

View reviewed changes

chappjc commented Feb 9, 2023

View reviewed changes

chappjc marked this pull request as draft February 9, 2023 15:25

chappjc force-pushed the dont-fail-estinitgas branch from d958f58 to 6101a6c Compare February 9, 2023 16:24

chappjc marked this pull request as ready for review February 9, 2023 16:27

chappjc mentioned this pull request Feb 10, 2023

enable eth and usdc.eth on mainnet #2013

Merged

martonp reviewed Feb 14, 2023

View reviewed changes

buck54321 reviewed Feb 16, 2023

View reviewed changes

chappjc added 4 commits February 16, 2023 10:16

client/asset/eth: don't fail in swapGas on estimateInitGas RPC failure

7153500

client/asset/eth: batched gas limit fallback

2f061e3

Before failing to Swap on account of insufficient reserves, use the the realistic gas limit when it is a batch swap.

client/asset/eth: context-dependent live estimate requirement

74554b5

actually include any non-zero redeem gas estimate

ba8b9fa

chappjc force-pushed the dont-fail-estinitgas branch from 6101a6c to ba8b9fa Compare February 16, 2023 16:23

chappjc mentioned this pull request Feb 17, 2023

client/asset/eth: batched gas limit fallback #2143

Merged

chappjc closed this Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/asset/eth: don't fail in swapGas on estimateInitGas RPC failure #2118

client/asset/eth: don't fail in swapGas on estimateInitGas RPC failure #2118

chappjc commented Feb 7, 2023 •

edited

JoeGruffins left a comment

chappjc Feb 9, 2023

chappjc Feb 9, 2023

chappjc commented Feb 9, 2023

JoeGruffins commented Feb 10, 2023

chappjc commented Feb 13, 2023

chappjc commented Feb 13, 2023

martonp Feb 14, 2023

chappjc Feb 14, 2023 •

edited

chappjc Feb 16, 2023

chappjc commented Feb 14, 2023

buck54321 Feb 16, 2023

chappjc Feb 16, 2023

chappjc Feb 16, 2023 •

edited

buck54321 Feb 16, 2023

chappjc Feb 16, 2023 •

edited

chappjc Feb 16, 2023

chappjc Feb 16, 2023

chappjc commented Feb 16, 2023

chappjc commented Feb 17, 2023

client/asset/eth: don't fail in swapGas on estimateInitGas RPC failure #2118

client/asset/eth: don't fail in swapGas on estimateInitGas RPC failure #2118

Conversation

chappjc commented Feb 7, 2023 • edited

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Feb 9, 2023

JoeGruffins commented Feb 10, 2023

chappjc commented Feb 13, 2023

chappjc commented Feb 13, 2023

Choose a reason for hiding this comment

chappjc Feb 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Feb 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Feb 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Feb 16, 2023

chappjc commented Feb 17, 2023

chappjc commented Feb 7, 2023 •

edited

chappjc Feb 14, 2023 •

edited

chappjc Feb 16, 2023 •

edited

chappjc Feb 16, 2023 •

edited