fix(js): validate route_idx in v2 header check; uniform events[x].type#1340
Merged
Conversation
The v2 header-validation helper compared interface_id and entry_id but silently ignored route_idx. For programs that mount the same interface at two different routes (RMRK-style routing, multi-tenant services, upgrade-in-place during cutover), an off-route reply or event passed validation when decoded by the wrong service instance. Per docs/sails-header-v1-spec.md, route_idx is a first-class routing identifier and 0x00 is the inference sentinel. The Rust client at rs/src/client/mod.rs:418-419,:872-873 already validates strictly; this brings JS in line. Asymmetric expected-only rule: throw when received != expected unless expected is 0 (the ctor decode path, which has no service route). This matches Rust's behavior for service paths and exempts ctors cleanly. decodeResult will inherit the same validation automatically once #1315 (fix/js-decoderesult-header) lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The v2 events map exposed `events[x].type` as a getStructDef *object* for named-field event variants but as a string for unit and tuple variants. Consumers that wanted a single string representation either had to special-case the named-field case or got `[object Object]`. The string form already existed in the same code path — `typeStr` at sails-idl-v2.ts:507 was computed and immediately discarded. This change just routes it to the public `type` field instead of the unused intermediate variable. BREAKING (runtime): consumers that did `events[x].type.fields` on named-field events will now get undefined. Migration: read `events[x].typeDef.fields` (already public, unchanged). The documented TypeScript type of `.type` is `any`, so this is not a compile-time break. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The same `events[x].type` object/string inconsistency exists at js/src/sails.ts:264 in the v1 Sails class — `getScaleCodecDef(type)` returns an object for non-tuple structs (js/util/src/types.ts:130-134), while `getScaleCodecDef(type, true)` stringifies. The intermediate `t` was never read elsewhere; removing it and routing `typeStr` to `events[x].type` matches the v2 fix one-for-one. Issue #1318 is titled "v2", but the v1 path has the identical bug. Same breaking note applies: consumers reading `.type.fields` on named-field events need to switch to `.typeDef`. Existing service.test.ts events test gains a named-field event variant (`Walked: struct { from: u32, to: u32 }`) — pre-fix it returned an object, post-fix it's a string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cleanups surfaced by /simplify pass over the #1316/#1318 fixes: - Replace local `concatBytes` helper and inline 3-line byte concatenation in both new test files with `u8aConcat` from `@polkadot/util`. - Hoist `new SailsProgram(parser.parse(idl))` from each test into `beforeAll`. The IDL is module-level and parses identically per call; re-parsing 5x per file was unnecessary work and kept the WASM linear memory clear loop hot. - Drop `(event.typeDef as any).entry_id` cast — `IServiceEvent` / `IEnumVariant` already declares `entry_id?: number`. - Trim the route_idx comment to just the spec citation; drop the redundant "skip equality for ctors" half (the code already says that). - Drop the regression-narrative comment on `service.test.ts:163` — the `expect(typeof ...).toBe('string')` line is self-documenting. No behavior change. 14 tests still pass across the three affected suites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements route index validation within the header matching logic, specifically allowing a sentinel value of zero for constructors to support inference. Additionally, it standardizes the representation of event types to ensure they are consistently returned as strings across both v1 and v2 IDL paths, resolving previous inconsistencies with named-field variants. Comprehensive test suites have been added to verify the new validation logic and the uniform shape of event types. I have no feedback to provide as the changes correctly address the requirements and include appropriate test coverage.
…o event predicates Codex review surfaced two follow-ups to the prior route_idx fix: 1. _assertMatchingHeader rejected received route_idx=0 when expected was non-zero. Per docs/sails-header-v1-spec.md §13.6, received route_idx=0 is the inference sentinel and must be accepted. Now both sides are symmetric: throw only when both are non-zero and disagree. 2. events[x].is() and the subscribe predicate matched on interface_id+entry_id only, so they returned true for the wrong service in multi-route programs while decode() then threw — an inconsistent surface. Both predicates now share the same matchesRoute() check used by the decode path. Tests added: - received route_idx=0 decodes successfully (function + event) - event.is() returns false for wrong concrete route_idx - event.is() returns true for inference sentinel route_idx=0
vobradovich
approved these changes
May 7, 2026
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
Two related sails-js bugfixes from open issues without linked PRs.
_assertMatchingHeadershould validateroute_idxfor multi-mounted services #1316 —_assertMatchingHeaderignoredroute_idx, so off-route replies/events for a multi-mounted interface decoded silently against the wrong service instance..typeis an object while unit/unnamed variants return strings #1318 —events[x].typereturned an object for named-field event variants and a string for unit/tuple variants. Consumers had to special-case named-field or got[object Object]. Same bug exists in v1 (js/src/sails.ts); fixed in both paths.Changes
ef074b87_assertMatchingHeader(v2) extended withroute_idxvalidation.3b02d760events[x].type(v2) routes the already-computedtypeStrto the public field; drops the unused intermediatet.608ae352js/src/sails.ts:264. Existingservice.test.tsevents test gains a named-field event variant.77549adau8aConcatfrom@polkadot/util, hoistparser.parse(idl)intobeforeAll, drop oneas anycast, trim two over-explanatory comments.5383b328received.routeIdx === 0as the spec's inference sentinel (was previously rejected when expected was concrete); apply the same route check toevents[x].is()and thesubscribepredicate so they no longer disagree withdecode()in multi-route programs.Decisions worth noting
route_idxrule (after Codex review): throw only when both sides assert concrete (non-zero) routes that disagree. Either side being0is the inference sentinel perdocs/sails-header-v1-spec.md§6.2 line 78 and §13.6 line 128 — accepted in both directions. This still catches the original sails-js:_assertMatchingHeadershould validateroute_idxfor multi-mounted services #1316 bug (RMRK-style off-route delivery) while preserving spec-compliant inference for legacy/single-instance receivers.is()andsubscribeparity: both predicates now sharematchesRoute()withdecode(), so the commonif (event.is(e)) event.decode(e)pattern is consistent. Without this,is()returned true for the wrong service whiledecode()threw — a confusing surface.decodeResultnot covered here: it doesn't call_assertMatchingHeaderonmaster. PR fix(js): validate reply prefix in decodeResult #1315 (fix/js-decoderesult-header) adds that call site; once it lands,decodeResultautomatically inherits this PR'sroute_idxvalidation..typeis an object while unit/unnamed variants return strings #1318 is a runtime breaking change for consumers reading.type.fieldson named-field events. The documented TS type isany, so it's not a TypeScript-level break. Migration: readevent.typeDef.fields(already public, unchanged).Tests
15 new test cases + 1 extended test case across 3 files. All pass:
Test plan:
decodePayloadrejects mismatched concreteroute_idxwith"Header mismatch"referencing both expected and got valuesdecoderejects mismatched concreteroute_idxroute_idx(asymmetric expected-only rule)route_idx === 0is accepted as the inference sentinel (function + event)event.is()returns false for wrong concreteroute_idx(consistent withdecode())event.is()returns true for inference sentinelroute_idx === 0route_idxdecodes successfully (positive case)events[x].typeis a string for unit, tuple, and named-field variants (v2)event.decode(...)after the fixevent.typeDef.fieldsescape hatch still worksservice.test.tsextended with named-field event — pre-fix returned object, post-fix returns stringReview
5383b328Closes #1316
Closes #1318
🤖 Generated with Claude Code