Remove mod/uni payjoin-ffi distinction#899
Conversation
Pull Request Test Coverage Report for Build 16542822689Details
💛 - Coveralls |
DanGould
force-pushed
the
no-uni-mod-distinction
branch
3 times, most recently
from
77d434e to
c16d8e2
Compare
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
concept ACK, looks like there are some missing uniffi exports causing the Python tests to fail.
DanGould
force-pushed
the
no-uni-mod-distinction
branch
from
c16d8e2 to
db58671
Compare
|
Ok it passes but I committed a bunch of formatting changes in dart by mistake. Troubleshooting that now |
They had fallen by the wayside since they're not checked in CI.
DanGould
force-pushed
the
no-uni-mod-distinction
branch
from
db58671 to
68cd128
Compare
- Note that this may break the uniffi feature-disabled code path which is intended and ok because it's being removed. - Make features enabled by payjoin_ffi/uniffi non-optional in Cargo.toml - Keep the feature, but empty, so that feature gates can be removed using controlled demolition for easier review. - Remove `not(feature = "uniffi")`-switched code. - Remove `uniffi` from python,dart generate scripts. - Remove bdk_integration_test which runs only without uniffi since the same behavior is run in each target foreign language.
Only touch files where all uniffi feature removal is trivial to enable by default.
DanGould
force-pushed
the
no-uni-mod-distinction
branch
from
68cd128 to
1b1376f
Compare
DanGould
force-pushed
the
no-uni-mod-distinction
branch
from
1b1376f to
774b541
Compare
|
Rebased in chunks to make it easier to review |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
This is huge! Thanks for the clean commit history, made it a breeze to review.
utACK 774b541
| #[cfg(feature = "uniffi")] | ||
| uniffi_bindgen() | ||
| } | ||
| fn main() { uniffi_bindgen() } |
There was a problem hiding this comment.
uniffi_bindgen() helper is no longer needed since it was only introduced to simplify the feature gate.
|
Re: removing the BDK integration test, I wonder if this bumps the priority of #741 so we can continue ensuring compatibility with BDK. Also re: https://github.com/orgs/payjoin/discussions/900 maybe consider other backends like Electrum? Maybe the better way to do this, instead of integration tests, would be to make the payjoin-cli wallet backend modular. |
|
We do have a smoke test in BDK compatibility in bull bitcoin mobile because they use BDK. I really like the idea of making payjoin-cli wallet backend modular, but I'm not sure if it can go into |
3 participants
None of our bindings depend on a non-uniffi interface any longer. Remove it
Perhaps we still want language-specific features so dart isn't alwaysa dependency but this was my first step. I'd also like to separate the bindings modules to reflect the multiple files in the
payjoinsource as well. There are definitely a lot of formatting changes I'd like to see but I avoided as many as possible so that the logic can be compared on its own for reviewers. This is big enough as-is.The bdk integration test was only testing the non-uniffi interface.
Because this is tested at the terminal bindings, this test has been
removed.
I chose to do this now because I'm overhauling the sender and FFI changes are way harder to review in two layers than one.