Add JsonInputArchive overrides to XDRCereal#5130
Add JsonInputArchive overrides to XDRCereal#5130drebelsky wants to merge 2 commits intostellar:masterfrom
Conversation
drebelsky force-pushed the add-cereal-overrides branch from ab727fc to 92f99fb Compare | auto setAlpha = [&assetCode, &issuer](auto& alpha) { | ||
| alpha.assetCode.fill(0); | ||
| std::copy(assetCode.begin(), assetCode.end(), alpha.assetCode.begin()); | ||
| alpha.issuer = issuer; | ||
| }; |
There was a problem hiding this comment.
In the JSONInputArchive Asset override, assetCode read from the "assetCode" string is not length-checked before copying into the fixed-size opaque_array in setAlpha(...). If the JSON contains a long string, std::copy(assetCode.begin(), assetCode.end(), alpha.assetCode.begin()) will write past the end of the destination array (UB). Validate assetCode.size() (<= 4 or <= 12 as appropriate) before copying, and throw/fail cleanly when it's out of range.
| std::string hex; | ||
| xdr::archive(ar, hex, field); | ||
| auto bin = stellar::hexToBin(hex); | ||
| releaseAssertOrThrow(bin.size() == N); | ||
| std::copy(bin.begin(), bin.end(), s.begin()); | ||
| } |
There was a problem hiding this comment.
The new JSONInputArchive overrides use releaseAssertOrThrow(...), but XDRCereal.h doesn't include util/GlobalChecks.h where it's defined. Add the include (ideally within the BUILD_TESTS block) so test builds don't depend on transitive includes.
| @@ -0,0 +1,684 @@ | |||
| #include "crypto/StrKey.h" | |||
| #include "test/Catch2.h" | |||
| #include "util/Decoder.h" | |||
There was a problem hiding this comment.
This test file uses releaseAssert(...) in helpers but doesn't include util/GlobalChecks.h where it is defined. Add the missing include to avoid compilation failures that depend on transitive includes.
| #include "util/Decoder.h" | |
| #include "util/Decoder.h" | |
| #include "util/GlobalChecks.h" |
| #include "crypto/StrKey.h" | ||
| #include "test/Catch2.h" | ||
| #include "util/Decoder.h" | ||
| #include <fmt/format.h> |
There was a problem hiding this comment.
<fmt/format.h> and crypto/StrKey.h appear unused in this test file; removing them will reduce compile time and avoid unused-include churn.
| #include "crypto/StrKey.h" | |
| #include "test/Catch2.h" | |
| #include "util/Decoder.h" | |
| #include <fmt/format.h> | |
| #include "test/Catch2.h" | |
| #include "util/Decoder.h" |
| #include "transactions/TransactionUtils.h" | ||
| #include "util/types.h" | ||
| #include <cereal/archives/json.hpp> | ||
| #include <string> |
There was a problem hiding this comment.
The new JSONInputArchive enum override uses std::unordered_map but XDRCereal.h doesn't include <unordered_map> (and it also uses std::back_inserter without including ). Add the missing standard headers (and keep them within the BUILD_TESTS guard if you want to limit production impact) to avoid non-deterministic builds relying on transitive includes.
| #include <string> | |
| #include <string> | |
| #include <unordered_map> | |
| #include <iterator> |
| if (std::is_same_v<std::decay<decltype(assetCode)>, | ||
| stellar::AssetCode12> && | ||
| first0 <= 4) | ||
| { | ||
| valid = false; | ||
| } |
There was a problem hiding this comment.
The ASSET_CODE_12 validity check uses std::decay<decltype(assetCode)> instead of std::decay_t<...> (or typename std::decay<...>::type). As written, the condition is effectively never true, so the first0 <= 4 rule for AssetCode12 won't be enforced and invalid codes may serialize as strings instead of raw bytes.
| xdr::archive(ar, assetCode, "assetCodeRaw"); | ||
| } | ||
| }; | ||
| std::string code; |
There was a problem hiding this comment.
std::string code; is now unused after switching to the archive(...) lambda. Please remove it (or use it) to avoid unused-variable warnings (which may be treated as errors in some builds).
| std::string code; |
Resolves #5041. Note that the overrides are intentionally hidden behind
BUILD_TESTSbecause we shouldn't use the input overrides on production code (we should store XDR as XDR, not our custom JSON overrides that break every so often). The overloads also don't get used anywhere else, but I think it's probably still worth having them + the tests in the repo, so we can quickly write an ad-hoc tool to parse core JSON if we need to again.Testing notes
The tests right now are written to check that we can roundtrip 1000 random values of most of our XDR types. Unfortunately, due to the heavy template instantiation, compilation takes ~1 minute on my laptop. With the current configurable values (1000 random values per type, use 1 for
levelsforxdr::generator_t, limit theautocheck::arbitraryto generating things of at most size 10), the tests take about a minute to run. For reference, 1000 random values for type, limitingautocheck::arbitraryto 5, and using thexdrppautocheck::generatoroverride took ~17 seconds. Limiting the nesting depth is probably fine since we're checking the nested structures.Alternatively, we could try roundtripping (by hand or generation) a subset of the particular overloads. This has the benefit of compiling faster. However, it also requires us to actively keep the tests up-to-date (the class list can be updated more intermittently, especially since including things like
BucketEntryhelp gives provide coverage for future types).There's also a basic set of unit tests that check that the overrides get called.