Skip to content

Create new v2_to_v2 integration test #85

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 13, 2025
Merged

Create new v2_to_v2 integration test #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d1c9ff7
Introduce bitcoin_ffi Psbt struct and test_utils runtime
benalleng May 1, 2025
fab9db6
Expose uniffi persistence traits as foreign
arminsabouri May 1, 2025
2f562f5
Introduce python integration tests v2_to_v2
benalleng May 9, 2025
265ad68
rename test files
benalleng May 9, 2025
83359b5
Add the python integration test to workflow
benalleng May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 43 additions & 27 deletions .github/workflows/build-python.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,23 @@ name: Build and Test Python
on: [push, pull_request]

jobs:
build-manylinux_2_28-x86_64-wheels:
name: "Build and test Manylinux 2.28 x86_64 wheels"
runs-on: ubuntu-24.04
build-wheels-and-test:
name: "Build and test wheels with Redis"
runs-on: ubuntu-latest
services:
redis:
image: redis:7-alpine
defaults:
run:
working-directory: python
container:
image: quay.io/pypa/manylinux_2_28_x86_64
env:
PLAT: manylinux_2_28_x86_64
PYBIN: "/opt/python/${{ matrix.python_dir }}/bin"
strategy:
matrix:
include:
- python: "3.9"
python_dir: "cp39-cp39"
- python: "3.10"
python_dir: "cp310-cp310"
- python: "3.11"
python_dir: "cp311-cp311"
- python: "3.12"
python_dir: "cp312-cp312"
- python: "3.13"
python_dir: "cp313-cp313"
steps:
- name: "Checkout"
uses: actions/checkout@v4
Expand All @@ -41,22 +34,29 @@ jobs:
with:
python-version: ${{ matrix.python }}

- name: "Install build dependencies"
run: |
sudo apt update
sudo apt install -y build-essential python3-dev

- name: "Use cache"
uses: Swatinem/rust-cache@v2

- name: "Generate payjoin-ffi.py and binaries"
run: PYBIN="/opt/python/${{ matrix.python_dir }}/bin" bash ./scripts/generate_linux.sh
run: |
PYBIN=$(dirname $(which python))
PYBIN="$PYBIN" bash ./scripts/generate_linux.sh

- name: "Build wheel"
# Specifying the plat-name argument is necessary to build a wheel with the correct name,
# see issue BDK#350 for more information
run: ${PYBIN}/python setup.py bdist_wheel --plat-name $PLAT --verbose
run: python setup.py bdist_wheel --verbose

- name: "Install wheel"
run: ${PYBIN}/pip install ./dist/*.whl
run: pip install ./dist/*.whl

- name: "Run tests"
run: ${PYBIN}/python -m unittest --verbose test/payjoin_unit_test.py
env:
REDIS_URL: redis://localhost:6379
run: python -m unittest -v

build-macos:
name: "Build and test macOS"
Expand Down Expand Up @@ -84,23 +84,39 @@ jobs:
with:
python-version: ${{ matrix.python }}

- name: Setup Docker on macOS
uses: douglascamata/setup-docker-macos-action@v1.0.0

- name: "Install Redis"
run: |
brew update
brew install redis

- name: "Start Redis"
run: |
redis-server --daemonize yes
for i in {1..10}; do
if redis-cli ping | grep -q PONG; then
echo "Redis is ready"
break
fi
echo "Waiting for Redis..."
sleep 1
done

- name: "Use cache"
uses: Swatinem/rust-cache@v2

- name: "Generate payjoin-ffi.py and binaries"
run: bash ./scripts/generate_macos.sh

- name: "Build wheel"
# Specifying the plat-name argument is necessary to build a wheel with the correct name,
# see issue BDK#350 for more information
run: python3 setup.py bdist_wheel --plat-name macosx_11_0_x86_64 --verbose
run: python3 setup.py bdist_wheel --verbose

- name: "Install wheel"
run: pip3 install ./dist/*.whl

- name: "Run tests"
run: python3 -m unittest --verbose test/payjoin_unit_test.py

- name: "Build arm64 wheel"
run: python3 setup.py bdist_wheel --plat-name macosx_11_0_arm64 --verbose
# Note: You can't install the arm64 wheel on the CI, so we skip these steps and simply test that the wheel builds
env:
REDIS_URL: redis://localhost:6379
run: python3 -m unittest -v
5 changes: 3 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ license = "MIT OR Apache-2.0"
exclude = ["tests"]

[features]
_test-utils = ["payjoin-test-utils", "tokio"]
_test-utils = ["payjoin-test-utils", "tokio", "bitcoind"]
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that bitcoind and related helpers (rpc client, sender/receiver wallets) should be provided by payjoin-test-utils's TestServices to avoid boilerplate and the additional dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't I still need a reliable way to access and pass the correct types across the ffi layer to bitcoind?

Copy link
Contributor

Choose a reason for hiding this comment

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

unless payjoin-test-utils re-exported bitcoind, I think you would need to rely on "bitcoind" as a dependency for a reliable way to access and pass bitcoind types across the ffi layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. "payjoin-test-utils re-exporting bitcoind" sounds like a good idea to me, especially since that specific dependency may change in the future.

_danger-local-https = ["payjoin/_danger-local-https"]
uniffi = ["uniffi/cli", "bitcoin-ffi/default"]

Expand All @@ -23,8 +23,10 @@ uniffi = { version = "0.29.1", features = ["build"] }

[dependencies]
base64 = "0.22.1"
bitcoin-ffi = { git = "https://github.com/bitcoindevkit/bitcoin-ffi.git", rev = "6b1d131" }
bitcoind = { version = "0.36.0", features = ["0_21_2"], optional = true }
bitcoin-ffi = { git = "https://github.com/benalleng/bitcoin-ffi.git", rev = "8e3a23b" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any value in holding off merging this until the upstream PR gets merged?

Copy link
Contributor Author

@benalleng benalleng May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think that's a @DanGould question, based on the state of review in that PR though it seems likely that the only thing that will change is how errors are built and presented.

Copy link
Contributor

Choose a reason for hiding this comment

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

some value in that tracking the rest of the ecosystem lets us merge projects when we get there, but I think we're a long way off and we won't stray too far.

I don't think bdk-ffi will depend on payjoin-ffi as an option any time soon, which would be the reason to stay on the same version of bitcoin-ffi afaict

hex = "0.4.3"
lazy_static = "1.5.0"
ohttp = { package = "bitcoin-ohttp", version = "0.6.0" }
payjoin = { version = "0.23.0", features = ["v1", "v2", "io"] }
payjoin-test-utils = { version = "0.0.0", optional = true }
Expand All @@ -37,7 +39,6 @@ url = "2.5.0"

[dev-dependencies]
bdk = { version = "0.29.0", features = ["all-keys", "use-esplora-ureq", "keys-bip39", "rpc"] }
bitcoind = { version = "0.36.0", features = ["0_21_2"] }
bitcoincore-rpc = "0.19.0"
http = "1"
ohttp-relay = "0.0.8"
Expand Down
2 changes: 1 addition & 1 deletion python/requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
python-bitcoinlib==0.12.2
toml==0.10.2
yapf==0.43.0

httpx==0.28.1
Copy link
Contributor

Choose a reason for hiding this comment

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

very pleased we're able to use httpx here instead of exposing rust's reqwest. I'm curious, though not particularly eager, to see if the native http clients for some foreign language can replace fetch_ohttp_keys so we don't have to ship a whole rust TLS stack with our binaries, too.

Loading
Loading