-
Notifications
You must be signed in to change notification settings - Fork 76
Add receiver builder #1002
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
Add receiver builder #1002
Conversation
Pull Request Test Coverage Report for Build 17304182660Details
💛 - Coveralls |
4675726 to
48fa534
Compare
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 dig the first commit (which passes tests)
The second one I think is more complicated than having a simple Uninitialized state (+26 -9). The empty Uninitialized state seems good enough for sender and I'd rather leave it for the receiver too. ReceiverBuilder design LGTM. I just ask that it's build is chained where possible and not assigned unless it needs to be in one case.
payjoin-cli/src/app/v2/mod.rs
Outdated
| let receiver_builder = | ||
| ReceiverBuilder::new(address, self.config.v2()?.pj_directory.clone(), ohttp_keys)? | ||
| .with_amount(amount); | ||
| let persister = ReceiverPersister::new(self.db.clone())?; | ||
| let session = Receiver::create_session( | ||
| address, | ||
| self.config.v2()?.pj_directory.clone(), | ||
| ohttp_keys, | ||
| None, | ||
| Some(amount), | ||
| )? | ||
| .save(&persister)?; | ||
| let session = receiver_builder.build().save(&persister)?; | ||
|
|
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 think let session = ReceiverBuilder::new() ..build().save(&persister)?; All chained is the way to go
|
If one of the states is to go, I think it's the |
|
The way to get rid of that rust-payjoin/payjoin/src/core/send/v2/mod.rs Lines 156 to 178 in bffb9b5
|
|
If you have a strong preference to only build the transition, then we should be changing the SenderBuilder to adopt the same pattern as the receiver once this goes in. |
48fa534 to
3062ce3
Compare
3062ce3 to
7d01870
Compare
The first typestate has always been an odd case. Stricly speaking you cannot resume from the uninitialized state. The earliest state you can resume from is `Initialized`. So this commit removes UnInitialized as a "state" and replaces it with a builder. Additionally a builder model allows the application to access the short id after the builder is created. The application may use session id as the key for the perister which is needed when you build().
7d01870 to
abfc6d3
Compare
DanGould
left a comment
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.
utACK abfc6d3
This makes sense and mirrors SenderBuilder.
let's see about removing the ReceiveSession::Uninitialized (now with no contents) in a follow up along with SendSession to see if congruence makes sense.
The initial typestate has always been an odd case. Stricly speaking you shouldn't be able to resume to an uninitialized state. The earliest state you can resume from is
Initialized. So this commit removes UnInitialized as a "state" and replaces create_session with a builder model.Note that
Uninitializedis still a variant on theRecevierSessionwhich represents a state before any events have been applied. I think there is value to removing theUninitializedvariant as applications have to still match on it when resuming. Instead we could have pub(crate)'dstructmethod serving the same purpose. Where the privatestructmethod can only process the first event and return theRecevierSessionsum type.Cherry picked off #995
Pull Request Checklist
Please confirm the following before requesting review: