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

payjoin-cli FeeRate arguments have differing types #452

Open
DanGould opened this issue Jan 2, 2025 · 2 comments
Open

payjoin-cli FeeRate arguments have differing types #452

DanGould opened this issue Jan 2, 2025 · 2 comments

Comments

@DanGould
Copy link
Contributor

DanGould commented Jan 2, 2025

max_fee_rate takes a u64 while --fee-rate takes an f32

I do think an f32 is natural for this type for granular.

FeeRate arguments need attention in general in this crate. Some arguments are named sat_per_vb when they take a FeeRate type and don't need to specify that, for example.

And in payjoin-cli the feerate type should really be parsed from the argument directly rather than having business logic do it inside send/receive functionality.

@nothingmuch
Copy link
Collaborator

Not sure if this is what you meant in the last paragraph, but IMO only main.rs should deal with anything that isn't FeeRate, be it float or int, sat/vb or sat/kwu, whatever...

DanGould added a commit that referenced this issue Jan 2, 2025
Expose descriptive, accurate send errors so that we can switch on
them in implementations and mark payjoin sessions in persistent
storage accurately. We only want to spawn sessions that are capable
of making forward progress and drop those that cannot.

Patch overview:
1. housekeeping
2. Introduce specific SenderBuilder errors
3. make CreateRequestError v2 only since v1 can only make have sender
builder errors
4. separate the modules to reduce feature flags

This separation is incomplete because I'm still uncertain what to do
with send::ValidationError's feature gated variants. Do I make a
send::ValidationError and a send::v2::ValidationError separate? How do
those get handled in ResponseError? Does ResponseError get split into
two versions, or do I just leave a feature gated variant? That's left
for the next PR predicated on this design being an appropriate one.

Note this pays back some tech debt but leaves some slop in payjoin-cli.
Rather than making this PR 10 commits to review I left combining FeeRate
parsing to a later PR (#452).

This error puts us on the path to #392 and #403
@DanGould
Copy link
Contributor Author

DanGould commented Jan 2, 2025

Yeah that's the vibe of what I was saying. We should use clap's Arg::value_parser to convert the string argument directly into the strongest appropriate type before passing to App functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

2 participants