If the previous value, FallbackFeeRate, failed validation, then validateBtcWalletConf would have returned before it set walletCfg.FeeRateLimitPerByte, which may have been valid, but it would go with the default instead.

I suggest were rework this a little, only applying the defaults to the fields that failed validation.

I removed the early return from the validate method.

I also noticed this values being re-applied on error were actually doing nothing, as they would be overwritten by the forms input, so I removed them, as well

oh, actually they need to be overwritten on error, otherwise tests fail.

Okay, undoing that

If we want default values if values not set can we set those first and then do validate and error if that errors?

Isn't it correct to return error if values are wrong here? This happens when opening an existing wallet correct? We don't want to silently ignore what the user thinks they set, overwrite it and use some values they don't want?

If we want default values if values not set can we set those first and then do validate and error if that errors?

What do you mean by set those value first? Right now the default values are populated from the configOpts and if changing its value on the form, it will not be the default values anymore.

Isn't it correct to return error if values are wrong here? This happens when opening an existing wallet correct? We don't want to silently ignore what the user thinks they set, overwrite it and use some values they don't want?

In the UI an error will be thrown, when creating the wallet, but I agree with you about returning the error, the reason I am not returning it, is to keep the old behavior:

Right now on the walletTypeSPV test it passes an empty wallet config struct, so it will fail if we return the errors here

	case walletTypeSPV:
		w, err := newUnconnectedWallet(cfg, &WalletConfig{})
		if err == nil {
			wallet = &ExchangeWalletFullNode{w}
			neutrinoClient := &tNeutrinoClient{data}
			wallet.node = &spvWallet{
				chainParams: &chaincfg.MainNetParams,
				wallet:      &tBtcWallet{data},
				cl:          neutrinoClient,
				tipChan:     make(chan *block, 1),
				chainClient: nil,
				acctNum:     0,
				txBlocks:    data.dbBlockForTx,
				checkpoints: data.checkpoints,
				log:         cfg.Logger.SubLogger("SPV"),
				loader:      nil,
			}
		}

This looks correct now. But I don't think we need the trace log as it will log as error later.

Isn't the only thing we need to do is to check that

if walletCfg.FallbackFeeRate > 0 && walletCfg.FallbackFeeRate < 1000 {
    return nil, fmt.Errorf("minimum fee rate is 1000 sats/kB")
}

If we just check for FallbackFeeRate it will still be possible to create a wallet with "invalid" values, for example:

someone tries to create a wallet with Highest acceptable fee rate less than the minimum value allowed (anything less than 1 sat/B), would create the wallet directories and db, but the form would throw an error making a wallet which can not be opened, like described in the issue.

Also, with toSatoshi() It rounds for the minimum value possible, so we just need to check if it is not 0, considering we are not checking for a maximum value.

Notice that in @buck54321's example code it permits 0, allowing the default to be applied elsewhere (caller).

The validator function can still check each of the settings that can result in an unopenable wallet.

So it does not set values on error, it sets if values are not specified.

Thanks for clarifying chapp, I had misunderstood this. I fixed it now.

Isn't the only thing we need to do is to check that

if walletCfg.FallbackFeeRate > 0 && walletCfg.FallbackFeeRate < 1000 {
    return nil, fmt.Errorf("minimum fee rate is 1000 sats/kB")
}

I'm saying this entire PR should be just these three lines. You didn't need to touch WalletConfig or create any new functions. We already had a pattern established for validating input values, We just weren't checking this condition.

okay, but the problem is actually on the Create method, which does not validate the spv wallet allowing it to create a wallet which can not be opened. Should I copy this validations over that method?

I see what you mean. It wasn't failing on creation but failed on open, which tanked the creation process. I guess wallets need to basically guarantee that the settings given to Create are valid.

We should also re-consider the sequence in CreateWallet that allows this to happen. Should we somehow delete the wallet if the full CreateWallet sequence doesn't complete? Or should we store the wallet (in db and wallets map) right after Create, so users could have recovered by adjusting configuration settings.

We should also re-consider the sequence in CreateWallet that allows this to happen. Should we somehow delete the wallet if the full CreateWallet sequence doesn't complete?

If in (*Core).CreateWallet it creates the wallet fine but then opening it fails, I agree it would be ideal to "uncreate" the wallet. I think that can be handled if the wallet is a Recoverer that allows it to be destroyed. We can follow-up with that I think. Ideally the Move method, which started out as Destroy in the draft if I recall correctly, would take an empty string to signal deletion instead of backup.

e.g.

// moveWalletData will move all wallet files to a backup directory, or delete them if empty string.
func (w *spvWallet) moveWalletData(backupDir string) error {
	if backupDir == "" {
		return os.RemoveAll(w.netDir)
	}
	...

...for later though.

I think this log needs to print what the smallest unit is, not what I put in. I know what I put in.

Also you should return for every error I think and return nil at the end if nothing errored, not set all to err and return that at the end. This is what is normally done, and it helps future devs who may change this and expect returning on error to have happened.

This is still not returning on error. It's not wrong, just not standard.

If we want default values if values not set can we set those first and then do validate and error if that errors?

Isn't it correct to return error if values are wrong here? This happens when opening an existing wallet correct? We don't want to silently ignore what the user thinks they set, overwrite it and use some values they don't want?

nit: space missing in comment

While true, the form has values of coin/kb. The error is also like this and in scientific notation. So maybe 1e-5 coin/kb?

If you're going to assign defaults here, you'll need want to do this in Create also, no? Maybe just move it in to validateBtcWalletConf.

Rather than WalletConfig pulling double duty, I suggest having a second struct

type baseWalletConfig struct {
	fallbackFeeRate  uint64 // atoms/byte
	feeRateLimit     uint64 // atoms/byte
	redeemConfTarget uint64
	useSplitTx       bool
}

and modifying validateBtcWalletConf to be something like...

func readBaseWalletConfig(walletCfg *WalletConfig) (*baseWalletConfig, error)

Set the defaults for zeros inside. Then you can embed baseWalletConfig in baseWallet.

type baseWallet {
    baseWalletConfig
    ...
}

and delete the existing fields.

I think this condition is impossible.

Could you move this down to right above the baseWallet definition.

Could you please embed this so you don't have to add .config. everywhere. It'll actually save me a bunch of conflicts in #1686 if you do.

You're variable name is shadowing the type name. How about just baseCfg?