client/eth: Respect user's max fee. #1485

JoeGruffins · 2022-02-24T02:29:05Z

Check initiation and redeem fee suggestions are not zero or too high. If
so attempt to get our own gas limit value. If that also fails or is above
our max limit, do not send the transaction.

closes #1336

Check initiation and redeem fee suggestions are not zero or too high. If so attempt to get our own gas rate limit value. If that also fails or is above our max limit, do not send the transaction.

JoeGruffins · 2022-02-24T08:25:00Z

Actually the server is pretty strict about this, so I guess we can't use our estimation for initiations if the server's is too high. It has to be the suggestion.

buck54321

We realistically never have a fee suggestion from the server, since the wallet implements FeeRater.

buck54321 · 2022-02-24T11:56:36Z

client/asset/eth/eth.go

-	tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, swaps.FeeRate, swaps.AssetVersion)
+	tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, gasFeeLimit, swaps.AssetVersion)


Can't do this part. This is validated by the server.

More than that, the "fee limit" was only ever intended to block order funding, nothing else. In fact, this is the setting that I fought to keep out of the wallet since Core's trade methods (prepareTrackedTrade) could have just as easily applied this user set limit in the one place it was intended. #874 (comment)
I suppose the limit can expand in scope somewhat to limit the wallet's own local estimate though.

chappjc · 2022-02-24T15:18:15Z

client/asset/eth/eth.go

+	if lowerRate > 0 {
+		eth.log.Warnf("Eth gas rate limit too low! Set above %d gwei to complete current transaction!",
+			lowerRate)
+		return 0, fmt.Errorf("the needed gas fee rate limit (%d gwei) is higher than set max (%d gwei)", lowerRate, eth.gasFeeLimit)


No way we can just block inits, redeems, or refunds.

chappjc · 2022-02-24T15:23:28Z

nACK, Unless really I'm misunderstanding something.

We cannot just not do the swap, redeem, or refund txn because the fee suggestion is "too high". Placing and funding the order commits us to executing the swap tnx, and we'd be insane not to refund or redeem on account of fee rates.
The fee rate limit setting was initially only meant for order funding. That would be in (*ExchangeWallet).FundOrder although when the setting was initially introduced, I fought hard to keep it out of the wallet entirely and instead applied at the level of Core, for exactly these reasons - scope of the setting in the wallet. EDIT: Possibly the limit can be applied to cap our own local estimates though.
Overpaying gas in eth with dynamic txns is not an issue at all afaik.

But I do approve of the formula for computing a local fee rate (localFeeRate := dexeth.WeiToGwei(tipCap.Add(baseFee.Mul(baseFee, big.NewInt(2)), tipCap))) and it seems that can be part of a FeeRate method just like the one that exists server-side. We've settled on this formula in several discussions I think: #1447 (comment), #1372 (comment), https://github.com/ethereum/go-ethereum/blob/356bbe343a30789e77bb38f25983c8f2f2bfbb47/accounts/abi/bind/base.go#L250-L255
I don't object to the configured fee rate limit being used to cap the result of that formula, although it is a deviation from original purpose of that setting.

chappjc · 2022-02-24T15:25:04Z

We realistically never have a fee suggestion from the server, since the wallet implements FeeRater.

Looks like only server has FeeRate implemented, but I do think it's reasonable to have client be a FeeRater using the tip + 2*base formula also.

JoeGruffins · 2022-02-25T03:00:17Z

We cannot just not do the swap, redeem, or refund txn because the fee suggestion is "too high". Placing and funding the order commits us to executing the swap tnx, and we'd be insane not to refund or redeem on account of fee rates.

While I agree, the server can currently insert any value here and unless the total gas goes over 1 eth (#1304) the wallet will not reject it. On the other side, the server could send something too low and screw the user, which this pr wouldn't solve. There is currently no resolution for txn with fees that are too low with eth. Low fee tx are a bigger problem with eth. IMO client can not blindly trust the server.

Also, I guess I misundersood the meaning of the gas limit. It only applies to initiations, and not to anything else.

Can't we just not do the swap if fee is too high/low though? I mean initiation. I agree it could be catastrophic to not redeem/refund. I would agree that after you initiate, you kinda agreed to pay whatever fees you need to to complete the swap.

chappjc · 2022-02-25T03:14:01Z

can currently insert any value here and ... wallet will not reject it

It does not need to reject it because this swaps.FeeRate is checked at match time. When the match is made, that is when the client checks the prescribed swap fee rate to make sure it's not higher than the limit that was recorded at order placement time when FundOrder enforces the user's fee rate limit. See parseMatches and it's check for swapRate > tracker.metaData.MaxFeeRate:

dcrdex/client/core/core.go

Lines 577 to 579 in 26b0c82

    
           if !isCancel && swapRate > tracker.metaData.MaxFeeRate { 
        
           	errs = append(errs, fmt.Sprintf("rejecting match %s for order %s because assigned rate (%d) is > MaxFeeRate (%d)", 
        
           		msgMatch.MatchID, msgMatch.OrderID, swapRate, tracker.metaData.MaxFeeRate))

and the db type where the expected max fee rate is recorded during order placement

dcrdex/client/db/types.go

Lines 259 to 261 in 26b0c82

    
           // MaxFeeRate is the dex.Asset.MaxFeeRate at the time of ordering. The rates 
        
           // assigned to matches will be validated against this value. 
        
           MaxFeeRate uint64

and FundOrder actually rejecting order funding if the server's max fee rate (above) is too high

dcrdex/client/asset/dcr/dcr.go

Lines 1061 to 1070 in 26b0c82

    
           if ord.FeeSuggestion > dcr.feeRateLimit { 
        
           	return nil, nil, fmt.Errorf("suggested fee > configured limit. %d > %d", ord.FeeSuggestion, dcr.feeRateLimit) 
        
           } 
        
           // Check wallet's fee rate limit against server's max fee rate 
        
           if dcr.feeRateLimit < ord.DEXConfig.MaxFeeRate { 
        
           	return nil, nil, fmt.Errorf( 
        
           		"%v: server's max fee rate %v higher than configured fee rate limit %v", 
        
           		ord.DEXConfig.Symbol, 
        
           		ord.DEXConfig.MaxFeeRate, 
        
           		dcr.feeRateLimit)

From PRs #874 and #898

chappjc · 2022-02-25T03:28:16Z

As for too low, sure we can use any fee rate minimum as long as its not over the MaxFeeRate because that would potentially eat into the reserves required for the swap(s) for the order.

On the high end limit though, if the wallet did block a swap/init because the fee was higher than it's configured fee limit, that could just be because the fee limit was changed after the order was placed (funded). I wouldn't object to a second very high limit as a sanity check, but it would have to be super high, as in beyond any setting the user might switch their gasFeeLimit setting to.

chappjc · 2022-02-25T03:36:16Z

As for too low, sure we can use any fee rate minimum as long as its not over the MaxFeeRate because that would potentially eat into the reserves required for the swap(s) for the order.

In fact, I would 100% support changes to all asset impls to do this. Particularly BTC where as taker you are using a fee rate prescribed at match time that was potentially a long time ago if the maker's swap took a while to confirm. It should be fine to use any rate x that is on swapRate <= x <= db.OrderMetaData.MaxFeeRate. For a FeeRater, the asset could use it's own rate as long as it is in that range. For a non-FeeRater it could do a c.feeSuggestionAny or a dc.fetchFeeRate.

JoeGruffins · 2022-02-25T03:36:22Z

I think I see it now. The ord.DEXConfig.MaxFeeRate is saved to the order metadata and used for all the matches, as a max. So it's not per match I guess? Ignoring changes in the fee rate from time of placing order?

chappjc · 2022-02-25T03:38:58Z

I think I see it now. The ord.DEXConfig.MaxFeeRate is saved to the order metadata and used for all the matches, as a max. So it's not per match I guess? Ignoring changes in the fee rate from time of placing order?

Right, you have a certain configured fee rate limit at order time. If the server's MaxFeeRate is less than that limit, it is recorded in the order's metadata. That max is enforced for all matches made for that order. When each match is made, the prescribed rate is checked against that recorded per-order limit.

JoeGruffins · 2022-02-25T03:46:17Z

Ok, then we were respecting the max in the manner intended.

chappjc · 2022-02-25T03:48:45Z

Yes although in this contorted way because the user's fee limit is configurable and this need to be recorded with the order metadata.

I like the idea of bumping the rate if it looks too low though

JoeGruffins · 2022-02-25T03:51:05Z

But the redeem is not checked at all, and we are using the suggestion.

JoeGruffins · 2022-02-25T04:02:04Z

Oh, I see, if it were a FeeRater, it would be using the local fee?

JoeGruffins · 2022-02-25T04:02:22Z

I think @martonp said on matrix he would add that.

chappjc · 2022-02-25T04:03:13Z

Ah, yeah, @buck54321 's point there is that the eth client should be a FeeRater, not needing the suggestion at all. @martonp said he was going to make that change in his PR. Would be good to coordinate with him on this so we're on the same page.

JoeGruffins · 2022-02-25T04:10:12Z

I guess I will close this pr and the linked issue. Will make a new issue for suggested fees that are too low.

JoeGruffins force-pushed the respectusermax branch 2 times, most recently from e60d00b to d3313f1 Compare February 24, 2022 06:52

client/eth: Respect user's max fee.

da0754d

Check initiation and redeem fee suggestions are not zero or too high. If so attempt to get our own gas rate limit value. If that also fails or is above our max limit, do not send the transaction.

JoeGruffins force-pushed the respectusermax branch from d3313f1 to da0754d Compare February 24, 2022 07:07

buck54321 reviewed Feb 24, 2022

View reviewed changes

chappjc reviewed Feb 24, 2022

View reviewed changes

JoeGruffins closed this Feb 25, 2022

This was referenced Feb 25, 2022

client/eth: Respect user's max fee rate. #1336

Closed

assets: Bump fee rate or gas cap when applicable. #1489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/eth: Respect user's max fee. #1485

client/eth: Respect user's max fee. #1485

JoeGruffins commented Feb 24, 2022 •

edited

Loading

JoeGruffins commented Feb 24, 2022 •

edited

Loading

buck54321 left a comment

buck54321 Feb 24, 2022

chappjc Feb 24, 2022 •

edited

Loading

chappjc Feb 24, 2022

chappjc commented Feb 24, 2022 •

edited

Loading

chappjc commented Feb 24, 2022

JoeGruffins commented Feb 25, 2022 •

edited

Loading

chappjc commented Feb 25, 2022 •

edited

Loading

chappjc commented Feb 25, 2022 •

edited

Loading

chappjc commented Feb 25, 2022 •

edited

Loading

JoeGruffins commented Feb 25, 2022

chappjc commented Feb 25, 2022 •

edited

Loading

JoeGruffins commented Feb 25, 2022

chappjc commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

chappjc commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

		tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, swaps.FeeRate, swaps.AssetVersion)
		tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, gasFeeLimit, swaps.AssetVersion)

client/eth: Respect user's max fee. #1485

client/eth: Respect user's max fee. #1485

Conversation

JoeGruffins commented Feb 24, 2022 • edited Loading

JoeGruffins commented Feb 24, 2022 • edited Loading

buck54321 left a comment

Choose a reason for hiding this comment

buck54321 Feb 24, 2022

Choose a reason for hiding this comment

chappjc Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc Feb 24, 2022

Choose a reason for hiding this comment

chappjc commented Feb 24, 2022 • edited Loading

chappjc commented Feb 24, 2022

JoeGruffins commented Feb 25, 2022 • edited Loading

chappjc commented Feb 25, 2022 • edited Loading

chappjc commented Feb 25, 2022 • edited Loading

chappjc commented Feb 25, 2022 • edited Loading

JoeGruffins commented Feb 25, 2022

chappjc commented Feb 25, 2022 • edited Loading

JoeGruffins commented Feb 25, 2022

chappjc commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

chappjc commented Feb 25, 2022

JoeGruffins commented Feb 25, 2022

JoeGruffins commented Feb 24, 2022 •

edited

Loading

JoeGruffins commented Feb 24, 2022 •

edited

Loading

chappjc Feb 24, 2022 •

edited

Loading

chappjc commented Feb 24, 2022 •

edited

Loading

JoeGruffins commented Feb 25, 2022 •

edited

Loading

chappjc commented Feb 25, 2022 •

edited

Loading

chappjc commented Feb 25, 2022 •

edited

Loading

chappjc commented Feb 25, 2022 •

edited

Loading

chappjc commented Feb 25, 2022 •

edited

Loading