feat(storable): implement Storable for unbounded 2-tuples#434
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Implements support for Storable serialization/deserialization of unbounded (A, B) tuples by replacing the existing todo!() paths, while keeping the bounded 2-tuple wire format unchanged.
Changes:
- Add unbounded
(A, B)decoding logic infrom_bytes(including thesize_lengths+size_aheader whenAis not fixed-size). - Add unbounded
(A, B)encoding logic into_bytes/into_byteswith dynamic size-length selection forA. - Add unit + proptest coverage for unbounded 2-tuple roundtrips and exact byte layouts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/storable/tuples.rs |
Implements unbounded 2-tuple wire format for encoding/decoding in the (A, B): Storable impl. |
src/storable/tests.rs |
Adds targeted unit tests and proptest roundtrip tests for the new unbounded 2-tuple behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lpahlavi
force-pushed
the
louis/unbounded-tuple-storable
branch
from
368467d to
9c779c2
Compare
|
|
|
|
|
|
alin-at-dfinity
approved these changes
Apr 21, 2026
Replaces the two `todo!()` panics in the `(A, B)` `Storable` impl
(`into_bytes_inner_2` and `from_bytes`) with a working implementation
for unbounded tuples. The bounded 2-tuple wire format is unchanged.
Wire format for unbounded `(A, B)`, documented in a comment inside the
impl block mirroring the 3-tuple style:
If A is fixed-size:
<a_bytes> <b_bytes>
Otherwise:
<size_lengths (1B)> <size_a (1-4B)> <a_bytes> <b_bytes>
`size_lengths` encodes (in 2 bits) the number of bytes used to store
`size_a`, matching the encoding already used by the 3-tuple path.
`B`'s length is always inferred from the remaining bytes.
Tests mirror the 3-tuple unbounded test coverage:
- `tuple_with_two_unbounded_elements_roundtrip`
- `tuple_with_two_elements_bounded_and_unbounded_roundtrip` (with byte count)
- `optional_tuple_with_two_unbounded_elements_roundtrip`
- `optional_tuple_with_two_elements_bounded_and_unbounded_roundtrip`
- `tuple_with_two_elements_test_bound`
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lpahlavi
force-pushed
the
louis/unbounded-tuple-storable
branch
from
9c779c2 to
3423b16
Compare
Contributor
|
(for posterity) I just noticed this PR and I would like to provide some context. Unbounded 2-tuple was originally left unimplemented because EXC-1550 explored a smooth The concern this PR introduces: the 2-tuple now carries two completely different wire formats, so a user who flips a type's Worth re-evaluating whether shipping this is better than the previous If we keep it I'd want at minimum:
@lpahlavi what do you think on whether we should keep it and face a silent-breakage risk or revert it? |
lpahlavi
added a commit
that referenced
this pull request
Apr 22, 2026
## Summary - Reverts #434, which implemented `Storable` for unbounded 2-tuples. - Restores the `todo!()` panics that fail loudly instead of silently mis-decoding data. ## Motivation As noted in [this comment](#434 (comment)) by @maksymar: The 2-tuple now carries two completely different wire formats (bounded vs unbounded), so a user who flips a type's `BOUND` from `Bounded` to `Unbounded` will silently mis-decode existing data. The 3-tuple doesn't have this problem since it uses one unified wire format for both variants. Reverting to `todo!()` is safer — it fails loudly rather than risking silent data corruption. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
todo!()panics in the(A, B)Storableimpl (into_bytes_inner_2andfrom_bytes) with a working implementation for unbounded tuples.Wire format for unbounded
(A, B)Documented in a comment inside the impl block, mirroring the 3-tuple style:
size_lengthsencodes (in 2 bits) how many bytes were used to represent A's length — identical to the encoding already used by the 3-tuple path. B is always the last element so its length is inferred from the remaining bytes and never stored explicitly.🤖 Generated with Claude Code