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.

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

Jump to bottom

Borrow instead of moving transaction when broadcasting #452

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Borrow instead of moving transaction when broadcasting #452

merged 1 commit into from
Oct 27, 2021

Conversation

klochowicz
Copy link
Contributor

@klochowicz klochowicz commented Oct 20, 2021 •
edited by notmandatory
Loading

Description

Cleanup - don't need to move transaction when broadcasting

All Submissions:

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

Bugfixes:

  • This pull request breaks the existing API

I guess it breaks the API, albeit in a really trivial way (clippy should suggest using a borrow)

@klochowicz klochowicz force-pushed the borrow-transaction-for-broadcasting branch from 1f87367 to 535f600 Compare October 20, 2021 22:47
rajarshimaitra
rajarshimaitra reviewed Oct 24, 2021
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Concept ACK. It seems unnecessary to move the tx while the internal broadcast is using reference itself.

LLFourn
LLFourn approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

ACK

@klochowicz klochowicz force-pushed the borrow-transaction-for-broadcasting branch from 535f600 to c0695f8 Compare October 25, 2021 05:53
@afilini
Copy link
Member

afilini commented Oct 26, 2021

Concept ACK, this needs to be rebased on top of #454 to fix the CI and then we can merge it

@klochowicz
Borrow instead of moving transaction when broadcasting
3d8efbf 
There's no need to take ownership of the transaction for a broadcast.
@klochowicz klochowicz force-pushed the borrow-transaction-for-broadcasting branch from c0695f8 to 3d8efbf Compare October 27, 2021 11:31
@klochowicz
Copy link
Contributor Author

Concept ACK, this needs to be rebased on top of #454 to fix the CI and then we can merge it

rebased! hopefully the tests will pass this time.

notmandatory
notmandatory approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 3d8efbf

Thanks, nice fix.

@notmandatory notmandatory merged commit 3d8efbf into bitcoindevkit:master Oct 27, 2021
@notmandatory notmandatory mentioned this pull request Nov 2, 2021
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajarshimaitra rajarshimaitra rajarshimaitra left review comments

@LLFourn LLFourn LLFourn approved these changes

@notmandatory notmandatory notmandatory approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 0.14.0 Feature Freeze
Development

Successfully merging this pull request may close these issues.
5 participants
@klochowicz @afilini @LLFourn @notmandatory @rajarshimaitra