Upgrade v0.20.0 #33

BitcoinZavior · 2024-08-16T21:48:35Z

[0.20.0]

APIs added

Make backwards-compatible v2 to v1 sends possible.

APIs changed

Removed contribute_non_nitness_input from v1 & v2.
Allow receivers to make payjoins out of sweep transactions (#259).
Encode &ohttp= and &exp= parameters in the &pj= URL as a fragment instead of as URI params (#298)

DanGould

My big concern is why there is a uniffi feature separate from what integration tests appear to be using. If the feature is to distinguish between the uniffi bindings here and the flutter (and other language bindings?) Then I wonder why they live in this crate rather than where they're consumed. Perhaps this is the well reasoned best design and some repetition is ok. I don't know, it just smelled to me on first blush.

Other than this this looks ok to me. I can't believe how complete these bindings seem to be 🔥

DanGould · 2024-08-17T01:31:15Z

CHANGELOG.md

@@ -0,0 +1,46 @@
+## [0.18.0]


will this include an [0.20.0] at the top? or am I confusing payjoin-ffi version with payjoin crate version?

Yes, the changelog has been updated to include 0.20.0

DanGould · 2024-08-17T01:34:50Z

Cargo.toml

+payjoin = {version = "=0.20.0",  features = ["send", "receive", "base64", "v2", "io"] }
+#payjoin={ git = "https://github.com/payjoin/rust-payjoin", rev = "941a6798f52f60d72061fc0a02b5b42146321453", features = ["send", "receive", "base64", "v2", "io"]}


if we follow semver faithfully, you could put "0.20.0" here and it would automatically upgrade to patch versions. This would be helpful in the case a security patch was released. Then, you can use payjoin-ffi patch versions for your own patches separate from the underlying dependency.

Sounds good 👍

DanGould · 2024-08-17T01:36:36Z

Cargo.toml

 bitcoincore-rpc = "0.19.0"
+http = "1"
+payjoin-directory = { git = "https://github.com/payjoin/rust-payjoin", features = ["danger-local-https"] }


I suppose you deserve a release from us if you're using payjoin-directory in tests

DanGould · 2024-08-17T01:37:06Z

python/CHANGELOG.md

@@ -0,0 +1,36 @@
+## [0.18.0]
+This release updates the python library to `payjoin` version `0.18.0`.


0.20.0 has been added now 👍

DanGould · 2024-08-17T01:39:15Z

python/CHANGELOG.md

+    - `V2PayjoinProposal` exposes `process_res`, `extract_v1_req`, `extract_v2_req`, `is_output_substitution_disabled`, `owned_vouts`, `psbt` &
+      `utxos_to_be_locked`.
+#### io module
+- Exposed `fetch_ohttp_keys()` to fetch the `ohttp` keys from the specified `payjoin` directory.      


fetch_ohttp_keys includes a whole http client. I might consider releasing this io stuff as a separate payjoin-ffi feature and downstream as a separate package (e.g. payjoin and payjoin-io pypy packages) since if I were a downstream user it'd be a lot of bloat to bring in a whole foreign http and tls stack just to make a single request instead of writing it in my target language. Just an optimization for later. Maybe this comment turns into its own issue

The fetch_ohttp_keys function is particularly valuable when working with v2, especially for those who prefer not to implement it from scratch. If we proceed with moving it into payjoin-io, we may consider creating a dedicated uniffi crate for it as well and then importing it in this uniffi package.

DanGould · 2024-08-17T01:40:29Z

python/src/payjoin/libpayjoin_ffi.dylib

Are you sure you want to commit and not ignore compiled binaries (I think that's what these and the .so and .dll files are)

So far, we've only released a Python pre-release using uniffi and have been using the bindings and binaries for testing Payjoin in Python. For the stable release, we plan to ignore the compiled binaries and bindings. Instead, we'll bundle it similarly tobdk-ffiand provide instructions for users to generate the bindings themselves if they need to.

DanGould · 2024-08-17T01:42:35Z

python/src/payjoin/payjoin_ffi.py

It seems like bdk-ffi does not commit the generated bindings. Assuming that's what this is, why do you choose to commit them in this repo rather than leave instructions to generate bindings? Is this part of the release process to show the completed, released source code?

Addressed in the above comment.

DanGould · 2024-08-17T01:48:02Z

src/receive/v1.rs

-        generate_script: impl Fn() -> Result<Vec<u8>, PayjoinError>,
+        generate_script: Box<dyn GenerateScript>,


I assume making the trait is necessary to produce bindings, but I'm not sure because this change is only happening now. Do I understand correctly? Was this just a stub before?

Yes, you’ve understood correctly. The trait is essential for uniffi. Initially, the function was only implemented for Flutter. With this commit, I've separated the function to accommodate both use cases.

DanGould · 2024-08-17T01:51:17Z

.github/workflows/build.yml

@@ -18,7 +18,7 @@ jobs:
          override: true
          profile: minimal
      - name: Build on Rust ${{ matrix.toolchain }}
-        run: cargo build --color always --all-targets
+        run: cargo build --color always --all-targets --features enable-danger-local-https


If this isn't testing the code path used with the uniffi feature, what is it testing? Why have a second implementation for many functions like pj_uri_builder?

Initially, we weren’t planning to test the code in Rust itself but rather in the platform code. However, we encountered an issue with signing using v2 in Flutter recently, which prompted us to create this integration test code as part of the debugging process.

The reason for having a second implementation for functions like pj_uri_builder is that we encountered a cannot move out of Arc error when using the uniffi version of the code on the Flutter side.

DanGould · 2024-08-17T01:51:43Z

src/receive/v2.rs

    #[cfg(feature = "uniffi")]
+    #[cfg(feature = "uniffi")]


BitcoinZavior · 2024-08-18T22:49:19Z

My big concern is why there is a uniffi feature separate from what integration tests appear to be using. If the feature is to distinguish between the uniffi bindings here and the flutter (and other language bindings?) Then I wonder why they live in this crate rather than where they're consumed. Perhaps this is the well reasoned best design and some repetition is ok. I don't know, it just smelled to me on first blush.

Other than this this looks ok to me. I can't believe how complete these bindings seem to be 🔥

The separation of the uniffi feature is deliberate, addressing its unique requirements—like wrapping complex structs with Arc for shared parameters and working around the lack of direct callback support. While this approach introduces some redundancy, it isolates these limitations from the core codebase, preventing them from affecting testing or Flutter integration. I plan to further reduce code duplication in future upgrades.

BitcoinZavior · 2024-08-18T23:02:35Z

Other than this this looks ok to me. I can't believe how complete these bindings seem to be 🔥

Thanks! 🧡

Review comments v0.20.0

BitcoinZavior and others added 13 commits August 12, 2024 12:11

Bump payjoin & payjoin-ffi to 0.20.0

a625367

mapped bitcoin::address::ParseError

968e9fa

rust-toolchain.toml added; nightly-2024-07-24

00ce4a9

replaced hide_parse_errors

f008f55

updated deprecated class

4d3d429

removed contribute_non_witness_input

5978c47

code cleanup

17d85c7

testcases updated

ed76b5c

Update Cargo.toml

c26983b

Update README.md

b5657ad

Update README.md

ad97d5f

Update Cargo.toml

1d0d971

Fixing test case

0cd12ba

BitcoinZavior requested a review from StaxoLotl August 16, 2024 22:12

DanGould reviewed Aug 17, 2024

View reviewed changes

BitcoinZavior added 2 commits August 18, 2024 13:54

removed duplicate #[cfg(feature = uniffi)]

abc6eb7

version updated to 0.20.0

14121f8

BitcoinZavior and others added 5 commits August 19, 2024 21:43

code formatted

881b07d

uniffi error resolved

35f6c74

Added payjoin v0.20 bindings

2411bc3

rebase

0f72d24

Merge pull request #34 from LtbLightning/review-comments-v0.20.0

cc718d3

Review comments v0.20.0

BitcoinZavior merged commit 9e79770 into main Aug 21, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade v0.20.0 #33

Upgrade v0.20.0 #33

BitcoinZavior commented Aug 16, 2024

DanGould left a comment •

edited

Loading

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

BitcoinZavior Aug 18, 2024

BitcoinZavior Aug 18, 2024

DanGould Aug 17, 2024

BitcoinZavior commented Aug 18, 2024

BitcoinZavior commented Aug 18, 2024 •

edited

Loading

		payjoin = {version = "=0.20.0", features = ["send", "receive", "base64", "v2", "io"] }
		#payjoin={ git = "https://github.com/payjoin/rust-payjoin", rev = "941a6798f52f60d72061fc0a02b5b42146321453", features = ["send", "receive", "base64", "v2", "io"]}

		@@ -0,0 +1,36 @@
		## [0.18.0]
		This release updates the python library to `payjoin` version `0.18.0`.

		generate_script: impl Fn() -> Result<Vec<u8>, PayjoinError>,
		generate_script: Box<dyn GenerateScript>,

Upgrade v0.20.0 #33

Upgrade v0.20.0 #33

Conversation

BitcoinZavior commented Aug 16, 2024

[0.20.0]

APIs added

APIs changed

DanGould left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BitcoinZavior commented Aug 18, 2024

BitcoinZavior commented Aug 18, 2024 • edited Loading

DanGould left a comment •

edited

Loading

BitcoinZavior commented Aug 18, 2024 •

edited

Loading