fix(docgen): derive JSON-RPC docs from the serializer contract - #12868
Conversation
|
Claude finished @dipkakwani's task in 10m 19s —— View job Review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| } | ||
| // A hand-rolled converter exposes no contract members, leaving the CLR shape as the only guess | ||
| _guessedTypeNames.Add(type.Name); |
There was a problem hiding this comment.
Low — type.Name makes the new warning ambiguous exactly where it matters.
The diagnostic is the only signal that a type fell back to guessing, so it should identify the type unambiguously. type.Name collapses same-named types in different namespaces (there are two BlockHeaderForRpc in the tree — Nethermind.Facade.Eth and Nethermind.Taiko.Tdx) and prints generics mangled as ArrayPoolList\1, which is the common case here since the buffer wrappers are precisely the converter-backed types. type.FullName ?? type.Name` costs nothing and keeps the set genuinely deduplicated.
Also worth noting for anyone reading the output: TransactionForRpc will always appear in this list — it carries [JsonConverter(typeof(TransactionJsonConverter))] and is abstract, so the CLR-shape guess only yields the seven base fields (type, hash, transactionIndex, blockHash, blockNumber, blockTimestamp, gas) and none of the per-type fields (nonce, to, from, value, input, gasPrice, …). That matches master, so it's not a regression, but it does mean the headline transaction shapes are still the guessed ones.
|
Claude finished @dipkakwani's task in 7m 51s —— View job Review:
|
| Earlier finding | State in 013594d |
|---|---|
| Enum short-circuit ran before the table | Table is now consulted first (line 365), so [typeof(TxType)] is live. TxTypeConverter.Write emits "0x0" / ByteArrayConverter.Convert(…, skipLeadingZeros: true), so _string_ (transaction type) is right and the 31 type fields are fixed. |
int/byte documented as hex |
Now _integer_. Confirmed IntConverter.Write is writer.WriteNumberValue(value) and no byte/uint converter is registered. long/ulong correctly stay hex — they route through NumericConverterHelper.Write and ForcedNumberConversion.Value defaults to NumberConversion.Hex = 0. BigInteger → _string_ (decimal integer) matches WriteStringValue(value.ToString(InvariantCulture)); double → _number_ matches WriteRawValue. |
Properties.Count != 0 conflated "no contract" with "everything ignored" |
Now contract?.Kind is JsonTypeInfoKind.Object, and the fallback reads fields as well as properties to match IncludeFields = true. |
catch (ArgumentException) too narrow |
Widened to ArgumentException or InvalidOperationException or NotSupportedException. |
type.Name collided across namespaces |
Now namespace-qualified. |
I also checked the two things most at risk from deleting TryGetEnumerableItemType's reflection, and both hold: ArrayPoolList<T> carries no [JsonConverter], so a non-byte instance still resolves to Kind == Enumerable and its items still expand; and every array row in the table (byte[], byte[][], Hash256[], double[], Bloom) is converter-backed, so Kind == None keeps the enumerable branch from overriding it. CappedArray<int> → array of _integer_ is right, because CappedArrayJsonConverter<T> delegates each element to IntConverter. By-ref parameters are handled too — GetContract throws for them, the catch swallows it, and the unwrapped scalar name wins, which is why retiring master's "Int64&" row is safe.
No dotnet permission in this job, so as before this is a read of the code and the serializer it depends on, not a run of the tool. A before/after diff of the generated markdown remains the real check, and build-tools.yml will catch any compile error in the new usings.
Medium
1. Member-level [JsonConverter]s are still invisible, so trace_* block numbers are documented as hex strings while the node writes bare numbers — JsonRpcGenerator.cs:427
GetSerializedMembers returns (p.Name, p.PropertyType) and drops p.CustomConverter, so the type table decides alone. Two converters deliberately opt out of hex: ParityTxTraceFromStore.BlockNumber uses ULongRawJsonConverter (writer.WriteNumberValue) and shows up in trace_block, trace_filter, trace_get and trace_transaction; BlockForRpc.Step / BlockHeaderForRpc.Step use NullableRawULongConverter, same thing. Both are documented as _string_ (hex integer). Nonce via BlockNonceConverter is a milder variant — a zero-padded 16-digit hex string documented as a hex integer.
Carried over from master's "UInt64" => "_string_ (hex integer)", so not a regression — I'm raising it because it is the same defect class the PR fixes at type level, and the contract already carries the answer one field away. Fixing it or explicitly scoping it out both work for me; what I'd avoid is leaving it unstated now that the table reads as authoritative. Fix this →
Low
2. The enum branch reads the attribute, but the counter-example doesn't use one — JsonRpcGenerator.cs:370. All three attributed enums in the surface (PruningStatus, AbiDescriptionType, StateMutability) use LowerCaseJsonStringEnumConverter<T>, so the output is correct today and admin_prune is genuinely fixed. But TxTypeConverter is registered in the options list, not as an attribute — invisible to this check and only correct because of its explicit table row — and it writes hex, showing that attribute-present ⇒ _string_ holds by coincidence of the current three.
3. Three notes on the fallback path — JsonRpcGenerator.cs:431-440. type.FullName would keep generics from printing as ArrayPoolList`1; GetProperties includes indexers, which the serializer never writes (prophylactic, nothing hits it today); and the new warning list is worth pasting somewhere durable, because at least two entries are load-bearing guesses — AccessListForRpc has no public members at all, so accessList documents as a bare _object_ instead of an array of {address, storageKeys}, and SyncingResult documents isSyncing/syncMode, neither of which SyncingResultJsonConverter ever writes.
Notes
Still no test, and AGENTS.md asks for a regression test on every bug fix. The PR body states the rationale (manual verification against geth and the golden testdata/*.json), and there is no DocGen test project, so I'm treating that as acknowledged rather than re-raising it — though a golden-file check over one namespace is what would keep the next serializer change from silently shifting a documented shape, since neither build-tools.yml nor update-docs.yml asserts the generated markdown. Style-wise the diff matches the file's idiom: explicit types, no var, LINQ consistent with the surrounding tool code, and the table is now alphabetical. KeyType is still unread despite the PR description mentioning it, so map of X continues to drop the key type — same as master, cosmetic.
Verdict: the direction and the mechanics are sound, and this fixes a real set of factually wrong published shapes. Nothing here can touch a running node — build-time docs tool, no consensus, security or hot-path exposure. Finding 1 is the only one I'd want a decision on before merge (a fix or a stated scope boundary); findings 2 and 3 are fine to defer.
• branch fix/docgen-serializer-contract
| // An enum serializes as its numeric value unless a converter writes the member name instead | ||
| if (type.IsEnum) | ||
| return "_integer_"; | ||
| return type.GetCustomAttribute<JsonConverterAttribute>() is null ? "_integer_" : "_string_"; |
There was a problem hiding this comment.
Low — correct for every enum in the surface today, but the signal it reads is the one the counter-example doesn't use.
I checked all three enums that carry the attribute — PruningStatus, AbiDescriptionType, StateMutability — and each uses LowerCaseJsonStringEnumConverter<T>, so _string_ is right and admin_prune is now correct. Two caveats worth a comment or a tweak rather than a change of behaviour:
- The counter-example is
TxType, whose converter is registered inEthereumJsonSerializer.CreateOptions(new TxTypeConverter(), line 120) rather than as an attribute — so a converter presence check via the attribute cannot see it, and it only comes out right because_knownTypeNameshas an explicit row above. Any future enum converter added to that list the same way silently regresses to_integer_. TxTypeConverteralso shows the inference direction isn't sound in general: it writes"0x0", i.e. a converter on an enum can emit hex rather than the member name, so attribute-present ⇒_string_holds by coincidence of the current three.
Both are covered by keying off the resolved contract instead — e.g. treating GetContract(type)?.Kind is JsonTypeInfoKind.None on an enum as "a converter decides" — or, if you'd rather keep the check as-is, by tightening the comment to say what is actually being detected (a member-name-writing converter attribute) so the next reader doesn't assume options-registered converters are covered.
| _guessedTypeNames.Add($"{type.Namespace}.{type.Name}"); | ||
|
|
||
| if (type.IsInterface && type.IsGenericType) | ||
| { | ||
| if (type.GetGenericTypeDefinition() == typeof(IEnumerable<>)) | ||
| { | ||
| itemType = type.GetGenericArguments().Last(); | ||
| isDictionary = false; | ||
| const BindingFlags memberFlags = BindingFlags.Public | BindingFlags.Instance; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| if (type.GetGenericTypeDefinition() == typeof(IDictionary<,>)) | ||
| { | ||
| itemType = type.GetGenericArguments().Last(); | ||
| isDictionary = true; | ||
| // The serializer sets IncludeFields, so public fields reach the wire alongside properties | ||
| return type.GetProperties(memberFlags).Select(p => (Member: (MemberInfo)p, Type: p.PropertyType)) | ||
| .Concat(type.GetFields(memberFlags).Select(f => (Member: (MemberInfo)f, Type: f.FieldType))) | ||
| .Where(m => m.Member.GetCustomAttribute<JsonIgnoreAttribute>()?.Condition is not JsonIgnoreCondition.Always) | ||
| .Select(m => (Name: GetFallbackName(m.Member), Type: m.Type)) | ||
| .OrderBy(m => m.Name, StringComparer.Ordinal); |
There was a problem hiding this comment.
Low — three notes on the fallback path, none blocking.
-
$"{type.Namespace}.{type.Name}"still mangles generics. It does disambiguate the twoBlockHeaderForRpc(Nethermind.Facade.EthvsNethermind.Taiko.Tdx), which was the point — but a constructed generic still prints asNethermind.Core.Collections.ArrayPoolList`1.type.FullName ?? $"{type.Namespace}.{type.Name}"gives the full construction for free. -
GetPropertiesincludes indexers, which the serializer never writes. A converter-backed type withpublic T this[int i]would gain a phantomitemmember here. Nothing in the documented surface hits it today (ArrayPoolList<T>/CappedArray<T>are intercepted by the buffer-wrapper branch before they reach expansion), so this is prophylactic:.Where(p => p.GetIndexParameters().Length == 0). -
Worth capturing what the new warning line prints. The value of the diagnostic is that it names pages that are guesses, and at least two of them are guesses in a load-bearing way:
AccessListForRpchas no public properties or fields at all (itsIEnumerable<Item> _itemsis private,Nethermind.Facade/Eth/RpcTransaction/AccessListForRpc.cs:20), soaccessListdocuments as a bare_object_while the wire is an array of{address, storageKeys}.SyncingResultdocumentsisSyncingandsyncMode, neither of whichSyncingResultJsonConverterever writes — it emits eitherfalseor{startingBlock, currentBlock, highestBlock}(Nethermind.Facade/Eth/SyncingResultJsonConverter.cs:22-33).
Both predate this PR. Since the run now lists them, pasting that list into the PR (or a follow-up issue) would turn it into a work queue instead of console output nobody reads.
| if (contract?.Kind is JsonTypeInfoKind.Object) | ||
| return contract.Properties | ||
| .Where(p => p.Get is not null) | ||
| .Select(p => (Name: p.Name, Type: p.PropertyType)) |
There was a problem hiding this comment.
The contract path maps each member by its CLR PropertyType, so a long or ulong field written by a raw-number converter such as LongRawJsonConverter would still be documented as a hex string. Is that per-property converter case meant to be covered here, or left to the type-level mapping?
There was a problem hiding this comment.
Not coverable by the type-level mapping e.g. struct-log gas and transaction gas are both ulong but need different encodings. Will fix it in a follow-up PR, since this is an existing issue in master.
There was a problem hiding this comment.
One same-family gap to note: the enum-with-converter branch (GetJsonTypeName) hardcodes string, so an enum whose converter emits a number would be mislabeled, another member-converter case. Could later share the same probe as in #12869 (safe for enums, though not for value-dependent unions like eth_syncing).
Building on the serializer-contract refactor (#12868): the member's CLR type alone does not determine its wire form. A `long`/`ulong` field carrying `[LongRawJsonConverter]`/`[ULongRawJsonConverter]` serializes as a raw JSON number, overriding the hex-quantity string its type would otherwise imply. Read the converter from the member (the contract's AttributeProvider, or the CLR member on the fallback path) and document such fields as JSON integers. Fixes the trace_* result blockNumber and the debug_* Geth struct-log entries, completing the number-vs-hex mismatches reported in #12838. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Building on the serializer-contract refactor (#12868): a member's CLR type does not determine its wire form. A field carrying an explicit [JsonConverter] can emit something other than the hex-quantity string its type implies - e.g. two `ulong` fields where one has a raw-number converter (struct-log gas vs tx gas, raised in review). Instead of hard-coding the raw-number converter types, serialize a sample value through the member's converter and read back the JSON token: a Number/Boolean overrides the type-based label, while a String keeps its editorial flavour (hex data, hash, ...). Probing is scoped to explicit member converters only; probing a type's default serialization is unsound for value-dependent unions (e.g. eth_syncing returns `false` or an object). Completes #12838: trace_* result blockNumber and debug_* struct-log entries (gas/gasCost/pc/refund/step) now render as JSON integers, matching the wire format and Geth/OpenEthereum/Erigon. Verified by diffing generated docs against the base branch: the only changes are these fields; eth_syncing and byte-buffer fields are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A member's CLR type does not determine its wire form. A field carrying an explicit [JsonConverter] can emit something other than the hex-quantity string its type implies - e.g. two `ulong` fields where one has a raw-number converter (struct-log gas vs tx gas). Instead of hard-coding the raw-number converter types, serialize a sample value through the member's converter and read back the JSON token: a Number/Boolean overrides the type-based label, while a String keeps its editorial flavour (hex data, hash, ...). Probing is scoped to explicit member converters; probing a type's default serialization is unsound for value-dependent unions (e.g. eth_syncing returns `false` or an object). Completes #12838 on top of #12868: trace_* result blockNumber and debug_* struct-log entries (gas/gasCost/pc/refund/step) now render as JSON integers, matching the wire format and Geth/OpenEthereum/Erigon. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
Partial #12828, #12836, #12838, #12839 and #12841. Closes #12826.
Changes
DocGen re-derived the serializer's rules by inspecting CLR types, and disagreed with them. It now reads the same contract the RPC layer serializes with, via the public
EthereumJsonSerializer.JsonOptions.JsonPropertyInfo.Nameis the final wire name andGet is nullmarks a hidden member, so naming policy,[JsonPropertyName],[JsonIgnore]conditions, public fields and inherited members are no longer re-implemented. This surfaces the block fieldseth_simulateV1andtrace_simulateV1omitted.eth_callwas an object with abytesmember instead of a hex string;traceAddresswas an object withisUncappedinstead of an array. Atypeof-keyed table replaces thetype.Name switchand covers these. By-ref andNullable<T>unwrap first, removing thehasValue/valuewrappers.TxTyperow was unreachable and every transactiontypewas documented as_integer_while the wire is"0x2".int,uintandbytehave no hex converter and serialize as JSON numbers;BigIntegerwrites decimal digits, not hex. Enums serialize numerically unless a converter writes the name instead, soadmin_pruneis a string.Kind/ElementType/KeyType, deleting ~55 lines of hand-rolled detection.JsonTypeInfoKindselects the fallback rather than an empty property count, which would conflate "no contract" with "every member ignored"; public fields are included to match the serializer'sIncludeFields; and a type the serializer refuses to model degrades to its CLR shape instead of aborting the run. Listing the fallbacks makes a guessed page visible.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Each changed shape was checked against geth's implementation and golden
testdata/*.json, plus Nethermind's existing CI-asserted JSON.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Fixed beyond the filed issues
eth_call_object_with abytesmember_string_ (hex data)debug_getRawBlock/RawHeader/RawTransaction,eth_getRawTransactionBy*array of _string_ (hex data)_string_ (hex data)eth_feeHistory→rewardPercentilesarray of _object_array of _number_eth_simulateV1/trace_simulateV1typeon every transaction and receipt (31 places)_integer__string_ (transaction type)admin_prune_integer__string_debug_trace*struct logs →depth_string_ (hex integer)_integer_admin_nodeInfo→ports.*;parity_netPeers→active/connected/max;clique_getSnapshot→votes;eth_simulateV1→code_string_ (hex integer)_integer_admin_peers→version_string_ (hex data)_integer_