-
Notifications
You must be signed in to change notification settings - Fork 39
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
Expose Sender::extract_v2 for bindings #382
Expose Sender::extract_v2 for bindings #382
Conversation
The send::Context typestate enum is not simple to bind to in UniFFI and would require abstracting distinct extract_v1 extract_v2 functions in order to cross the FFI boundary. Exposing this method is a simple fix to make such abstraction unnecessary.
payjoin/src/send/mod.rs
Outdated
Ok(rs) => self.extract_v2(ohttp_relay, rs), | ||
Ok(_rs) => { | ||
let (req, context_v2) = self.extract_v2(ohttp_relay)?; | ||
Ok((req, Context::V2(context_v2))) | ||
} |
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 wonder if it even makes sense to have this match extract_highest_version
since it still requires a match on the resulting Context
to handle both processing v1 and v2
It may be simpler to just have the downstream implementor switch on whether or not extract_v2 errors, then try extract_v1 (if it's a version error), and otherwise return an error.
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 agree that the try/catch pattern on extract_v2 seems cleaner.
7a136f0
to
0c49db3
Compare
The send::Context typestate enum is not simple to bind to in UniFFI, and the extract_highest_version function is not very useful because it still requires the caller to match on the resulting Context.
0c49db3
to
f6247ff
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 deleted the Context enum and extract_highest_version
in favor of the "try extract_v2" approach. Also refactored http posts to save vertical space.
tACK new changes up to f6247ff. I can't "Approve" my own PR |
The send::Context typestate enum is not simple to bind to in UniFFI and would require abstracting distinct extract_v1 extract_v2 functions in order to cross the FFI boundary. Exposing this method is a simple fix to make such abstraction unnecessary.