-
Notifications
You must be signed in to change notification settings - Fork 96
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
base: master
Are you sure you want to change the base?
multi: Add fee estimate oracle for tatanka #2769
Conversation
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. |
// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
// 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Signed-off-by: Philemon Ukane <[email protected]>
Signed-off-by: Philemon Ukane <[email protected]>
Signed-off-by: Philemon Ukane <[email protected]>
534bdaa
to
a00d151
Compare
This PR adds the
txfee
pkg that provided external fee estimates fortatanka
. To test this PR, please change the demo client network totestnet
ormainnet
.Chains with
mainnet
andtestnet
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