Make corepc-node optional in payjoin-test-utils#1424
Make corepc-node optional in payjoin-test-utils#1424DanGould wants to merge 2 commits intopayjoin:masterfrom
Conversation
Pull Request Test Coverage Report for Build 23634000801Details
💛 - Coveralls |
payjoin-test-utils/src/lib.rs
Outdated
| #[cfg(feature = "bitcoind")] | ||
| use bitcoin::Amount; | ||
| #[cfg(feature = "bitcoind")] | ||
| pub use corepc_node; | ||
| #[cfg(feature = "bitcoind")] | ||
| use corepc_node::AddressType; | ||
| #[cfg(feature = "bitcoind")] | ||
| use tracing::Level; |
There was a problem hiding this comment.
nit: to avoid this kind of confusing feature gating we should move bitcoind to its own module and feature gate the whole thing once
There was a problem hiding this comment.
o yeah I was only concerned about the gates being clean in a group and forgot about that option. good call.
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
concept ACK with one structural change request
DanGould
force-pushed
the
test-utils-no-bitcoind
branch
2 times, most recently
from
9d2151a to
87d590c
Compare
DanGould
force-pushed
the
test-utils-no-bitcoind
branch
from
87d590c to
b22f5e6
Compare
Add a bitcoind feature flag to gate corepc-node dependency and move bitcoind helpers behind #[cfg(feature = "bitcoind")]. Downstream crates opt in via features = ["bitcoind"].
DanGould
force-pushed
the
test-utils-no-bitcoind
branch
from
b22f5e6 to
5c59429
Compare
|
I will merge 5c59429 as tested but I also have a follow-up commit on this branch to make it optional for ffi. This is useful for Nix in a user that doesn't have network access for agents. PR there coming immediately after. |
|
Ah nevermind I tried to merge with the ACK but am not allowed because of the requested changes, so I pushed the 2nd commit. |
Split _test-utils into _test-utils (constants + TestServices only) and _test-utils-bitcoind (adds BitcoindEnv, RpcClient, etc.). This lets FFI consumers like Dart unit tests import test constants without triggering the corepc-node/bitcoind download. Update all binding-generation and test scripts to use _test-utils-bitcoind where integration-test types are needed.
DanGould
force-pushed
the
test-utils-no-bitcoind
branch
from
4099199 to
cd56b57
Compare
There was a problem hiding this comment.
This approach seems harmless so ACKing now, but I just realized something that I wish I'd thought of on my first review: couldn't this whole thing be avoided by just making the download flag optional on corepc-node? So test utils would always ship with bitcoind functionality but the 40mb binary and network access become optional. In the nix.flake we already skip download and set BITCOIND_EXE.
EDIT: #514 just got merged. I wonder if it solves the issue you were experiencing with the networkless agent?
4 participants
Test constants (ORIGINAL_PSBT, EXAMPLE_URL, etc.) and the TestServices infrastructure don't require bitcoind. Only init_bitcoind* functions need the corepc-node crate, which downloads a ~40MB binary at build time.
Gate corepc-node behind a
bitcoindfeature (default = on) so downstream consumers can import payjoin-test-utils for constants without triggering the bitcoind download. This unblocks nix environments and CI setups that provide their own bitcoind via BITCOIND_EXE.The split lets FFI bindings import test constants (originalPsbt(), exampleUrl()) without downloading bitcoind — so dart test, python
-m pytest, npm test work in any environment, not just ones with network access. No new bindings needed — the constants are already
exported through payjoin-ffi's test_utils.rs; the only blocker was corepc-node's build.rs firing unconditionally.
This was claude baby. I needed this for an agent in nix with no network.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.