Skip to content
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

Add Transaction struct, update PSBT and Blockchain to use it #296

Merged

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Jan 17, 2023

Description

Add new Transaction structure that can be created from transaction consensus encoded bytes. Update PartiallySignedTransaction.extract_tx() to return a Transaction instead of a the transaction bytes. Update Blockchain.broadcast() to take a Transaction parameter.

Notes to the reviewers

Fixes #157.

Changelog notice

Added

  • New Transaction structure that can be created from or serialized to consensus encoded bytes.

Changed

  • PartiallySignedTransaction.extract_tx() returns a Transaction instead of a the transaction bytes.
  • Blockchain.broadcast() takes a Transaction instead of a PartiallySignedTransaction.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory force-pushed the broadcast_transaction branch 2 times, most recently from b333000 to 4b28398 Compare January 17, 2023 04:20
@notmandatory notmandatory self-assigned this Jan 17, 2023
@notmandatory notmandatory added the enhancement New feature or request label Jan 17, 2023
@notmandatory notmandatory added this to the Release 0.27.0 milestone Jan 17, 2023
@notmandatory notmandatory force-pushed the broadcast_transaction branch 2 times, most recently from d5d81cf to f8037d7 Compare January 23, 2023 18:15
@thunderbiscuit thunderbiscuit self-requested a review January 23, 2023 18:46
@notmandatory notmandatory marked this pull request as ready for review January 23, 2023 20:12
@notmandatory notmandatory force-pushed the broadcast_transaction branch from f8037d7 to 1ce5546 Compare January 23, 2023 20:27
@notmandatory
Copy link
Member Author

I marked this as ready to review, it's rebased and I don't expect any more code changes. But I still need to do manual testing to confirm the transaction broadcast.

@notmandatory notmandatory force-pushed the broadcast_transaction branch from 1ce5546 to 8e54ada Compare January 23, 2023 20:30
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Tested ACK 8e54ada. I ran a script using bdk-jvm and your test vector. Looks good! Another win for the LDK/BDK integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

Provide a way to broadcast raw transaction for LDK compatibility
2 participants