meta: payjoin-cli should be a better reference implementation

[nothingmuch]

recently payjoin-cli has been getting a lot of pull requests that on their own seem reasonable for a user facing app, optimizing the user experience.

however, its intended users are developers (and inevitably agents). payjoin-cli is a reference implementation, as such it's main purpose should be to clarify to users of the library how to use the payjoin crate correctly in a real world setting.

sorry to be a bit vague and negative about this, but it really feels like efforts spent polishing up payjoin-cli to users or to make it more useful are actually detracting from its usefulness as a reference implementation, and meanwhile there are many low hanging fruit for refactoring it for clarity that are not being worked on (c.f. #478)

starting from payjoin-cli's main.rs, that file is not too obtuse but still full of feature gating related boilerplate. i guess that's unavoidable. then there's the cli/config parsing, that eventually leads to constructing of separate concrete variants of the app types, via the AppTrait interface but technically not relying on it (since the concrete types are used).

if you then go into app/v1.rs or app/v2/mod.rs, low level details interspersed with high level ideas, there's a shortage of whitespace separating chunks of code relating to a single logical idea from chunks of code that deal with something else, there's an overwhelming amount of boilerplate

just some observations from scrolling v2/mod.rs for about 2 mins:

1. AppTrait hides some details/shared code between v1 and v2, and kinda has to be read before
2. send command implementation precedes receive, even though the receiver initiates
3. before getting there there's eyesores like this:

impl<Status: StatusText> fmt::Display for SessionHistoryRow<Status> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(
            f,
            "{:<W_ID$} {:<W_ROLE$} {:<W_DONE$} {:<W_STATUS$}",
            self.session_id.to_string(),
            self.role.as_str(),
            match self.completed_at {
                None => "Not Completed".to_string(),
                Some(secs) => {
                    // TODO: human readable time
                    secs.to_string()
                }
            },
            self.error_message.as_deref().unwrap_or(self.status.status_text())
        )
    }
}

which is of course necessary on some level but make no sense without context and make finding the entry point difficult
4.

                let http = http_agent(&self.config)?;
                let body = String::from_utf8(req.body.clone()).unwrap();
                println!("Sending fallback request to {}", &req.url);
                let response = http
                    .post(req.url)
                    .header("Content-Type", req.content_type)
                    .body(body.clone())
                    .send()
                    .await
                    .with_context(|| "HTTP request failed")?;

http_agent isn't clearly a constructor, it's not clear why req.body needs to be converted to a UTF8 string without an understanding of BIP 78 and some of the details. i guess the idea is to convey that req contains all the information needed to construct a request regardless of the HTTP client used, but http_agent doesn't convey that the HTTP client is arbitrary, or that this is converting things from generic types into specific ones. comments might help with this but i also think optimizing the code for readability, like destructuring req, and either having http_agent do more lifting wrt boiilerplate, or perhaps less (to emphasize that req is agnostic to the HTTP client's request type), but as it stands it's kinda neither here nor there
5. then there's just kinda awkward things, like:

        let session =
            ReceiverBuilder::new(address, self.config.v2()?.pj_directory.as_str(), ohttp_keys)?
                .with_amount(amount)
                .with_max_fee_rate(self.config.max_fee_rate.unwrap_or(FeeRate::BROADCAST_MIN))
                .build()
                .save(&persister)?;

.with_max_fee_rate(self.config.max_fee_rate.unwrap_or(FeeRate::BROADCAST_MIN))

why don't we have a config.max_fee_rate() that handles the optionality and FeeRate::BROADCAST_MIN instead of doing that in the AppTrait impl's top level functions?

i am familiar with these apis, i know know what to expect and i still feel like can't see the forest from the trees =(

---

anyway, to make this issue a bit more constructive and not just a list of complaints, how can we improve readability of payjoin-cli, to humans (and also to agents) so that it better achieves its stated goal? how can we do this while avoiding it becoming a maintenance burden?

this should of course not shy away from dealing with the messy stuff that reality imposes, this is after all an example for how to integrate the crate into a real world app (whereas doc examples or the tests can show how to deal with small self contained things)

[codaMW]

I'd like to start with the point 5 refactor, adding a max_fee_rate() method to Config that encapsulates the unwrap_or(FeeRate::BROADCAST_MIN) default, removing that detail from the AppTrait impl. Small, scoped, and directly addresses one of the readability concerns. Does this seem like a reasonable first step?

[DanGould]

efforts spent polishing up payjoin-cli to users or to make it more useful are actually detracting from its usefulness as a reference implementation

I think this is a trade-off that has been made but I don't think it's a necessary one. We can avoid making this mistake you've identified in the future.

The reference serves not only the developers but also the designers, and feedback from real users is how we must inform which ergonomics to present to users. I aspire to make it obvious for implementers both which affordances are possible to give users via UI and most importantly which affordances should be given to users assuming your user is the same audience as bitcoin-cli. And then the developers are another layer of users with a different but equally valid set of requirements. That said, if changes is done while trading off code readability for usability by taking shortcuts with crufty code, I agree that progress isn't being made toward a better reference.

It seems that special attention paid to avoiding that false trade-off by improving, or at least not damaging, abstractions at review time would avoid the key bother you've laid out here. Adding that to my personal review checklist for payjoin-cli.

All of the suggestions sound appropriate and I'm eager for the PRs that advance them.

[nothingmuch]

The reference serves not only the developers but also the designers, and feedback from real users is how we must inform which ergonomics to present to users.

isn't that still a reference for developers, for how to also take such considerations into account? which i agree, such considerations should be in scope

It seems that special attention paid to avoiding that false trade-off by improving, or at least not damaging, abstractions at review time would avoid the key bother you've laid out here. Adding that to my personal review checklist for payjoin-cli.

also setting things up so that contributors time isn't wasted working on things that we eventually decide not to merge

for example #1473, which resulted from the TODO command re formatting the seconds in the snippet i provided above has gone through a review cycle, multiple comments from multiple people only to eventually get closed... in hindsight this makes sense, whether or not a specific number is formatted as a number of seconds or a human readable data doesn't really make a difference, it's not payjoin-cli's job to demonstrate how to format dates well at least at this time (although arguably would be in scope if we strengthened the recommendation re expiry time, c.f. #1449 #1035 and #1457)

that was the motivating example since the causal chain was a well intended TODO comment highlighting incomplete functionality which in hindsight we preferred stay incomplete sparked a well intended issue being created, discussions, and a well intended PR that in hindsight cost people time an effort. especially when it comes to new contributors, the experience of having a contribution rejected can be discouraging, so the reason i opened this issue is to start a discussion about how we can better avoid that and clarify the guidelines

a reasonable stab at a heuristic is "is this functionality specific to payjoin?" and if the answer is not then there's a good chance we are better off without it even if it makes for a nicer experience, but of course it can't go too far, e.g. cli vs. config file parsing is not really specific to payjoin but some mechanism

if we can come up with a clearer guideline and document it in CONTRIBUTING.md i think would make a bigger impact than just being more dilligent in review