fix(dpp): surface identifier-typed document properties as base58 in JS - #4577
Conversation
The Document properties getter serialized Value::Identifier through the non-human-readable serde path, so identifier-typed properties (typed by binary document deserialization and index-key synthesis alike) reached JS as Uint8Array. The chained-query surface documents them as base58 — the form where-clauses accept back as a pagination cursor — and the platform-test-suite asserts it, failing the Test Suite and browser CI jobs on every PR since #4567. Convert Value::Identifier values to base58 strings in the properties getter (map keys already got this via stringify_map_keys_for_object); other binary properties stay Uint8Array, and toObject/toJSON are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — 40 ahead in queue (commit a768ceb) |
The suite drives the network through @dashevo/evo-sdk (wasm-dpp2 under it) since the chained-query coverage landed, but neither package was in the e2e-tests-changed filter, so changes to them shipped without the suite running. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDocument property serialization now exposes identifier values as Base58 strings while preserving binary values as ChangesIdentifier property serialization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Document properties now expose identifier-typed values as base58 strings while preserving existing representations for other values; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DocumentWasm
participant IdentifierConversion
participant JavaScriptObject
DocumentWasm->>IdentifierConversion: serialize document properties
IdentifierConversion->>IdentifierConversion: convert identifiers to Base58 text
IdentifierConversion->>JavaScriptObject: return identifiers as strings and bytes as Uint8Array
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4577 +/- ##
============================================
- Coverage 87.10% 84.88% -2.22%
============================================
Files 2756 2786 +30
Lines 359634 370054 +10420
============================================
+ Hits 313247 314118 +871
- Misses 46387 55936 +9549
🚀 New features to boost your workflow:
|
2 participants
Issue being fixed or feature implemented
Since #4567/#4568 merged, the platform-test-suite "Test Suite" and "Test Suite in browser (1)" CI jobs fail on every PR against v4.2-dev:
The
Documentpropertiesgetter in wasm-dpp2 serialized property values through the non-human-readable serde path, so identifier-typed properties reached JS asUint8Array. The chained-query surface documents them as base58 strings — that's the form inner where-clauses accept back, so a proven join value works as a pagination cursor directly (the README's documented pattern; aUint8Arraywouldn't even deserialize into the query's JSON where-clauses) — and the functional spec asserts it.No schema threading is needed for the fix: every typed decode path already marks these values as
Value::Identifier— binary document deserialization (DocumentPropertyTyperead) for full documents anddecode_value_for_tree_keysfor indexOnly index-key synthesis — so the variant alone identifies them at the JS boundary.What was done?
platform_value_to_object_with_base58_identifiersto wasm-dpp2's serialization conversions: recursively convertsValue::Identifiervalues to base58Value::Textbefore the usual object serialization. Map keys already surfaced identifier keys as base58 viastringify_map_keys_for_object; now values match.Documentpropertiesgetter uses it. Other binary properties (Value::Bytes*) still surface asUint8Array, andtoObject()/toJSON()are unchanged.This fixes the surface uniformly for every read path that returns
Documentwrappers (getDocuments,getChainedDocumentsinner and outer halves, DPNS, document history), in both Node and browser bundles — the failure was never browser-specific; the Node "Test Suite" job fails on the same assertion (e.g. run 33496971715).How Has This Been Tested?
toBytes/fromByteswith a contract carrying an identifier-typed property (contentMediaType: application/x.dash.dpp.identifier) surfaces that property as the base58 string, while other properties keep their shapes. This exercises the sameValue::Identifier-producing decode path the chained query verifier uses.cargo check/clippyfor wasm-dpp2 on wasm32 clean.IndexOnlyDocument.spec.jsasserts this exact surface against a local network in the Test Suite CI jobs on this PR.Also added
packages/wasm-dpp2/**andpackages/js-evo-sdk/**to thee2e-tests-changedCI filter: the suite drives the network through@dashevo/evo-sdk(wasm-dpp2 under it) since #4567, but changes to those packages didn't trigger the Test Suite jobs — including this PR's fix, which would otherwise merge unproven.Breaking Changes
None (no consensus impact). The JS
Document.propertiesgetter now returns base58 strings instead ofUint8Arrayfor identifier-typed properties; the only in-repo consumer asserting on this surface is the chained-query functional spec, which expects base58.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests