-
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
Replace Configuration
with ergonomic RequestBuilder
#106
Conversation
d4f3825
to
c1b048b
Compare
UriExt::check_pj_supported may be able to become pub(crate) |
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.
Looks good! thanks for working on this.
/// An HTTP client will own the Request data while Context sticks around so | ||
/// a `(Request, Context)` tuple is returned from `RequestBuilder::build()` | ||
/// to keep them separated. | ||
pub fn from_psbt_and_uri( |
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.
This function is used only by the sender?
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.
yes it's only available in the send
mod.
.map_err(InternalCreateRequestError::Url)?; | ||
let body = serialize_psbt(&psbt); | ||
Ok(( | ||
Request { url, body }, |
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.
can we return the url as part of the context and then leave it to the receiver to construct the Request
as they already have the PSBT
?
that will allow us to drop the request from the return here I guess..
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 point of this interface is to create the request on the behalf of the client in such a way that they make as few decisions as possible and have as few opportunities for errors as possible. Request
is thus a complete, serialized HTTP request that gets consumed by the http client, while the Context
sticks around and is only the data required to handle the response.
Because Request memory gets moved we want these to be distinct structs as detailed in this comment
pub fn recommended( | ||
psbt: &Psbt, | ||
payout_scripts: impl IntoIterator<Item = ScriptBuf>, | ||
pub fn build_recommended( |
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.
we can implement the build_x
in a nicer way using enums and then have two build functions when one is pub with enum mapping and one private
enum BuildMethods {
recommended: {
...
}
additionalfees {
...
}
}
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.
I'm not sure I follow. How does this simplify the interface from a downstream perspective? I'm not sure I'm familiar with this pattern.
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.
ah am seeing now that non_incen..
and addtional_fee
are called from recommended
, so that wont work.
though about this
9 enum BuildConfiguration {
8 Recommended(FeeRate),
7 AdditionalFee {
6 max_fee_contribution: bitcoin::Amount,
5 change_index: Option<usize>,
4 min_fee_rate: FeeRate,
3 clamp_fee_contribution: bool,
2 },
1 NonIncentivizing,
117 }
and then we can match
to specific items in enum and call build()
in each different case with different inputs. anyway, u can ignore this :D
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.
glad you're thinking through the design. thanks for the review
pub fn min_fee_rate(mut self, fee_rate: FeeRate) -> Self { | ||
self.min_fee_rate = fee_rate; | ||
self | ||
fn build(self) -> Result<(Request, Context), CreateRequestError> { |
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.
my biggest concern with this is whether or not to mutate self in place or just return what we've got. I think this is OK as is
There is no need for a static function when we build in the builder instead of inside crate::uri.
Use the type system to ensure a built Request has valid optional parameters.
RequestBuilder
patternUri
and static request builder functionsClose #19
This change helps #101 by allowing
Context
to become a version agnostic trait.More deletes than additions!