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.

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

multi: Add fee estimate oracle for tatanka #2769

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ukane-philemon
Copy link
Contributor

This PR adds the txfee pkg that provided external fee estimates for tatanka. To test this PR, please change the demo client network to testnet or mainnet.

Chains with mainnet and testnet coverage are: btc, doge, dcr, eth, ltc, dash, bch, zcash. Some fee estimate sources require Free API keys that can be generated on the provider's site.

Closes #2726

@ukane-philemon
Copy link
Contributor Author

A quick note: A tatanka only provides fee estimates for its chains. We might consider providing a fee estimate even if Tatanka does not run a particular chain, mainly for client consumption. The same applies to fiat rates.

dex/txfee/oracle.go Show resolved Hide resolved
Comment on lines +14 to +16
// sources. Fee estimate values are in atoms for dcr, gwei for ethereum,
// satoshis for bitcoin and bitcoin clone blockchains (per byte sat), or the
// lowest non-divisible unit in other non-Bitcoin blockchains.
Copy link
Contributor

Choose a reason for hiding this comment

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

They are all in the DEX AtomicUnit of the asset, right?

Copy link
Member

Choose a reason for hiding this comment

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

@ukane-philemon I though wei for eth was fine. It looks like it is returning wei.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are all in the DEX AtomicUnit of the asset, right?

Not sure what you mean, but they are all in their smallest denom, except eth (gwei). But a review from @JoeGruffins suggests wei so yes.

Copy link
Member

@buck54321 buck54321 May 19, 2024

Choose a reason for hiding this comment

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

We use "atomic units" to refer to the integer units used with asset.Wallet and asset.Backend. How asset.Wallet and asset.Backend scale the "atomic units" internally is of no concern outside of the respective wallet packages, really. Well, and here I guess. That said, we already know that there are some problems with this system. Specifically, Polygon and other evm blockchains can have fee rates smaller than 1 gwei per gas. This means that for swaps, we have to lock at least 1 gwei per gas, even if the actual network rate is substantially lower. The rate actually assessed is limited by the block's base fee rate, of course, but fee rates can determine minimum lot sizes and mm bot spreads etc. Luckily, 1 gwei / gas is very, very small even for ethereum, so the effects are not really noticeable. I do think it's worth discussing alternative ways to encode fee rates and maybe values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically, Polygon and other evm blockchains can have fee rates smaller than 1 gwei per gas. This means that for swaps, we have to lock at least 1 gwei per gas, even if the actual network rate is substantially lower. The rate actually assessed is limited by the block's base fee rate, of course, but fee rates can determine minimum lot sizes and mm bot spreads etc. Luckily, 1 gwei / gas is very, very small even for ethereum, so the effects are not really noticeable

I think we should stick to gwei for eth for now.

dex/txfee/types.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/oracle.go Outdated Show resolved Hide resolved
dex/txfee/oracle.go Outdated Show resolved Hide resolved
dex/txfee/oracle.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/oracle.go Outdated Show resolved Hide resolved
dex/txfee/oracle.go Outdated Show resolved Hide resolved
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

For some reason the demo is hanging although I see the reply logic I think.

Comment on lines +14 to +16
// sources. Fee estimate values are in atoms for dcr, gwei for ethereum,
// satoshis for bitcoin and bitcoin clone blockchains (per byte sat), or the
// lowest non-divisible unit in other non-Bitcoin blockchains.
Copy link
Member

Choose a reason for hiding this comment

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

@ukane-philemon I though wei for eth was fine. It looks like it is returning wei.

dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/oracle.go Show resolved Hide resolved
dex/txfee/oracle.go Outdated Show resolved Hide resolved
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Partial review. Just some notes.

dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/sources.go Outdated Show resolved Hide resolved
dex/txfee/oracle.go Outdated Show resolved Hide resolved
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tatanka: Make fee oracles more robust.
4 participants