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

server/eth: Match fee rate = MaxFeeRate #1447

Merged
merged 6 commits into from
Feb 8, 2022
Merged

server/eth: Match fee rate = MaxFeeRate #1447

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
466f7c1
server/eth: Match fee rate = MaxFeeRate
martonp Jan 25, 2022
23d7495
server/eth: Update fee rate to 2*baseFee+tipCap
martonp Jan 25, 2022
8e353d7
Update based on review.
martonp Jan 26, 2022
71ce046
Updates based on review
martonp Feb 3, 2022
978857d
Add documentation regarding legacy transactions
martonp Feb 4, 2022
0f83b18
eth/client: Only check GasFeeCap() when getting node balance
martonp Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions client/asset/eth/eth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ var (
dex.Simnet: 42, // see dex/testing/eth/harness.sh
}

minGasTipCap = dexeth.GweiToWei(2)

findRedemptionCoinID = []byte("FindRedemption Coin")
)

Expand Down Expand Up @@ -697,9 +695,7 @@ func (eth *ExchangeWallet) Swap(swaps *asset.Swaps) ([]asset.Receipt, asset.Coin
return nil, nil, 0, fmt.Errorf("unfunded contract. %d < %d", totalInputValue, totalSpend)
}

// TODO: Fix fee rate. The current fee rate returned from the server
// will not allow this to be mined on simnet.
tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, 200 /*swaps.FeeRate*/, swaps.AssetVersion)
tx, err := eth.node.initiate(eth.ctx, swaps.Contracts, swaps.FeeRate, swaps.AssetVersion)
if err != nil {
return fail(fmt.Errorf("Swap: initiate error: %w", err))
}
Expand Down Expand Up @@ -789,9 +785,7 @@ func (eth *ExchangeWallet) Redeem(form *asset.RedeemForm) ([]dex.Bytes, asset.Co

// TODO: make sure the amount we locked for redemption is enough to cover the gas
// fees. Also unlock coins.
// TODO: Fix fee rate. The current fee rate returned from the server
// will not allow this to be mined on simnet.
tx, err := eth.node.redeem(eth.ctx, form.Redemptions, 200 /*form.FeeSuggestion*/, contractVersion)
tx, err := eth.node.redeem(eth.ctx, form.Redemptions, form.FeeSuggestion, contractVersion)
if err != nil {
return fail(fmt.Errorf("Redeem: redeem error: %w", err))
}
Expand Down
5 changes: 3 additions & 2 deletions client/asset/eth/nodeclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,8 +488,9 @@ func (n *nodeClient) netFeeState(ctx context.Context) (baseFees, tipCap *big.Int
return nil, nil, err
}

if tip.Cmp(minGasTipCap) < 0 {
tip = new(big.Int).Set(minGasTipCap)
minGasTipCapWei := dexeth.GweiToWei(dexeth.MinGasTipCap)
if tip.Cmp(minGasTipCapWei) < 0 {
tip = new(big.Int).Set(minGasTipCapWei)
}

return base, tip, nil
Expand Down
4 changes: 3 additions & 1 deletion client/asset/eth/nodeclient_harness_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,9 @@ func feesAtBlk(ctx context.Context, n *nodeClient, blkNum int64) (fees *big.Int,
if err != nil {
return nil, err
}
tip := new(big.Int).Set(minGasTipCap)

minGasTipCapWei := dexeth.GweiToWei(dexeth.MinGasTipCap)
tip := new(big.Int).Set(minGasTipCapWei)

return tip.Add(tip, hdr.BaseFee), nil
}
Expand Down
1 change: 1 addition & 0 deletions dex/networks/eth/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const (
// in over which we consider the chain to be out of sync.
MaxBlockInterval = 180
EthBipID = 60
MinGasTipCap = 2 //gwei
)

var (
Expand Down
1 change: 1 addition & 0 deletions run_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ go test "${dumptags[@]}" dcrlive ./server/asset/dcr
go test "${dumptags[@]}" btclive ./server/asset/btc
go test "${dumptags[@]}" ltclive ./server/asset/ltc
go test "${dumptags[@]}" bchlive ./server/asset/bch
go test "${dumptags[@]}" harness,lgpl ./server/asset/eth
go test "${dumptags[@]}" pgonline ./server/db/driver/pg

# Return to initial directory.
Expand Down
12 changes: 12 additions & 0 deletions server/asset/btc/btc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,18 @@ func (btc *Backend) FeeRate(_ context.Context) (uint64, error) {
return btc.estimateFee(btc.node)
}

// SupportsDynamicTxFee returns true if the tx fee for this asset adjusts based
// on market conditions
func (*Backend) SupportsDynamicTxFee() bool {
return false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't need to implement this here and in dcr if you made SupportsDynamicTxFee part of an optional interface like we're doing with the client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm now thinking this might not work like I expected, since it's just a boolean return value. So we'd verify that the interface is a DynamicFeeSupporter or whatever, and then still check the boolean? What we really need is something akin to WalletInfo, but on the server side. I'm okay with this as is too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it works as is. @martonp can try some stuff out I guess, but I can see the optional method being SwapFeeRate, which would make feeFetcher unaware of the whole dynamic<->max_fee relationship. Maybe we like that dynamic/max concept front and center?

Specifically, changing

// SwapFeeRate returns the tx fee that needs to be used to initiate a swap.
// This fee will be the max fee rate if the asset supports dynamic tx fees,
// and otherwise it will be the current market fee rate.
func (f *feeFetcher) SwapFeeRate(ctx context.Context) uint64 {
	if f.Backend.SupportsDynamicTxFee() {
		return f.MaxFeeRate()
	}
	return f.FeeRate(ctx)
}

into

// SwapFeeRate returns the tx fee that needs to be used to initiate a swap.
// This will use the Backend's SwapFeeRate method if it exists, otherwise
// it will request the fee rate based on current network conditions.
func (f *feeFetcher) SwapFeeRate(ctx context.Context) uint64 {
	if fr, ok := f.Backend.(asset.SwapFeeRater); ok {
		return fr.SwapFeeRate()
	}
	return f.FeeRate(ctx)
}

The question is do we hide the whole dynamic-must-use-max concept in the Backend's methods or do we apply that notion at the higher levels like done currently in this PR?

Copy link
Collaborator Author

@martonp martonp Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't work because the backend doesn't have access to the MaxFeeRate. It's stored in the dex.Asset.

Copy link
Member

@chappjc chappjc Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, that's right. Max fee rate is a market/swapper concept, so must be done higher up. ✔️


// ValidateFeeRate checks that the transaction fees used to initiate the
// contract are sufficient.
func (*Backend) ValidateFeeRate(contract *asset.Contract, reqFeeRate uint64) bool {
return contract.FeeRate() >= reqFeeRate
}

// CheckAddress checks that the given address is parseable.
func (btc *Backend) CheckAddress(addr string) bool {
_, err := btc.decodeAddr(addr, btc.chainParams)
Expand Down
6 changes: 6 additions & 0 deletions server/asset/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ type Backend interface {
// Synced should return true when the blockchain is synced and ready for
// fee rate estimation.
Synced() (bool, error)
// SupportsDynamicTxFee returns true if the tx fee for this asset adjusts based
// on market conditions.
SupportsDynamicTxFee() bool
// ValidateFeeRate checks that the transaction fees used to initiate the
// contract are sufficient.
ValidateFeeRate(contract *Contract, reqFeeRate uint64) bool
}

// OutputTracker is implemented by backends for UTXO-based blockchains.
Expand Down
12 changes: 12 additions & 0 deletions server/asset/dcr/dcr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,18 @@ func (dcr *Backend) FeeRate(ctx context.Context) (uint64, error) {
return atomsPerB, nil
}

// SupportsDynamicTxFee returns true if the tx fee for this asset adjusts based
// on market conditions
func (*Backend) SupportsDynamicTxFee() bool {
return false
}

// ValidateFeeRate checks that the transaction fees used to initiate the
// contract are sufficient.
func (*Backend) ValidateFeeRate(contract *asset.Contract, reqFeeRate uint64) bool {
return contract.FeeRate() >= reqFeeRate
}

// BlockChannel creates and returns a new channel on which to receive block
// updates. If the returned channel is ever blocking, there will be no error
// logged from the dcr package. Part of the asset.Backend interface.
Expand Down
15 changes: 15 additions & 0 deletions server/asset/eth/coiner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ type baseCoin struct {
backend *Backend
secretHash [32]byte
gasPrice uint64
gasTipCap uint64
dynamicTx bool
txHash common.Hash
value uint64
txData []byte
Expand Down Expand Up @@ -145,9 +147,11 @@ func (eth *Backend) baseCoin(coinID []byte, contractData []byte) (*baseCoin, err
// transaction with a very low gas price may need to be resent with a
// higher price.
zero := new(big.Int)
var dynamicTx bool
rate := tx.GasPrice()
if rate == nil || rate.Cmp(zero) <= 0 {
rate = tx.GasFeeCap()
dynamicTx = true
if rate == nil || rate.Cmp(zero) <= 0 {
return nil, fmt.Errorf("Failed to parse gas price from tx %s", txHash)
}
Expand All @@ -158,6 +162,15 @@ func (eth *Backend) baseCoin(coinID []byte, contractData []byte) (*baseCoin, err
return nil, fmt.Errorf("unable to convert gas price: %v", err)
}

var gasTipCap uint64
priorityFee := tx.GasTipCap()
if priorityFee != nil {
gasTipCap, err = dexeth.WeiToGweiUint64(priorityFee)
if err != nil {
return nil, fmt.Errorf("unable to convert priority fee: %v", err)
}
}

// Value is stored in the swap with the initialization transaction.
value, err := dexeth.WeiToGweiUint64(tx.Value())
if err != nil {
Expand All @@ -168,6 +181,8 @@ func (eth *Backend) baseCoin(coinID []byte, contractData []byte) (*baseCoin, err
backend: eth,
secretHash: secretHash,
gasPrice: gasPrice,
gasTipCap: gasTipCap,
dynamicTx: dynamicTx,
txHash: txHash,
value: value,
txData: tx.Data(),
Expand Down
51 changes: 47 additions & 4 deletions server/asset/eth/eth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ type ethFetcher interface {
headerByHeight(ctx context.Context, height uint64) (*types.Header, error)
connect(ctx context.Context, ipc string, contractAddr *common.Address) error
shutdown()
suggestGasPrice(ctx context.Context) (*big.Int, error)
suggestGasTipCap(ctx context.Context) (*big.Int, error)
syncProgress(ctx context.Context) (*ethereum.SyncProgress, error)
swap(ctx context.Context, secretHash [32]byte) (*dexeth.SwapState, error)
transaction(ctx context.Context, hash common.Hash) (tx *types.Transaction, isMempool bool, err error)
Expand Down Expand Up @@ -231,11 +231,54 @@ func (eth *Backend) InitTxSizeBase() uint32 {

// FeeRate returns the current optimal fee rate in gwei / gas.
func (eth *Backend) FeeRate(ctx context.Context) (uint64, error) {
bigGP, err := eth.node.suggestGasPrice(ctx)
hdr, err := eth.node.bestHeader(ctx)
if err != nil {
return 0, err
return 0, fmt.Errorf("error getting best header: %w", err)
}
return dexeth.WeiToGweiUint64(bigGP)

if hdr.BaseFee == nil {
return 0, errors.New("eth block header does not contain base fee")
}

suggestedGasTipCap, err := eth.node.suggestGasTipCap(ctx)
if err != nil {
return 0, fmt.Errorf("error getting suggested gas tip cap: %w", err)
}

feeRate := new(big.Int).Add(
suggestedGasTipCap,
new(big.Int).Mul(hdr.BaseFee, big.NewInt(2)))

feeRateGwei, err := dexeth.WeiToGweiUint64(feeRate)
if err != nil {
return 0, fmt.Errorf("failed to convert wei to gwei: %w", err)
}

return feeRateGwei, nil
}

// SupportsDynamicTxFee returns true if the tx fee for this asset adjusts based
// on market conditions
func (*Backend) SupportsDynamicTxFee() bool {
return true
}

// ValidateFeeRate checks that the transaction fees used to initiate the
// contract are sufficient. For most assets only the contract.FeeRate() cannot
// be less than reqFeeRate, but for Eth, the gasTipCap must also be checked.
func (eth *Backend) ValidateFeeRate(contract *asset.Contract, reqFeeRate uint64) bool {
coin := contract.Coin
sc, ok := coin.(*swapCoin)
if !ok {
eth.log.Error("eth contract coin type must be a swapCoin but got %T", sc)
return false
}

if sc.dynamicTx && sc.gasTipCap < dexeth.MinGasTipCap {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably just enforce dynamicTx = true for all of our transactions.

return false
}

return contract.FeeRate() >= reqFeeRate
}

// BlockChannel creates and returns a new channel on which to receive block
Expand Down
Loading