Repository navigation
feat: add Value::same_scalar_data for comparing single values across variants - #5125
Conversation
…variants `Value::same_scalar_data` is `equal_underlying_data` for single values: bytes-like variants compare by their bytes, integers by value at any width, and anything else with `==`. It differs in two ways. A map or an array on either side never matches. When either side is a float, both are read as `f64` (an integer converting as `to_float` does) and compared by bits, so `-0.0` is not `0.0` and a NaN equals the same NaN, as a float's tree key tells them apart. `equal_underlying_data` is left as it is: shipped generations call it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesProperty Agreement Comparison
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The property-agreement change is mergeable after normal checks; no actionable issue remains from this review. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new rule changes which document values can satisfy a reference agreement. Contract type checks and the existing mismatch and identity checks remain in place, and no new authorization bypass was established. Input normalization and behavior across protocol upgrades remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
github-actions
Bot
added
the
waiting-bots
|
🕓 Queued for automated review — 3rd in line, estimated start in ~30 min (commit a410611)
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The additive helper has no production callers and leaves existing comparison behavior unchanged; no blocking issues were found. Verification identified an inaccurate tree-key equivalence claim, an avoidable allocation, and overlapping documentation/test gaps around integer rounding. All 10 targeted same_scalar_data tests passed at the exact reviewed head.
🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 4: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff adds a small, self-contained Value::same_scalar_data method with extensive tests, reuses existing conversions and equality logic, and introduces no callers or changes to existing consensus behavior. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 95% left, weekly 99% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort medium); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort medium); agentphase2-reviewer
🤖 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/rs-platform-value/src/eq.rs`:
- [SUGGESTION] packages/rs-platform-value/src/eq.rs:229-233: Do not claim float bits exactly match document index keys
The claimed equivalence does not hold for every float accepted by this helper. Signed zeros actually produce distinct keys, contrary to the original review's counterexample. However, DocumentPropertyType::encode_float branches on val < 0.0, so negative NaNs take the sign-bit-XOR branch instead of the complement branch. Consequently, f64::from_bits(0xfff8000000000000), a negative NaN, and f64::from_bits(0x8007ffffffffffff), a negative finite subnormal, both encode as 0x7ff8000000000000 even though same_scalar_data rejects the pair. Document this API as bit-level equality rather than exact tree-key equivalence, and update the matching test comment and PR explanation. This does not require changing the existing encoder or the helper's intentionally documented bitwise semantics.
- [SUGGESTION] packages/rs-platform-value/src/eq.rs:253-256: Use as_float to avoid constructing discarded errors
Comparing a Float against a nonnumeric scalar calls to_float on that scalar, allocating the String inside Error::StructureError before immediately discarding the error. The existing as_float accessor has the same numeric variants and casts but returns None without allocating. Using it preserves every comparison result while avoiding unnecessary error construction in this boolean predicate.
- [SUGGESTION] packages/rs-platform-value/src/eq.rs:478-486: Pin integer rounding and document its nontransitive comparison
The integer/float tests exercise only exactly representable integers, leaving the explicitly documented rounding behavior untested. Add assertions showing that U64(2^53 + 1) matches Float(2^53) but does not match U64(2^53). Both integers match that float, so also state in the API documentation that the comparison is not transitive. Boundary assertions can cover I128(i128::MIN) against Float(-2.0f64.powi(127)) and U128(u128::MAX) against Float(2.0f64.powi(128)), with an infinity contrast. The original proposed I128 assertion used -2^63; that is incorrect because i128::MIN is -2^127.
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
A transition may carry an identifier or a byte array as an array of U8 values, which to_identifier_bytes already reads as those bytes. Treating every array as a container refused such a value against the same bytes stored as an identifier. An array whose every element is a U8 now compares as its bytes; any other array, and any map, still never matches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
waiting-bots
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
…uality - read both sides with as_float, which returns None instead of building an error that is thrown away - describe the float rule as bit equality: the tree key encoding maps a few distinct bit patterns to one key, so the two are not the same - note that rounding past 2^53 makes the integer/float comparison not transitive, and pin it with a test, including the i128::MIN and u128::MAX boundaries and an infinity contrast Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
waiting-bots
Value::same_scalar_data now reads an array of U8 values as the bytes it lists (#5125), as the index key comparison did, so a moderation charter application that writes its target contract that way agrees with the stored identifier again. Note 56, the check's comment and the book say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at a27bd29: the additive helper leaves existing comparison behavior unchanged, and all three prior findings are fixed. One non-blocking performance suggestion remains in the array comparison path; no blocking issues were found. Local validation passed 1,304 unit/integration tests, 94 all-feature documentation tests, and clippy with warnings denied; the working tree remains unchanged.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff adds a small, self-contained comparison helper in rs-platform-value/src/eq.rs with extensive edge-case tests and no callers or changes to existing consensus behavior. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 97% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort medium); agentphase2-reviewer,gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort medium); agentphase2-reviewer
🤖 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/rs-platform-value/src/eq.rs`:
- [SUGGESTION] packages/rs-platform-value/src/eq.rs:258-267: Compare array bytes without materializing both operands
When either operand is an Array, held_bytes collects each valid U8 array into a Vec and copies any bytes-like counterpart into another Vec solely for comparison. This uses temporary memory proportional to both inputs and traverses complete valid inputs even when their lengths or first bytes differ. Because this API explicitly supports arbitrarily long values, the overhead is not limited to fixed-size identifiers. Split array/array and array/bytes comparisons, check lengths first, and compare borrowed elements directly while requiring each array element to be Value::U8. This preserves the documented semantics without temporary buffers and permits early rejection. Returning Cow for bytes-like operands would avoid their copies, but would still leave the array allocation.
| fn held_bytes(value: &Value) -> Option<Vec<u8>> { | ||
| match value { | ||
| Value::Array(items) => items | ||
| .iter() | ||
| .map(|item| match item { | ||
| Value::U8(byte) => Some(*byte), | ||
| _ => None, | ||
| }) | ||
| .collect(), | ||
| _ => value.as_bytes_slice().ok().map(<[u8]>::to_vec), |
There was a problem hiding this comment.
🟡 Suggestion: Compare array bytes without materializing both operands
When either operand is an Array, held_bytes collects each valid U8 array into a Vec and copies any bytes-like counterpart into another Vec solely for comparison. This uses temporary memory proportional to both inputs and traverses complete valid inputs even when their lengths or first bytes differ. Because this API explicitly supports arbitrarily long values, the overhead is not limited to fixed-size identifiers. Split array/array and array/bytes comparisons, check lengths first, and compare borrowed elements directly while requiring each array element to be Value::U8. This preserves the documented semantics without temporary buffers and permits early rejection. Returning Cow for bytes-like operands would avoid their copies, but would still leave the array allocation.
source: glm-5.3-flash (phase1-reviewer: general, architecture-layering, rust-quality); gpt-6-astra (phase2-reviewer: rust-quality)
…V14) (#5102) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-28T17:49:51.584Z |
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
2 participants
Basic explanation
What this does: It adds one method,
Value::same_scalar_data, that answers "do these two single values hold the same data?" even when they are stored in different variants: the number 513 as aU64and as aU16, or an identifier and the same 32 bytes. It works like the existingValue::equal_underlying_data, with two differences: a map, or an array of anything butU8values, never matches (an array ofU8s is the bytes it lists, asto_identifier_bytesreads it), and two floats must have exactly the same bits, so-0.0and0.0are different. Nothing calls it yet. #5102, which is stacked on this pull request, uses it to compare the two sides of arefersTopropertyAgreement.Example: there is no "before" for users, since the method is new. The difference from
equal_underlying_data:equal_underlying_datasame_scalar_dataFloat(-0.0)andFloat(0.0)==onf64)Float(NaN)and the sameFloat(NaN)U64(1_700_000_000_000)andFloat(1.7e12)Map([("a", U8(1))])andMap([("a", U64(1))])Array([U64(1)])andArray([U64(1)])Array([U8(7); 32])andIdentifier([7; 32])U64(513)andU16(513)Identifier([7; 32])andBytes32([7; 32])Text("")andText("\0")Value: Comparing two single values the way a document index sees them (floats by their bits) no longer needs a hand-written helper per caller, and the comparison lives next to
equal_underlying_datawhere its sibling rules are.Risks: None on its own: the method is new and has no caller in this pull request.
equal_underlying_datais not touched, since shipped generations call it at every protocol version.Issue being fixed or feature implemented
#5102 compares a
propertyAgreementpair as two values rather than as two index keys. It addedDocumentPropertyType::values_are_equalin dpp for that, which mostly repeatsValue::equal_underlying_data.equal_underlying_datacannot serve directly: it recurses into maps and arrays, compares floats with==(so-0.0equals0.0, whose tree keys differ), and does not equate a float with an integer, while a document storesdateandnumberproperties as floats and a transition may carry one as an integer. It also cannot change, because the document replace transformer and proof verification call it at every protocol version. This adds the single-value comparison to platform-value under its own name, so #5102 can drop its dpp helper.What was done?
Value::same_scalar_data(&self, other: &Value) -> boolinpackages/rs-platform-value/src/eq.rs, next toequal_underlying_data:Arrayon either side: equal only when both sides hold the same bytes, reading an array whose every element is aU8as the bytes it lists (asto_identifier_bytesdoes) and any bytes-like variant as its bytes. A transition may carry an identifier or a byte array that way.Mapon either side: not equal.Floaton either side: both sides read asf64throughas_float(so an integer converts, rounding past 2^53 as storage does, which makes the comparison not transitive there) and compared withf64::to_bits. A non-number against a float is not equal.equal_underlying_data, which for two single values compares bytes-like variants (Bytes,Bytes20,Bytes32,Bytes36,Identifier) by their bytes, integers by value throughas_i128_unified(aU128pasti128::MAXequals only the sameU128), and everything else with==: two nulls are equal, a null equals nothing else, text compares as text, booleans as booleans.The doc comment explains that floats compare by bits (so
-0.0and0.0differ, as their tree keys do; this is bit equality, not tree key equality, since the key encoding maps a few distinct bit patterns to one key) and that there is no size limit, unlike the 255 bytes of a tree key.How Has This Been Tested?
Ten new tests in
eq::same_scalar_data_tests, each checking both argument orders give the same answer:should_not_equate_the_empty_string_with_a_nul_charactershould_equate_the_same_integer_carried_at_different_widths(U64/U16, I64/I32, U8/I128)should_equate_a_u128_past_i128_only_with_the_same_u128should_equate_an_identifier_with_the_same_32_bytes(againstBytes32andBytes)should_equate_equal_strings_and_byte_arrays_longer_than_a_tree_key(256 ASCII bytes, 70 emoji, 300 bytes)should_compare_floats_by_their_bits(-0.0/0.0, NaN payloads, with theequal_underlying_datacontrast)should_read_an_integer_against_a_float_as_the_float_it_converts_toshould_never_equate_a_map_or_an_array_of_anything_but_bytesshould_read_an_array_of_u8_as_the_bytes_it_listsshould_equate_two_nulls_and_nothing_else_with_a_nullshould_not_equate_values_of_different_kindsBreaking Changes
None. The method is additive and changes no existing behaviour.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
a410611/self-reviewedonce the bots are doneWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit