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

client/asset/zec: separate Zcash wallet #2553

Merged
merged 5 commits into from Nov 19, 2023
Merged

Conversation

buck54321
Copy link
Member

@buck54321 buck54321 commented Oct 3, 2023

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.

Copy link
Contributor

@martonp martonp left a 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.

client/asset/zec/zec.go Outdated Show resolved Hide resolved

// WalletConfig are wallet-level configuration settings.
type WalletConfig struct {
UseSplitTx bool `ini:"txsplit"`
Copy link
Contributor

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?

Copy link
Member Author

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.

client/asset/zec/zec.go Outdated Show resolved Hide resolved

if acctBal.Transparent+locked != fullTBalance {
return nil, errors.New(
"there appears to be some transparent balance that is not in the zeroth account. " +
Copy link
Contributor

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?

Copy link
Member Author

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.

client/asset/btc/coinmanager.go Outdated Show resolved Hide resolved
client/asset/btc/redemption-finder.go Outdated Show resolved Hide resolved
client/asset/btc/redemption-finder.go Outdated Show resolved Hide resolved
Comment on lines +759 to +762
deserializeTx func([]byte) (*wire.MsgTx, error)
serializeTx func(*wire.MsgTx) ([]byte, error)
calcTxSize func(*wire.MsgTx) uint64
hashTx func(*wire.MsgTx) *chainhash.Hash
Copy link
Contributor

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?

Copy link
Member Author

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.

client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/zec/zec.go 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.

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.

client/asset/btc/coinmanager.go Show resolved Hide resolved
client/asset/btc/coinmanager.go Outdated Show resolved Hide resolved
client/asset/btc/coinmanager.go Outdated Show resolved Hide resolved
client/asset/btc/redemption-finder.go Outdated Show resolved Hide resolved
client/asset/btc/redemption-finder.go Outdated Show resolved Hide resolved
client/asset/btc/redemption-finder.go Outdated Show resolved Hide resolved
client/asset/btc/redemption-finder.go Outdated Show resolved Hide resolved
client/asset/zec/zec.go Outdated Show resolved Hide resolved
client/asset/zec/zec.go Outdated Show resolved Hide resolved
Comment on lines +1101 to +1118
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
}
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

After restarting the wallet the pre-size message is strange as well, BTC ZEC:
image

@buck54321
Copy link
Member Author

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

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.

@buck54321 buck54321 force-pushed the zec-take2 branch 4 times, most recently from ee423d6 to 79f2976 Compare November 11, 2023 15:38
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.

Working well.

Separates the Zcash wallet. Reuse redemption finding and utxo management.
Enabled shielded-split funding of orders.
@buck54321 buck54321 merged commit 281e6bc into decred:master Nov 19, 2023
5 checks passed
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.

None yet

3 participants