zec: implement zip 317 #2338
Closed
zec: implement zip 317 #2338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
buck54321
force-pushed
the
zec-fees
branch
2 times, most recently
from
5385420 to
eb2af19
Compare
martonp
reviewed
May 4, 2023
server/asset/btc/tx.go
Outdated
| @@ -26,6 +26,9 @@ type Tx struct { | |||
| // Used to conditionally skip block lookups on mempool transactions during | |||
| // calls to Confirmations. | |||
| lastLookup *chainhash.Hash | |||
| // fees is the fees paid in the tx. fees is used Zcash. It is exposed by the | |||
| // Satisfies the asset.OrderEstimator interface. | ||
| func (be *ZECBackend) CalcOrderFunds(swapVal, inputCount, inputsSize, maxSwaps uint64) uint64 { | ||
| return dexzec.RequiredOrderFunds(swapVal, inputCount, inputsSize, maxSwaps) | ||
| } |
There was a problem hiding this comment.
var _ asset.OrderEstimator = (*ZECBackend)(nil)
chappjc
reviewed
May 4, 2023
docs/images/zip-0317.tex
Outdated
| @@ -0,0 +1,20 @@ | |||
| \documentclass[]{article} | |||
There was a problem hiding this comment.
Why don't we just link to the ZIP 317 page? I don't think we need to check in a 120 KB png file either.
There was a problem hiding this comment.
Ooooh. I was reading at https://github.com/zcash/zips/blob/main/zip-0317.rst, where the tex doesn't render
buck54321
commented
May 9, 2023
| @@ -4951,11 +5031,10 @@ var dummyP2PKHScript = []byte{0x76, 0xa9, 0x14, 0xe4, 0x28, 0x61, 0xa, | |||
|
|
|||
| // EstimateSendTxFee returns a tx fee estimate for sending or withdrawing the | |||
| // provided amount using the provided feeRate. | |||
| func (btc *intermediaryWallet) EstimateSendTxFee(address string, sendAmount, feeRate uint64, subtract bool) (fee uint64, isValidAddress bool, err error) { | |||
| func (btc *baseWallet) EstimateSendTxFee(address string, sendAmount, feeRate uint64, subtract bool) (fee uint64, isValidAddress bool, err error) { | |||
There was a problem hiding this comment.
This makes the electrum wallets TxFeeEstimators. Anything wrong with that @chappjc?
There was a problem hiding this comment.
Not in principle. With your move of estimateSendTxFee to be a baseWallet method, it looks like the existing (*electrumWallet).listUnspent is adequate.
buck54321
force-pushed
the
zec-fees
branch
2 times, most recently
from
86a225e to
c5eeb35
Compare
|
Replaced by #2553 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support for ZIP-0317 fee calculations, though we've always done fees for Zcash wrong. Previously, fee should have been 1000 zats per tx, regardless of size. Now, fees are 5000 zats per "logical action".
Making these calculations possible required a pretty deep integration. I've tried to do so in a way that makes this easy to review. This has been roughly tested manually.