Add const XDR serialization - #580
leighmcculloch wants to merge 63 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 551 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
fuzz/Cargo.toml:28
- This target source is also missing from
fuzz/fuzz_targets, so Cargo manifest loading and the new fuzz CI job fail. Please addledger-close-meta.rsor remove this bin and its corresponding Makefile/README references.
| | Target | Type | | ||
| | --- | --- | | ||
| | `transaction-envelope` | `TransactionEnvelope` | | ||
| | `ledger-close-meta` | `LedgerCloseMeta` | |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 554 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
README.md:70
- This repeats the unsupported “every type” claim: the scalar aliases
Int32,Int64,Uint32, andUint64have no inherentconst_xdr_lenorconst_to_xdrmethods. Please document that they use the correspondingConstWriter::write_*primitives instead.
fuzz/Cargo.toml:21
- These two bin entries point at files that are not present in the PR. Cargo resolves every declared target before building, so
make fuzzcurrently fails immediately with “can't find binledger-close-meta” (as the PR's fuzz-corpus job also reports) and never runs any corpus. Please add both target source files or remove their manifest/Makefile entries until they exist.
fuzz/src/lib.rs:46 - This treats the normal
(Err, Err)case as a fuzz failure.Arbitraryroutinely rejects short or unsuitable byte streams, and when both forms reject the same input there is no encoder discrepancy; panicking here makes ordinary mutations look like crashes. Only panic when exactly one result isOk, as the differential unit test already does.
fuzz/README.md:15 - The target table omits the newly declared
spec-entrytarget, so the fuzzing documentation is incomplete.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 554 changed files in this pull request and generated no new comments.
Suppressed comments (2)
fuzz/Cargo.toml:21
- The manifest declares the
transaction-envelopeandledger-close-metafuzz binaries, but neither referenced source file exists (onlyfuzz_targets/spec-entry.rsis present). As a result, those advertised targets—and the Makefile coverage loop that enumerates every manifest target—fail when Cargo tries to build them. Please either add both target sources or remove these stale entries.
fuzz/README.md:16 - These documented targets have no corresponding files, while the actual
spec-entrytarget is omitted, so the example command fails as written. Replace the table rows and command with thespec-entrytarget that this PR actually adds.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 554 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
fuzz/README.md:20
- This example cannot run because
fuzz_targets/transaction-envelope.rsis absent. Use the includedspec-entrytarget so the documented command works.
fuzz/Cargo.toml:21
- These two
[[bin]]entries point to files that are not present in the PR; onlyfuzz_targets/spec-entry.rsexists. This leaves the fuzz package advertising targets that cannot be built or run. Remove the stale entries (or add the missing target files).
fuzz/README.md:15 - This table lists two fuzz targets that are not included, while omitting the actual
spec-entrytarget. Update the table to match the targets that can be run from this checkout.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 554 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
fuzz/README.md:20
- This example starts an unbounded fuzz run even though the section below explains that every iteration leaks and requires bounded campaigns. Following it will eventually exhaust memory; use the existing target and add a time or run bound.
fuzz/Cargo.toml:21
- The
transaction-envelopetarget declared here has nofuzz_targets/transaction-envelope.rsfile (andledger-close-meta.rsis also absent). Consequently the README's example command and both advertised targets fail when selected. Please add the target sources or remove the stale declarations and documentation.
| /// the same bytes. Unlike the streaming path it enforces no depth or length | ||
| /// limits: a const value is fixed at compile time, so there is no untrusted | ||
| /// input to bound. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 554 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
README.md:70
- This repeats the same over-broad API guarantee as the crate docs. The
constmodule also re-exports transparent primitive aliases (Int32,Int64,Uint32, andUint64), which cannot have these inherent methods; callers must use the correspondingConstWriterprimitive methods. Please qualify this text before regenerating the README.
fuzz/README.md:20 - This command fails because the package defines only the
spec-entryfuzz target (fuzz/Cargo.toml:19-21), nottransaction-envelope.
fuzz/README.md:15
- These targets do not exist in this fuzz package:
fuzz/Cargo.toml:19-21defines onlyspec-entry. Update the table so readers can discover the target that this PR actually adds.
| @@ -1,3 +1,5 @@ | |||
| .PHONY: all test build doc install readme fuzz fuzz-reduce fuzz-corpus-json watch generate generate-files clean fmt publish | |||
Tip
Reviewing Tips
I would focus a review of this change on two areas:
ConstWritertype and the primitive XDR encoding methods.Note
Part of a stack of PRs that must merge in this order.
A first group of PRs change contract specs so that they are produced at compile time instead of at proc-macro execution time. This provides the foundation to construct the specs from information that is only known at compile time, like the fully qualified name of a type:
A second group of PRs changes the type names that the sdk stores in specs are fully qualified type names, and then the cli reduces them down to unique simple identifiers. During the contract build the types are given names like
::mycrate::mymod::MyTypeinstead ofMyType. Then the cli reduces them back down to simple names after spec shaking. Qualified type names make it possible to uniquely identify types in the spec, even when they have the same name. This resolves several problems with contract specs the type identify problem (stellar/rs-soroban-sdk#1570), type aliases limitations (stellar/rs-soroban-sdk#1857 stellar/rs-soroban-sdk#1063), and optimise spec shaking data section size (stellar/rs-soroban-sdk#1978):Note that downstream clients and SDKs should see no, or little, change because the cli will during the build process reduce the fully qualified names back to simple unique names. Contracts that had colliding type names, which meant they could not be used with clients, will now work.
A third group of PRs are an optimisation to spec shaking v2, and will use the new unique type names to shake type specs by reachability, producing a dependency graph (thanks @mootz12), so that only spec entries that can't be reached from fns, like errors and events, get spec markers from dead-code-elimination:
What
Add a
constfeature giving every generated type an alternative XDR encoding that can be used in const contexts. Add a static-lifetime borrowing{Type}Constcounterpart for every generated type that directly or transitively contains heap data.Why
The owned types require heap allocation to construct, so XDR values can't be built from borrowed slices inside const contexts. The const variant types can be constructed in const contexts.
XDR encoding is used by soroban-sdk to encode contract specs during proc-macro execution. The soroban-sdk will be changing when it encodes contracts specs to during compilation so that the specs can contain information only available during compile.
Known limitations
The
{Type}Consttypes are locked down only for use with borrowed data from static lifetimes, which isn't entirely necessary. An earlier implementation I did in #560 and #562 had these borrowed types as{Type}Ref<'a>where the lifetime was configurable and the type looked like it was useful as a ref type. However, in practice, because of the nature that these ref types get nested within arrays inside other ref types, they are not convertible to and from owned types and not really useful outside of a const static lifetime context. For that reason I've kept the types hyper focused on the immediate use case. If we ever have a use case for a more general ref variant of generated types we can reexplore #560.The const implementation is a separate implementation because a const implementation cannot use traits, which means it can't stream to a writer. It is not practical to maintain a single implementation that offers the same features of both. So instead there are two implementations and a test ensures they work identically.
I would have really liked to fuzz test the const xdr encoding with the existing xdr encoding, however I don't see a way to do that. We can't convert from owned to const types because of the way the const types are structured. Even if they were ref types with lifetimes we can't generate ref types from arbitrary because there would be nothing to own the values. Open to feedback on this, it's possible a slightly different way of architecting the ref types might make this possible.