Skip to content

Feature/v1 #39

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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Feature/v1 #39

wants to merge 2 commits into from

Conversation

ulrichard
Copy link
Collaborator

@ulrichard ulrichard commented Jan 15, 2025

I adapted the signature counts in the tests to make the tests pass.
In the old version there was a flag remove_partial_sigs in SignOptions.
I removed the test tampered_proof_sighash_tx because the expected error was not thrown, and apparently doesn't exist any more.

@ulrichard ulrichard force-pushed the feature/v1 branch 3 times, most recently from ea66d36 to 443a165 Compare January 16, 2025 10:35
@ulrichard ulrichard force-pushed the feature/v1 branch 2 times, most recently from 3c3ebfc to 10c2b54 Compare January 23, 2025 10:29
Copy link

@tvpeter tvpeter left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. Waiting for this PR to update reserves feature on bdk-cli. Thank you.

let expected = r#"cHNidP8BAH4BAAAAAmw1RvG4UzfnSafpx62EPTyha6VslP0Er7n3TxjEpeBeAAAAAAD/////2johM0znoXIXT1lg+ySrvGrtq1IGXPJzpfi/emkV9iIAAAAAAP////8BUMMAAAAAAAAZdqkUn3/QltN+0sDj9/DPySS+70/862iIrAAAAAAAAQEKAAAAAAAAAAABUQEHAAABAR9QwwAAAAAAABYAFOzlJlcQU9qGRUyeBmd56vnRUC5qIgYDKwVYB4vsOGlKhJM9ZZMD4lddrn6RaFkRRUEVv9ZEh+ME7OUmVwAA"#;
let expected = r#"cHNidP8BAH4BAAAAAmw1RvG4UzfnSafpx62EPTyha6VslP0Er7n3TxjEpeBeAAAAAAD/////MQvsP2eDTCk3vWfQJ50IOFWLwuTHPsnYikR1hosdK0sAAAAAAP3///8BUMMAAAAAAAAZdqkUn3/QltN+0sDj9/DPySS+70/862iIrAAAAAAAAQEKAAAAAAAAAAABUQEHAAABAR9QwwAAAAAAABYAFOzlJlcQU9qGRUyeBmd56vnRUC5qIgYDKwVYB4vsOGlKhJM9ZZMD4lddrn6RaFkRRUEVv9ZEh+ME7OUmVwAA"#;

assert_eq!(psbt_b64, expected);
Copy link

Choose a reason for hiding this comment

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

This is failing because the Psbt produced is version 2 while the expected Psbt value you supplied is version 1. This is coming from here. Every other data is the same.
Screenshot 2025-02-10 at 5 53 11 PM
Screenshot 2025-02-10 at 5 53 27 PM

@notmandatory
Copy link
Member

You should start with bdk_wallet 1.2, it has some good improvements. https://github.com/bitcoindevkit/bdk/releases/tag/wallet-1.2.0

Otherwise ACK

@ulrichard ulrichard force-pushed the feature/v1 branch 6 times, most recently from 7c55933 to e4b104d Compare April 25, 2025 14:01
@tvpeter
Copy link

tvpeter commented Apr 26, 2025

tACK e4b104d

@notmandatory
Copy link
Member

Looks like you need to take the WASM CI job out for this PR too or you can rebase it after merging #38 is merged.

@ulrichard
Copy link
Collaborator Author

@notmandatory I rebased on master after merging the other branch. So, everything should be fine.

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