Replace Configuration with ergonomic RequestBuilder
#106
Conversation
DanGould
force-pushed
the
req-builder
branch
3 times, most recently
from
d4f3825 to
c1b048b
Compare
|
UriExt::check_pj_supported may be able to become pub(crate) |
| /// 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.
This function is used only by the sender?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
RequestBuilderpatternUriand static request builder functionsClose #19
This change helps #101 by allowing
Contextto become a version agnostic trait.More deletes than additions!