fix(wasm-sdk): support binary grove path elements#3657
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWASM SDK system queries now accept binary-safe grove path/key inputs (string or Uint8Array), decode them to raw bytes, and return expanded PathElement objects with byte-backed fields and derived metadata (elementType, sum, referenceTarget) via new JS getters. ChangesBinary-Safe GroveDB Path Queries
Sequence Diagram(s)sequenceDiagram
participant JS as JS (Facade)
participant WASM as wasm-sdk
participant Grovedb as Grovedb
JS->>WASM: pass GrovePathSegment[] (path, keys)
WASM->>WASM: decode segments -> byte vectors
WASM->>Grovedb: query KeysInPath with raw bytes
Grovedb-->>WASM: Element / proof data
WASM->>JS: PathElementWasm (with byte fields & metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
✅ Review complete (commit 3bc6c58) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR is a sound additive change in wasm-sdk to accept Uint8Array path/key inputs and surface richer element metadata. No blocking consensus or correctness issues in wasm-sdk itself. Three notable correctness/UX concerns: (1) the js-evo-sdk facade still narrows path/keys to string[], so the new binary capability isn't reachable through the recommended TS surface without unsafe casts; (2) the legacy path: Vec<String> field encodes non-printable binary keys as "0x<hex>" strings that do NOT round-trip through decode_path_input/decode_key_input, silently corrupting subsequent grove walks; (3) PathElement's toObject/toJSON (via impl_wasm_serde_conversions!) will serialize the new binary fields as number arrays and sum as a string, diverging from the wasm-bindgen getter shape. Several nitpicks around silent BigInt/reference error swallowing, redundant match arms, an extra Uint8Array copy, the decimal-string heuristic, and thin test coverage of GroveElementType variants.
Reviewed commit: 0165eaf
🟡 7 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
🟡 suggestion: Evo SDK facade still narrows path/keys to `string[]`, blocking binary use from TS callers
packages/js-evo-sdk/src/system/facade.ts (lines 44-53)
wasm-sdk now accepts GrovePathSegment[] (string | Uint8Array) for getPathElements and getPathElementsWithProofInfo, but SystemFacade.pathElements / pathElementsWithProof still declare path: string[], keys: string[]. TypeScript users of js-evo-sdk (the recommended SDK) cannot pass raw binary segments through the supported facade without unsafe casts, so the new binary capability is unreachable from the normal entry point. Widen both signatures to wasm.GrovePathSegment[] and update the proof variant's return type as needed.
💡 Suggested change
async pathElements(path: wasm.GrovePathSegment[], keys: wasm.GrovePathSegment[]): Promise<wasm.PathElement[]> {
const w = await this.sdk.getWasmSdkConnected();
return w.getPathElements(path, keys);
}
async pathElementsWithProof(path: wasm.GrovePathSegment[], keys: wasm.GrovePathSegment[]):
Promise<wasm.ProofMetadataResponseTyped<wasm.PathElement[]>> {
const w = await this.sdk.getWasmSdkConnected();
return w.getPathElementsWithProofInfo(path, keys);
}
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/queries/system.rs`:
- [SUGGESTION] lines 555-568: Legacy `path: Vec<String>` field is non-round-trippable for binary keys
When a Uint8Array containing non-printable bytes is passed (e.g. `[0x80, 0xff]`), `bytes_to_display` encodes it as the literal string `"0x80ff"` and stores it into `PathElementWasm::path: Vec<String>` exposed via the existing `path` getter. The grove-walking pattern of feeding a returned path segment back into `getPathElements` is broken: `decode_path_input(PathInputValue::String("0x80ff"))` returns the 6 UTF-8 bytes `[0x30,0x78,0x38,0x30,0x66,0x66]`, not the original 2-byte key. The original binary key is silently corrupted into a 6-byte ASCII key, and the next query returns a missing element with no error. Either drop string `path` segments for non-UTF8 keys (return `None`/empty so callers cannot misuse the field), or document explicitly that the legacy `path` field is lossy and `pathBytes`/`key` MUST be used whenever any segment originated from a Uint8Array.
- [SUGGESTION] lines 459-472: `PathElement.toObject()`/`toJSON()` will not produce the advertised JS shape for new binary fields
`PathElementWasm` opts into `impl_wasm_serde_conversions!`, which routes `toObject`/`toJSON` through serde. The newly added fields (`key: Vec<u8>`, `path_bytes: Vec<Vec<u8>>`, `value_bytes: Option<Vec<u8>>`, `reference_target: Option<Vec<Vec<u8>>>`) have no `#[serde(with = "serde_bytes")]` annotation, so serde will emit them as `number[]` / nested `number[][]`, and `sum` as a string. Callers using `toObject()`/`toJSON()` (including indirect use via `ProofMetadataResponseWasm` serialization helpers) will observe a different shape than the wasm-bindgen getters (`Uint8Array`, `BigInt`). Either annotate the byte fields with `serde_bytes` (and serialize `sum` as a numeric/string variant matching the getter shape), or skip them from the serde representation if the getters are intended to be the only public surface.
- [SUGGESTION] lines 645-660: Reference-resolution errors are silently discarded
`element_reference_target` uses `path_from_reference_path_type(...).ok()`, so an invalid reference (e.g. `UpstreamRootHeightReference` with `no_of_elements_to_keep > parent_path.len()` returning `ElementError::InvalidInput`) yields `referenceTarget = undefined` — indistinguishable from "this element is not a reference at all." A UI that uses `elementType === "reference"` to decide whether to dereference will be confused. Either log via `tracing::warn!` at this boundary or surface the failure in a separate `referenceTargetError` field so tree-explorer callers can show meaningful feedback.
- [SUGGESTION] lines 550-555: `decode_path_string`'s numeric-string heuristic is silent and lossy for legitimate UTF-8 segments
Any path string that happens to parse as `u8` (0–255) is coerced to a single byte — so a path segment literally named `"42"` becomes `[42]`, while `"256"` falls through to its 3-byte ASCII form. This asymmetry is now baked into the WASM boundary forever and is invisible to callers who built a JSON-style string path. Add a doc comment on `decode_path_input` describing the behavior, and consider emitting a `tracing::warn!` when the numeric branch fires so callers are nudged toward `Uint8Array` for unambiguous representation.
- [SUGGESTION] lines 568-590: `is_instance_of::<Uint8Array>` fails on cross-realm typed arrays
The check lowers to JS `instanceof Uint8Array` against the realm hosting the wasm-bindgen glue. A Uint8Array constructed in a different realm (iframe, ServiceWorker, vm context, jest worker) will not satisfy `instanceof` against the wasm module's `globalThis.Uint8Array`, fall through to the `as_string()` branch, fail that, and surface the misleading error `"path[i] must be a string or Uint8Array"` for a value that *is* a Uint8Array. Node's `Buffer` extends the same-realm Uint8Array so it's unaffected; browser callers passing data received via `postMessage` from a worker can hit this. Either duck-type via `ArrayBuffer.isView(value)` (with a name/byteLength probe) or at minimum document the same-realm requirement on `GrovePathSegment`.
- [SUGGESTION] lines 1388-1509: `GroveElementType` enum advertises ~40 variants; tests exercise ~8
The TS `GroveElementType` union lists every Element variant including newer wrapper-prefixed names (`nonCountedSumTree`, `notSummedBigSumTree`, `commitmentTree`, `mmrTree`, `bulkAppendTree`, `denseAppendOnlyFixedSizeTree`, etc.), but tests only touch `item`, `tree`, `sumTree`, `sumItem`, `reference`, `referenceWithSumItem`, `provableSumTree`, and one nested-wrapper case. The dispatch in `element_type_name` / `non_counted_element_type_name` / `not_summed_element_type_name` / `not_counted_or_summed_element_type_name` has subtle naming rules (e.g. NotSummed prefix only on sum-bearing trees). A table-driven test asserting each variant maps to the exact string in the TS enum would catch typos and drift when grovedb adds Element variants.
In `packages/js-evo-sdk/src/system/facade.ts`:
- [SUGGESTION] lines 44-53: Evo SDK facade still narrows path/keys to `string[]`, blocking binary use from TS callers
`wasm-sdk` now accepts `GrovePathSegment[]` (`string | Uint8Array`) for `getPathElements` and `getPathElementsWithProofInfo`, but `SystemFacade.pathElements` / `pathElementsWithProof` still declare `path: string[], keys: string[]`. TypeScript users of `js-evo-sdk` (the recommended SDK) cannot pass raw binary segments through the supported facade without unsafe casts, so the new binary capability is unreachable from the normal entry point. Widen both signatures to `wasm.GrovePathSegment[]` and update the proof variant's return type as needed.
PastaPastaPasta
force-pushed
the
codex/wasm-sdk-binary-grove-path-elements
branch
3 times, most recently
from
5cd13f9 to
fb78197
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3657 +/- ##
=========================================
Coverage 87.17% 87.17%
=========================================
Files 2601 2601
Lines 318160 318160
=========================================
+ Hits 277370 277371 +1
+ Misses 40790 40789 -1
🚀 New features to boost your workflow:
|
PastaPastaPasta
force-pushed
the
codex/wasm-sdk-binary-grove-path-elements
branch
2 times, most recently
from
68cc9f4 to
aaf11a0
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Client-side wasm-sdk surface widening to accept Uint8Array path/key segments and richer PathElement metadata. No consensus, proof, or DPP code touched. One real compatibility regression in legacy path rendering for byte-form keys whose UTF-8 representation parses as a decimal u8 (e.g. "0"–"255"), plus two FFI nitpicks (cross-realm Buffer rejection, double-copy of Uint8Array inputs).
Reviewed commit: aaf11a0
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/queries/system.rs`:
- [SUGGESTION] lines 732-743: Byte-form keys whose UTF-8 parses as a decimal u8 lose their legacy `path` string
`decode_key_input` for `PathInputValue::Bytes` delegates to `bytes_to_round_trippable_path_display` (lines 819-831), which checks round-trippability with `decode_path_string_silent` — the *path* string decoder that treats decimal strings in the u8 range as a single byte. Keys are documented as plain UTF-8, not legacy decimal-u8 segments, so for any key whose bytes are the UTF-8 of a decimal in 0..=255 the check fails: bytes `[0x39, 0x36]` decode to `"96"`, `decode_path_string_silent("96")` returns `[0x60]`, which mismatches the original bytes, and `legacy_path_segment` becomes `None`. The same key passed as the string `"96"` keeps `legacy_path_segment = Some("96")`, so `PathElement.path` (lines 485, 509) is emitted as `[]` for the byte form but `["96"]` for the string form. Callers that migrate to `Uint8Array` for safety but still rely on the legacy `path` field regress for any UTF-8 key that happens to be a small-integer literal. Use a key-specific round-trip rule (e.g. `std::str::from_utf8(bytes).ok().map(str::to_owned)`) instead of reusing the path-segment decoder.
aaf11a0 to
3bc6c58
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior substantive findings are resolved on this head. Remaining items are minor: a backward-compat regression in legacy path rendering for valid non-ASCII UTF-8 segments, plus four small nitpicks (cross-realm Node Buffer rejection, double-copy of incoming Uint8Array, silently swallowed BigInt error in the sum getter, and a dead/redundant wrapper arm in two element-name helpers).
Reviewed commit: 3bc6c58
🟡 1 suggestion(s) | 💬 4 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/queries/system.rs`:
- [SUGGESTION] lines 819-843: Legacy `path` is dropped for valid non-ASCII UTF-8 segments that would round-trip via the string API
`bytes_to_round_trippable_path_display` and `bytes_to_round_trippable_key_display` require every byte to be `is_ascii_graphic()` (or space). That is stricter than the round-trip rules actually used on this boundary. `decode_key_input(PathInputValue::String(value))` uses `value.as_bytes()`, so *any* valid UTF-8 key round-trips through the legacy string API. `decode_path_string()` falls back to UTF-8 bytes for anything that does not parse as a decimal `u8`, so any non-decimal valid UTF-8 path segment also round-trips. As written, callers that switch from `string` to `Uint8Array` for an input like `"café"` (or any other non-ASCII UTF-8 segment) get `path: []` back even though the byte sequence is safely representable. Loosen the filter to UTF-8 + (for paths) the existing `decode_path_string_silent` round-trip check; for keys, drop the ASCII-only condition entirely.
The legacy-display fallback for binary grove path inputs was gated on every byte being ASCII-graphic-or-space, which dropped perfectly valid round-trip cases. For keys, decode_key_input(String) just calls value.as_bytes(), so any valid UTF-8 trivially round-trips; for paths, the existing decode_path_string_silent equality check is the real correctness guarantee. Loosen both filters so callers that switch from string to Uint8Array inputs like "café" still get the human-readable legacy path segment instead of an empty path. Addresses CodeRabbit review on PR #3657.
- Accept cross-realm Node Buffer in is_uint8_array_value by also matching constructor.name === "Buffer". - Skip the redundant JS-side Uint8Array allocation when decoding path/key segments by reinterpreting the already-verified JsValue via unchecked_ref. - Drop the dead wrapper arm in not_summed_/not_counted_or_summed_element_type_name; the catch-all already dispatches wrappers correctly through element_type_name. - Surface BigInt construction failures in the sum getter via expect() instead of silently producing undefined, so a future invariant violation is visible. Addresses CodeRabbit review nits on PR #3657.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All six agent reviews (general, security, FFI from both Claude and Codex) converge on zero findings at head bfd640e. All five prior findings from 3bc6c58 are confirmed FIXED: UTF-8 round-trip for legacy path display, cross-realm Buffer acceptance, redundant Uint8Array copy removal via unchecked_ref, dead wrapper match arms removed in not_summed_/not_counted_or_summed_ helpers, and the sum getter's silent BigInt error replaced with .expect(). The latest delta is scoped to the wasm-sdk JS/WASM boundary (no consensus, proof, or server-side code touched) and introduces no new correctness, security, or boundary issues.
The legacy-display fallback for binary grove path inputs was gated on every byte being ASCII-graphic-or-space, which dropped perfectly valid round-trip cases. For keys, decode_key_input(String) just calls value.as_bytes(), so any valid UTF-8 trivially round-trips; for paths, the existing decode_path_string_silent equality check is the real correctness guarantee. Loosen both filters so callers that switch from string to Uint8Array inputs like "café" still get the human-readable legacy path segment instead of an empty path. Addresses CodeRabbit review on PR #3657.
- Accept cross-realm Node Buffer in is_uint8_array_value by also matching constructor.name === "Buffer". - Skip the redundant JS-side Uint8Array allocation when decoding path/key segments by reinterpreting the already-verified JsValue via unchecked_ref. - Drop the dead wrapper arm in not_summed_/not_counted_or_summed_element_type_name; the catch-all already dispatches wrappers correctly through element_type_name. - Surface BigInt construction failures in the sum getter via expect() instead of silently producing undefined, so a future invariant violation is visible. Addresses CodeRabbit review nits on PR #3657.
Latest v3.1-dev brought in a new grovedb Element variant ProvableCountProvableSumTree. Add it to element_sum (it carries a sum) and to every element_type_name dispatcher, plus the GroveElementType TS union, so PathElement keeps reporting an element type instead of failing exhaustiveness checks.
PastaPastaPasta
force-pushed
the
codex/wasm-sdk-binary-grove-path-elements
branch
from
bfd640e to
04bd8f4
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of PR #3657 at 04bd8f4. The PR is scoped to wasm-sdk grove path-element surface plus the js-evo-sdk facade. The latest delta (bfd640e..04bd8f4) is a single additive commit registering Element::ProvableCountProvableSumTree in element_sum and the four element_type_name dispatchers, with matching TS union strings. No carried-forward prior findings remain open. The only new finding worth posting is a test-coverage gap: the table-driven element-type regression test added earlier in this PR was not extended to cover the new variant. The codex-ffi-engineer finding about Swift dropping DriveInternalError is dropped — that code lives outside this PR's diff (rs-sdk-ffi/src/error.rs and swift-sdk/SDK.swift are not modified here).
💬 1 nitpick(s)
NITPICK: Extend the element-type regression test to cover ProvableCountProvableSumTree
packages/wasm-sdk/src/queries/system.rs (line ?)
The latest commit adds Element::ProvableCountProvableSumTree handling to element_sum (line 869), element_type_name (line 917), and all three wrapper dispatchers — non_counted_element_type_name (943), not_summed_element_type_name (963), and not_counted_or_summed_element_type_name (977) — exposing four new GroveElementType TS strings. The should_map_supported_grovedb_element_types table was added earlier in this PR specifically to pin the dispatcher↔TS-union mapping, but it does not include a row for the new variant or any of its wrapper-prefixed names. A future typo or refactor in any of the four new branches will silently slip past the test. Add at least one case for the base variant, and ideally one wrapped form, to restore the invariant the table-driven test was designed to enforce.
(
Element::ProvableCountProvableSumTree(None, 1, 2, None),
"provableCountProvableSumTree",
),
(
Element::NonCounted(Box::new(Element::ProvableCountProvableSumTree(
None, 1, 2, None,
))),
"nonCountedProvableCountProvableSumTree",
),
(
Element::NotSummed(Box::new(Element::ProvableCountProvableSumTree(
None, 1, 2, None,
))),
"notSummedProvableCountProvableSumTree",
),
(
Element::NotCountedOrSummed(Box::new(Element::ProvableCountProvableSumTree(
None, 1, 2, None,
))),
"notCountedOrSummedProvableCountProvableSumTree",
),
3 participants
Issue being fixed or feature implemented
This PR fixes the wasm/evo-sdk exact-key GroveDB lookup path used by proof-tree exploration.
The underlying DAPI
KeysInPathquery already accepts bytes, but the public wasm boundary only acceptedstring[]for path segments and keys. That forced callers with raw proof-tree keys or 32-byte identifiers to represent opaque bytes as JavaScript strings before the query reached Rust. That is unsafe for arbitrary GroveDB keys: strings are text, not byte containers, so bytes such as0x80or0xffcannot be round-tripped through that API shape.This also improves the returned
PathElementshape. Fetching an exact key is not enough for a tree explorer unless the caller can tell whether the result is an item, tree, sum tree, reference, etc., and can keep using raw key/path/value bytes for follow-up lookups.This PR intentionally does not add arbitrary subtree/range enumeration.
PathQuery/SizedQuerystill needs a follow-up protocol/server/SDK API beyond this wasm-sdk exact-key binding.What was done?
getPathElementsandgetPathElementsWithProofInfoto acceptArray<string | Uint8Array>for both path segments and keys.Uint8Arraypath/key segments are copied as raw bytes."96"still decodes to[96]."96"decodes to[0x39, 0x36]; callers should useUint8Arrayfor numeric or opaque binary keys.pathstrings for non-round-trippable binary keys. Callers that need byte-accurate navigation should usepathBytesandkey.Uint8Arraydetection more tolerant of cross-realm typed arrays by checkingArrayBuffer.isViewplus typed-array constructor name, not only same-realminstanceof.PathElementadditively while preserving the existing base64valuegetter:key: Uint8ArraypathBytes: Uint8Array[]valueBytes?: Uint8ArrayelementType?: GroveElementTypesum?: bigintreferenceTarget?: Uint8Array[]referenceTargetError?: stringelementType, sum, value bytes, and reference metadata from decoded GroveDBElementvariants, including newer wrapper variants onv3.1-dev.referenceTargetthrough GroveDB'spath_from_reference_path_typehelper. If GroveDB rejects a reference path, the failure is surfaced asreferenceTargetErrorinstead of silently looking like a non-reference.PathElement.toJSON()JSON-safe: byte fields serialize as base64 strings andsumserializes as a string.PathElement.toObject()match the typed getter shape: byte fields areUint8ArrayandsumisBigInt.@dashevo/evo-sdkSystemFacade.pathElements/pathElementsWithProofsignatures towasm.GrovePathSegment[], so the new binary capability is exposed through the recommended SDK facade.How Has This Been Tested?
cargo test -p wasm-sdk system::tests --no-default-features --features mocks,all-system-contractsCC_wasm32_unknown_unknown=/opt/homebrew/opt/llvm/bin/clang cargo check -p wasm-sdk --target wasm32-unknown-unknownCC_wasm32_unknown_unknown=/opt/homebrew/opt/llvm/bin/clang npx yarn workspace @dashevo/wasm-sdk buildnpx yarn workspace @dashevo/wasm-sdk test:unitnpx yarn workspace @dashevo/evo-sdk buildnpx yarn workspace @dashevo/evo-sdk test:unitThis pull request was created by Codex.
Breaking Changes
None. Existing string inputs and the existing base64
PathElement.valuegetter are preserved. The richer byte/type fields are additive.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Tests