cli: reconsider version flag semantics

[vadim-anfv]

as a sender or receiver if i use --bip78=false, to me that would imply also not payjoinining with bip 78 peers, but the implemented behavior is to choose the default version. renaming these to prefer-bip78 etc might be more descriptive, but perhaps the entire meaning of these flags should be reconsidered given the recent shift towards discouraging bip 78 more

Originally posted by @nothingmuch in #1530

Current state

• --bip78 (and --bip77) is a marker flag - true if passed, no value accepted:

  $ payjoin-cli --bip78=false receive 50000
  error: unexpected value 'false' for '--bip78' found; no more were expected
  

• Both flags coexist but are mutually exclusive in practice, enforced by a manual check in Config::determine_version:

  $ payjoin-cli --bip77 --bip78 receive 50000
  Error: Multiple version flags specified. Please use only one of: --bip77, --bip78
  

Proposal

Given the recent shift towards discouraging bip 78, we can simplify the flags by dropping --bip77:

• default to v2
• switch to v1 only when --bip78 is passed

Like:

payjoin-cli receive 50000           # default: v2
payjoin-cli --bip78 receive 50000   # legacy: v1

Scope: changes only in the CLI args and Config::new.

Happy to PR once I get a concept ACK.

[vadim-anfv]

Since this issue already touches Config::new, I noticed we can also do a small refactor of the Config builder methods here.

Currently these are free functions:

        let mut config = config::Config::builder();
        config = add_bitcoind_defaults(config, cli)?;
        config = add_common_defaults(config, cli)?;

        let version = Self::determine_version(cli)?;

        match version {
            Version::One => {
                ...
                config = add_v1_defaults(config, cli)?;
            }
            Version::Two => {
                ...
                config = add_v2_defaults(config, cli)?;
            }
        }

I'd like to turn them into a fluent chain:

        let version = Self::determine_version(cli)?;

        let mut config = config::Config::builder()
            .add_bitcoind_defaults(cli)?
            .add_common_defaults(cli)?
            .add_version_defaults(cli, version)?; // new method extracted from Config::new

Moving the functions into trait methods:

// Builder is an external type, so we need an extension trait to add methods to it
trait BuilderExt: Sized {
    fn add_bitcoind_defaults(self, cli: &Cli) -> Result<Self, ConfigError>;
    fn add_common_defaults(self, cli: &Cli) -> Result<Self, ConfigError>;
    fn add_version_defaults(self, cli: &Cli, version: Version) -> Result<Self, ConfigError>;
}

impl BuilderExt for Builder {
    fn add_bitcoind_defaults(self, cli: &Cli) -> Result<Self, ConfigError> { ... }
    fn add_common_defaults(self, cli: &Cli) -> Result<Self, ConfigError> { ... }
    fn add_version_defaults(self, cli: &Cli, version: Version) -> Result<Self, ConfigError> { ... }
}

No UI/semantics changes, just refactoring.
I'll add it as a separate commit in the PR for this issue.

[DanGould]

to decide after payjoin-cli 1.0, punting for the time being because I don't think the need to switch optional backwards compatibility is strictly necessary to prove the reference is correct