Skip to content

feat!: Add #[non_exhaustive] to ChangeSet structs #1980

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

notmandatory
Copy link
Member

Description

Add #[non_exhaustive] to ChangeSet structs in tx_graph, keychain_txout, local_chain.

Add #[non_exhaustive] attribute to the ChangeSet structs to prevent downstream crates from exhaustively matching or constructing instances using struct literal syntax, allowing future additions without breaking compatibility.

The change improves forward compatibility by allowing new fields to be added without breaking downstream code in future releases.

Notes to the reviewers

The motivation for this is to prevent breaking changes in these structs but will still require downstream persistence crates to implement and support any new fields added.

@ValuedMammal suggested creating a persistence test suite which I think is a great idea but should be done in a separate PR.

Changelog notice

Breaking Changes

  • Added #[non_exhaustive] attribute to ChangeSet structs in tx_graph, keychain_txout, local_chain modules.
    • Match expressions on these structs must now include wildcard (..) patterns.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo +nightly 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

@notmandatory notmandatory added this to the Wallet 3.0.0 milestone Jun 18, 2025
@notmandatory notmandatory self-assigned this Jun 18, 2025
@notmandatory notmandatory added the api A breaking API change label Jun 18, 2025
@evanlinjin
Copy link
Member

The persistence test suite will make this work. It's unfortunate we didn't think about that solution earlier.

@notmandatory notmandatory moved this to In Progress in BDK Chain Jun 18, 2025
@notmandatory notmandatory force-pushed the feat/non_exhaustive_changesets branch 3 times, most recently from c1a34a8 to 49e140f Compare June 19, 2025 03:06
…in_txout, local_chain

Add #[non_exhaustive] attribute to the ChangeSet structs to prevent downstream
crates from exhaustively matching or constructing instances using struct
literal syntax, allowing future additions without breaking compatibility.

The change improves forward compatibility by allowing new fields to be added
without breaking downstream code in future releases.
@notmandatory notmandatory force-pushed the feat/non_exhaustive_changesets branch from 49e140f to e643f83 Compare June 19, 2025 03:39
@notmandatory
Copy link
Member Author

I fixed the tests, I don't think they look too bad with the non-exhaustive change sets.

@notmandatory notmandatory marked this pull request as ready for review June 19, 2025 03:41
@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Chain Jun 19, 2025
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

The wallet::ChangeSet struct used to be non_exhaustive, which was reverted after I had argued in #1591 that it shouldn't be.

Why do we now think the sub-changesets should be non_exhaustive? To restate my point: having your code break is a good thing if you're handling the changeset types manually, because then you're forced to update your persistence. If you force a non-breaking API via non_exhaustive, users could miss persisting important fields after they had been added.

@notmandatory
Copy link
Member Author

Ah ok I completely forgot we discussed this last year. It seems we have conflicting goals:

  • I want to be able to add new, default-able fields in persisted data as non-breaking changes so we don't need to do a new major release every time we add a feature that requires persisting new data.
  • You want existing persistence code to break when new fields are added to persisted data so all implementations are forced to stay up-to-date.

I think a part of the problem is we're trying to support a persistence API that is tightly coupled to our ChangeSet structs. I feel like there should be a way to support both goals. Possibly with some sort of persistence store traits or persistence conformance test suite, maybe both. But I'll do some more thinking on it, and open to any other ideas.

@notmandatory notmandatory marked this pull request as draft June 19, 2025 16:36
@notmandatory notmandatory moved this from Needs Review to In Progress in BDK Chain Jun 19, 2025
@tnull
Copy link
Contributor

tnull commented Jun 19, 2025

  • I want to be able to add new, default-able fields in persisted data as non-breaking changes so we don't need to do a new major release every time we add a feature that requires persisting new data.

ChangeSet in part of the public API. If you add new data fields that a user is required to persist, it's by definition a breaking change, no?

Possibly with some sort of persistence store traits or persistence conformance test suite

What you describe here is a BDK-internal data model/serialization logic, which was decided against some time ago. But yeah, if that is the conclusion, I'd be very happy about that outcome (especially if it considers forwards compat.). ;)

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
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants