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

refactor(wallet): cleanup and remove unused code in create_tx #1768

Merged
merged 1 commit into from
Dec 23, 2024
Merged

refactor(wallet): cleanup and remove unused code in create_tx #1768

merged 1 commit into from
Dec 23, 2024
+13 −29

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Dec 10, 2024 •
edited by ValuedMammal
Loading

Description

Cleanup and remove unused code in Wallet::create_tx, this was noticed during review of #1763. See: #1763 (comment)

fixes #1710

Notes to the reviewers

In addition to removing the unneeded assignments to fee_amount and received I also refactored creation of the change output to be an if let instead of match statement since it only needs to do something if there is Excess::Change.

I should have done this cleanup as part of #1048.

Changelog notice

None, only internal cleanup.

Checklists

All Submissions:

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

@notmandatory notmandatory self-assigned this Dec 10, 2024
@notmandatory notmandatory mentioned this pull request Dec 10, 2024
Merged
6 tasks
@Gudnessuche
Copy link

All tests still pass after doing this?

@notmandatory notmandatory added the audit Suggested as result of external code audit label Dec 10, 2024
oleonardolima
oleonardolima reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima 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 a0ba177

@notmandatory notmandatory force-pushed the refactor/cleanup_create_tx branch from a0ba177 to cc90507 Compare December 12, 2024 17:50
@notmandatory
Copy link
Member Author

Re-based and fixed use Excess.

oleonardolima
oleonardolima reviewed Dec 12, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK cc90507

ValuedMammal
ValuedMammal approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK cc90507

@ValuedMammal ValuedMammal force-pushed the refactor/cleanup_create_tx branch from cc90507 to a2f7a8b Compare December 23, 2024 15:16
ValuedMammal
ValuedMammal approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

Rebased to trigger CI

reACK a2f7a8b

@notmandatory notmandatory merged commit 4a3675f into bitcoindevkit:master Dec 23, 2024
21 checks passed
@notmandatory notmandatory deleted the refactor/cleanup_create_tx branch December 23, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleonardolima oleonardolima oleonardolima left review comments

@ValuedMammal ValuedMammal ValuedMammal approved these changes

Assignees

@notmandatory notmandatory

Labels
audit Suggested as result of external code audit module-wallet
Projects
BDK
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanup create_tx fee_amount usage
4 participants
@notmandatory @Gudnessuche @oleonardolima @ValuedMammal