-
Notifications
You must be signed in to change notification settings - Fork 37
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
Complete receive payjoin feature groundwork #18
Conversation
Before that, ensure the checks happen.
04dec82
to
e48402b
Compare
@@ -38,14 +38,14 @@ type InternalResult<T> = Result<T, InternalValidationError>; | |||
/// Builder for sender-side payjoin parameters | |||
/// | |||
/// These parameters define how client wants to handle PayJoin. | |||
pub struct Params { | |||
pub struct Configuration { | |||
disable_output_substitution: bool, | |||
fee_contribution: Option<(bitcoin::Amount, Option<usize>)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any value putting params here as a field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this configuration is going to hold this data then yes, I think it makes sense to be part of this. The idea of a builder to expose this interface in an accessible way rather than expose optional_parameters::Params
as pub makes sense to me.
I'm starting to think of this Configuration
struct more as a sender::RequestBuilder
that can turn into a (Request, Context)
when you call from_psbt_and_uri
. Maybe the signature for that method should be
impl RequestBuilder {
fn create_pj_request(psbt: Psbt, pj_url: Uri<PayJoin>) -> Result<(Request, Context)>;
}
instead of on the manually-validated Uri<PayJoinParams>
how it is now: pj_url.create_pj_request(psbt, pj_params: Configuration) -> Result<(Request, Context)>
.
Then create_pj_request could validate the endpoint check_pj_supported()
during that call instead of requiring it be done by hand before the call, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The builder could have .psbt()
, pj_uri()
, .build()
methods in addition to the existing ones like .non_incentivizing()
to more closely resemble a builder pattern, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, builder pattern kinda makes sense. We might need more than one to implement type state, since both URI and PSBT are mandatory. Alternatively we could just accept both in a single method.
The absolute minimum requirements to use this branch payjoin crate release as a dependency for receiving a PayJoin (in integration tests, nolooking, and hopefully a bitcoind payjoin-server). It's still missing output substitution and coin selection logic, but that can be scripted as in https://github.com/kixunil/loptos for a experiments before it's part of the library.
This is largely a redeux of the "Parse sender params" change reviewed in both Kixunil#30 and Kixunil#27 by @Kixunil and @RCasatta
Notable Design Decisions
psbt()
once the safety checks are complete.optional_parameters
is only used in thereceiver
feature so it's put in that folder. I still think ultimatelyoptional_parameters::Params
should be the type for validation andsender::Configuration
should depend on it. They are the Sender's optional parameters after all, the receiver just needs to parse them.log
was optional, we depend on it here.Proposal
trait &impl_proposal!
macro on intermediate type state to expose commonoutput_substitution_disabled
parametertagging @cryptoquick also. in case you're in getting a receiver into bitmask or at least distributing software bitmask can send PayJoins to :)