Send and Receive #2
Conversation
…and synchronization
|
Please review @StaxoLotl @DanGould |
|
Coming along well, will take a look again when the first bindings are generated |
There was a problem hiding this comment.
Absolutely incredible work that passes tests on my machine. This review helped me find a few issues upstream to fix too. The rust-payjoin master branch has a slightly different receive flow around fees and payload preparation I'll have to package into a release which should reduce some of the binding surface area too.
My main concern is around passing function pointers, which I thought in a conversation with @thunderbiscuit we concluded was possible. It would be much better to do that, as in the existing implementation tests, rather than require downstream trait implementations.
Passing function pointers would allow us to keep the typestate engine and get rid of the uninitialized / Mutex struct problem.
Truly excellent to see this effort moving forward!
| //TODO; RECREATE ADDRESS STRUCTURE | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct Address<T> | ||
| where | ||
| T: bitcoin::address::NetworkValidation, | ||
| { | ||
| pub internal: _BitcoinAdrress<T>, | ||
| } |
There was a problem hiding this comment.
Is this still TODO?
I gather that rust-bitcoin is trying to remove the NetworkValidation complexity from Address, so this isn't supercritical to get perfect on the first pass. It can stabilize with rust-bitcoin 1.0 rust-bitcoin/rust-bitcoin#1819
There was a problem hiding this comment.
This should be removed, I will remove it in a future commit.
| /// | ||
| /// Call this after checking downstream. | ||
| pub fn check_can_broadcast( | ||
| self, can_broadcast: Box<dyn CanBroadcast>, |
There was a problem hiding this comment.
why is CanBroadcast a trait rather than a function that returns a bool like the others? Also, I understand that bitcoin core rpc expects a Vec, but I think this function really only requires a single tx_hex: String as a generic.
There was a problem hiding this comment.
Because using CanBroadcast trait is the recommended way to use callbacks when we use uniffi for generating code (But can look into using function callback instead)
Also, I understand that bitcoin core rpc expects a Vec, but I think this function really only
requires a single tx_hex: String as a generic.
I got inspiration from the test code integration.rs
let proposal = proposal
.check_can_broadcast(|tx| {
Ok(receiver
.test_mempool_accept(&[bitcoin::consensus::encode::serialize_hex(&tx)])
.unwrap()
.first()
.unwrap()
.allowed)
})
.expect("Payjoin proposal should be broadcastable");`
But I think I can change this to a Vec.
| pub fn get_transaction_to_schedule_broadcast(&self) -> Transaction { | ||
| let res = self.internal.get_transaction_to_schedule_broadcast(); |
There was a problem hiding this comment.
get_ prefix is not typically used in rust code. I guess this needs to be changed upstream!
https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
|
|
||
| /// A mutable checked proposal that the receiver may contribute inputs to to make a payjoin. | ||
| pub struct PayjoinProposal { | ||
| pub internal: Mutex<Option<PdkPayjoinProposal>>, |
There was a problem hiding this comment.
Why is a Mutex introduced here? Seems like that adds additional complexity? There should be no situation where this struct exists but the proposal is not initiated since it has no constructor and can only be created from calling the prior type's check.
There was a problem hiding this comment.
There are a few reasons to use a mutexGaurd
PayjoinProposal.
This is the PayjoinProposal struct without mutex_gaurd,
pub struct PayjoinProposal {
internal: PdkPayjoinProposal,
}
impl PayjoinProposal {
pub fn contribute_witness_input(&mut self, txout: TxOut, outpoint: OutPoint) {
// let mut guard = self.get_proposal_mutex_guard();
self.internal.contribute_witness_input(txout.into(), outpoint.into());
// [guard.as](http://guard.as/)_mut().unwrap().contribute_witness_input(txout.into(), outpoint.into());
}
}
1, In the above function we have to use `&mut self` in the parameter,
but unfortunetly, `uniffi` , doesn't allow that.
2, prepare psbt takes a `mut self`, that will be a problem as well,
since uniffi requires a `&self` in the function for properly generating
binding code.
In short mutuxes are used to ensure safe and synchronised access to shared resources in multi-threaded or multi-process programs.
But good to keep checking, I am sure there are instance where we can do without a mutex.
FYI I started off with tons more mutex' and removed after some analysis. If you come across any which you are sure dont need ro be a mutex please point it out.
There was a problem hiding this comment.
Could the mutex be removed and this simplified by passing an owned self instead of a mutable reference?
There was a problem hiding this comment.
Yes, if we dont want to use mutex here we can remove it. probably worth looking if there are other instances where mutex is not required. I will updated in a future commit in the python branch.
| pub(crate) fn get_proposal(&self) -> MutexGuard<Option<PdkPayjoinProposal>> { | ||
| self.internal.lock().expect("PayjoinProposal") | ||
| } |
There was a problem hiding this comment.
Seems like this method could not exist if there were not Mutex
There was a problem hiding this comment.
Yes you are right, but this is more of a helper function.
| pub(crate) struct Configuration { | ||
| pub(crate) internal: PdkConfiguration, | ||
| pub struct Configuration { | ||
| pub internal: Mutex<Option<PdkConfiguration>>, |
There was a problem hiding this comment.
Again, why is a Mutex necessary? When would this struct exist where internal = None?
There was a problem hiding this comment.
Same as before, yes we can remove it if we feel mutex is unnecessary here.
| struct MockScriptOwned {} | ||
| struct MockOutputOwned {} | ||
| impl IsOutoutKnown for MockOutputOwned { | ||
| fn is_known(&self, _: OutPoint) -> Result<bool, anyhow::Error> { | ||
| Ok(false) | ||
| } | ||
| } | ||
| impl IsScriptOwned for MockScriptOwned { | ||
| fn is_owned(&self, script: ScriptBuf) -> Result<bool, anyhow::Error> { | ||
| { | ||
| let network = Network::Bitcoin; | ||
| Ok(Address::from_script(script, network.clone()).unwrap() | ||
| == Address::from_str("3CZZi7aWFugaCdUCS15dgrUUViupmB8bVM") | ||
| .unwrap() | ||
| .assume_checked() | ||
| .unwrap()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This smells like an antipattern. We should be able to pass functions pointers instead of implementing traits, no?
There was a problem hiding this comment.
We won't be able to pass functions pointers directly in payjoin_ffi.udl file because of uniffi. Using traits is the option we have at the moment. Might be more options to deal with this in the future.
Passing callbacks is defenitly possible in rust, but is not recomened by uniffi, the recomended Uniffi approach is to use Traits. Ref: https://mozilla.github.io/uniffi-rs/udl/callback_interfaces.html? highlight=callback#using-callback-interfaces |
|
I am merging this branch, I have opened issues for the un addressed comments. |
Fixes #4 and Fixes #5