-
Notifications
You must be signed in to change notification settings - Fork 102
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
client/asset/zec: separate Zcash wallet #2553
Conversation
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.
Initial pass. I will review and test more.
|
||
// WalletConfig are wallet-level configuration settings. | ||
type WalletConfig struct { | ||
UseSplitTx bool `ini:"txsplit"` |
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.
Should UseSplitTx
be removed since splitting is the default behavior that cannot be disabled?
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.
Shielded splits are default behavior now when transparent funds are insufficient, but I left the purely transparent split machinery in place to avoid overlocks.
|
||
if acctBal.Transparent+locked != fullTBalance { | ||
return nil, errors.New( | ||
"there appears to be some transparent balance that is not in the zeroth account. " + |
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.
Is this not required for shielded balances?
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.
No. Shielded balance is properly handled by the account system, afaict. The zcashd RPC stupidity is only related to transparent outputs. I can explain more on request, but it's somewhat complicated how they borked their API.
deserializeTx func([]byte) (*wire.MsgTx, error) | ||
serializeTx func(*wire.MsgTx) ([]byte, error) | ||
calcTxSize func(*wire.MsgTx) uint64 | ||
hashTx func(*wire.MsgTx) *chainhash.Hash |
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.
All these are only used by zcl now. Why doesn't zcl get the same treatment as zec?
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.
I probably will, but I'm not going to do it in this PR. I actually had removed a ton of BTCCloneCFG
fields and their related baseWallet
fields and functionality in my initial run at this, and had some major headaches when zcl was merged 🤣. I'm looking forward to cleaning up again though.
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.
Trading on testnet with everything at default values saw this:
Error notifying DEX of swap for match c790bc72a27a8503cf2034e2b9f953700c08361dc655572cec51a344b847f6ed: error sending 'init' message: rpc error: error code 25: low tx fee
But the trade went through fine.
The dexcctl/simnet-setup.sh
script will not work because funds are in somewhere thats not account 0. Also loadbot has been broken for a while for zec.
Was working well until I tried to book an order with pre-sized funds and the wallet seems to be bricked now. I commented in-line but can provide more details if needed.
if !splitUsed || splitLocked >= noSplitLocked { // locked check should be redundant | ||
opt.Boolean.Reason = "avoids no ZEC overlock for this order (ignored)" | ||
opt.Description = "A split transaction for this order avoids no ZEC overlock, " + | ||
"but adds additional fees." | ||
opt.DefaultValue = false | ||
return opt // not enabled by default, but explain why | ||
} |
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.
Maybe I should know this but what is the deal with zec and splits? I tried turning on pre-sized funds but it looks to have bricked something. Server keeps telling me:
[ERR] CORE: notify: |ERROR| (order) In-Flight Order Error - In-Flight order with ID 11 failed: new order request with DEX server 127.0.0.1:17273 market zec_dcr failed: rpc error: error code 36: not enough funds. need at least 124400, got 110000 - Order:
Even after turning splits off.
UI shows that I have funds:
available 0.88315572
locked 0
immature 0
Restarting the client or wallet does not resolve it. Unable to sell 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.
This is expected on the testnet bison.exchange. It doesn't reflect the changes in this PR, and this PR makes significant changes to the server code (remember that this code will never land in a v0.x release, so we're free to break backwards compatibility most of the time). I'll put the testnet server on this branch today. |
ee423d6
to
79f2976
Compare
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.
Working well.
Separates the Zcash wallet. Reuse redemption finding and utxo management. Enabled shielded-split funding of orders.
The Zcash wallet is so different from Bitcoin that maintenance was problematic and implementing new features is very tedious. This PR creates a full Zcash wallet that doesn't embed
ExchangeWalletNoAuth
. Many btc types and functions are exported, and efforts are made to share critical code. In particular, the coin management and redemption finding utilities are broken out in to separate types that both zec and btc can use.With the Zcash wallet separated, Zcash can now be shielded-by-default and I can work around the API hurdles I've been hung up on.
Replaces #2338.