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

Remove deprecated checksum routines #1253

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Jan 2, 2024

Description

This PR removes the routines get_checksum and get_checksum_bytes (in the bdk crate, descriptor/checksum.rs), which have been deprecated in 0.24.0. Consequently, the routine calc_checksum_bytes_internal is consolidated into calc_checksum_bytes, as the boolean parameter exclude_hash is not needed anymore. See also the two TODOs in the touched file.

Notes to the reviewers

In the second commit, the signature of the function calc_checksum_bytes slightly changes, as the desc parameter now is declared as mut, in order to change the local variable within the function. My rust experience is rather limited, so I'm not sure if this is a problem for users. IIUC, this is comparable to changing a pass-by-value parameter in C++ from const std::string desc to std::string desc, which is relevant only for the function implementation, but doesn't change the interface.

Changelog notice

  • Remove deprecated get_checksum and get_checksum_bytes routines

Checklists

All Submissions:

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

There are deprecated since 0.24.0, i.e. they can be removed now.
Now that the deprecated `get_checksum{,_bytes}` routines are removed,
the checksum is always computed with ignored data after the first '#',
i.e. the boolean parameter `exclude_hash` is not needed anymore and
the functionality can be consolidated into `calc_checksum_bytes`.
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

ACK ed91a4b

Looks good to me, thanks for taking the time to clean up the code! :)

@evanlinjin
Copy link
Member

@theStack thank you for the contribution. However our policy is that all commits need to be signed otherwise we cannot merge! Please sign your commits!

@notmandatory notmandatory added the api A breaking API change label Jan 16, 2024
@theStack
Copy link
Contributor Author

@theStack thank you for the contribution. However our policy is that all commits need to be signed otherwise we cannot merge! Please sign your commits!

The commits are signed, but apparently I forgot to upload my GPG public key to GitHub. Did that now, so the commits are shown as "Verified". Is there anything more to do?

@danielabrozzoni
Copy link
Member

It should be fine now, thanks!

@danielabrozzoni danielabrozzoni merged commit fbd1d65 into bitcoindevkit:master Jan 22, 2024
12 checks passed
@theStack theStack deleted the remove_deprecated_checksum_routines branch January 22, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants