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

Remove TxGraph::missing_heights as ChangeSet::missing_heights_from should have replaced it #1126

Closed
wants to merge 3 commits into from
Closed

Remove TxGraph::missing_heights as ChangeSet::missing_heights_from should have replaced it #1126

wants to merge 3 commits into from

Conversation

evanlinjin
Copy link
Member

Description

ChangeSet::missing_heights_from is introduced as a simpler alternative to TxGraph::missing_heights. We should remove TxGraph::missing_heights and use ChangeSet::missing_heights_from instead.

Notes to the reviewers

WIP

Changelog notice

Remove TxGraph::missing_heights and use ChangeSet::missing_heights_from instead.

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

@evanlinjin evanlinjin force-pushed the rm_missing_heights branch 2 times, most recently from 301558c to 6406d53 Compare September 15, 2023 03:11
@evanlinjin
feat(wallet): add From impls for wallet types for convenience
64f9af2 
Also publicly expose `bdk::wallet::Update` as `bdk::Update`.
@evanlinjin
feat(chain)!: rm TxGraph::missing_heights
8e82bfa 
We use `ChangeSet::missing_heights_from` in favour of
`TxGraph::missing_heights`.

`test_missing_blocks` is renamed to `test_changeset_missing_blocks_from`
and the test is updated.
@nondiremanuel nondiremanuel mentioned this pull request Sep 23, 2023
Closed
40 tasks
@nondiremanuel nondiremanuel added this to the 1.0.0-beta.0 milestone Sep 26, 2023
@notmandatory notmandatory modified the milestones: 1.0.0-beta.0, 1.0.0-alpha.4 Nov 13, 2023
@notmandatory
Copy link
Member

I'm moving this back to new alpha.4 release since it looks like a large-ish functional change.

@LLFourn
Copy link
Contributor

LLFourn commented Nov 16, 2023

I think I've expressed directly to evan that I think this is a bad change. TxGraph::missing_heights is a useful method on its own. The names should be made consistent though.

@nondiremanuel nondiremanuel mentioned this pull request Nov 19, 2023
Closed
54 tasks
@nondiremanuel nondiremanuel mentioned this pull request Dec 2, 2023
Closed
55 tasks
@notmandatory notmandatory added the discussion There's still a discussion ongoing label Dec 5, 2023
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@notmandatory notmandatory added the api A breaking API change label Jan 16, 2024
@LLFourn
Copy link
Contributor

LLFourn commented Jan 17, 2024

Haven't heard anything back from @evanlinjin so closing this for now.

@LLFourn LLFourn closed this Jan 17, 2024
@LLFourn LLFourn mentioned this pull request Feb 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@evanlinjin evanlinjin

Labels
api A breaking API change discussion There's still a discussion ongoing
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging this pull request may close these issues.
4 participants
@evanlinjin @notmandatory @LLFourn @nondiremanuel