⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Validate FFI primitive inputs + refresh bindings/tests#1263

Open
chavic wants to merge 3 commits intopayjoin:masterfrom
chavic:chavic/primitives-validation
Open

Validate FFI primitive inputs + refresh bindings/tests#1263
chavic wants to merge 3 commits intopayjoin:masterfrom
chavic:chavic/primitives-validation

Conversation

@chavic
Copy link
Collaborator

@chavic chavic commented Jan 10, 2026

This PR addresses #1262 and hardens the FFI boundary by validating primitive inputs and surfacing explicit errors. Updates integration tests/bindings to match the new error shapes and removes silent acceptance of invalid fee rates/amounts.

Follow-ups

Decide final script/witness size caps and document rationale.
Add Dart integration fix for large fee rate parsing (separate PR).

@coveralls
Copy link
Collaborator

coveralls commented Jan 10, 2026

Pull Request Test Coverage Report for Build 21681318162

Details

  • 3 of 5 (60.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 83.193%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/core/psbt/mod.rs 0 2 0.0%
Totals Coverage Status
Change from base Build 21675282596: -0.01%
Covered Lines: 10142
Relevant Lines: 12191

💛 - Coveralls

@benalleng benalleng mentioned this pull request Jan 13, 2026
9 tasks
@chavic chavic force-pushed the chavic/primitives-validation branch 2 times, most recently from cd3391a to a44a282 Compare January 19, 2026 17:40
@chavic chavic force-pushed the chavic/primitives-validation branch 2 times, most recently from 59affc6 to 93bc4e8 Compare January 27, 2026 11:10
@chavic chavic force-pushed the chavic/primitives-validation branch 5 times, most recently from 055a2bb to 53db1cb Compare February 3, 2026 17:13
@chavic chavic marked this pull request as ready for review February 4, 2026 09:52
Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

Instead of updating the msrv for dart I think we can simply use cargo install --locked wasm-bindgen-cli on the js workflow to prevent the install step from trying to update the deps

@chavic
Copy link
Collaborator Author

chavic commented Feb 4, 2026

Instead of updating the msrv for dart I think we can simply use cargo install --locked wasm-bindgen-cli on the js workflow to prevent the install step from trying to update the

Thanks... applying that now

@chavic chavic force-pushed the chavic/primitives-validation branch from 759be1b to 10ee724 Compare February 4, 2026 14:35
@chavic chavic requested a review from benalleng February 4, 2026 14:38
@chavic chavic force-pushed the chavic/primitives-validation branch 3 times, most recently from edad0fc to a1711eb Compare February 4, 2026 15:15
@chavic chavic force-pushed the chavic/primitives-validation branch 4 times, most recently from 84e2546 to 732c40a Compare February 4, 2026 16:57
@chavic chavic force-pushed the chavic/primitives-validation branch from 732c40a to f331ef3 Compare February 4, 2026 17:21
Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

CACK f331ef3 got the commits nicely reviewable again thanks for that.

Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

ACK f331ef3 I have just a few nits and a broader question but overall I think it looks good! Thanks again for getting these commits easier to read 👍

).build().save(recvPersister).pjUri();

final psbt =
"cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA=";
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should have access to this from test_utils. I think I'd like to see this properly imported here.

"bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?amount=1&pj=https://example.com",
).checkPjSupported();
const psbt =
"cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA=";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here for the test_utils import. though we would have to add it to the test_utils/index.js it seems

Copy link
Collaborator

Choose a reason for hiding this comment

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

The unit tests seem to have inconsistent tests. Is there a specific reason that this is the case? The python unit tests here seem to be the most complete

Copy link
Collaborator

Choose a reason for hiding this comment

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

A question for a followup perhaps but should we consider making a primitives module to keep this kind of file in case we end up needing more like it?

Copy link
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

Jumped the gun, I think I would like this changed moved and explained into a new commit. I totally didn't realise it was a readme change..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants