Skip to content

fix(docs): merge_chains outdated documentation #1806

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 1 commit into
base: master
Choose a base branch
from

Conversation

oleonardolima
Copy link
Contributor

Description

It's a minor documentation fix, as reported during the audit and referenced in bitcoindevkit/bdk_wallet#59.

Notes to the reviewers

I'm unsure if anything else / any other explanation should be included in the documentation. Let me know if you think more context should be added to it.

Changelog notice

  • Update bdk_chain::local_chain::merge_chains documentation.

Checklists

All Submissions:

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

@oleonardolima oleonardolima added the documentation Improvements or additions to documentation label Jan 21, 2025
@oleonardolima oleonardolima self-assigned this Jan 21, 2025
@oleonardolima oleonardolima force-pushed the fix/merge-chains-docs branch from 5efa472 to 50b036c Compare March 20, 2025 13:45
- it's a small fix for `merge_chains` docs, reported on audit.
- adds an `Errors` section to cover what scenarios it can fail.
@oleonardolima oleonardolima force-pushed the fix/merge-chains-docs branch from 50b036c to 166a8f9 Compare March 20, 2025 13:48
@oleonardolima
Copy link
Contributor Author

@ValuedMammal Great, thanks for the suggestions! I've rebased the branch and applied the suggestions.

@ValuedMammal ValuedMammal moved this to Needs Review in BDK Chain Mar 27, 2025
Copy link
Collaborator

@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 166a8f9

@notmandatory notmandatory added this to the Wallet 2.0.0 milestone Apr 26, 2025
@@ -545,31 +545,44 @@ impl std::error::Error for ApplyHeaderError {}

/// Applies `update_tip` onto `original_tip`.
///
/// On success, a tuple is returned `(changeset, can_replace)`. If `can_replace` is true, then the
/// `update_tip` can replace the `original_tip`.
/// On success, a tuple is returned ([`CheckPoint`], [`ChangeSet`]).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// On success, a tuple is returned ([`CheckPoint`], [`ChangeSet`]).
/// On success, a tuple is returned ([`CheckPoint`], [`ChangeSet`]). A [`CannotConnectError`] occurs when the `original_tip` and `update_tip` chains are disjoint.

I'd move the rest of the explanation for CannotConnectError to that type's doc.

But I don't want to bike-shed it too much so I'll also ACK this as is and you decide.

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 166a8f9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

3 participants