bdk icon indicating copy to clipboard operation
bdk copied to clipboard

feat!: Add #[non_exhaustive] to ChangeSet structs

Open notmandatory opened this issue 6 months ago • 4 comments

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 avatar Jun 18 '25 18:06 notmandatory

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

evanlinjin avatar Jun 18 '25 21:06 evanlinjin

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

notmandatory avatar Jun 19 '25 03:06 notmandatory

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 avatar Jun 19 '25 16:06 notmandatory

  • 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.). ;)

tnull avatar Jun 19 '25 16:06 tnull