PSBT interface mockup #457
Conversation
src/util/psbt/responsibilities.rs
Outdated
| #[derive(Debug)] | ||
| pub enum PsbtSignError<E: Debug> { | ||
| /// The PSBT is not valid for signig according to BIP-174 | ||
| ValidationError, |
There was a problem hiding this comment.
This should be more fine-grained.
src/util/psbt/responsibilities.rs
Outdated
| fn create_transaction( | ||
| &mut self, | ||
| outputs: &[TxOut], | ||
| change_output: Option<Script>, |
There was a problem hiding this comment.
Maybe we want to have a different trait that allows this particular privacy optimization to avoid panics/useless error conditions.
sgeisler
force-pushed
the
2020-08-psbt-interfaces
branch
from
2d9a42b
to
a7f3149
Compare
There was a problem hiding this comment.
Not sure I understand core API differences with the other proposal (#456); and in parts where I see API diffs I left some questions
sgeisler
force-pushed
the
2020-08-psbt-interfaces
branch
from
a7f3149
to
bde457a
Compare
|
@dr-orlovsky The main difference to #456 is that here the objects implementing responsibilities never hold the PSBT, but only the state they need to repeatedly perform their functionality and are thus reusable. As noted in the discussion in #456 enforcing a certain PSBT workflow seems futile, so there is no wrapping of PSBTs going on here. The notion of an abstract |
|
Oh, I see. My only objection that this leads, as a direct consequence, to the ability to change PSBT outside of normal flow, so we do not have a state machine of responsibilities chaining each other |
|
Such a state machine sounds nice in theory, but in practice we can't enforce that much without an overly complex representation of the PSBT in the type system. Looking at the example workflows in the BIP I'm pretty sure that PSBT is too general for this to be useful. |
sgeisler
force-pushed
the
2020-08-psbt-interfaces
branch
3 times, most recently
from
cecea5d
to
649a166
Compare
sgeisler
force-pushed
the
2020-08-psbt-interfaces
branch
from
649a166
to
22d2730
Compare
|
I did an alternative implementation, pls see #456 |
There was a problem hiding this comment.
I don't really understand what the goal is of these kind of traits. Is it just a way to provide some basic signing code that a wallet can like "plug" into? I mean it seems to me that these traits would for almost all applications stay within the application, adding little value by making the traits "common".
The code that generalizes signing over a xpriv is useful though. But it seems to be a hassle to create all this framework just to provide that code?
I'd be curious though to experiment with this in rust-wallet!
| fn create_transaction( | ||
| &mut self, | ||
| outputs: &[TxOut], | ||
| ) -> Result<PartiallySignedTransaction, PsbtCreationError<Self::Error>>; |
There was a problem hiding this comment.
Is this method supposed to create a transaction without inputs or is the implementor responsible for adding inputs to fulfill the tx?
There was a problem hiding this comment.
Yes, it's the implementor's responsibility to select UTXOs, that's an integral part of every wallet imo: tracking and selecting UTXOs.
| #[derive(Debug)] | ||
| pub enum PsbtCreationError<E: Debug> { | ||
| /// The wallet doens't control a sufficient amount of Bitcoins to fund the transaction | ||
| InsufficientFunds, |
There was a problem hiding this comment.
This probably answers my earlier question.
| fn sign_psbt( | ||
| &mut self, | ||
| psbt: PartiallySignedTransaction, | ||
| ) -> Result<PartiallySignedTransaction, PsbtSignError<Self::Error>>; |
There was a problem hiding this comment.
Is it on purpose that this clones the entire PSBT instead of having a mutable reference? Perhaps 2 variants of this method could be provided? One could be provided by using the other.
There was a problem hiding this comment.
It doesn't clone. It takes it by value, mutates it and either returns the altered version or an error. The problem with mut references is that it's much too easy to leave the PSBT in an invalid state in case of an early exit due to an error.
| let key = match self.derive_priv(&ctx, &path) { | ||
| Ok(key) => key, | ||
| Err(crate::util::bip32::Error::Ecdsa(e)) => return Err(e), | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
Oh, yeah, I mostly hacked that together to test the API.
|
I'll leave some comments, but I consider this whole endeavor to be futile. |
|
Agree, closing my version as well |
Alternative idea for #455
Currently it covers the following functionalities:
PsbtWallet)PsbtWallet)SignPsbt)PartiallySignedTransaction::merge)I wouldn't consider merging any of the PSBT interface PRs before every trait was implemented at least once and a working wallet could be built using these. But I consider this a good basis for discussion.