multi: refactor wallet configuration

[buck54321]

Password fields are not echoed. Boolean fields are displayed as checkboxes. Wallets can be reconfigured through the wallets view.

Some UI updates for the dynamic config form. Inputs are only half width now, and the description is displayed as a tooltip.

[chappjc]

Looks good on cursory review, although I'd change username=satoshi to username=tacotime. :)

[JoeGruffins]

For some reason I am not able to load the entire config from file, only the cert field is populated.

[buck54321]

@JoeGruffins Not sure what you're seeing. I do know that if I try to load the dcrd configuration file instead of the dcrwallet configuration file, It'll populate rpccert and rpclisten, because dcrd and dcrwallet configs share those field names.

If you could dive a little deeper and see if you can track down the issue, I'd appreciate it. As always, make sure that your web assets are rebuilt and do a hard refresh in your browser.

[JoeGruffins]

Configs are loading fine. I must have missed a step somewhere, as I see no problem now. Sorry.

[chappjc]

Should this also wallet.Lock() before disconnecting?

[buck54321]

I believe this came up somewhere else recently. I didn't think it was in-scope for this PR, but I think that wallets should lock internally during disconnection.

[chappjc]

Fair enough, we'll address this in a follow up.

[chappjc]

Can we disabled the input until #562 goes in, forcing it to be unchecked on submission? Or would that break the form submission entirely?

[buck54321]

I'll just comment out the ConfigOption for now. I just wanted to show off the boolean/checkbox inputs, really.

[chappjc]

With the suggested rpcserver/types.go change, dcr and btc newwallet worked as expected with:

$    ./dexcctl newwallet 42 ~/dextest/dcr/alpha/w-alpha.conf '{"account":"default"}'
App password:
Wallet password:
dcr wallet created and unlocked

$   ./dexcctl newwallet 0 ~/dextest/btc/harness-ctl/alpha.conf '{"walletname":"gamma"}'
App password:
Wallet password:
btc wallet created and unlocked

@JoeGruffins I know you have a similar setup workflow, so please note the above.

[chappjc]

Do we want this to match dcrwallet's txfee setting?

--txfee= Transaction fee per kilobyte (default: 0.0001 DCR)

dcrwallet doesn't have a fallbackfee setting and I'm honestly not sure what happens if fee estimation fails with dcrwallet or even which txn rpcs try to estimate a smart fee vs just using txfee.

Using fallbackfee for DCR does make it more consistent though...

[buck54321]

estimatesmartfee is actually dcrd, and it looks like it can only error in estimateMedianFee here, and probably can't happen on mainnet or testnet3.

edit: and probably not on simnet the way we use it in our harness

[chappjc]

I was just referring to the way bitcoin wallets use fallbackfee for authoring transactions with the wallet when fee estimation fails, not just by the estimatesmartfee RPC directly. With decred, it's dcrwallet making transactions using txfee presumably (although smart fee estimation would be preferred when dcr fees get higher). So I was just wondering if it makes sense to use txfee from a dcrwallet config to get fallbackfee since "fallbackfee" is only a bitcoin thing.

EDIT: But I prefer consistency with these settings to the extent possible, and txsplit isn't a setting for either wallet, so I'm ok with having a number of these settings not be recognized by the wallet software.

[buck54321]

We're not using it as a default transaction fee in the same way as dcrwallet, but as a value to use when estimatesmartfee fails, which is how it's used for bitcoin, so I thought retaining the name made sense.

[buck54321]

as a value to use when estimatesmartfee fails

Which also means that it'll probably never be used on mainnet anyways, if that means anything.

[chappjc]

Yeah, I don't have a problem keeping the name. But estimatesmartfee falis on mainnet until the node has a chance to watch mempool and a few blocks go by (which mempool txns are getting included is part of it so it really does require bitcoind to be running for a bit or a very recent fee_estimates.dat and mempool.dat). Demonstrated in #501 This will get used on mainnet and not just for freshly syncd nodes.

[JoeGruffins]

Working well for me.

[chappjc]

On the GUI it's showing and asking for Sat/kB

but both NewWallet implementations for dcr and btc are treating this as BTC/kB or DCR/kB with toAtoms/toSats:

feesPerByte := toAtoms(walletCfg.FallbackFeeRate / 1000)
if feesPerByte == 0 {
	feesPerByte = defaultFee
}
logger.Debugf("fallbackfeerate: %v => feesPerByte: %v", walletCfg.FallbackFeeRate, feesPerByte)

shows

2020-07-30 16:11:30.551 [DBG] CORE[btc]: fallbackfeerate: 100000 => feesPerByte: 10000000000
2020-07-30 16:11:32.988 [DBG] CORE[dcr]: fallbackfeerate: 20000 => feesPerByte: 2000000000

I guess the issue is we're trying to handle bitcoin.conf that has fallbackfee=0.0002 (float of BTC/kB), but inputting from the GUI in integer sat/atom based units.

Also, with Sat/kB and Atoms/kB, that's currently problematic since any extra precision is truncated when this is converted to Sat/B and Atoms/B. However, I'm thinking it's quite likely we'll switch everything under the hood to per kB.

[buck54321]

I think I've got the units right now. Nice analysis of the situation. I'm not a fan of units like DCR/kB, but it does seem to be the standard for UI units, and does allow for higher precision. I hesitate to settle on a unit at the DEX level. Rather we should move towards a system where the Backend or ExchangeWallet software handles any calculations that deal with transaction fees (we're actually pretty close already), though some operations like simple comparison would still be feasible at higher levels. We should attempt to remove all mention of specific fee units from the spec. As long as the Backend and ExchangeWallet agree, that's all that matters.

Part of my reasoning here is that we already know that some blockchains use different base units for fees. Ethereum uses Gwei, which is 1e-9 ETH and deals in gas instead of bytes, I think.

[chappjc]

For a wallet UI that might be true, although I personally find an integer sat/kB much easier to grasp with since you don't have to count leading zeros and do math in your head to make sure it's the sat/byte or sat/kB that is reasonable. (Plus the floating point precision problem - we can't even represent 0.0001 without precision loss.) Most "bitcoin fees" websites will express the rate in sat/byte as it's a nice readable number even though it's not precise as sat/kB.

Anyway, no need to iterate on this further. I'm pretty happy as long as the fee rate gets to int64 inside the ExchangeWallet asap, and it does.

[chappjc]

Ugh this TestClientMap issue. I think we might be testing too much on the internals. Anyway #575 fixes it (again). The previous data race fix unintentionally created the problem (again).