Skip to content
This repository was archived by the owner on Jun 4, 2025. It is now read-only.

Conversation

@DanGould
Copy link

@DanGould DanGould commented Nov 1, 2024

  • 1st commit simplifies lib.rs with pub use and wildcard exports and builds with flags properly
  • 2nd commit matches feature name danger-local-https
  • 3rd commit creates new modules to more closely match the rust-payjoin source it wraps
  • 4th commit binds to 0.21 prerelease including passing integration tests

PYTHON INTEGRATION IS NOW WRONG! V1-to-V1 tests ARE NOW WRONG / USELESS, and were removed. these must be FIXED: payjoin/rust-payjoin#736. I have deleted the v1-to-v1 tests but they may be revived to satisfy v2.

Python is being ignored since our immediate goal is to get payjoin-flutter working

@DanGould DanGould changed the title Simplify Update to payjoin-0.21 Nov 1, 2024
@DanGould DanGould requested a review from spacebear21 November 1, 2024 00:23
@DanGould DanGould force-pushed the simplify branch 2 times, most recently from 98cba40 to 024b334 Compare November 1, 2024 17:04
Copy link

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about FFI but this looks fairly straightforward.

Out of curiosity, is it possible to use bitcoin-ffi for the rust-bitcoin bindings instead of making our own?

@DanGould
Copy link
Author

DanGould commented Nov 1, 2024

Yes it is possible to depend on bitcoin-ffi types (TxIn, Txout, OutPoint, Network) and upstream our PsbtInput type to that dependency. I'm hoping they release, but we can depend on a git commit from that repo.

edit: see #3

Add compilation flags so tests don't cause build problems.
Simplify lib.rs with `pub use ...::*` wildcards.
Add bitcoin, ohttp, request modules. Remove types module.
README.md Outdated

# Run the integration test
cargo test --package payjoin_ffi --test bitcoin_core_integration_test v1_to_v1_full_cycle
cargo test --package payjoin_ffi --test bitcoin_core_integration v1_to_v1_full_cycle

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was deleted afaict

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed by removing much of this section of the readme since we spin up using bitcoind crate now

src/send/uni.rs Outdated

#[uniffi::export]
impl SenderBuilder {
//TODO: Replicate all functions like this & remove duplicate code

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create a follow-up issue for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoot I thought I removed this. The TODO item is actually completed and I think was copied over from the original to make separate ffi/uniffi fns

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Remove support for v1 send,receive code. Separate uniffi exports where
wrappers are required in order to support payjoin-flutter and other
non-uniffi bindings. Use procedural macros to define uniffi bindings.

Irrelevant v1-to-v1 tests were removed.
Copy link

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c96d28c


# Run the integration test
cargo test --package payjoin_ffi --test bitcoin_core_integration_test v1_to_v1_full_cycle
cargo test --package payjoin_ffi --test bdk_integration_test v1_to_v1_full_cycle

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i missed this one in my earlier review but this test is also obsolete.

@DanGould DanGould merged commit 385a7ce into payjoin:main Nov 13, 2024
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test payjoin_ffi backwards compatibility with v1

2 participants