Adopt wire-sysio table_id namespace isolation#10
Conversation
Mirrors Wire-Network/wire-libraries-ts#5 (packages/sdk-core). Binary ABI (hard break): table_def.name is now a length-prefixed string (was an 8-byte sysio::name uint64). table_id (uint16 LE, DJB2 hash of the table name % 65536) and secondary_indexes (vector<index_def>) are appended to each table entry. A protobuf_types trailing extension field is also read/written for forward compatibility. The ABI.Table interface gains optional table_id and secondary_indexes fields and a new ABI.Index interface. This mirrors wire-sysio Wire-Network/wire-sysio#288. get_table_rows response shape (back-compat): KV-backed tables on wire-sysio Wire-Network/wire-sysio#290+ return each row as {key, value, payer?} instead of the decoded struct directly. ChainAPI detects this shape and unwraps to expose just the value (and captures payer into ram_payers when show_payer is set), preserving the existing API contract for callers. EOSIO chains emitting the legacy flat or {data,payer} shapes are unaffected. Tests: test/abi.ts covers table_id round-trip, long names, secondary indexes, boundary values (0/65535), missing-field defaults, the protobuf_types trailer, and a multi-table end-to-end encode/decode. test/chain-api.ts covers the new KV shape unwrap, show_payer capture, and backward compatibility with both legacy row shapes.
dtaghavi
left a comment
There was a problem hiding this comment.
Overview
This PR adopts the wire-sysio table namespace isolation format (wire-sysio#288 / #290). It has two independent concerns:
- Binary ABI format change (
src/chain/abi.ts):table_def.namewidens from an 8-bytesysio::nameuint64 to a length-prefixed string;table_id(uint16 LE) andsecondary_indexes(vector<index_def>) are appended per table; aprotobuf_typestrailing extension is always read/written. get_table_rowsrow-shape detection (src/api/v1/chain.ts): Detects the new{key, value, payer?}KV shape from wire-sysio#290 and unwraps it, preserving the existing API contract for callers.
Test coverage is solid — round-trip, boundary values (0, 65535), missing optional fields, and all four row-shape combinations.
Issues
🔴 isWireKvShape detection is ambiguous
const isWireKvShape =
Array.isArray(rows) &&
rows.length > 0 &&
typeof rows[0] === 'object' &&
rows[0] !== null &&
'key' in rows[0] &&
'value' in rows[0];Any table whose row type happens to have both a key field and a value field (e.g. a KV-store metadata table, a settings table) will be silently misinterpreted. The heuristic looks at field names on the decoded JSON, so a table with struct {key: string, value: uint64} would have its rows incorrectly unwrapped.
A safer signal would be to check the ABI/table type being queried, or require the caller to opt in via a flag. At minimum the detection could require that key is itself an object (not a scalar) — in the wire-sysio shape it is always a composite scope+primary_key object — which would rule out tables with a scalar key field.
🟡 ABI.Table.name type is still NameType
export interface Table {
name: NameType; // was: sysio::name uint64; now: free-form string
...
}The comment explicitly says the field is now a free-form string that can exceed 12 characters, but the declared type is still NameType. If NameType is string | Name, this is technically fine, but it's misleading — Name enforces the 12-char/valid-charset constraint. The type should be widened to string to match the semantic change.
🟡 No range validation on table_id
table_id is typed as number (52-bit float range) but the wire format is uint16. A value > 65535 silently truncates on encode:
encoder.writeByte(tid & 0xff);
encoder.writeByte((tid >> 8) & 0xff);A guard (if (tid > 0xFFFF) throw ...) or at least a clamping comment would prevent silent data corruption for hand-constructed ABIs.
🟡 protobuf_types forward-compat is only one field deep
if (decoder.canRead()) {
decoder.readString(); // discard
}The comment says this allows "any subsequent extension appended to abi_def" to parse cleanly, but it only skips one field. If wire-sysio later appends a second extension after protobuf_types, this decoder will fail. A loop (while (decoder.canRead()) decoder.readString()) would be more robust, though it would still silently skip unknown data.
🟡 Missing payer falls back to Name.from('')
ram_payers!.push(Name.from(row.payer ?? ''));The test documents this as intentional, but an empty Name is a potentially surprising value. If the intent is "no payer recorded", undefined pushed into the array (or a sentinel) would make the absent-payer case explicit to callers.
Minor / Style
- The inline
// Wire-sysio PR Wire-Network/wire-sysio#288:comments in the binary decoder are unusually long for code comments. The why (format changed) is worth keeping; the GitHub PR references belong in the commit message or a CHANGELOG, not inline. idx.table_idin the secondary-index encoder has no?? 0fallback, which is fine sinceIndex.table_idis required — but there is a type inconsistency:Table.table_idis optional whileIndex.table_idis required. Worth making them consistent.- No test exercises decoding a real binary blob produced by wire-sysio. An integration-style golden-bytes test would give confidence that the byte offsets match the C++
table_defstruct layout exactly.
Summary
The core approach is sound and the backward-compat handling is careful. The main concern worth addressing before merge is the isWireKvShape ambiguity — a table with key/value field names today would silently misbehave. The NameType → string widening and table_id range guard are lower risk but worth fixing for correctness. Everything else is advisory.
Fix issues surfaced in review of PR #10: - Tighten isWireKvShape detection: require `key` to be a non-null object so that user tables with scalar `key`/`value` fields are not misinterpreted as the wire-sysio KV shape. - Report missing payer as `undefined` (not Name.from('')) in the new KV shape; widen ram_payers type to (Name | undefined)[] so the absent-payer case is explicit. - Widen ABI.Table.name type from NameType to string to match the widened table_def.name semantics. - Validate table_id and index table_id as uint16 on encode so out-of-range values throw instead of silently truncating. - Drain remaining string-typed trailing extensions in the decoder for forward-compat with future abi_def extensions (not just one). - Align optionality: Index.table_id is optional (matches Table.table_id) with encoder falling back to 0. - Trim verbose inline GitHub PR references from comments; keep the "why" (format changed). Tests: - Two new guards: out-of-range table_id and index table_id throw. - Forward-compat: decoder drains extra string-typed trailing fields. - Golden-bytes layout fixture catches silent byte-layout drift vs the C++ sysio::chain::table_def struct. - ChainAPI: missing payer reported as undefined; user table with scalar key+value is not unwrapped.
dtaghavi
left a comment
There was a problem hiding this comment.
Follow-up review — all issues addressed
The second commit ("Address PR #10 review feedback") resolves every item raised in the initial review. Quick confirmation:
| Issue | Status |
|---|---|
isWireKvShape ambiguity (scalar key/value fields) |
✅ Fixed: requires key to be a non-null object; new test exercises the scalar collision case |
ABI.Table.name still typed as NameType |
✅ Fixed: widened to string |
No table_id range validation |
✅ Fixed: ABI.assertUint16() throws on out-of-range; two new tests (65536, -1, index case) |
protobuf_types forward-compat only one field deep |
✅ Fixed: while (decoder.canRead()) drains all trailing string extensions; test appends two extra fields |
Missing payer coerced to Name.from('') |
✅ Fixed: pushes undefined; GetTableRowsResponse.ram_payers widened to (Name | undefined)[] |
| Verbose inline PR-ref comments | ✅ Trimmed |
Index.table_id required vs Table.table_id optional inconsistency |
✅ Fixed: Index.table_id is now optional, encoder falls back to 0 |
| No golden-bytes test | ✅ Fixed: pinned byte-layout fixture added |
One thing worth watching
The while (decoder.canRead()) decoder.readString() drain loop handles future string-typed extensions gracefully, and the comment correctly notes that non-string extensions would need explicit handling. That's the right acknowledgment for now — just worth keeping in mind when consuming future wire-sysio abi_def revisions.
This is in good shape. LGTM.
Summary
table_def.nameis now a length-prefixed string (was an 8-bytesysio::nameuint64). Each table entry gainstable_id(uint16 LE, DJB2 hash % 65536) andsecondary_indexes(vector<index_def>). Aprotobuf_typestrailing extension is read/written for forward compat. Mirrors KV table_id namespace isolation wire-sysio#288.get_table_rowsrow shape (back-compat): KV-backed tables on wire-sysio Unified get_table_rows API: scope, find, index_name wire-sysio#290+ return{key, value, payer?}per row.ChainAPI.get_table_rowsdetects this shape and unwraps to expose the value directly, preserving the existing API contract. Legacy EOSIO flat-row and{data,payer}shapes are unaffected.ABI.Tablegains optionaltable_idandsecondary_indexes; newABI.Indexinterface mirrorssysio::chain::index_def.Mirrors Wire-Network/wire-libraries-ts#5 which applied the same changes to the monorepo's
packages/sdk-core.