sdk-core: adopt wire-sysio table_id namespace isolation

[heifner]

Summary

Wire-sysio PR Wire-Network/wire-sysio#288 widened table_def.name from sysio::name (uint64) to free-form std::string and appended table_id (uint16, DJB2 hash of the table name % 65536) and secondary_indexes (vector<index_def>) for KV-table per-table namespace isolation. The wire format break means any sdk-core caller that loads a contract ABI from on-chain via get_raw_abi parses misaligned starting at the first table — name reads the wrong bytes, every subsequent field is shifted, and ricardian_clauses ends up reading garbage.

PR Wire-Network/wire-sysio#290 separately changed the get_table_rows HTTP response shape for KV-backed tables. Each row used to be the decoded struct directly (or {data, payer} when show_payer was set); the unified endpoint now returns {key, value, payer?} per row.

This PR takes a hard break on the binary format (no fallback to the legacy EOSIO 8-byte name encoding) and a backward-compatible unwrap on the HTTP response shape.

Companion PRs:

• KV table_id namespace isolation wire-sysio#288 — chain-side table_id namespace isolation
• Unified get_table_rows API: scope, find, index_name wire-sysio#290 — unified get_table_rows API
• Migrate production contracts from multi_index/singleton to kv::table wire-sysio#291 — sysio.token migrated to kv::scoped_table
• KV table_id namespace isolation wire-cdt#49 — kv::scoped_table + abigen secondary_indexes
• Update abi_def schema for wire-sysio table_id namespace isolation wirejs-native#4 — TS schema mirror for abi_def
• Adapt to wire-sysio table_id namespace isolation; long table name support abieos#12 — abieos C API + binary format update
• Adopt wire-sysio table_id namespace isolation; pass table names as strings node-abieos#8 — drops the lossy uint64 round-trip
• Adapt KV delta handler to wire-sysio table_id format hyperion-history-api#9 — KV delta handler + AbiDefinitions

Hard binary-format break

ABI.fromABI() now only parses wire-sysio binary ABIs (table_def.name as a string). Loading a binary ABI from a classical EOSIO chain via get_raw_abi would mis-parse — those chains still emit the legacy 8-byte name encoding. Two paths around this if needed:

1. Pin an older sdk-core version when targeting EOSIO mainnets
2. Use the JSON ABI path (ABI.from(jsonString)) for cross-chain code — JSON parsing is unchanged

Breaking type change: ram_payers entries may be undefined

GetTableRowsResponse.ram_payers widened from Name[] to (Name | undefined)[]. Wire-sysio KV rows return payer as an optional field; when it is absent, the wrapper now pushes undefined so the absent-payer case is explicit rather than silently coerced into Name.from(undefined) (which previously threw at runtime).

Downstream code that iterates ram_payers and calls methods on entries directly must now guard against undefined:

// Before — compiles and runs against non-KV chains
const payer = result.ram_payers![i].toString()

// After — type-safe and handles KV rows with missing payer
const entry = result.ram_payers?.[i]
const payer = entry ? entry.toString() : "<no payer>"

Legacy EOSIO responses with fully-populated payer fields are unaffected at runtime but will still need the guard to satisfy the compiler.

Changes

packages/sdk-core/src/chain/Abi.ts

• New ABI.Index interface ({name, key_type, table_id}) mirroring sysio::chain::index_def.
• ABI.Table gains optional table_id: number and secondary_indexes: Index[] fields. Optional so hand-built test fixtures and ABIs constructed programmatically don't need to specify them; the encoder defaults to 0 / [] when omitted.
• fromABI():
  • reads name as a length-prefixed string instead of Name.fromABI(decoder) (the 8-byte sysio::name path)
  • consumes table_id (uint16 LE) and secondary_indexes (vector<index_def>) after the existing type field
  • reads the trailing protobuf_types extension after enums; the drain loop is version-gated to sysio::abi/1.x so a future non-string extension fails safely rather than silently corrupting the decoded ABI
• toABI() mirrors the parser. The Table.name field is now typed string (was NameType) since the wire format carries a free-form string; the encoder writes it directly via encoder.writeString(table.name). The trailing empty protobuf_types is version-gated to sysio::abi/1. so round-tripping a non-1.x ABI stays byte-exact.
• readUint16LE / writeUint16LE helpers collapse the three duplicated sites (table + 2 in the secondary-index loop) into one path.

packages/sdk-core/src/api/v1/Chain.ts

get_table_rows() now detects the unified KV row shape (each row has both key and value keys, and key is itself a non-null object) and unwraps to the inner value before downstream processing. The legacy EOSIO shapes (decoded struct directly, or {data, payer} on show_payer) are left untouched.

const isWireKvShape =
  Array.isArray(rows) &&
  rows.length > 0 &&
  typeof rows[0] === "object" &&
  rows[0] !== null &&
  "key" in rows[0] &&
  "value" in rows[0] &&
  typeof rows[0].key === "object" &&
  rows[0].key !== null

Requiring key to be an object avoids misinterpreting user tables whose row struct happens to have scalar fields named key and value. A residual false-positive remains for user tables whose top-level struct contains a nested struct named key AND a field named value — that shape is pinned in a regression test so the behavior is explicit.

When show_payer is set on a KV row, payer is captured into ram_payers the same way as the legacy {data, payer} path. Missing payer fields push undefined rather than throwing (see the breaking-change note above).

Tests

22 test cases in the two touched suites; full sdk-core suite is 311/311 green.

packages/sdk-core/tests/chain/Abi.test.ts

• Round-trips a table with table_id + empty secondary_indexes
• Round-trips a long table name (>12 chars) — the whole reason name was widened
• Round-trips secondary_indexes with checksum256 key_type (the case the wire-cdt fix(eslint): identity + no-swallow error law; enhance shared NestedError #49 abigen fix unblocked end-to-end)
• table_id 0 and 65535 boundary values
• Missing table_id defaults to 0 on encode + decode
• protobuf_types extension is consumed without affecting enums
• Encoder always emits the empty protobuf_types trailer (round-trip validated, not a single-byte check)
• Encoder rejects table_id outside uint16 range (table + secondary index)
• Decoder drains multiple trailing string-typed extensions for sysio::abi/1.x (forward-compat)
• Decoder does NOT drain trailing bytes for unknown ABI versions (fail-safe for non-string future extensions)
• Golden bytes: minimal single-table ABI matches the expected byte-for-byte layout
• Multi-table ABI end-to-end round-trip (structs + actions + secondary_indexes)

packages/sdk-core/tests/api/v1/Chain.test.ts

• Unwraps the new {key, value} shape into plain rows
• Unwraps the new shape with show_payer + captures ram_payers
• Preserves legacy plain-row shape from EOSIO chains
• Preserves legacy {data, payer} show_payer shape from EOSIO
• Empty rows array works for both shapes
• Missing payer in new shape is reported as undefined
• Does not unwrap user table with scalar key + value fields (first-order false-positive guard)
• Does not unwrap user table whose key is itself an object (pins the known residual false-positive)

Out of scope

• packages/sdk-core/src/resources/{Ram,Rex,Powerup}.ts call get_table_rows for tables (rammarket, rex_pool, powup_state) that do not exist in wire-sysio and have not for some time. These resources were already dead on Wire chains regardless of any of the in-flight wire-sysio PRs and remain dead after this commit.
• packages/sdk-core/src/types/SystemContractTypes.ts is auto-generated and would benefit from a regen pass after sysio.token's migration to kv::scoped_table merges (#291), but the field schemas of the structs it generates do not depend on the binary table_def layout.

[dtaghavi]

Code Review

Overall this is a clean, well-scoped PR with clear motivation and good test coverage. The summary and companion-PR links make the context easy to follow. A few things worth discussing:

---

Chain.ts — KV shape detection

Potential false positive on the shape heuristic

const isWireKvShape =
  Array.isArray(rows) &&
  rows.length > 0 &&
  typeof rows[0] === "object" &&
  rows[0] !== null &&
  "key" in rows[0] &&
  "value" in rows[0]

Any legacy EOSIO table whose row struct happens to have fields named key and value will be incorrectly treated as the new KV shape. eosio.token's stat table ({supply, max_supply, issuer}) is safe, but contracts with generic key/value column names are not. A stronger discriminant—e.g. checking that rows[0].key is itself an object (composite key), or verifying the absence of other decoded fields—would tighten this. Even a comment acknowledging the known collision risk would help future maintainers.

show_payer without KV shape still throws on missing payer

The new KV path uses row.payer ?? "" (safe), but the legacy path still does:

rows = rows.map(({ data, payer }) => {
  ram_payers!.push(Name.from(payer))  // throws if payer is undefined
  return data
})

If a legacy node omits payer for a show_payer request, the behaviour is now asymmetric between the two paths. Low-risk in practice, but worth making consistent.

---

Abi.ts — Binary parser

canRead() only skips one trailing extension

if (decoder.canRead()) {
  decoder.readString() // discard
}

This consumes exactly one trailing field. If a future abi_def appends a second extension after protobuf_types, the parser will leave bytes on the decoder and either mis-parse subsequent fields or silently succeed with a garbage ABI. while (decoder.canRead()) { decoder.readString() } would be strictly more future-proof at essentially no cost.

table_id overflow is silently truncated

The encoder masks to uint16:

encoder.writeByte(tid & 0xff)
encoder.writeByte((tid >> 8) & 0xff)

A caller passing table_id: 70000 gets silently truncated to 70000 % 65536 = 4464. A runtime guard (if (tid > 0xffff) throw ...) or at minimum a comment would prevent subtle bugs in hand-built ABIs.

Manual byte assembly instead of a helper

tidLo | (tidHi << 8) appears three times (table + each index). If ABIDecoder grows a readUint16LE() helper later, this pattern would need updating in multiple places. Not a blocker, just worth noting.

---

Tests

encoder always emits protobuf_types test is brittle

expect(bytes[bytes.length - 1]).toBe(0x00)

This relies on an empty ABI happening to produce a trailing 0x00 byte, which only holds because the varuint encoding of length-0 is 0x00. If any other field is added after protobuf_types in the future this test will silently pass for the wrong reason. Round-tripping via decodeAbi(encodeAbi(abi)) and asserting on the decoded enums length (already done elsewhere) would be more robust.

No negative test for table_id > 65535

Given the silent-truncation issue above, a test asserting the truncation behaviour (or the error, if a guard is added) would pin the contract and prevent a future regression.

isWireKvShape false-positive case is not tested

A test with a legacy table having both key and value fields (e.g. {key: "primary", value: 42, owner: "alice"}) would document the current behaviour and catch any future change to the detection heuristic.

---

Minor / nits

• String(table.name) works but will stringify a Name object as its canonical string representation — worth a one-line comment confirming that's the intended coercion path, since it's the same value Name.toString() returns.
• The ABI.Index.table_id field is required, while ABI.Table.table_id is optional. The asymmetry is defensible (secondary indexes always have one on-chain) but a JSDoc note on Index.table_id would help callers building ABI objects programmatically understand why they can't omit it.
• out of scope section in the PR body is a nice touch — appreciated.

---

Summary

The binary-format break is clearly necessary and well-documented. The KV shape unwrap is backward-compatible and the test matrix is thorough. The main things I'd want addressed before merging are:

1. Harden or document the KV shape heuristic against the false-positive case.
2. Either guard against table_id > 65535 or document the truncation.
3. Consider while (decoder.canRead()) for the protobuf_types trailer.

Everything else is minor. Tests are solid and the companion-PR chain is clearly laid out.

[dtaghavi]

Follow-up Review (current diff)

The previous review was written against an earlier iteration. Several of those issues have since been addressed — the typeof rows[0].key === \"object\" discriminant was added, the drain loop was widened to while (decoder.canRead()), assertUint16 was introduced, and the legacy show_payer null-safety was fixed. This pass covers what remains.

---

Chain.ts — KV shape detection

Heuristic can still misidentify user tables with an object key field

The updated check correctly rejects scalar key values, but a user-defined table like {key: {name: \"alice\"}, value: 42} (where key is an embedded struct) still triggers the KV path and would be silently unwrapped. The existing test covers scalar key/value but not the object-key false-positive case. At minimum, a comment noting this residual risk (or a doc test demonstrating the known gap) would help future maintainers.

---

Abi.ts — Binary parser

while (decoder.canRead()) drain loop is fragile against non-string extensions

The comment already calls this out: "Non-string extensions added in the future will need explicit handling here." The risk is that a future abi_def extension using a non-string wire type (e.g. a varuint count followed by entries) would cause the drain loop to misread the length prefix as a string length and silently produce a corrupt ABI. A version string comparison (e.g. version >= \"sysio::abi/1.3\" → drain) would scope the drain to known-good versions and force an explicit update when the wire format is extended again.

Duplicated two-byte uint16 LE reads

const tidLo = decoder.readByte()
const tidHi = decoder.readByte()
const table_id = tidLo | (tidHi << 8)

This pattern appears three times (once for the table, once per secondary index). If ABIDecoder ever gains a readUint16LE() helper this will need three updates. A local inline helper or a note to consolidate if the decoder is extended would be low-cost.

---

Types.ts — Breaking type change

ram_payers?: (Name | undefined)[] is a public API break

The JSDoc comment on GetTableRowsResponse.ram_payers is clear, but any downstream consumer iterating ram_payers with Name calls (e.g. p.toString()) will silently get a runtime error once they hit an undefined entry from a KV row with a missing payer. This is expected given the wire-sysio shape, but worth a changelog entry or a migration note in the PR body so integrators know to audit their payer consumers.

---

Tests

No test for the object-key false-positive case

The does not unwrap user table that happens to have scalar key+value fields test covers {key: string, value: number}. A companion case with {key: {scope: \"alice\"}, value: 42} (object key, scalar value) would document whether the current heuristic is conservative enough for that shape or not.

encoder always emits protobuf_types test checks a single trailing byte

expect(bytes[bytes.length - 1]).toBe(0x00)

This is correct today but will silently pass for the wrong reason once any other zero-valued byte lands at the end. The golden-bytes test covers this more robustly — it may be worth dropping the single-byte assertion or replacing it with a decode round-trip + enums.length check.

---

Nits

• ABI.Index.table_id is typed number | undefined (optional ?) even though the chain always provides it. Marking it required (or at minimum adding a note that the encoder defaults to 0) would reduce ambiguity for callers building Index objects programmatically.
• The inline comment // Binary order matches sysio::chain::table_def. in fromABI is useful — worth mirroring the same note in toABI so the encoder is equally self-documenting.

---

Summary

The main items still worth addressing:

1. Object-key false-positive in isWireKvShape — add a test (and optionally a comment) documenting the residual case.
2. while (decoder.canRead()) fragility — consider scoping to a known version range or adding a forward-compat comment with a clear escalation path.
3. ram_payers breakage — add a migration note in the PR body or CHANGELOG for downstream integrators.

Everything else is minor. The test suite is thorough, the golden-bytes fixture is a great safeguard against silent wire-format drift, and the companion-PR chain is clearly laid out. Code is ready to merge once the above are discussed.

[dtaghavi]

Third-pass review — all prior feedback addressed

Kevin addressed every item from the two prior review passes. This is a clean final state. A few small observations below, none of them blockers.

---

What landed well

• readUint16LE / writeUint16LE helpers — the duplication is gone and the call sites are readable.
• Version-gated drain (version.startsWith("sysio::abi/1.")) — exactly the right scope. The test that appends [0xff, 0xff, 0xff, 0xff, 0x01] to a sysio::abi/2.0 ABI is a good proof that the gate prevents mis-parse rather than just silently skipping.
• The pinned false-positive test in Chain.test.ts (does not unwrap user table whose key is itself an object) — documenting known-bad behavior with a comment pointing to a future improvement path is the right call here.
• Index.table_id JSDoc — the "required on-chain, optional for hand-built" distinction is now explicit.

---

Minor observations (not blocking)

Encoder always emits protobuf_types regardless of version

toABI unconditionally writes encoder.writeString("") at the end. For an ABI with version: "sysio::abi/2.0", the encoded bytes will include the extension even though the decoder's version gate would skip draining it. In practice there is no sysio::abi/2.0 today so this is harmless, but a comment like // sysio::abi/1.x always ends with protobuf_types co-located with the writeString("") call would make the coupling explicit if the version format ever changes.

protobuf_types test still carries the single-byte assertion

expect(bytes[bytes.length - 1]).toBe(0x00)

The added round-trip check (verifying decoded.enums is preserved) significantly reduces the brittleness concern — the trailing 0x00 can now only be an empty string terminator rather than a coincidental zero. The golden-bytes test below it is still the authoritative pin. No change needed; just noting it's fine as-is.

No CHANGELOG / migration note for ram_payers type widening

GetTableRowsResponse.ram_payers changed from Name[] to (Name | undefined)[]. The JSDoc comment covers the intent clearly. If there's a CHANGELOG or BREAKING-CHANGES file in the repo it would be worth a one-liner there for integrators auditing their payer consumers — but if the project doesn't maintain one, the JSDoc is sufficient.

---

Verdict

All three major items from the prior reviews are resolved: the version-gated drain handles the fragility concern, readUint16LE/writeUint16LE collapsed the duplication, and the object-key false-positive is documented with a pinned regression test. Test count went from 304 to 309 and the golden-bytes fixture gives strong protection against silent wire-format drift.

Approved.

[dtaghavi]

Fourth-pass review — third-pass feedback fully addressed

All three items from the last review are resolved:

Prior concern	Resolution
Encoder always emits protobuf_types regardless of version	toABI is now version-gated (this.version.startsWith("sysio::abi/1.")) — symmetric with the decoder
Array-valued key field false-positive (typeof [] === "object")	!Array.isArray(rows[0].key) added; new test does not unwrap user table whose key field is an array pins the behavior
Golden bytes test used short version "v" (non-1.x)	Now uses "sysio::abi/1.2"; a companion golden bytes test for "sysio::abi/2.0" proves the trailer is absent for non-1.x versions

The WireKvRow type alias is a nice touch — it removes the inline any casts and documents the expected wire shape in one place.

---

Two minor nits (not blocking)

WireKvRow is defined mid-function, after the expression that uses it

const isWireKvShape = ...          // computed first
type WireKvRow = { ... }           // type alias follows
...
rows.map((row: WireKvRow) => ...)  // used here

TypeScript hoists type aliases so this compiles, but readers scanning the function top-to-bottom hit the reference before the definition. Moving WireKvRow to just above isWireKvShape (or to module scope) would make the order read naturally.

decoder skips protobuf_types test relies on the implicit default version

const original = new ABI({ enums: [...] })

The test comment says the encoder emits the trailer for sysio::abi/1.x, but doesn't specify a version, leaning on the default being sysio::abi/1.1. Adding version: "sysio::abi/1.2" explicitly — like the other tests in this file — would make the assumption visible and keep the test self-contained if the default ever changes.

---

Verdict

The encoder/decoder symmetry is now tight, the heuristic discrimination is as strong as it can reasonably be without ABI awareness, and the golden bytes fixtures give solid protection against future drift.

Approved.

[jglanz]

Looks good, you may need to rebase