Import payjoin-ffi #701

spacebear21 · 2025-05-16T20:42:13Z

This PR imports payjoin-ffi, more-or-less as-is, from https://github.com/LtbLightning/payjoin-ffi.

The accompanying commits fix various compilation and CI issues.

Some remaining issues to consider:

Do we keep the MIT/APACHE dual-license in payjoin-ffi or do we have permission to bring it in line with the rest of rust-payjoin (MIT only).
Do we need to support MSRV 1.63 for payjoin-ffi?
The python CI is still very slow and would now impact our main development pipeline. We should figure out how to speed this up or maybe just run it on a weekly basis instead of every PR?

coveralls · 2025-05-16T20:50:52Z

Pull Request Test Coverage Report for Build 15140100106

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 82.275%

Totals
Change from base Build 15123468832:	0.0%
Covered Lines:	5561
Relevant Lines:	6759

💛 - Coveralls

DanGould · 2025-05-19T15:53:50Z

Do we keep the MIT/APACHE dual-license in payjoin-ffi or do we have permission to bring it in line with the rest of rust-payjoin (MIT only).

Yes, leave it as it is by bringing those licenses inside payjoin-ffi folder

Do we need to support MSRV 1.63 for payjoin-ffi?

No I really do not think so but that will depend on the downstream target's msrv. Certainly not for Bull Bitcoin nor for Cake nor for Boltz. Perhaps @kirilzh has an idea which MSRV would be required for bindings in his target environment he could share.

The python CI is still very slow and would now impact our main development pipeline. We should figure out how to speed this up or maybe just run it on a weekly basis instead of every PR?

Perhaps we could only run it if there were changes to payjoin-ffi files, python, or when proposing releases? It seems like our longest rust-payjoin actions are on the order of ~5 minutes for a baseline. Another thing we could do Is use merge queue and only run the python stuff once ack+merged, and then just fail a merge if Ci fails at that point.

DanGould · 2025-05-19T16:09:22Z

In order to merge this so we can work to get it to align with the current tip, we could also merge it without adding it to the root workspace, allowing it to live in its own independent workspace and then follow up with the fixes you propose here.

arminsabouri · 2025-05-19T16:45:45Z

The python CI is still very slow and would now impact our main development pipeline. We should figure out how to speed this up or maybe just run it on a weekly basis instead of every PR?

IIRC believe we do some duplicate builds in our generate scripts. For reference this is the BDK ffi generate scripts.

DanGould · 2025-05-19T17:52:47Z

Yes I took a stab at the speedup but struggled here: LtbLightning/payjoin-ffi#89

spacebear21 · 2025-05-19T20:55:53Z

I've made the following changes since the first round of review:

Removed payjoin-ffi from the workspace and explicitly exclude it from the workspace
- The exclusion prevents "error: current package believes it's in a workspace when it's not" when attempting to compile payjoin-ffi.
Undid updates to primary workspace's Cargo dependencies and lockfiles
Skip payjoin-ffi tests if rustc 1.63 version is detected
Only run python CI workflow if python files were touched
Remove Receiver::id in payjoin-ffi since it was just removed in Expose shortid method for SessionContext #698

DanGould

Concept ACK 781730b. I tested with test_local.sh but have some questions.

I was surprised that the payjoin-ffi tests didn't take a whole lot longer. One concern reviewing was that CI would fail on intermediate commits. You seem to have ordered things so that this would not have happened. ⭐️

I'd like to duplicate 5dfd484 and mark which commit from LtbLightning this came from for continuity. Can you recall how
you produced the single commit so that a reviewer such as myself might try to reproduce a 0-diff commit with that one?

Same for 362dada

This is quite a few changes so I feel a responsibility to do further diligence before smashing that merge button

Brought over as-is from https://github.com/LtbLightning/payjoin-ffi at commit d3a2b6f25970e217ab540f701e303746aec279a0. Copied the entire repository into the `payjoin-ffi` directory and removed the `.git` directory. The following author list can be obtained with by running `git shortlog -se` at that commit. Co-authored-by: Armin Sabouri <armins88@gmail.com> Co-authored-by: benalleng <benalleng@gmail.com> Co-authored-by: Bitcoin Zavior <93057399+BitcoinZavior@users.noreply.github.com> Co-authored-by: BitcoinZavior <BitcoinZavior@GMail.Com> Co-authored-by: Brandon Lucas <thebrandonlucas@gmail.com> Co-authored-by: Dan Gould <d@ngould.dev> Co-authored-by: DanGould <d@ngould.dev> Co-authored-by: Kirill Zhukov <zhukov@block.xyz> Co-authored-by: kumulynja <b@kumuly.dev> Co-authored-by: River Kanies <river.kanies@gmail.com> Co-authored-by: spacebear <144076611+spacebear21@users.noreply.github.com> Co-authored-by: spacebear <git@spacebear.dev> Co-authored-by: StaxOLotl <BitcoinStaxOLotl@Proton.Me> Co-authored-by: user <benalleng@gmail.com>

benalleng · 2025-05-20T14:13:00Z

I was surprised that the payjoin-ffi tests didn't take a whole lot longer. One concern reviewing was that CI would fail on intermediate commits. You seem to have ordered things so that this would not have happened.

Hopefully I can tamp out the complexity for the macos build as we still are setting up redis and docker independently there. As well hopefully the caching can bring down our build times even more in the future.

`payjoin::ImplementationError`. Update payjoin-ffi's error wrapper to reflect that move.

The ohttp_relay parameter was changed from a Url to an IntoUrl trait in payjoin#692. Update the payjoin-ffi wrappers to reflect this.

Codespell CI action found these typos.

Enforce the same formatting rules as the rest of rust-payjoin. This was done by removing the custom `rust-toolchain.toml` and `rustfmt.toml` then applying the formatting with `cargo fmt`.

Run rust test and lint checks in the main rust workflow. The python checks are added as a separate workflow, which only runs if python files were touched.

It was removed from rust-payjoin in payjoin#698.

spacebear21 · 2025-05-20T14:23:14Z

I'd like to duplicate 5dfd484 and mark which commit from LtbLightning this came from for continuity. Can you recall how
you produced the single commit so that a reviewer such as myself might try to reproduce a 0-diff commit with that one?

Same for 362dada

The latest push just updates those two commit messages to explain how they were produced.

spacebear21 · 2025-05-20T14:31:02Z

Hopefully I can tamp out the complexity for the macos build as we still are setting up redis and docker independently there. As well hopefully the caching can bring down our build times even more in the future.

Ah I was wondering about that, the other python build/test jobs complete in 7m which isn't too bad but the macOS one takes close to 20min.

DanGould · 2025-05-20T14:39:10Z

If I remember github CI only gives you a crappy mac machine for free. If it's a productivity boost from a macXL runner (or so I think it's called) we can pay for that

DanGould

ACK 781730b

Built and diffed both of the previously mentioned commits, bringing over ffi and formatting, on top of their parents and found no differences between what I produced by following instructions and the commits in this branch.

Huge moment!

spacebear21 force-pushed the import-payjoin-ffi branch from 48e57a6 to 4df9057 Compare May 19, 2025 17:44

spacebear21 force-pushed the import-payjoin-ffi branch 2 times, most recently from 10165e1 to 781730b Compare May 19, 2025 20:23

spacebear21 marked this pull request as ready for review May 19, 2025 20:50

spacebear21 requested a review from DanGould May 19, 2025 20:56

DanGould reviewed May 20, 2025

View reviewed changes

spacebear21 added 7 commits May 20, 2025 10:20

Exclude payjoin-ffi from workspace

a22fb75

Move ImplementationError to core in payjoin-ffi

49f6670

`payjoin::ImplementationError`. Update payjoin-ffi's error wrapper to reflect that move.

Update extract_v2 signature in payjoin-ffi

a071022

The ohttp_relay parameter was changed from a Url to an IntoUrl trait in payjoin#692. Update the payjoin-ffi wrappers to reflect this.

Fix typos

e9b3a40

Codespell CI action found these typos.

Format payjoin-ffi

d9aa818

Enforce the same formatting rules as the rest of rust-payjoin. This was done by removing the custom `rust-toolchain.toml` and `rustfmt.toml` then applying the formatting with `cargo fmt`.

Run payjoin-ffi workflows in CI

5c672b1

Run rust test and lint checks in the main rust workflow. The python checks are added as a separate workflow, which only runs if python files were touched.

Remove Receiver::id in payjoin-ffi

67a174d

It was removed from rust-payjoin in payjoin#698.

spacebear21 force-pushed the import-payjoin-ffi branch from 781730b to 67a174d Compare May 20, 2025 14:21

DanGould approved these changes May 20, 2025

View reviewed changes

spacebear21 merged commit b35a3cd into payjoin:master May 20, 2025
13 checks passed

Import payjoin-ffi #701

Import payjoin-ffi #701

Uh oh!

Conversation

spacebear21 commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15140100106

Details

💛 - Coveralls

Uh oh!

DanGould commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented May 19, 2025

Uh oh!

arminsabouri commented May 19, 2025

Uh oh!

DanGould commented May 19, 2025

Uh oh!

spacebear21 commented May 19, 2025

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented May 20, 2025

Uh oh!

spacebear21 commented May 20, 2025

Uh oh!

spacebear21 commented May 20, 2025

Uh oh!

DanGould commented May 20, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spacebear21 commented May 16, 2025 •

edited

Loading

coveralls commented May 16, 2025 •

edited

Loading

DanGould commented May 19, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading