Repository navigation
[2.x] Reject duplicate set items in dynamic codecs - #1208
lorisleiva wants to merge 1 commit into
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds an encode-time uniqueness check to setTypeNode codecs in @codama/dynamic-codecs. visitSetType now wraps the array-like codec in a new assertUniqueItems pre-encode transform that encodes each item, keys it by its base16 string, and throws the new CODAMA_ERROR__DYNAMIC_CLIENT__DUPLICATE_SET_ITEM (2500021) with { firstIndex, index, nodePath } on the first collision. Decoding is untouched. The error code, context type and message are registered in @codama/errors, and the tests cover integer/bigint equivalence, tuples, structs with defaulted fields, nested paths and decode passthrough.
The implementation is sound. Comparing by encoded bytes is the right call here — it sidesteps structural equality on arbitrary JS values and naturally handles 42 vs 42n, omitted-default fields, and item-level transforms (since item is the already-transform-wrapped codec from visit(node.item, this)). The non-array passthrough keeps the existing UNEXPECTED_VALUE_TYPE error for bad inputs, and because the check is applied inside visitSetType, the set's own transforms still layer on top via the intercept visitor. Order of errors is also sensible: a wrong-typed item throws the item's own UNEXPECTED_VALUE_TYPE from item.encode before the uniqueness check can run, which is the same error the inner codec would have produced.
Things to address
- README is now out of date.
packages/dynamic-codecs/README.mddescribesSetTypeNodeas "Same as arrays." in the value-format table, and the "Invalid values" section only documentsUNEXPECTED_VALUE_TYPE. Both should mention that encoding rejects duplicate items withDUPLICATE_SET_ITEM, since that's exactly the kind of behavioural difference a consumer would look up there. - No changeset in the diff. Per
CONTRIBUTING.md, user-facing changes need one, and this touches two publishable concerns:@codama/dynamic-codecsgains a new encode-time rejection, and@codama/errors(fixed group) gains a new code. If the seeded major changeset is intended to absorb this during the rc candidacy, ignore this — but a dedicated entry would make the behaviour change discoverable in the changelog.
Notes for other reviewers
- See the inline note on
assertUniqueItems: becausetransformCodec's pre-encode hook runs in bothgetSizeFromValueandwrite, variable-size sets end up encoding every item twice for the check on top of the real encode. Not a correctness issue and almost certainly fine for a dynamic codec, just worth knowing about. - Worth a quick sanity check that
CodamaErrorstores__codeoncontextthe way theduplicateSetItemErrortest helper assumes (context: { __code, ...context }with exact matching). It mirrors Kit'sSolanaError, so I expect it does, and the tests would fail loudly otherwise.
a1a4ae3 to
7c1d78c
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Everything from the first pass has been addressed:
- README now flags the duplicate rejection in the
SetTypeNoderow and documentsDUPLICATE_SET_ITEM(with itsindex/firstIndex/nodePathcontext and the decode-keeps-duplicates caveat) in the "Invalid values" section. Reads well and matches the implementation. assertUniqueItemsdocblock now calls out the extra encode per item and the double run on variable-size codecs, so the cost is discoverable.- Tests cover the two gaps I raised: non-array input still surfaces
UNEXPECTED_VALUE_TYPEwithexpectedType: 'array'and the set's ownnodePath, and a wrong-typed item throws the item's error withnodePath: [set, item]before any duplicate check runs. The[1, 'x', 'x']input is a nice choice since it would otherwise be a duplicate, which pins the precedence properly.
Code is unchanged in substance and still looks correct: base16 keying of item.encode(...) output, first-collision reporting, non-array passthrough, and stack.getPath() captured at visit time consistent with assertValueType.
The only open item is the changeset, which is still absent from the diff. As before, if the seeded major changeset for the rc pre-release is meant to absorb this, that's fine — otherwise @codama/dynamic-codecs and @codama/errors both have user-visible changes here that would warrant an entry. Not blocking on it since that's a release-workflow call for you to make.

This PR makes
setTypeNodeencoders reject items that encode to the same bytes, which restores the uniqueness check that v1'sdynamic-clientgot from superstruct.CODAMA_ERROR__DYNAMIC_CLIENT__DUPLICATE_SET_ITEM(2500021).