feat(chain): make slug_name a first-class ABI type - #619
Conversation
slug_name fields rendered as `{"value": <uint64>}` and now render as the decoded
slug. The carrier is dual: a value below 2^42 has no string spelling, so it
renders as the raw integer — keeping the conversion total and injective, and
never throwing, because only chain_code is bound to the proven source outpost,
so a non-canonical token_code is plantable and a throw would make a whole table
unreadable over get_table_rows.
The KV leaf and both key switches delegate to those conversions, so the carrier
exists in one place and key bytes are unchanged (the struct-node path recursed
one uint64 child to write_be64; the leaf path is write_be64). from_variant keeps
a transitional `{value}` arm because a slug is object-only without variant
conversions, so no JSON writer can straddle the cross-repo landing window; it
goes when no writer emits the object form.
Both test fixtures that declared a `slug_name` struct are renamed to
composite_key. abigen matches builtins on the bare name, so they would have been
emitted as the builtin, taken the leaf branch, and stopped exercising struct-key
expansion while still passing.
Change-Id: I8fd77d41b598210bcad0807eec7c97509eb150e6
…dary
fc::json quotes a uint64 above 0xffffffff (io/json.cpp:695), so the integer
carrier came back from a next_key cursor as a decimal STRING and hit
from_variant's validating string arm, which rejected it as too long. That broke
next_key -> lower_bound for every non-canonical slug key >= 2^32 — reachable
because token_code is a key field of sysio.reserv::reserves and, per this
type's own threat model, plantable. The struct carrier it replaced round-tripped
fine, so this was a regression, not a pre-existing gap.
Length disambiguates exactly: a canonical slug is at most 8 symbols, a
stringified uint64 past 0xffffffff is at least 10 digits. "12345678" still
parses as a slug, so `"7"` remains the slug 7 rather than the integer.
Also from review of the parent commit:
- sysio.epoch_tests was the last writer of the {"value":N} object form, so it
passed only through the transitional from_variant arm this change documents as
deletable. Converted to the string carrier.
- Delete the codename() test wrapper — 11 definitions, 2 lambdas, 305 call
sites. fc::variant already takes const char*, std::string and string_view
directly, and takes fc::slug_name through to_variant, so the wrapper was
identity. Three of its doc comments still described the {"value":N} carrier
this campaign replaced.
- get_table_test.hpp's tripwire comment named composite_key where it meant
slug_name, so it forbade the name the struct actually has — pointing the next
author at the rename that would restore the false green.
- get_table_tests.cpp asserted in the present tense that the struct fixture is
the sysio.chains key shape; that table keys on a leaf now.
New tests, each closing a gap a green suite was hiding:
- codename_tests: the JSON-text round trip (fc::json appeared nowhere in either
changed test file, so every case stopped at the variant layer), and the
writable input surface — const char*, std::string, string_view, "..."_s and an
fc::slug_name value all landing on one cell.
- abi_tests: the built_in_types registration, which had exactly one assertion
guarding it anywhere in the tree.
- be_key_codec_tests: multi-leaf slug keys. Three of the five registry tables key
on 2-3 slugs; every shape here was a single leaf, which cannot observe a
field-ordering or offset error.
unit_test 1535, plugin_test 295, contracts_unit_test 762, codename_tests 37.
Change-Id: I56856a431d42ca21cefe73174f53a9cf6707dda7
…lug_name
"v6" is not a version. wire-sysio is 1.0.0 with only v1.0.0 tags; the label
informally named the data-model revision that introduced the registry entities
and the slug_name rename, and it reads like a release nobody can resolve.
Removed from 88 of 90 mentions — comments, one ilog string, and a python
docstring — saying what is meant instead ("the data-model refactor", "the
registry tables", or the commit).
Two are deliberately kept, and a blind sweep would have broken both:
CHANGELOG.md's "Bump snapshot version to v6" is a real snapshot-format version
in historical record, and test_http_client.cpp's `tcp::v6()` is IPv6 in live
code.
Also:
- get_table_test's struct-key fixture no longer mentions slug_name. It exists
to drive struct-key expansion in the BE key codec and has nothing to do with
that type; the comment now states the hazard generally (a key struct named
after any ABI builtin is emitted AS the builtin and silently stops testing
struct expansion) rather than naming an unrelated type and preserving the
fixture's former name.
- Remove regchain_code_cell_renders_as_the_decoded_slug. It spent a full
contract deploy in the heaviest binary to assert row["code"].is_string(), a
property of abi_serializer and to_variant with no contract logic involved;
abi_tests::slug_name_builtin_type pins it against a synthetic one-field ABI.
- Restructure the be_key_codec key_shape comment. The sentence defining what a
key_shape IS had been spliced mid-paragraph into an explanation of to_key
routing, leaving the definition unfindable.
Change-Id: I7f3d339ad9bade25c4c7838ad64d69f24db0d409
…t precedence From a six-lane review of the previous two commits. from_variant's over-long string arm routed everything to as_uint64(), which goes through boost::lexical_cast — and that does NOT reject a sign for an unsigned target, it WRAPS. So "-12345678" was admitted as 18446744073697205938, and a JSON bound of that shape paged silently from the far end of a table where it had previously been a clean 500. Guarded with an all-digits check, which also restores the diagnostic: a non-numeric over-long string now falls through to the validating parse and reports `invalid slug_name '...': too long` again rather than `Couldn't parse uint64_t`. The number spelling had the same hole and the first fix missed it, leaving the two arms disagreeing — "-1" rejected as a string, accepted as a number. Both reject now. null/false/true still coerce through as_uint64 exactly as they do for every other uint64 key leaf; that is deliberate and now documented in the test, since 0 is a legitimate slug (the absent sentinel). Precedence: every shipped registry ABI carries a `slug_name` struct_def alongside the field, and `slug_name` is the ONLY builtin name so shadowed — symbol, symbol_code, name, asset and checksum256 appear in no structs[] entry. set_abi has no collision check, so those ABIs are genuinely ambiguous and are resolved only by lookup order. That was untested at BOTH resolution sites: - the action/row data path (built_in_types before structs) — abi_tests now declares the shadowing struct and asserts the serializer's own post-set_abi view, is_builtin_type && is_struct, which proves set_abi kept the struct rather than dropping it. Behavioural assertions alone could not discriminate: if the struct won, a string input would not render differently, it would fail to encode at all, since the struct branch throws pack_exception for a non-object input. - the key codec (leaf_kind_of before abi.structs) — its own case, on a fresh abi_def rather than make_test_abi(), whose avoidance of the name `slug_name` is deliberate and load-bearing for the struct-expansion and typedef-chain paths. `is_leaf` is the discriminator there: if the struct won, encode_key would demand the nested form. Also: two v6 mentions the earlier sweep missed, in .proto files its grep never covered (the same sweep removed the C++ mirrors of those exact section headers), and a sentence that sweep mangled in a docstring. contracts_unit_test 761, unit_test 1536, plugin_test 295, codename_tests 38. Change-Id: I70d61911aa5f39d34c64b143265fd21c8f6d4d22
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Found two correctness issues, detailed inline. The ABI/key-codec path, generated fixtures, and byte compatibility otherwise look consistent in targeted review.
| // arm above already rejects "-12345678". | ||
| if (v.is_int64() && v.as_int64() < 0) | ||
| slug_name_traits::throw_invalid(std::to_string(v.as_int64()), "negative"); | ||
| s = slug_name{ v.as_uint64() }; |
There was a problem hiding this comment.
[P2] Validate every numeric carrier before converting
This guard only covers a top-level int64, but as_uint64() also accepts doubles and the transitional object arm returns before reaching it. Today JSON -1.0 becomes UINT64_MAX, 1.5 becomes 1, and {"value":-1} / {"value":"-1"} also wrap to UINT64_MAX. The malformed object is accepted by ABI packing and the BE bound codec, so this bypasses the exact protection against paging from the far end. Please route every raw numeric carrier through one checked unsigned decoder (including nested value) and reject negative, fractional, and out-of-range inputs; add regressions for the double and object forms.
There was a problem hiding this comment.
You were right, and the object arm was still bypassing the guard after the carrier rewrite. Fixed in 459de80.
Confirming the mechanism against variant.cpp's as_uint64: it coerces where this needs to validate — int64_type does static_cast<uint64_t>(-1) → UINT64_MAX, double_type truncates (1.5 → 1), bool_type/null_type give 0/1, and string_type routes through fc::to_uint64 → boost::lexical_cast, which does not reject a sign. Since from_variant feeds encode_field, a bound of {"value": -1} encoded be64(UINT64_MAX) and paged from the far end of the table, exactly as you described.
The arm now goes through one checked unsigned decoder (detail::checked_packed_value): uint64 taken as-is, int64 rejected when negative, string required to be all digits (so lexical_cast throws on overflow instead of wrapping), and every other variant type — double, bool, null, array, object — rejected outright.
Two notes on the shape of the fix:
- The top-level numeric arm is gone entirely rather than validated. The carrier is now the canonical string only, so
-1,-1.0and1.5are refused at the type check before any decode. A JSON number is also refused rather than coerced, because the alphabet contains digits —"123"is itself a canonical slug whose packed value is nothing like 123. - Canonicality is deliberately NOT required inside the object arm. A value with no spelling is precisely what the string carrier cannot express, so that arm is the only way to name such a key as a
lower_bound. Requiring canonicality there would remove the ability to page past a planted row.
Regressions added in codename_tests (38 cases now), covering the double and object forms you asked for plus null/bool/empty/overflow: {"value": -1}, {"value": -12345678}, {"value": "-1"}, {"value": "+1"}, {"value": 1.5}, {"value": -1.0}, {"value": "1.5"}, {"value": "1E3"}, {"value": null}, {"value": true}, {"value": ""}, and a 25-digit overflow — with {"value": 7}, {"value": "7"} and a canonical packed value still accepted.
Suites on the fix: unit_test 1536/1536, plugin_test 295/295, contracts_unit_test 761/761, codename_tests 38/38, abi_tests 65/65, be_key_codec_tests 19/19, get_table_tests 25/25.
| /// the spelling: a canonical slug is at most max_len symbols and a stringified | ||
| /// uint64 past 0xffffffff is at least 10 digits. The carrier could NOT have | ||
| /// been a numeric string chosen freely — the slug alphabet contains digits, so | ||
| /// `"7"` is itself a valid canonical slug. |
There was a problem hiding this comment.
[P2] Keep canonical numeric-looking slugs unambiguous in the merged consumer
This PR intentionally emits "7" for the canonical slug, but the already-merged prerequisite wire-tools-ts#99 calls Number(value) before SlugName.from(value) for every string (and pins "101" as decimal). As a result packed "7" (149533581377536) is read as 7; valid slugs such as "1E3" and "0X10" are also interpreted as JS numeric syntax. Registry actions allow these codes, and the decoder feeds reserve/UWREQ/balance row filters, so matching rows silently disappear. Please update the merged consumer to mirror the carrier rule documented here—short bare strings are canonical slugs, while the long decimal escape and legacy {value} object remain numeric—before merging this PR.
There was a problem hiding this comment.
Done — the merged consumer is fixed and on master: wire-tools-ts#101, merged as bd2745e0.
Your diagnosis was exact, and it survived a change in this PR that made the carrier canonical-only (the long decimal escape is gone; a bare string is now the ONLY top-level carrier). slugValue's string arm ran Number(value) first and fell back to SlugName.from only on NaN, so:
"ETH" -> SlugName.from("ETH") ✓ "7" -> 7 ✗ (should be 149533581377536)
"WIRE" -> SlugName.from("WIRE") ✓ "101" -> 101 ✗
"" -> 0 ✓ "1E3" -> 1000 ✗
"0X10" -> 16 ✗
decodeSlugString's own JSDoc had already conceded the ambiguity was unresolvable by string-sniffing and named the fix — "split this decoder by the field's ABI carrier, after which this helper and both string arms below are deleted" — so that is what #101 does. A bare string is parsed as a slug; the transitional { value } wrapper holds a packed u64 and stays numeric, and now rejects anything but an unsigned decimal, mirroring this PR's own checked_packed_value so both sides of the wire refuse the same shapes.
One thing worth flagging, because it is not fixed by merge ordering and nearly shipped broken: I classified all 44 slugValue call sites against the contract headers. All but one read a sysio::slug_name row field (opreg::balances/wtdwqueue, uwrit::locks/uwreqs, reserv::reserves, tokens) — the {value} wrapper before this PR, a canonical string after, so both arms cover them either way. The exception is flow-batch-operator-termination, which reads entry.action.chain_code off operators.recent_actions, i.e. std::vector<opp::attestations::OperatorActionLog> — protobuf, where chain_code is uint64 (attestations.proto field 7) and reserve_code is uint64 (field 9). Those never render as a spelling; they render as a number, or a quoted decimal past 0xffffffff, which every real chain code exceeds. They stay uint64 regardless of this PR, so they get their own packedSlugValue.
The general rule that falls out: there are TWO code carriers, and only the field's DECLARED type distinguishes them — a decimal spelling and a digit-only slug spelling are identical by shape, so any shape-sniffing decoder is guessing.
#101 is therefore order-independent: CI green, slugUtils 17/17, full cluster-tool suite 1505/1506 (the one failure is an unrelated stale sibling build on my host, absent in CI).
to_variant renders the canonical spelling ("" for zero) and throws for a
value below 2^42, which zero_terminates leaves with no spelling at all;
from_variant takes that string plus the transitional {value} object and
refuses everything else. The integer arm, its JSON-text length re-route,
and the all-digits and negative guards that patched it are all deleted.
One carrier means a caller writes a slug field exactly one way and a
reader never branches on the JSON type. The throw is safe because
get_table_rows already wraps each row's key decode and value render in
its own try/catch, so a planted non-canonical row costs one cell.
Adds the slug_name builtin to generate-sysio-contract-types.py, which had
no entry and resolved only through the struct_def the ABIs still ship —
every slug field would have become `unknown` once abigen stops emitting
it. Adds get_table_tests (sec-11): a kv table keyed on slug_name, named
after the builtin on purpose so the leaf branch runs end to end.
Change-Id: I9351990c0aad6127139f23c09f5ce3b50b85bbcd
as_uint64 coerces where this needs to validate: it wraps a negative int64
to UINT64_MAX, truncates a double, turns null/bool into 0/1, and its string
path ignores a sign. from_variant feeds encode_field, so a bound of
{"value": -1} encoded be64(UINT64_MAX) and paged from the far end of the
table. Canonicality stays unrequired there — a value with no spelling is
the one thing the string carrier cannot express, so that arm is the only
way to name such a key as a bound.
Addresses the P2 on #619.
Change-Id: Ie1c552d2d9a480d3faf961bf02e4f3281845bd78
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
The two findings from my previous pass are addressed. This pass found two reachable availability regressions in the replacement string-only/throwing carrier, plus one live consumer that still assumes the old object shape. Details inline.
| const std::string text = s.to_string(); | ||
| // `pack` is the non-validating encoder, so this is a pure round-trip test: | ||
| // the value is canonical exactly when its own spelling recovers it. | ||
| FC_ASSERT(slug_name::pack(text) == s.value, |
There was a problem hiding this comment.
[P1] Keep rendering total for stored raw slugs
The get_table_rows catch does not contain this throw for in-process consumers. When binary_to_variant hits one noncanonical field it replaces the entire row value with raw bytes; values_only then exposes that blob, while the underwriter scans call row.get_object() unconditionally, so the outer scan_cycle catch drops every pass. This state is attestation-plantable without using this JSON decoder: dispatch_operator_action constructs token_code directly from the protobuf uint64 and depositinle persists it in an operator balance without a canonicality or registry check; createuwreq likewise stores raw token/reserve codes and intentionally admits the missing-reserve/zero-quote case. A value such as 7 can therefore stop processing of every legitimate request. Please keep binary rendering total—for example, emit the already-supported, type-disjoint {"value": } object for values without a spelling—or reject/contain them before persistence, and add a values_only/underwriter regression.
There was a problem hiding this comment.
You were right on every element of this, and the PR description was wrong. I had published "the throw is contained by construction" twice — in this header's doc comment and in the description — and the containment argument only ever covered get_table_rows. I've deleted both.
Confirming your mechanism against source, because it is worse than one cell:
get_table_rowsdoes fall back to hex per-cell — key atchain_plugin.cpp:2875, value at:2887,collect_next_keyat:2926.underwriter_pluginhas no such boundary.scan_cycle()istry { do_scan_cycle(); } FC_LOG_AND_DROP()(underwriter_plugin.cpp:1060-1064) — onetryaround the entire pass — anddo_scan_cyclereaches 17 unconditionalget_object()calls, 10 of them directly on a scanned row. Withvalues_onlyexposing the raw-bytes blobbinary_to_variantsubstitutes for a row whose field failed to render, theget_object()throws and the cycle is dropped. One unspellable code stalls every underwriter commit, exactly as you said, and it is plantable through the protobufuint64paths (dispatch_operator_action→depositinle,createuwreq) without this decoder in the loop at all.
Where I've landed differs from your suggested remedy, and I want to be explicit about it rather than quietly not doing it.
A type-disjoint second carrier is not the fix; validating at the proto boundary is. The second carrier keeps rendering total but leaves the unspellable value stored — so it still reaches the underwriter scan, the outpost mirrors, and the contract's own reads, where no carrier choice helps. It also re-creates the ambiguity this PR exists to remove: a decimal and a spelling are indistinguishable by JSON shape, so any field accepting both makes the reader guess.
What makes the boundary check exact is new in 5415b86f7b: a code must start with a letter. leading_alphabet = [A-Z], so no legal code can be spelled like a number, and "is this uint64 a code?" collapses to one call — slug_name::is_valid_literal. Digits and _ stay legal after the first position (V1, USDC, TRAIL_). It costs nothing real: every slug spelling used anywhere in the platform already starts with a letter, and the only non-conforming ones in the tree were 7 test-only occurrences. It is an optional traits member behind a concept, so sysio::name is untouched — name_tests 15/15 pins that.
On sequencing: this follows #619 rather than gating it. The check belongs at sysio.msgch's dispatch path and the opreg/uwrit/reserv writers — i.e. with the field conversions in landing 4, where those writers live. Nothing is deployed and no contract holds data, so there is no value to plant in the interval; and landing the carrier first is what lets the boundary validate against one known-good spelling rule instead of reconciling two. If you'd rather it land inside this PR I'll move it, but it would mean touching the contract writers from a libfc-only change.
| // throws; get_table_rows catches per row and falls back to hex. | ||
| const fc::slug_name s{ r.read_be64() }; | ||
| fc::variant v; | ||
| fc::to_variant(s, v); |
There was a problem hiding this comment.
[P2] Preserve a reusable JSON cursor for raw slug keys
Falling back to hex after this throw does not preserve pagination under json=true. If an unspellable slug is the page cursor, collect_next_key (and the secondary-index equivalent) returns bare hex, but the next request parses that value as JSON and encode_key requires an object, so the documented pass-next_key-back-as-the-bound flow fails. Raw reserve/token keys are plantable through the uint64 attestation paths. Please emit a lossless JSON object carrier for unspellable slug leaves, or otherwise make the fallback explicitly round-trip under json=true, and add forward/reverse pagination coverage with a raw slug at the page boundary.
There was a problem hiding this comment.
Confirmed, and the inbound half of what you're asking for is already here by design — it's the outbound half that is still hex.
from_variant's transitional object arm accepts {"value": <uint64>} and deliberately does not require canonicality, precisely so an unspellable key can still be named as a bound; that is documented in the header as the reason the arm exists independently of the abigen window. So encode_key already round-trips the object form. What collect_next_key emits for such a leaf is bare hex (chain_plugin.cpp:2926), which is the break you describe: hex goes back out, JSON parsing gets a string, encode_key wants an object.
Making collect_next_key emit {"value": N} for an unspellable leaf closes it, and it is small. I've put it in the same follow-up as the proto-boundary validation from the P1 thread rather than in this PR, for one reason worth stating: once the boundary validates, an unspellable slug cannot be stored, so it cannot become a page cursor. The two changes are the same defect seen from two ends — the cursor fix makes the state recoverable, the boundary check makes it unreachable — and landing them together is what lets the pagination coverage you're asking for assert the stronger property (no such row exists) instead of the weaker one (we page past it).
The leading-letter rule added in 5415b86f7b is what makes that boundary check exact — [A-Z] in the first position only, so no legal code is spellable as a number and is_valid_literal is the whole predicate.
Flagged in the description's out-of-scope section so it isn't lost. If you want the collect_next_key object carrier plus forward/reverse boundary pagination coverage in this PR instead, say so and I'll add it here — it's self-contained in chain_plugin and doesn't depend on the contract work.
| 'int128': 'string', 'uint128': 'string', | ||
| 'float32': 'number', 'float64': 'number', 'float128': 'string', | ||
| 'name': 'string', 'string': 'string', 'bytes': 'string', | ||
| 'name': 'string', 'slug_name': 'string', 'string': 'string', 'bytes': 'string', |
There was a problem hiding this comment.
[P1] Update the remaining chain-table reader before switching the carrier
The merged wire-tools-ts#101 fixes slugValue, but one live consumer bypasses it: ExternalOutpostSteps.ts:399-407 still maps each getChains() row with String(code.value). getChains() is only a typed cast over the RPC response, so after this change outpost.code is the bare string "ETH"; .value is undefined, every expected code becomes "undefined", and runOutboundEnvelopesQueued can never match the packed outenvelopes.chain_code values, so external-outpost bootstrap times out. Its unit fixture at ExternalOutpostSteps.test.ts:147-165 still pins {code:{value}}, which is why #101 did not expose this. Please route this reader through slugValue(outpost.code) and update the fixture to cover the new string carrier (plus the legacy object during rollout) before landing #619.
There was a problem hiding this comment.
Fixed — wire-tools-ts#103 (51f61a54), open and mergeable.
Your diagnosis was right, and it surfaced something #101 had missed entirely: the two codes on that gate arrive in different carriers, so one decoder cannot serve both.
outpost.codefromgetChains()is aslug_nameABI field → after this PR it is the bare string"ETH", soString(code.value)yields"undefined". NowslugValue(outpost.code).row.chain_codeonoutenvelopesis a protobufuint64, not aslug_namefield. It never renders as a spelling — it arrives as a JSON number, or as a quoted decimal once the value exceeds0xffffffff, which every real chain code does ("ETH"is23373212024832). Routing it throughslugValuewould read that decimal as a spelling. NowpackedSlugValue(row.chain_code).
That split — by the field's declared type, not by sniffing the string — is the only thing that can distinguish them, and it is why #101 grew packedSlugValue as a separate function rather than widening slugValue. The comparison is now packed-number to packed-number on both sides, and expectedLabel renders via SlugName.toString.
The fixture was the reason this stayed invisible, so it no longer pins one shape: ExternalOutpostSteps.test.ts is parameterized over a SlugCarrier identity enum with it.each, covering the string carrier and the legacy {value} object during rollout. 30/30 in the package; -t carrier gives 2 passed / 11 skipped.
One note on this thread's premise, since 5415b86f7b changed it: a code must now start with a letter, which makes the two carriers shape-disjoint — a spelling always starts [A-Z], a packed decimal always starts [0-9]. The declared-type split stays correct and stays the authority, but it is no longer the only thing standing between these two readings.
Makes the string carrier unambiguous by construction: no legal code can be spelled like a number, so a bare JSON string is always a code and never a decimal. Digits and '_' stay legal in every position after the first. leading_alphabet is an OPTIONAL traits member, so sysio::name is unaffected. Change-Id: I0b19b790af92ed20b48b7a8c17a71ce9c583ada5
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
The latest head is still not approvable. The acknowledged underwriter-liveness and json=true cursor failures are deferred rather than fixed, and wire-tools-ts#103 is still open, so current master still has the old code.value reader. The new leading-letter commit also leaves the host, contract, SDK, and generated-schema rules inconsistent; details inline.
| // an ambiguity no reader can resolve from the value alone. Digits and '_' | ||
| // remain legal in every position after the first ("V1", "USDC", "TRAIL_"). | ||
| // The empty string is unaffected: it is the zero sentinel, not a spelling. | ||
| static constexpr std::string_view leading_alphabet{ "ABCDEFGHIJKLMNOPQRSTUVWXYZ" }; |
There was a problem hiding this comment.
[P1] Enforce this invariant on every producer before relying on it
This rule currently exists only in host fc::slug_name. The active contract mirror still accepts digit- and underscore-leading spellings in both its runtime constructor and s literal, and binary action/protobuf paths deserialize raw values without validating the spelling. The current SDK likewise accepts and tests leading "0" and "". Those producers can therefore still persist a packed value such as "7", while this PR's to_variant now asserts on it; in a values_only underwriter scan, the fallback becomes a scalar blob and the unconditional row.get_object() drops the entire scan cycle. A later landing does not make this head internally safe. Please land the contract/raw-writer validation and coordinated SDK rule before relying on the throwing renderer, with a regression proving such a value cannot be stored, or keep rendering total until that enforcement is present.
There was a problem hiding this comment.
You were right that a later landing does not make this head safe, and right that the rule reached only some producers. Both are fixed here rather than deferred — 9f899338c8 (wire-sysio) and e52be2ca (wire-cdt #119), which now merge together.
The contract mirror is deleted, not synced. That is the part I had wrong when I answered your P1 on the other thread: I described this as needing the rule added in more places. It did not — contracts/sysio.opp.common/include/sysio.opp.common/slug_name.hpp was a fourth hand-written implementation (its own constructor, its own alphabet table, its own six comparison operators, its own _s), compiled into all six registry contracts. It is gone; the ten includes point at <sysio/slug_name.hpp>. Same type name, so none of the 299 sysio::slug_name uses changed. The contracts now inherit the leading rule instead of needing their own copy of it, and there is no fourth rule left to drift.
The raw-writer validation is in. chain_code was already proven — source_chain_binding_ok binds it to the delivering outpost — but token_code / reserve_code ride the forgeable payload and reach slug_name through the non-validating raw constructor, exactly as you said. Seven sites in sysio.msgch now gate on the code having a canonical spelling and DROP the attestation otherwise (dispatch_operator_action, dispatch_underwrite_commit, dispatch_reserve_create, dispatch_reserve_create_cancel), built as a sibling of source_chain_binding_ok — same [[nodiscard]] bool, same diagnostic, never check(), since a check there would halt evalcons.
The regression you asked for is sysio_dispatch_tests/dispatch_drops_uncanonical_token_code: it delivers a DEPOSIT_REQUEST carrying token_code = 7, asserts the delivery still succeeds (dropped inside dispatch, not reverted) and that the operator's balance vector is unchanged — i.e. the value cannot be stored, so no row can later fail to render.
The coordinated SDK and schema rules landed too: sdk-core in wire-libraries-ts#83 (b1c5ab2) — on the write path only, because toString is a reader and a throwing renderer is the very failure you identified — and the generated-schema pattern in the P2 thread below.
Deleting the mirror exposed three things that were invisible while it existed, which is the strongest argument that deleting it was the right call rather than syncing a fourth copy:
- abigen never treated
slug_nameas a builtin. It was declaredusing slug_name = basic_name<slug_name_traits>, and a comment asserted the builtin match would suppress emission. An alias never reaches that match — abigen resolves it to the underlying template and emits a typedef plus a struct_def for the instantiation, and the host then rejects the ABI outright withduplicate_abi_type_def_exception: type already exists 'slug_name'. That took out 523 of 762 contract tests the first time the contracts actually compiled against CDT's type.sysio::nameescapes it only by being a derived struct;slug_namenow is one too. add_structdescribed a builtin's base, leaking an orphanbasic_name_slug_name_traitsstruct_def into five ABIs. Guarded ine52be2ca.namewas escaping this by luck — nothing force-adds it today, but the kv-key path would have leaked its base the moment a table were keyed on a barename.- A consensus-halting abort on untrusted input.
parse_wire_account_nameexists precisely to keepname's constructor from aborting insideevalcons— and it constructed the name first, then ran its own round-trip check. With the constructor stricter,"underwriter."aborted the whole delivery instead of being logged asinvalid_wire_account. It now asksname::is_valid_literalbefore constructing.is_valid_name_string— a second hand-rolled mirror of the same rules, used bysysio.dclaimfor the same abort-avoidance — is deleted for the same reason.
Verification on the merged head: contracts_unit_test 760/760, unit_test 1537/1537, plugin_test 295/295, test_fc 656 with only the 4 pre-existing test_http_client.cpp failures, wire-cdt ctest -L unit_tests 32/32.
Still open and not claimed as fixed: the json=true cursor in the P2 thread above. Once nothing unspellable can be stored it cannot become a page cursor, but collect_next_key still emits bare hex for such a leaf and I would rather leave that thread open than close it on an argument.
| # entry must precede the structs lookup: every registry ABI still ships a | ||
| # `slug_name` struct_def, and without this the field would resolve to that | ||
| # `{value: uint64}` shape (or, once abigen stops emitting it, to `unknown`). | ||
| 'slug_name': {'type': 'string', 'pattern': '^[A-Z0-9_]{0,8}$'}, |
There was a problem hiding this comment.
[P2] Mirror the leading-letter rule in generated schemas
This pattern still accepts "7", "LEAD", and other spellings the new fc::slug_name parser rejects, so generated schema validation now disagrees with the ABI serializer. Please encode the new grammar while retaining the empty sentinel—for example, ^(?:[A-Z][A-Z0-9]{0,7})?$—and add generator/schema coverage for both invalid leaders and the empty value.
There was a problem hiding this comment.
Applied exactly as you wrote it — ^(?:[A-Z][A-Z0-9_]{0,7})?$, in 9f899338c8.
The optional outer group is the part I'd have got wrong on my own: it keeps the empty string valid, which matters because "" is the zero sentinel and every registry row that has never been assigned a code renders as it.
Verified against the domain the type now accepts and refuses:
accept: "" ETH USDC V1 TRAIL_ Z1234567 Z_______ ABCDEFGH
reject: 7 101 1E3 0X10 _LEAD 12345678 eth ABCDEFGHI
That is the same accept/reject split as fc::slug_name, so the generated schema and the ABI serializer now agree by construction rather than by inspection.
The contracts carried their OWN slug_name -- a fourth implementation beside the
host, CDT and sdk-core -- so the leading-letter rule reached three of four. It is
deleted; the ten includes point at <sysio/slug_name.hpp>, which carries the rule.
Same type name, so no call site changes.
A code arriving in a forgeable payload field reaches slug_name through the
non-validating raw constructor. chain_code is proven by source_chain_binding_ok,
but token_code / reserve_code are not, so msgch now drops an attestation whose
codes have no canonical spelling -- never check(), which would halt evalcons.
basic_name gets one validation algorithm shared with CDT: validity_error() backs
both is_valid_literal() and the constructor, which previously disagreed. That
disagreement was real -- "abcdefghijklm"_n compiled as abcdefghijkl2 while
name{"abcdefghijklm"} threw, and the same for a trailing pad. Both paths now
agree, pinned by name_tests::literal_and_runtime_validation_agree.
parse_wire_account_name asks name::is_valid_literal before constructing: it built
the name first and checked after, so a stricter constructor aborted the whole
evalcons delivery. is_valid_name_string, a hand-rolled mirror of the same rules,
is deleted for the same reason.
The generated-schema pattern gains the leading rule so it agrees with the ABI
serializer.
Change-Id: I617dcb6abcf2ba2d7c79863a73fd5a6d758a9b50
Artifact-only. sysio.epoch has no source change in this branch; its wasm moves because the contracts now build against the CDT carrying the shared basic_name, reached via <sysio/sysio.hpp>. Separated from the source commit so the toolchain delta is visible rather than folded into an unrelated diff. Change-Id: Ia9e62c39869993c53299d7287ca54fd08e77d6bb
…bi-builtin Contracts rebuilt against the merged tree; the only ABI delta against master is the slug_name struct_def the builtin replaces. Change-Id: I2d8aee9d2d40736c7ad230d84af1f809366e17a7
No source change here: it includes <sysio/name.hpp>, which #119 moves onto the shared basic_name. Change-Id: Ie441e50995c45d7d7d2b706347e442f66ad38b72
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
The carrier and schema fixes are consistent, but two persistence gaps remain. The first keeps the underwriter liveness defect permissionlessly reachable. The previously reported json=true cursor failure also remains live while raw slugs can still be stored. Please also refresh the landing notes: wire-tools-ts#103 and wire-cdt#119 are still open, and this head now imports the CDT header supplied by #119.
| /// consensus. | ||
| /// | ||
| /// `path` labels the dispatch path in the diagnostic. True iff every code is canonical. | ||
| [[nodiscard]] bool payload_codes_canonical(std::initializer_list<sysio::slug_name> codes, |
There was a problem hiding this comment.
[P1] Validate SwapRequest codes before persisting the UWREQ
This gate still misses ATTESTATION_TYPE_SWAP_REQUEST: dispatch_attestation forwards its opaque bytes directly to createuwreq, which constructs the source/target token and reserve slugs from raw protobuf uint64s. Its missing/inactive-reserve zero-quote path intentionally continues and persists the request, so a swap with a valid target chain but token/reserve value 7 still creates an unrenderable row. Rendering that row makes values_only expose the fallback scalar, and the underwriter's unconditional row.get_object() drops the entire scan cycle. Validate the decoded SwapRequest codes inside createuwreq before lookup/persistence, emit SwapRevert so the source deposit is refunded, and add a SWAP_REQUEST regression; the new test only covers OperatorAction.
There was a problem hiding this comment.
You were right, and the gap is structural rather than a miss in the gate: msgch's dispatch arm forwards SWAP_REQUEST to uwrit::createuwreq as opaque bytes, so none of the seven payload_codes_canonical sites can see it.
Fixed in 444962ccca. createuwreq now validates the four payload codes immediately after the provenance check and emits SwapRevert — refund, not drop, exactly as you said, because the deposit is escrowed on the source outpost and a silent skip would strand it. emit_swap_revert packs raw .value uint64s, so the revert still works when the offending code is the unspellable one.
Your reading of the zero-quote path was exactly right, and I verified it rather than assuming. Removing the new guard and re-running the regression shows the row IS created, and that reading it back throws inside the depot's own ABI unpack:
unpack_exception: Unable to unpack built-in type 'slug_name'
while processing 'uw_request_t.dst_token_code'
s.is_canonical(): slug_name 7 is not a code and has no string spelling
required_reserves_active is false when the reserve is MISSING, so the fail-closed guard never fires and reqs.emplace stores the code verbatim.
The regression is sysio_dispatch_tests/swap_request_uncanonical_code_is_refunded: a bad target token and a bad source reserve, both with a valid target chain — your case — plus a canonical control proving the guard rejects the code rather than the shape of the request.
I validate four codes, not six. The two chain codes are covered by the registry: src_chain_code is bound to the proven delivering outpost, dst_chain_code must pass chain_registered_active, and the P2 fix below makes a registered code a renderable one by construction. That dependency is why both landed together.
| /// the way a reader can. Delete this arm once no writer emits the object form. | ||
| inline void from_variant(const fc::variant& v, slug_name& s) { | ||
| if (v.is_object()) { | ||
| s = slug_name{ detail::checked_packed_value(v.get_object()["value"]) }; |
There was a problem hiding this comment.
[P2] Guard raw object values at persistence writers
This object arm deliberately accepts noncanonical raw values so a bad key can still be named as a bound, but it is also used to pack action JSON. The privileged regchain, regtoken/regctok, bootstrap regreserve, and opreg setconfig writers then persist those slugs without is_canonical() checks. Thus a legacy-shape administrative input with value 7 can create state that to_variant cannot render; bootstrap registry rows may be irreversible and can trigger the same values_only/cursor failures. Keep this raw arm for bounds, but reject noncanonical codes in every action that originates persisted state and add regression coverage.
There was a problem hiding this comment.
Confirmed and fixed in 444962ccca. Before this, is_canonical() appeared exactly once in the entire contracts tree — the msgch gate — and none of the writers you named checked anything.
New sysio.opp.common/registry_codes.hpp carries one check_codes, wired into regchain (code), regtoken (code), regctok (chain+token), regreserve (chain+token+reserve), and setconfig (both codes in every chain_min_bond across all three vectors). These are privileged top-level actions rather than dispatch handlers, so unlike the OPP surfaces they can check() — it reverts one admin transaction and writes nothing. The header deliberately mirrors registry_metadata.hpp's shape: a throwing helper for the writers, while a never-throw handler keeps asking is_canonical() itself.
You were also right that the raw object arm has to stay, and I checked why before touching it: that arm is what lets an unspellable stored key be fed back as a query bound, which is the open collect_next_key thread above. Validating there would break that round trip, so guarding the writers is the only remedy that does not.
Regressions, all driven through {"value": 7} since that is the only arm admitting such a code: regchain_uncanonical_code_rejected, regtoken_regctok_uncanonical_code_rejected, regreserve_uncanonical_code_rejected, setconfig_rejects_uncanonical_collateral_code.
One thing your comment led me to that is not fixed here: none of regctok, regreserve, or setconfig checks that a referenced code is REGISTERED — only that it is spellable. setconfig is the one with teeth: an unregistered (chain_code, token_code) makes meets_role_min demand a balance that cannot exist, so every non-bootstrapped operator of that type silently never activates while the bootstrapped set keeps the chain producing. Filed as WIRE-390 rather than widened into this PR, since it is a dangling-reference defect rather than a rendering one.
…them A code with no canonical spelling can be written but not read back: to_variant asserts is_canonical(), so the stored row throws on every later render. Guard where the state originates -- the five privileged registry writers check(), while createuwreq reverts, since dropping a SwapRequest would strand the user's escrowed deposit. Change-Id: I0e527bb29afb129d3e755bba8dff6ffd0eccc42e
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed 444962c. The SwapRequest refund guard and privileged persistence-writer checks resolve the prior findings. Given the confirmed pre-launch scope and absence of other production contracts using slug_name, I agree that raw-slug json=true cursor recovery can be deferred; WIRE-390 is likewise a separate pre-existing bootstrap-integrity issue. Approved. Preserve the coordinated rollout: land wire-tools-ts#103 before this carrier change, then wire-cdt#119 immediately after it, followed by wire-libraries-ts#83.
Mirrors wire-cdt 9b3bbeb8, keeping the two basic_name implementations diffable. The traits concept requires only convertible-to-string_view, but validity_error used find()/operator[] on the traits member directly and the symbol-width derivation used size(). One private binding now serves every use. test_slug_name adds a policy whose alphabet is only convertible. Change-Id: Ibb3f77f4ddc447964f72dc05976183c4f6e0c2fd
The `0x` tag identifies the CARRIER; it does not prove the bytes came from this
request. An unchecked absolute bound skipped prefixing entirely, so `scope=B`
with a bound naming scope A seeked into A while the default upper bound was
still the end of B -- the scan returned every scope in between. A bare `0x`
decoded to nothing and started at the front of the table, and `find` shares the
bound path, so it escaped its own scope the same way.
parse_bound now requires an absolute bound to be at least the prefix length and
to match it exactly, and rejects rather than clamps: a bound naming another
scope is a caller error, and quietly returning a different range is how that
stays invisible. One guard covers lower, upper and find, since find feeds both.
Also: - slug_name.hpp carried a stale doc block from when the renderer threw, and
claimed the lossy cases match `name`. They do not: name's alphabet is 2^5
with no gaps over all 64 bits, so every raw uint64 IS canonical and its
render is total AND injective. Only slug_name can lose.
- docs/get-table-rows-api.md described every bound as a key object and every
next_key as scope-stripped; neither holds for the raw cursor.
- sslugobjs gains `byalt`, a SLUG-typed secondary. `bypayload` is a uint64 and
cannot fail to decode, so the secondary cursor's raw fallback was
unreachable rather than merely uncovered; sec-11h now drives it in both
directions, and sec-11i covers the scope guard across lower/upper/find on
the primary and secondary paths.
Change-Id: I483ec801369e6154a8748af23dd025ac7e6a1cbc
|
Both review-body points are addressed in
The scoped secondary raw-fallback path — this was not merely untested, it was UNREACHABLE.
Two related notes, since this round touched the same fifty lines three times:
|
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
The cursor fix and functional behavior look sound. Two documentation contracts still encode behavior that the implementation no longer has; please correct them before approval.
| /// in two ways with no counterpart in `name`: | ||
| /// | ||
| /// - a non-empty leading symbol renders a string from_variant then REJECTS (loud); | ||
| /// - a value below 1<<42 has an empty leading slot and renders "" — |
There was a problem hiding this comment.
[P3] Cover the other silent-normalization paths
These two bullets are not exhaustive. Because to_string() ignores bits 48-63 and stops at the first zero symbol, slug_name{slug_name::pack("ETH") | (1ULL << 63)} renders as "ETH" and reparses successfully while silently dropping bit 63; slug_name{slug_name::pack("A") | 1} likewise renders as "A" and drops the data after the zero terminator. A non-empty leading slot therefore does not imply the loud-rejection outcome described here. Please describe noncanonical rendering more generally as either rejecting or silently normalizing, and add representative high-bit/post-terminator coverage; the variant_non_canonical_render_is_lossy_the_same_two_ways_name_is test and its surrounding comments need the same correction.
There was a problem hiding this comment.
You are right, and I checked both examples before agreeing rather than taking them on faith — they hold exactly as you describe. Fixed in cd0246e51d.
pack("ETH") | 1<<63 -> "ETH" -> re-parses to pack("ETH") != original
pack("A") | 1 -> "A" -> re-parses to pack("A") != original
to_string() reads only bits 0-47 and stops at the first zero symbol, so bits 48-63 and anything past an interior zero are never looked at. Both render valid spellings that re-parse cleanly, to a different value than they came from. So there are three outcomes, not two, and my claim that a non-empty leading symbol implies the loud one was simply wrong.
The consequence is the part worth having in the header, and it is the one I had missed: two distinct raw values can share a spelling, so a successful render proves nothing about what was stored. is_canonical() is what separates them — which is also why the registry writers call it rather than trusting a render, and I have said so in the other thread's file.
The header now gives the three cases in three lines, and variant_non_canonical_render_is_lossy_the_same_two_ways_name_is is renamed to variant_non_canonical_render_rejects_or_silently_normalizes — its old name asserted the false claim on its own. It covers the high-bit and post-terminator values in a two-element loop, checking the render succeeds and the value that comes back differs.
Kept deliberately short: this is a description of misuse, not a new guard against it.
| * | ||
| * A `slug_name` is carried on the wire and in chain state as a packed `uint64`, but its | ||
| * JSON carrier is the canonical STRING spelling: `fc::slug_name`'s `to_variant` renders | ||
| * `to_string()` and asserts the value round-trips. Not every 64-bit value has such a |
There was a problem hiding this comment.
[P3] Refresh the canonicality-guard rationale
fc::slug_name::to_variant is now deliberately total: it calls to_string() without asserting canonicality. An uncanonical value therefore no longer necessarily makes value-row rendering throw, makes get_table_rows fall back to hex, or drops an underwriter scan. The guard itself is still valuable because registry identities must remain canonical and lossy rendering must not alias distinct raw values, but this block should state that invariant instead. Please also sweep the copied throw/stall rationale in the contract writer and test comments (notably sysio.opreg.cpp, sysio.uwrit.cpp, and the registry/dispatch tests).
There was a problem hiding this comment.
Agreed on both halves — the rationale described the old throwing renderer, and the guard is still the right thing for a reason the block did not state. Fixed in cd0246e51d.
What it says now: to_variant is TOTAL, so an uncanonical code does not fail loudly, it renders MISLEADINGLY — pack("ETH") | 1<<63 renders "ETH" and re-parses to a different value. The guard therefore keeps two invariants a registry needs: a registered code stays canonical, and distinct raw values never alias onto one spelling. A depot registry has no erase action, so a bad row is permanent — and now silent, which is what makes refusing at the writer worth more than it was when rendering threw.
Swept the copied rationale at every site I could find carrying it: sysio.opreg.cpp (the setconfig collateral-code helper and the deposit-path canonicality check) and the registry tests in sysio.chains_tests.cpp, sysio.tokens_tests.cpp, sysio.reserv_tests.cpp and sysio.opreg_tests.cpp. A grep for the old throw/hex/stall wording now comes back empty across contracts/, libraries/, tests/ and unittests/.
Comments and one test only — no production code changed, and get_table_test's artifacts are untouched. contracts_unit_test 810/810, codename_tests 41/41.
Two documentation contracts still described behavior the code no longer has.
The lossy cases were given as two -- loud rejection or an empty render -- with a
non-empty leading symbol implying the loud one. That is false. to_string() reads
only bits 0-47 and stops at the first zero symbol, so anything in bits 48-63 or
after an interior zero is never looked at: `pack("ETH") | 1<<63` renders "ETH"
and `pack("A") | 1` renders "A", both VALID spellings that re-parse cleanly to a
different value than they came from. There are three outcomes, and the leading
symbol does not say which. The consequence worth knowing is that two distinct
raw values can share one spelling, so a successful render proves nothing;
is_canonical() is what separates them.
The registry guard's rationale still said an uncanonical code makes rendering
throw, makes get_table_rows fall back to hex, and drops an underwriter scan.
The renderer is total now, so none of that holds -- but the guard matters more
for it, not less: the damage went from loud to silent. Restated as the invariant
it actually keeps, and swept through the writer and the registry tests.
Comments and one test only; no production code and no artifact change.
Change-Id: I8551023bbc9eb64cc78b44364a47e8097c5a5576
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
The new examples and tests correctly document the three rendering outcomes, and I found no functional regression. I am requesting changes because the total-renderer documentation sweep is still incomplete in two places and leaves contradictory API guidance.
| * The zero code passes deliberately: it spells as `""`, which is a valid literal that | ||
| * packs back to zero, so it renders and round-trips like any other code. Whether an | ||
| * EMPTY code belongs in a given registry row is that registry's own question -- this | ||
| * guard is only about values that cannot be rendered at all. |
There was a problem hiding this comment.
[P3] Finish the total-renderer rationale sweep
Every raw slug_name now has a string rendering, so this guard protects exact canonical round-tripping and prevents aliases; it is not about values that cannot render. Equivalent obsolete claims that malformed rows are unrenderable, make rendering throw, fall back to hex, or stall a scan also remain in sysio.chains.cpp:146-147, sysio.tokens.cpp:55-56, sysio.msgch.cpp:341-344, sysio.uwrit.cpp:1062-1069, and the dispatch-test comments at 1455-1461 and 1648-1655. Please update those comments while preserving the checks and refund/drop behavior.
There was a problem hiding this comment.
Right on all of it — fixed in 07ebfc9466.
The first pass grepped for the exact phrasings it had just written and missed every variant, which is why five sites survived it. Doing it properly this time also turned up three you did not name: sysio.reserv.cpp at the regreserve writer and again at the create-reserve key comment, and a second site in sysio.tokens.cpp for the chaintoken pair.
Every one now says the same thing, inverted: the row is not unreadable, it is wrong. Rendering is total, so an uncanonical code renders a spelling that packs back to a different value — possibly another real code's — and nothing downstream reports it. The checks and the refund/drop behaviour are untouched; only the reasons are.
One thing your comment made visible that I would not have found on my own: "a value with no canonical spelling" was itself the phrasing underneath the original error. to_string() is total, so every value HAS a spelling — what an uncanonical one lacks is a canonical one. That elision is exactly what made "two outcomes, and a non-empty leading symbol means the loud one" sound reasonable when I wrote it. Normalized across the six files that carried it, abi_tests.cpp included.
registry_codes.hpp's zero-code note is corrected the same way: the guard is about values that do not survive the round trip, not values that cannot be rendered.
test_slug_name.cpp's carrier preamble is fixed too — it claimed the render throws AND that the string is the only carrier, while the transitional {"value": N} object is accepted a few cases later in the same suite. It now describes one emitted carrier plus that input exception, and says why the exception exists: it is the only way to name a value the string carrier cannot express.
PR description updated from "its two lossy modes" to the three outcomes.
contracts_unit_test 810/810, abi_tests 65/65, codename_tests 41/41, no artifact change.
|
|
||
| // ── variant carrier ──────────────────────────────────────────────────────── | ||
| // ONE carrier: the canonical string spelling. A slug renders as its text, zero | ||
| // as "", and a value with no spelling throws. The cases below pin that single |
There was a problem hiding this comment.
[P3] Correct the carrier contract summary
This preamble still describes the old contract: rendering a value without a canonical spelling is total/lossy rather than throwing, and the transitional {"value": N} object is an accepted non-string input later in this same suite. Please describe the single emitted string carrier plus that transitional input exception. The PR description also still says “its two lossy modes”; update it to match the new three-outcome explanation.
There was a problem hiding this comment.
Fixed in 07ebfc9466 — see the sibling thread for the full sweep; this file was part of it.
The preamble was wrong twice over. It said a value with no canonical spelling throws, which stopped being true when the renderer became total, and it called the string the only carrier while variant_accepts_the_transitional_object_carrier sits a few cases below it accepting {"value": N}.
It now says: one EMITTED carrier, the canonical string, with a render that is total — an uncanonical value still produces a string, which is what the reject-or-normalize case below pins. Input additionally accepts the transitional object, and the preamble now gives the reason rather than omitting it: that object is the only way to name a value the string carrier cannot express, which is why its arm deliberately does not require canonicality.
The PR description's "its two lossy modes" is updated to the three outcomes as well.
The previous pass grepped for the exact phrasings it had written and missed
every variant, so five more sites kept asserting that an uncanonical code
cannot be rendered: sysio.chains.cpp, sysio.tokens.cpp, sysio.msgch.cpp,
sysio.uwrit.cpp and the two dispatch tests. Three more turned up that the
review had not named -- sysio.reserv.cpp twice and a second site in
sysio.tokens.cpp.
Each says the same thing now, inverted: the row is not unreadable, it is WRONG.
Rendering is total, so an uncanonical code renders a spelling that packs back to
a different value -- possibly another real code's -- and nothing downstream
reports it.
"a value with no spelling" was the phrasing underneath the original mistake, and
it is false: to_string() is total, so every value HAS a spelling. What an
uncanonical one lacks is a CANONICAL spelling. Normalized across the six files
that used it, including abi_tests.cpp.
test_slug_name.cpp's carrier preamble also still said the render throws and that
the string is the only carrier, while the transitional {"value": N} object is
accepted later in the same suite. Both corrected.
Comments only; no production code, no artifact change.
Change-Id: Ibb4b37a3d36fe6cac183bd2732bfdad39451c801
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
The two exact findings from the prior review are corrected, and this follow-up introduces no runtime change. I am still requesting changes because the documentation sweep leaves two contradictory contracts: number handling still relies on a digit-leading slug that the leading-letter rule forbids, and several updated comments collapse the documented three noncanonical-render outcomes back to only silent normalization.
| return; | ||
| } | ||
| // A number is REJECTED, never coerced. The slug alphabet contains digits, so | ||
| // `"123"` is itself a canonical slug whose packed value is nothing like 123 |
There was a problem hiding this comment.
[P3] Make the carrier rationale obey the leading-letter rule
"123" is not a canonical slug anymore: leading_alphabet requires [A-Z] in the first position, so the validating constructor rejects it. The same stale rationale remains in abi_tests.cpp:749-751, which also calls the string the only carrier “in both directions” despite the transitional {value} input. Please describe the actual contract instead: JSON numbers are never a slug carrier; to_variant always emits a string (losslessly only for canonical values), while from_variant accepts a validating string plus the transitional object. The public header at lines 133-134 and the PR description’s “One carrier” section need the same wording.
There was a problem hiding this comment.
Right, and this one was self-contradicting rather than merely stale — fixed in 88c97b086a.
The rationale justified refusing a JSON number by claiming "123" (and "7" in abi_tests) is itself a canonical slug whose packed value differs from the literal. The leading-letter rule this PR adds makes both invalid, so the argument rested on an example the same PR outlaws. It has been that way since 5415b86f7b and I did not revisit the justification when I added the rule.
What it says now, in both places: a JSON number is never a slug carrier in either direction. The field carries a SPELLING, and leading_alphabet already makes that string unambiguous — no legal code can be read as a decimal — so a numeric carrier would only add a second way to say the same thing.
The header block at 133-140 no longer calls the string "the only carrier": to_variant ALWAYS emits a string (the canonical spelling for a canonical value, "" for zero, otherwise text that does not round-trip), and from_variant takes that string plus the transitional {"value": N} object. abi_tests' number case says the same, and notes that a bare number is not that object.
PR description's "One carrier" section updated to match — the diagram and the number paragraph both.
| /// uint64s and reach a slug_name through the non-validating raw constructor, so a | ||
| /// forged payload can carry a value that does not round-trip through its spelling. | ||
| /// Such a value can never have been registered, and rendering is total so it will | ||
| /// not announce itself: it renders a string that packs back to something else, which |
There was a problem hiding this comment.
[P3] Preserve all three noncanonical render outcomes
This wording covers only silent normalization. A digit-leading raw such as uint64_t{34} << 42 renders "7", which the validating parser rejects; it does not reparse to a different code. The accurate invariant is that rendered text either fails validation or silently reparses to a different raw identity. Please apply that wording here and to the mirrors in sysio.chains.cpp:147-148, sysio.tokens.cpp:56-57, sysio.uwrit.cpp:1066-1069, and sysio.dispatch_tests.cpp:1457-1459,1652-1654. The dispatch comments at 1495 (“fail to render”) and 1674 (“renderable”) also still describe the pre-total-renderer behavior.
There was a problem hiding this comment.
Correct, and this is my over-correction — fixed in 88c97b086a.
The previous sweep replaced "cannot be rendered" with "renders a spelling that packs back to a different value", which describes only the SILENT outcome and drops the loud one I had documented two commits earlier in the same file. uint64_t{34} << 42 renders "7", which the validating parser rejects; it does not re-parse as another code.
Every site now carries the whole invariant: the rendered text either FAILS validation on the way back, or silently re-parses as a DIFFERENT code. Applied to sysio.msgch.cpp, sysio.chains.cpp, sysio.tokens.cpp, sysio.uwrit.cpp and both dispatch-test comments, plus the two further dispatch sites you flagged — 1495's "fail to render" and 1674's "renderable" — which were still describing the pre-total-renderer behaviour outright.
Checks and refund/drop behaviour untouched; comments only. contracts_unit_test 810/810, abi_tests 65/65, codename_tests clean, no artifact change.
…ree outcomes Two contradictions, both introduced here. The reason given for refusing a JSON number was that `"123"` / `"7"` are canonical slugs whose packed values differ from the literal -- but the leading-letter rule this PR adds makes both invalid, so the argument used an example the same PR outlaws. The actual contract: a slug field carries a SPELLING, and `leading_alphabet` already makes that string unambiguous, so a numeric carrier would only add a second way to say the same thing. It is refused in both directions; the transitional object is the one exception. The previous sweep then replaced "cannot be rendered" with "renders a spelling that packs back to a different value" -- which describes only the SILENT outcome and drops the loud one documented two commits earlier. A digit-leading raw such as 34<<42 renders "7", which validation rejects. Every site now carries the whole invariant: the rendered text either FAILS validation on the way back, or silently re-parses as a DIFFERENT code. Swept msgch, chains, tokens, uwrit, the two dispatch-test comments, and the two further dispatch sites still saying "fail to render" / "renderable". The header's carrier block and abi_tests' number case now also state that to_variant ALWAYS emits a string and that from_variant takes the string plus the transitional object, rather than calling the string the only carrier in both directions. Comments only; no production code, no artifact change. Change-Id: Iaf1c474d5a4e327c4b47eef1cda567cffaa4f56d
left a comment
There was a problem hiding this comment.
Full clean-room review of current head 88c97b086a across all 80 changed files, public documentation, the PR description, contract persistence and refund paths, ABI/key encoding, the pagination matrix, and cross-repository consumers. The implementation is coherent, and I found no additional runtime, security, consensus, or ABI-layout defect in the intended production paths.
I am requesting changes for the two public contract issues inline and this rollout issue:
[P2] Correct the landing prerequisites. The description says wire-tools-ts#103 does not gate this PR, but that PR is still open and current tools master still evaluates String(code.value) in runOutboundEnvelopesQueued; with the new spelling carrier that becomes "undefined" and the external-outpost bootstrap gate cannot match. #103 therefore gates any integration or deployment using current tools master, even if the repository merges are performed as a short coordinated train. wire-cdt#119 also supplies <sysio/slug_name.hpp> and is required to rebuild these changed contract sources. Please state the coordinated merge and deployment ordering explicitly.
Please also remove the current description contradiction: the One carrier section says proto-boundary validation lands later, and Out of scope repeats that, while this head now implements the validation and refund guards.
The remaining P3 correctness and consistency items are included inline in this same review so they can be resolved in one pass rather than discovered piecemeal.
| - `rows` — array of `{key, value}` objects. When `show_payer=true`, includes `payer` field. | ||
| - `more` — `true` if there are more rows beyond `limit`. | ||
| - `next_key` — use as `lower_bound` for the next page. Scope is stripped (pass same `scope` param). | ||
| - `next_key` — use as `lower_bound` for the next page. Usually a JSON key object with the scope stripped (pass the same `scope` param); for a key the ABI cannot name it is a `0x` raw cursor instead — an opaque, complete key. Feed either back verbatim. |
There was a problem hiding this comment.
[P2] Resume reverse pages through upper_bound
This tells every caller to put next_key in lower_bound, but reverse scans use a different cursor contract. The reverse implementation makes next_key the last returned key and requires upper_bound = next_key so the exclusive bound advances below it (chain_plugin.cpp:2872-2878, 3071-3072); this PR follows that rule in its own raw-cursor tests at get_table_tests.cpp:909, 973, and 1045. Following this documentation instead repeats the current top row or applies the wrong half-range. Please update this line and the pagination section at lines 167-179, including the example, to say forward uses lower_bound and reverse uses upper_bound.
As part of the same public API contract, please enumerate all accepted json=true forms: a JSON key object, untagged scope-relative hex, and a 0x complete-key cursor. The untagged form matters because displayed-key fallback emits it. The corresponding comments in chain_plugin.hpp:533,556 and chain_plugin.cpp:2471-2474,2820-2824 should be aligned too.
There was a problem hiding this comment.
Fixed in fc6ed3d0d6. You are right, and the tests in this PR were already contradicting the doc — get_table_tests.cpp:909, 973 and 1045 all feed a reverse next_key to upper_bound, which is the behaviour, while the doc said lower_bound unconditionally.
Line 42 and the pagination section now say forward feeds lower_bound, reverse feeds upper_bound, with the reason stated (a forward next_key is the first key NOT returned; a reverse one is the last key that WAS returned, so the exclusive bound has to advance below it) and a reverse example alongside the forward one.
The three accepted json=true forms are enumerated in the lower_bound row and in the pagination section: a JSON key object and untagged hex, both within-scope and prefixed on the way in; and a 0x raw cursor, a complete key used verbatim. The untagged form is called out as what the displayed-key fallback emits, which is also why it had to keep working — see the other thread.
chain_plugin.hpp's lower_bound and next_key field docs carry the same wording now, including the direction rule.
| static constexpr std::string_view alphabet{ alphabet_storage, | ||
| sizeof(alphabet_storage) - 1 }; | ||
|
|
||
| // A code must START with a letter. This is what makes the string carrier |
There was a problem hiding this comment.
[P2] Propagate the leading-letter grammar to public authoring docs
This changes the accepted nonempty spelling to [A-Z][A-Z0-9_]{0,7}, but the public bootstrap contract still advertises [A-Z0-9_] with only a length limit. Please update docs/platform-bootstrap-config.md:51-52,164-168 and libraries/opp/proto/sysio/opp/bootstrap/bootstrap.proto:23-25,51,71, then add digit-leading and underscore-leading mutations to libraries/opp/test/test_bootstrap_platform_config.cpp so the documented rule is pinned.
Also correct contracts/sysio.chains/include/sysio.chains/sysio.chains.hpp:79-80: reflected/raw action deserialization does not validate the packed member. The new writer guard is what enforces canonicality before persistence.
There was a problem hiding this comment.
Fixed in fc6ed3d0d6. The public authoring contract advertised a grammar the code rejects, so a bootstrap config with "1ETH" would pass documentation review and fail at runtime.
platform-bootstrap-config.md (the codes bullet and the V2 invariant) and bootstrap.proto (the header block and both field comments) now state [A-Z][A-Z0-9_]{0,7} — must start with a letter, at most 8 characters. The proto edit is comments only: no field, number or type changed, so the wire format and generated code are untouched; I verified the diff carries nothing but comment lines.
Pinned by a new case in test_bootstrap_platform_config.cpp — v2_slug_grammar_requires_a_leading_letter — covering digit-leading (7, 1ETH, 0X10), underscore-leading (_LEAD, ________), lowercase, out-of-alphabet and over-length, plus the accepted spellings. slug_ok's own comment described the old alphabet too; corrected.
Worth flagging how close that came to being useless: the case lives in test_opp, and my first verification run built plugin_test unit_test test_fc contracts_unit_test. The suite reported "5 cases, No errors" — the STALE binary, without the new case in it. Running the case by name is what exposed it. It now runs: 6 cases, passing.
sysio.chains.hpp:79-80 is corrected as well — reflected/raw action deserialization writes the packed member directly and validates nothing; a string spelling goes through the validating constructor, a raw uint64 does not, and the writer guard is what enforces canonicality before persistence.
| // throws invalid_type_inside_abi; with a same-named ABI struct present it | ||
| // would instead serialize as {"value":N}. The converted table-read sweep in | ||
| // contracts/tests cannot catch either case, because fc::slug_name's | ||
| // from_variant accepts the string, the integer AND the object form, so those |
There was a problem hiding this comment.
[P3] Finish the carrier and ABI truth sweep on this head
This rationale is wrong in two concrete ways: from_variant rejects a bare integer, as this same test proves at lines 749-754, and all five committed registry ABIs in this PR now contain zero slug_name struct definitions. The shadowing struct remains valuable as a legacy/deployed-ABI compatibility fixture, but it should be described that way. Please make the same current-vs-legacy correction in unittests/be_key_codec_tests.cpp:164-166 and contracts/tools/generate-sysio-contract-types.py:112-114.
The exhaustive wording sweep also has three remaining factual edges: database_utils.hpp:371-375 says every noncanonical rendering re-encodes to a different key, although digit/unused-symbol spellings fail validation instead; slug_name.hpp:93-98 accidentally includes raw zero among empty-leading values without a spelling even though "" is its canonical sentinel; and basic_name.hpp:113-116 describes is_valid_literal(string) as a predicate over a raw packed value. Please correct these in this same pass.
There was a problem hiding this comment.
All of it confirmed and fixed in fc6ed3d0d6.
The ABI claim. Verified directly: all five registry ABIs on this branch carry zero slug_name struct_defs — abigen stopped emitting them once slug_name became a real builtin, which is this PR's own doing. So "every registry ABI still ships one" was true when written and false on this head. abi_tests, be_key_codec_tests and generate-sysio-contract-types.py now describe the shadowed struct as a legacy/deployed-ABI compatibility fixture: something set_abi and the key codec must keep resolving the same way, not something current output contains. The generator note also says what changes without the builtin entry — unknown now, rather than the {value: uint64} shape.
abi_tests' rationale also claimed from_variant accepts a bare integer, which the same test disproves twenty lines down; corrected in the same pass.
The three edges, each verified:
database_utils.hpp:371-375— "would re-encode to a DIFFERENT key" is the SILENT outcome only; a digit-leading or unused-symbol spelling fails validation instead. This is the third site of that same claim, and the reason is worth stating: I have been fixing the instances you name rather than searching for the invariant. Both outcomes are now stated here.slug_name.hpp:93-98— raw zero was counted among values with no canonical spelling, but""IS its spelling and packs straight back. The set is now non-zero empty-leading values, unused symbol values, and anything setting bits 48-63.basic_name.hpp:113-116—is_valid_literal(str)takes the SPELLING; asking whether a raw packed value round-trips isis_canonical(), which calls it on its ownto_string(). Corrected, and it now points at the right predicate for the other question.
| // next_key is the resume token and is absolute for its own reasons | ||
| // (see to_raw_cursor) — both remain feedable as bounds, because | ||
| // untagged hex still means "within the scope". | ||
| obj["key"] = fc::to_hex(row.key.data() + scope_prefix_size, |
There was a problem hiding this comment.
[P3] Keep the scope-only displayed fallback reusable
When a legal raw row key consists only of the scope prefix, row.key.size() - scope_prefix_size is zero and this emits "". Empty lower_bound, upper_bound, or find is treated as absent at lines 2687/2695, so this displayed key is not feedable despite the comment immediately above saying both forms remain feedable. The tagged next_key path is already correct. Please emit to_raw_cursor(row.key) when the within-scope remainder is empty, mirror the fix in the primary fallback at lines 3140-3143, and extend the scope-only raw-row test to return and replay the displayed rows[].key.
There was a problem hiding this comment.
Confirmed and fixed in fc6ed3d0d6. This is the same zero-length trap as the original cursor finding — I fixed it for next_key with the tagged cursor and left it in the DISPLAY path, then wrote the comment above it asserting both forms stayed feedable. That claim was only ever true for the non-empty remainder.
Both sites now emit to_raw_cursor(row.key) when row.key.size() == scope_prefix_size, and the comment says why rather than asserting the asymmetry is fine:
obj["key"] = row.key.size() == scope_prefix_size
? to_raw_cursor(std::string_view(row.key.data(), row.key.size()))
: fc::to_hex(row.key.data() + scope_prefix_size,
row.key.size() - scope_prefix_size);It does mean a displayed key changes representation for exactly one shape of row — tagged absolute instead of relative hex. I took that deliberately over a displayed key that cannot be replayed, and it is why the doc now enumerates untagged within-scope hex as an accepted bound form alongside the 0x cursor: both appear in rows[].key, so both have to be feedable.
plugin_test 297/297, contracts_unit_test 810/810, abi_tests 65/65, be_key_codec_tests 20/20, codename_tests 41/41, test_opp 6/6, no artifact drift.
…t public docs
The displayed row key kept its scope-relative hex fallback even when the
within-scope remainder was EMPTY -- a row whose whole key is the scope prefix
rendered as "", which every bound path treats as absent, so the key could not be
replayed. The comment above it claimed both forms stayed feedable; that was only
true for the non-empty case. Both sites now emit the tagged complete key there.
Public documentation corrections, all of which described behavior the code does
not have:
- get-table-rows-api.md told every caller to feed next_key to lower_bound.
Reverse scans need upper_bound -- next_key is the last key RETURNED, so the
exclusive bound must advance below it, which this PR's own reverse tests
already do. Added the reverse example and enumerated the three accepted
json=true bound forms (key object, untagged within-scope hex, 0x complete
cursor); aligned the two chain_plugin.hpp field docs.
- platform-bootstrap-config.md and bootstrap.proto still advertised
[A-Z0-9_] with only a length limit, so a config with "1ETH" passed
documentation review and failed at runtime. Both now state
[A-Z][A-Z0-9_]{0,7}, pinned by a new bootstrap-validator case covering
digit-leading, underscore-leading, lowercase, out-of-alphabet and
over-length. The proto edit is comments only -- no field, number or type
changes, so the wire format is untouched.
- sysio.chains.hpp said deserialization enforces the slug format. It does not:
reflected/raw action data writes the packed member directly. The writer
guard is what enforces canonicality.
Three factual edges in the carrier documentation:
- database_utils.hpp said a noncanonical rendering re-encodes to a different
key; a digit-leading spelling fails validation instead. Third site of that
same claim.
- slug_name.hpp counted raw zero among values without a canonical spelling --
"" IS zero's spelling and packs straight back.
- basic_name.hpp described is_valid_literal(str) as a predicate over a packed
value; it takes the spelling. is_canonical() is the packed-value question.
And the five registry ABIs on this branch no longer emit a slug_name struct_def
at all, so abi_tests, be_key_codec_tests and the TS generator now describe that
fixture as legacy/deployed-ABI compatibility rather than as what ships today.
Change-Id: Ic56dad94db488c800b4eb2c4cbd7d4b71b16ff8f
…bi-builtin Change-Id: I34558a7cc6cb5f5714b3646e08cd8d3f6b3f6483 # Conflicts: # contracts/sysio.dclaim/sysio.dclaim.wasm # contracts/sysio.msgch/src/sysio.msgch.cpp # contracts/sysio.msgch/sysio.msgch.wasm # contracts/sysio.system/sysio.system.wasm
slug_name is an ABI builtin on this branch, so a stat row's chain_code and
token_code render as strings, not {"value": N} objects.
Change-Id: I2734fbc0fd711a561621b15933561aa55002290b
| // V2's grammar is `[A-Z][A-Z0-9_]{0,7}`, and the LEADING rule is what makes a bare JSON | ||
| // string unambiguous -- no legal code can be read as a decimal. The public authoring docs | ||
| // advertise that grammar, so it is pinned here rather than left to the constructor. | ||
| BOOST_AUTO_TEST_CASE(v2_slug_grammar_requires_a_leading_letter) { |
There was a problem hiding this comment.
[P2] Validate reserve codes through the config validator
This test pins slug_ok in isolation, but validate() only calls it for chains[].code and tokens[].code; the reserve loop never checks r.code(). After this PR, regreserve rejects noncanonical codes through check_codes, so a config with, for example, reserves[0].code = "1BAD" passes the launch validation and then fails once bootstrap reaches regreserve, after earlier irreversible actions may have run. Please call slug_ok(r.code()) in the reserve loop and add validate-level mutations for digit- and underscore-leading reserve codes (and ideally the empty code) so this test covers the actual preflight path.
There was a problem hiding this comment.
Fixed in 4d61929. validate() now runs slug_ok(r.code()) in the reserve loop, and validator_rejects_mutations covers "1BAD", "_BAD" and "" as reserve codes. Each case asserts that the V2 reserve code error fires, not just that some error does.
The empty code is stricter than the chain: an empty slug packs to zero, which is canonical, so regreserve would accept it. It's rejected here for consistency with the existing V2 rule for chain and token codes.
regreserve rejects a non-canonical reserve code, so validate() must check reserves[].code against the V2 slug grammar like chain and token codes. Change-Id: I234bd451fee5c3c76b61498c2bc208d18acb2fea
left a comment
There was a problem hiding this comment.
Re-reviewed current head 4d61929. The reserve-code preflight gap is fixed with validator-level coverage, and I found no remaining actionable PR-introduced issues. Local test_opp passes; the still-running CI matrix should remain the merge gate.
…bi-builtin Change-Id: I5ad0223d16464ec9a5ab6f3af2ed88b0ca29c858 # Conflicts: # plugins/chain_plugin/src/chain_plugin.cpp
left a comment
There was a problem hiding this comment.
Reviewed head 8a85586. One actionable finding is detailed inline: the query engine still requires the legacy slug_name struct definition that the updated contract ABIs remove.
Validation: a compiled reproduction using this head's descriptor compiler rejected the builtin form with Unsupported ABI type and accepted the legacy struct form. All 41 slug tests passed (305 assertions). The full integration suite was not run.
|
|
||
| built_in_types.emplace("symbol", pack_unpack<symbol>()); | ||
| built_in_types.emplace("symbol_code", pack_unpack<symbol_code>()); | ||
| built_in_types.emplace("slug_name", pack_unpack<fc::slug_name>()); |
There was a problem hiding this comment.
[P1] Update the query engine to recognize the new builtin
This registers slug_name as an ABI builtin, and the updated contract ABIs remove its struct definition, but plugins/query_engine_plugin/src/query_values.cpp:292–305 explicitly excludes slug_name from builtin handling and only accepts it when that legacy struct exists. After deploying these ABIs, queries against tables containing slug fields fail during planning with QUERY_SEMANTICS: Unsupported ABI type, including COUNT(*), because create_plan compiles the entire table schema before planning the projection. A compiled reproduction against this head confirms that the builtin form fails while adding back the legacy struct succeeds. Please update the descriptor compiler to accept the builtin and add regression coverage using the new ABI shape.
There was a problem hiding this comment.
Fixed in 1e66942. slug_name is now compiled like any other primitive, so it decodes, filters and renders as its canonical string. I removed the two struct-form branches (the one in the descriptor compiler and the one in key decoding) rather than keeping them alongside: the host now rejects an ABI that redefines a builtin, so that form can no longer reach this code. The planner needed no change, because a text literal against a slug_name key leaf already encodes through be_key_codec.
Regression test: query_integration/builtin_slug_name_fields_compile_filter_and_render uses the new ABI shape (a slug_name field, no struct definition). It checks that COUNT(*) compiles and that WHERE code = 'SOL' filters and renders "SOL". With the fix reverted it fails with your error (query_error: Unsupported ABI type on the COUNT(*) plan). Both query engine binaries pass in full (65 and 13 cases).
slug_name has no struct definition in a contract ABI any more, so a table with a slug field failed to plan (Unsupported ABI type), COUNT(*) included. Compile it like every other primitive and drop the struct-form branches: the host rejects an ABI that redefines a builtin, so that form is unreachable. Change-Id: Id57363cc43fa4087e62e3d961b2945c25fcab76e
left a comment
There was a problem hiding this comment.
Reviewed head 1e66942 using the open-code-review-delegate workflow. The previously reported query-engine slug_name builtin issue is fixed, and no new actionable findings remain.
Local validation passed for 27 shipped table schemas and both builtin and legacy slug key decoding. The unchanged unsupported bytes key in sysio.liq::parked is a pre-existing limitation. The full integration suite was not run.
2 participants
Landing 2 of the
slug_namecarrier unification. Merge order is load-bearing:<sysio/slug_name.hpp>, which these contract sources now include. Required to rebuild this branch at all.ExternalOutpostSteps.ts:402still evaluatesString(code.value); against the spelling carriercodeis"ETH", socode.valueisundefinedand the external-outpost bootstrap gate compares against"undefined"and never matches. Allowsysio.*system contracts to bill users for RAM #103 replaces that read.slugValuealready accepts a spelling, a packed number and the{value}object, so it does not block; land it with the train.An earlier revision of this description said #103 did not gate this PR. That was wrong: #103's own branch handles every carrier, but current tools master does not, and master is what an integration run builds.
What changes
A
slug_namefield rendered as{"value": <uint64>}and now renders as the decoded slug — across all 103 pre-existing fields insysio.{chains,tokens,opreg,reserv,uwrit}, not just the ones this diff names. That half of the change appears nowhere in the diff.A code must start with a letter
This is the rule that makes one carrier possible: no legal code can be spelled like a number, so a bare JSON string is always a code and never a decimal. Without it
"7"was simultaneously a valid code (packed149533581377536) and a valid decimal, while"1E3"/"0X10"collided with JS numeric syntax — the direct cause of the mis-decode wire-tools-ts#101 had to fix. Digits and_stay legal in every position after the first (V1,USDC,TRAIL_);""is the zero sentinel, not a spelling.It costs nothing in the domain — every slug spelling used anywhere in the platform already starts with a letter (
ETH,WIRE,SOLANA,PRIMARY,USDC,LIQSOL, …); only 7 test-only occurrences needed updating.leading_alphabetis an optional traits member detected by a concept, sosysio::namedoes not gain the rule.One validation algorithm, shared with CDT
fc::basic_nameandsysio::basic_name(#119) now share ONE predicate —validity_error()returnsnullptror the traits' message for the first rule broken,is_valid_literal()isvalidity_error(str) == nullptr, and both constructors gate on it.validity_errorandpackare token-identical across the two repos (same md5), so the two files stay diffable.This fixes a real
sysio::namebug. The host had two validators that disagreed, and the tree asserted both behaviours ~130 lines apart in one file:The literal path never checked final-symbol width or a trailing pad, so a literal could compile to a value it does not spell. Both paths now share one algorithm, pinned by
name_tests::literal_and_runtime_validation_agree. No runtime or consensus change:packis untouched, so every packed value is identical — only the compile-time literal gate tightens, and only on spellings that were already packing something else.The contracts use CDT's
slug_namenowcontracts/sysio.opp.common/.../slug_name.hppwas a fourth implementation beside the host, CDT and sdk-core. Deleted; the ten includes point at<sysio/slug_name.hpp>, and none of the 299sysio::slug_nameuses changed. That exposed two abigen defects, both fixed in #119:slug_namewas an alias, so it never reached abigen's builtin match and the host rejected the ABI withduplicate_abi_type_def_exception(523 of 762 contract tests); andadd_structdescribed a builtin's base, leaking an orphanbasic_name_slug_name_traitsstruct_def into five ABIs.One carrier
The renderer is TOTAL, exactly like
sysio::chain::name. An earlier revision threw on an uncanonical value; that was the wrong place to enforce it. A renderer is a READ path, and a throwing one turns one bad row into a failure of everything that scans it —underwriter_plugin'sscan_cycle()is onetryaround the whole pass, so one uncanonical code would stop every underwriter commit, not one cell. Validation belongs on the WRITE path, and this PR puts it there: thesysio.msgchdispatch sites drop an attestation whose payload codes are uncanonical (with a refund where custody was taken), and the privileged registry writers refuse one outright.An uncanonical value still renders — what it loses is the round trip, in one of two ways: the text either FAILS validation on the way back, or silently re-parses as a DIFFERENT code.
is_canonical()is what separates them; a successful render does not.A JSON number is never a slug carrier in either direction. The field carries a SPELLING, and
leading_alphabetalready makes that string unambiguous — no legal code can be read as a decimal — so a numeric carrier would only add a second way to say the same thing. The transitional{"value": N}arm reaches a RAW uint64 and is the one place a malformed number can land on a key, so it goes through a checked unsigned decoder (rejecting negative, fractional, non-numeric, out-of-range) rather thanas_uint64, which coerced-1toUINT64_MAX. Canonicality is deliberately not required there — it is the only way to name a key the string carrier cannot express. The arm goes when no writer emits the object form.Forgeable payload codes are validated
token_code/reserve_codearrive in the payload and reachslug_namethrough the non-validating raw constructor. Three dispatch sites insysio.msgch(five payload codes) now drop an attestation whose codes have no canonical spelling — same[[nodiscard]] boolshape assource_chain_binding_ok, nevercheck(), since a check there haltsevalcons. Pinned bysysio_dispatch_tests/dispatch_drops_uncanonical_token_code.Pagination cursors
next_keyis the canonical spelling whenever the codec can NAME the key. A key with no canonical spelling comes back as a raw cursor — the0xtagfc::from_hexalready trims, followed by the hex of the COMPLETE stored key:A json=true bound dispatches on its first NON-WHITESPACE character:
{is a key object,0xa raw cursor, anything else bare hex meaning within-scope. Carrying the whole key is load-bearing —kv_setrequires only that the COMPLETE key be nonempty, so a scoped row whose entire key is the 8-byte prefix has an empty remainder, and an emptynext_keyreads as no bound and restarts the page. json=false is untouched.Key bytes are unchanged
sysio::kvnever calls CDT'sto_key—make_keywrites into a closedbe_key_stream, so a struct key recurses towrite_be64, exactly what the leaf path does. No migration; existing rows stay addressable.The TS generator needed the builtin too:
slug_name -> stringnow sits inPRIMITIVE_TS/PRIMITIVE_SCHEMAahead of the structs lookup, matchingabi_serializer. Without it every slug field falls through tounknownthe moment abigen stops emitting the struct_def (#119).Verification
unit_testplugin_testcontracts_unit_testtest_fctest_http_client.cppfailurectest -L unit_tests1eb3352c, the #119 head this branch imports)codename_tests·name_tests·abi_tests·be_key_codec_tests·get_table_testsThe failures are
deep_mind_tests/deep_mind,savanna_misc_tests/verify_block_compatibitityandhttp_client_file_download_tests/stale_metadata_reconnect_failure_cleans_up_safely— all pre-existing on this host, none touchingslug_name. The reference-data pair compares against artifacts a localBUILD_TEST_CONTRACTS=ONrebuild invalidates;sysio.biosis unchanged, so there is nothing to regenerate. Staging the committed test-contract artifacts over the build-dir copies takesunit_testfrom 12 failures to exactly that pre-existing 3 with no source change, which is what identifies the drift as environmental.The carrier is pinned at four layers, because
slug_nameresolves through a different lookup at each:codename_tests(the conversion, the leading-letter rule, the total render and its three outcomes — reject, render "", or render a VALID spelling that re-parses to a different value — the derived type'sstd::hashas a compile-time guard, a round trip through JSON text),abi_tests(the action/row path and builtin-vs-struct precedence),be_key_codec_tests(the key leaf and leaf-vs-struct byte identity), andget_table_testssec-11 end to end on scoped and unscoped kv tables — including the raw cursor in both directions, pinned to its exact bytes so a relative one cannot pass, a cursor carrying nothing but the scope prefix, a JSON bound with leading whitespace, and an all-whitespace bound refused.Out of scope
The absent binding in
source_chain_binding_okfortoken_code/reserve_code— proving them the waychain_codeis proven — is a pre-existing gap this makes legible but does not cause, and it lands with the field conversions. The write-path canonicality guards themselves are IN this PR, at the dispatch sites and the registry writers.Two scoped-pagination defects found while fixing the cursor path are pre-existing on master and independent of
slug_name: the primary bound path prefixes ajson=falsebound that is already the complete stored key, so ajson=falseresume lands past its own scope; and a scoped SECONDARY query is never confined to its scope when no bound is supplied, so it returns other scopes' rows. Both are fixed in #636, stacked on this branch.