Skip to content

feat(platform): add GetShieldedNotesCount query for sync progress#3769

Merged
QuantumExplorer merged 2 commits into
v3.1-devfrom
feat/shielded-notes-count-rpc
May 29, 2026
Merged

feat(platform): add GetShieldedNotesCount query for sync progress#3769
QuantumExplorer merged 2 commits into
v3.1-devfrom
feat/shielded-notes-count-rpc

Conversation

@QuantumExplorer
Copy link
Copy Markdown
Member

@QuantumExplorer QuantumExplorer commented May 29, 2026

Issue being fixed or feature implemented

Wallets have no cheap way to learn the total number of notes in the
shielded pool. During a shielded sync the client streams note chunks
but can't show a determinate progress bar because it doesn't know the
denominator (total notes) until it has fetched everything.

This adds a query returning the current leaf count of the shielded
notes commitment tree (= total notes). A wallet calls it once at the
start of a sync to seed a progress-bar denominator
(notes_total → mmr_chunks_total → percentage).

Extracted as a standalone PR from the broader shielded-sync work so it
can land and deploy independently — the interleaved-streaming and
dual-progress-bar changes depend on this query existing on-chain.

What was done?

GetShieldedNotesCount, wired end-to-end and proved (mirrors its
sibling GetShieldedPoolState):

  • proto (platform.proto): request carries prove: bool; response
    is oneof result { uint64 total_notes_count; Proof proof }. Plus
    dapi-grpc codegen registration (added to VERSIONED_RESPONSES) and
    regenerated JS/web/nodejs/java/objc/python clients.
  • drive-abci handler at query::shielded::notes_count (+ v0) with
    proved/unproved branches and tests; gRPC route in service.rs.
  • drive verifier verify_shielded_notes_count (+ round-trip tests)
    and its verify_shielded_notes_count FeatureVersion slot.
  • platform-version: notes_count query slot (v0/v1/v2_test all
    (0,0,0)); no protocol bump.
  • rs-dapi-client transport route + rs-dapi drive_method! arm.
  • drive-proof-verifier: ShieldedNotesCount(pub u64) + FromProof.
  • rs-sdk: Fetch + FetchCurrent (proved), Query honoring
    prove, and helper
    platform::types::shielded::fetch_shielded_notes_count(&sdk).

Why it's provable

The on-chain notes tree is an
Element::CommitmentTree(total_count, chunk_power, flags). total_count
is the element's first field and its serialized bytes are hashed into
the Merk value hash, so the app/root hash binds it — grovedb's own
comment: "total_count and height … are already authenticated by the
Merk value hash."
The proved path issues a single-key PathQuery at the
shielded pool path (no subquery), so GroveDB returns the serialized
CommitmentTree element and the verifier decodes total_count (field
0) against the verified root. Same mechanism GetShieldedPoolState uses
for its stored balance.

How Has This Been Tested?

  • cargo check -p drive -p drive-abci -p dash-sdk -p drive-proof-verifier — clean.
  • cargo test -p drive-abci --lib query::shielded::notes_count — 3 passed
    (none-version → decoding error; unproved → value; proved → proof).
  • cargo test -p drive --lib verify::shielded::verify_shielded_notes_count — 3 passed (round-trip: 0 on fresh state, N after inserts).
  • cargo test -p drive-proof-verifier --lib shielded_notes_count — 1 passed.
  • cargo fmt --all --check — clean.
  • gRPC clients regenerated via yarn workspace @dashevo/dapi-grpc build.

Breaking Changes

None. Additive query; new version slots initialized to 0/(0,0,0),
no protocol-version bump. Existing clients unaffected; nodes must deploy
this build to answer the new query.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added a new getShieldedNotesCount endpoint to query the total count of shielded notes in the platform.
    • Users can optionally request cryptographic proof to verify the count.
    • Integrated SDK support for fetching shielded notes count data.

Review Change Stack

@QuantumExplorer QuantumExplorer requested a review from shumkov as a code owner May 29, 2026 08:56
@github-actions github-actions Bot added this to the v3.1.0 milestone May 29, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e9b062f9-f628-4314-ae27-440d0cf86fd9

📥 Commits

Reviewing files that changed from the base of the PR and between 4e6b4ef and 30d3da0.

📒 Files selected for processing (1)
  • packages/rs-sdk/src/platform/query.rs

📝 Walkthrough

Walkthrough

Adds Platform gRPC RPC getShieldedNotesCount with versioned request/response messages, implements server query (proof and non-proof paths), Drive-side proof verification, proof-to-type conversion, multi-language client artifacts, and SDK fetch helpers with platform-version bookkeeping.

Changes

Shielded Notes Count Query Implementation

Layer / File(s) Summary
Proto RPC definition and build configuration
packages/dapi-grpc/protos/platform/v0/platform.proto, packages/dapi-grpc/build.rs
Proto adds getShieldedNotesCount RPC with GetShieldedNotesCountRequest (v0: prove flag) and GetShieldedNotesCountResponse (v0: result oneof returning total_notes_count or Proof, plus metadata). Build config marks both as versioned for attribute derivation.
Generated gRPC client stubs and JS artifacts
packages/dapi-grpc/clients/*/, clients/drive/v0/nodejs/drive_pbjs.js, clients/platform/v0/*
Protobuf compiler generated client bindings and message types across Java, NodeJS (drive/platform), protoc JS, Objective‑C, Python, and web/TypeScript, including promise/callback variants and full message serialization/oneof wiring.
gRPC transport and dAPI service wiring
packages/rs-dapi-client/src/transport/grpc.rs, packages/rs-dapi/src/services/platform_service/mod.rs
Transport maps GetShieldedNotesCountRequest to PlatformGrpcClient::get_shielded_notes_count. Platform service adds handler via drive_method! delegating to Drive client with existing cache/trace behavior.
Query service endpoint
packages/rs-drive-abci/src/query/service.rs
QueryService implements get_shielded_notes_count routing through handle_blocking_query, invoking Platform::query_shielded_notes_count.
Core query logic with proof/non-proof branching
packages/rs-drive-abci/src/query/shielded/notes_count/mod.rs, v0/mod.rs
Version dispatch validates request version and feature bounds. V0 branches on prove: proof path builds a single-key PathQuery and returns Proof result with metadata; non-proof path calls drive.shielded_pool_notes_count and returns total_notes_count. Tests cover missing version, empty state (0), and proof generation.
Server-side proof verification
packages/rs-drive/src/verify/shielded/verify_shielded_notes_count/mod.rs, v0/mod.rs
Adds Drive::verify_shielded_notes_count dispatch and v0 verifier that replays PathQuery, verifies subset/full proofs, enforces single-element result, and extracts verified count. Tests check fresh state and post-insert counts and unknown-version error handling.
Response type and proof parsing
packages/rs-drive-proof-verifier/src/types.rs, proof.rs
Newtype ShieldedNotesCount(u64) represents authenticated leaf count; FromProof impl decodes proof+metadata, calls Drive verifier, verifies Tenderdash proof, and returns typed result. Test ensures empty response yields NoProofInResult.
SDK client API and fetch implementations
packages/rs-sdk/src/platform/fetch.rs, query.rs, types.rs, types/shielded.rs, mock/requests.rs
Publishes shielded module, adds Fetch impl mapping ShieldedNotesCount to GetShieldedNotesCountRequest, implements Query<GetShieldedNotesCountRequest> for NoParamQuery (requires prove=true), provides FetchCurrent impl and fetch_shielded_notes_count helper, and registers mock response type.
Platform version bounds configuration
packages/rs-platform-version/src/version/...
Adds notes_count: FeatureVersionBounds to shielded query versions (v0, v1, v2 mock) and adds verify_shielded_notes_count field in Drive verify method versions (v1).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • dashpay/platform#3711: Refactor of the SDK query API and settings handling; related to the new NoParamQuery implementation and prove behavior.

Suggested labels

ready for final review

Suggested reviewers

  • shumkov
  • thepastaclaw

Poem

🐰 I counted leaves beneath the shade,

proofs in paw, no numbers made,
commitment tree now tells the tale,
shielded notes, hop on the trail!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding a GetShieldedNotesCount query to support wallet sync progress tracking.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/shielded-notes-count-rpc

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented May 29, 2026

✅ Review complete (commit 30d3da0)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

❌ Patch coverage is 82.25256% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.33%. Comparing base (9e00c8b) to head (30d3da0).
⚠️ Report is 1 commits behind head on v3.1-dev.

Files with missing lines Patch % Lines
packages/rs-drive-proof-verifier/src/proof.rs 0.00% 22 Missing ⚠️
...s-drive-abci/src/query/shielded/notes_count/mod.rs 85.26% 14 Missing ⚠️
...ify/shielded/verify_shielded_notes_count/v0/mod.rs 86.13% 14 Missing ⚠️
...verify/shielded/verify_shielded_notes_count/mod.rs 94.28% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           v3.1-dev    #3769      +/-   ##
============================================
+ Coverage     87.17%   87.33%   +0.15%     
============================================
  Files          2607     2594      -13     
  Lines        319589   317375    -2214     
============================================
- Hits         278614   277164    -1450     
+ Misses        40975    40211     -764     
Components Coverage Δ
dpp 87.74% <ø> (ø)
drive 85.95% <88.23%> (+<0.01%) ⬆️
drive-abci 89.60% <89.62%> (+<0.01%) ⬆️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.17% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 48.81% <0.00%> (-0.34%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/query.rs (1)

1183-1202: 💤 Low value

Minor grammar issue in panic message.

Line 1193: "query with proof are not supported" should be "query with proof is not supported" (singular verb agreement).

📝 Suggested grammar fix
-            unimplemented!("query with proof are not supported for GetShieldedNotesCountRequest");
+            unimplemented!("query with proof is not supported for GetShieldedNotesCountRequest");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-sdk/src/platform/query.rs` around lines 1183 - 1202, The panic
message in the Query<GetShieldedNotesCountRequest> impl for NoParamQuery is
grammatically incorrect; update the unimplemented! invocation that triggers when
settings.prove is true to use the singular verb form, e.g. change "query with
proof are not supported for GetShieldedNotesCountRequest" to "query with proof
is not supported for GetShieldedNotesCountRequest" so the message reads
correctly when settings.prove causes the unimplemented! panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/rs-sdk/src/platform/query.rs`:
- Around line 1183-1202: The panic message in the
Query<GetShieldedNotesCountRequest> impl for NoParamQuery is grammatically
incorrect; update the unimplemented! invocation that triggers when
settings.prove is true to use the singular verb form, e.g. change "query with
proof are not supported for GetShieldedNotesCountRequest" to "query with proof
is not supported for GetShieldedNotesCountRequest" so the message reads
correctly when settings.prove causes the unimplemented! panic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c83c3a6f-053e-441b-9c1a-c39b2d069c2e

📥 Commits

Reviewing files that changed from the base of the PR and between 9e00c8b and d8bf39c.

📒 Files selected for processing (19)
  • packages/dapi-grpc/build.rs
  • packages/dapi-grpc/protos/platform/v0/platform.proto
  • packages/rs-dapi-client/src/transport/grpc.rs
  • packages/rs-dapi/src/services/platform_service/mod.rs
  • packages/rs-drive-abci/src/query/service.rs
  • packages/rs-drive-abci/src/query/shielded/mod.rs
  • packages/rs-drive-abci/src/query/shielded/notes_count/mod.rs
  • packages/rs-drive-abci/src/query/shielded/notes_count/v0/mod.rs
  • packages/rs-drive-proof-verifier/src/types.rs
  • packages/rs-drive-proof-verifier/src/unproved.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v0.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs
  • packages/rs-platform-version/src/version/mocks/v2_test.rs
  • packages/rs-sdk/src/mock/requests.rs
  • packages/rs-sdk/src/platform/fetch_unproved.rs
  • packages/rs-sdk/src/platform/query.rs
  • packages/rs-sdk/src/platform/types.rs
  • packages/rs-sdk/src/platform/types/shielded.rs

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Verified the PR end-to-end: GetShieldedNotesCount is wired consistently from proto → drive-abci → version slots → verifier → SDK, mirroring the existing unproved GetCurrentQuorumsInfoRequest pattern. No blocking issues. A few nitpicks remain: a doc comment mismatch ("MMR" vs the actual CommitmentTree), an unreachable error fallback in the SDK helper, a grammar slip in the unimplemented! message, and a missing unit test for the new parser branch. The unimplemented!() panic intentionally mirrors prior art in the same file.

🟡 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/rs-drive-proof-verifier/src/unproved.rs`:
- [SUGGESTION] packages/rs-drive-proof-verifier/src/unproved.rs:299-319: New `FromUnproved` impl for `ShieldedNotesCount` lacks parser-level tests
  Neighboring unproved decoders in this file (e.g., `CurrentQuorumsInfo`, `EvoNodeStatus`) carry unit tests covering happy paths and malformed/missing-field cases. Because `GetShieldedNotesCount` has no proved alternative, the SDK fully depends on this parser to correctly reject missing `version` (`Error::EmptyVersion`) and missing `metadata` (`Error::EmptyResponseMetadata`). Adding a small test matrix here would catch future proto/codegen regressions before they reach shielded-sync flows.

Comment thread packages/rs-drive-proof-verifier/src/types.rs Outdated
Comment thread packages/rs-sdk/src/platform/types/shielded.rs Outdated
Comment thread packages/rs-sdk/src/platform/query.rs Outdated
Comment thread packages/rs-sdk/src/platform/query.rs Outdated
Comment thread packages/rs-drive-proof-verifier/src/unproved.rs Outdated
New query returning the current leaf count of the shielded notes
commitment tree (= total notes). Wallets call it once at the start of
a shielded sync to seed a determinate progress-bar denominator
(notes_total → mmr_chunks_total → percentage), instead of having to
fetch every note to learn the total.

The count IS cryptographically provable. The on-chain notes tree is an
`Element::CommitmentTree(total_count, chunk_power, flags)`; `total_count`
is the element's first field and its serialized bytes are hashed into
the Merk value hash, so the app/root hash binds it (grovedb's own
comment: "total_count and height ... are already authenticated by the
Merk value hash"). The query therefore supports a proof, mirroring its
sibling GetShieldedPoolState:

- request carries `prove: bool`; response is
  `oneof result { uint64 total_notes_count; Proof proof }`.
- proved path issues a single-key PathQuery at the shielded pool path
  (`[ShieldedBalances, "M"]`, key `[SHIELDED_NOTES_KEY]`) with no
  subquery, so GroveDB returns the serialized CommitmentTree element;
  the verifier decodes `total_count` (field 0) from the proved element.

Wired end-to-end (mirrors GetShieldedPoolState):
- proto + dapi-grpc codegen registration (now in VERSIONED_RESPONSES;
  regenerated JS/web/nodejs/java/objc/python clients).
- drive-abci handler at query::shielded::notes_count (+ v0) with
  prove/unproved branches + tests (unproved, proved, none-version).
- drive verifier `verify_shielded_notes_count` (+ round-trip tests) and
  its `verify_shielded_notes_count` FeatureVersion slot.
- notes_count FeatureVersionBounds query slot (v0/v1/v2_test all 0,0,0).
- rs-dapi-client transport route + rs-dapi drive_method! arm.
- drive-proof-verifier `ShieldedNotesCount(pub u64)` + FromProof.
- rs-sdk Fetch + FetchCurrent (proved) + Query honoring `prove`;
  helper `platform::types::shielded::fetch_shielded_notes_count(&sdk)`.
- `platform::types::shielded` made `pub` so the helper is reachable.

No protocol-version bump (new slots initialized to 0 / (0,0,0)).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@QuantumExplorer QuantumExplorer force-pushed the feat/shielded-notes-count-rpc branch from d8bf39c to 4e6b4ef Compare May 29, 2026 10:01
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-sdk/src/platform/query.rs`:
- Around line 1183-1199: The current Query<GetShieldedNotesCountRequest> impl
for NoParamQuery panics when settings.prove is false; remove the
unimplemented!() branch and always construct and return a
GetShieldedNotesCountRequest populated with the appropriate Version::V0 payload
where get_shielded_notes_count_request::GetShieldedNotesCountRequestV0.prove is
set to the incoming settings.prove value (so both proved and unproved requests
are supported), leaving the surrounding function signature (fn query for
NoParamQuery) unchanged.

In `@packages/rs-sdk/src/platform/types/shielded.rs`:
- Around line 66-72: The helper fetch_shielded_notes_count currently calls the
proof-verified path ShieldedNotesCount::fetch_current which forces expensive
proof work; change it to use the lightweight, non‑proved fetch path (e.g.
ShieldedNotesCount::fetch_lightweight or the SDK's equivalent short/pathless
fetch method) and return that value instead of using fetch_current so the wallet
sync-progress query is cheap; update the call in fetch_shielded_notes_count to
the lightweight variant and keep the rest of the function signature/return
handling the same.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cd26666b-c7e2-4f56-ad03-9f38911bf154

📥 Commits

Reviewing files that changed from the base of the PR and between d8bf39c and 4e6b4ef.

📒 Files selected for processing (38)
  • packages/dapi-grpc/build.rs
  • packages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.js
  • packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java
  • packages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.js
  • packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js
  • packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h
  • packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m
  • packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h
  • packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m
  • packages/dapi-grpc/clients/platform/v0/python/platform_pb2.py
  • packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py
  • packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts
  • packages/dapi-grpc/clients/platform/v0/web/platform_pb.js
  • packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts
  • packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js
  • packages/dapi-grpc/protos/platform/v0/platform.proto
  • packages/rs-dapi-client/src/transport/grpc.rs
  • packages/rs-dapi/src/services/platform_service/mod.rs
  • packages/rs-drive-abci/src/query/service.rs
  • packages/rs-drive-abci/src/query/shielded/mod.rs
  • packages/rs-drive-abci/src/query/shielded/notes_count/mod.rs
  • packages/rs-drive-abci/src/query/shielded/notes_count/v0/mod.rs
  • packages/rs-drive-proof-verifier/src/proof.rs
  • packages/rs-drive-proof-verifier/src/types.rs
  • packages/rs-drive/src/verify/shielded/mod.rs
  • packages/rs-drive/src/verify/shielded/verify_shielded_notes_count/mod.rs
  • packages/rs-drive/src/verify/shielded/verify_shielded_notes_count/v0/mod.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v0.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs
  • packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs
  • packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs
  • packages/rs-platform-version/src/version/mocks/v2_test.rs
  • packages/rs-sdk/src/mock/requests.rs
  • packages/rs-sdk/src/platform/fetch.rs
  • packages/rs-sdk/src/platform/query.rs
  • packages/rs-sdk/src/platform/types.rs
  • packages/rs-sdk/src/platform/types/shielded.rs
✅ Files skipped from review due to trivial changes (10)
  • packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m
  • packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts
  • packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts
  • packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py
  • packages/rs-sdk/src/mock/requests.rs
  • packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h
  • packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js
  • packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h
  • packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java
  • packages/dapi-grpc/clients/platform/v0/web/platform_pb.js
🚧 Files skipped from review as they are similar to previous changes (11)
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs
  • packages/rs-drive-abci/src/query/shielded/mod.rs
  • packages/rs-dapi-client/src/transport/grpc.rs
  • packages/rs-platform-version/src/version/mocks/v2_test.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v0.rs
  • packages/rs-sdk/src/platform/types.rs
  • packages/rs-dapi/src/services/platform_service/mod.rs
  • packages/rs-drive-abci/src/query/service.rs
  • packages/rs-drive-proof-verifier/src/types.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs
  • packages/rs-drive-abci/src/query/shielded/notes_count/mod.rs

Comment thread packages/rs-sdk/src/platform/query.rs
Comment thread packages/rs-sdk/src/platform/types/shielded.rs
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Prior finding reconciliation for 4e6b4ef: carried forward STILL VALID prior findings: none; fixed/outdated prior findings: #1:unproved-shielded-notes-count-parser-tests, #2:shielded-notes-count-doc-mmr-vs-commitment-tree, #3:fetch-shielded-notes-count-unreachable-ok-or, #4:get-shielded-notes-count-prove-true-unimplemented, #5:get-shielded-notes-count-unimplemented-grammar; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.

Verified the d8bf39c..4e6b4ef delta and cumulative PR. The PR now adds a fully proved variant of GetShieldedNotesCount (proto prove/oneof result, drive-abci proved branch, new verify_shielded_notes_count in drive with v0 + round-trip tests, SDK Fetch/FetchCurrent/FromProof); the old FromUnproved impl is removed. Four prior findings are FIXED, one is OUTDATED. The lone carried-forward nit (panic on prove=false) is a module-wide convention shared by every shielded query in query.rs (GetShieldedPoolStateRequest, GetShieldedAnchorsRequest, GetMostRecentShieldedAnchorRequest) — out of scope for this PR. No new blocking issues in the delta.

No in-scope findings remain after re-validating the prior review against this head.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

✅ DashSDKFFI.xcframework built for this PR.

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "438e04cb0d78bd43bc1018ae4b5c66d66852eafd21758d0c668ad56c3d5e4810"
)

Xcode manual integration:

  • Download 'DashSDKFFI.xcframework' artifact from the run link above.
  • Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
  • If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

QuantumExplorer added a commit that referenced this pull request May 29, 2026
The shielded progress-bar denominator now comes "for free" from the
proof that already delivers each note chunk — no separate RPC, no
server change, no proof-size change, and crucially no dependency on
GetShieldedNotesCount being deployed (so the bars work against
currently-deployed nodes).

The note-fetch proof is a GroveDB layered proof that subqueries INTO
the shielded notes CommitmentTree, so the parent
`Element::CommitmentTree(total_count, ..)` is necessarily present in
the proof bytes (it chains to the root hash and feeds total_count into
the inner CommitmentTree subquery verification — the
`add_parent_tree_on_subquery: false` flag only suppresses sibling
subtree walks, not the queried tree's own element). We were simply
discarding it.

`verify_shielded_encrypted_notes` now extracts it by running an
additional `verify_subset_query` against the SAME proof bytes with a
single-key PathQuery for the CommitmentTree element (no subquery), then
decoding `total_count` (field 0). A round-trip test inserts N notes,
builds a real note-fetch proof via the server-side PathQuery, and
asserts the extracted count == N — proving the parent element is in the
proof.

Plumbing:
- verifier returns `(root_hash, notes, total_count)`.
- `ShieldedEncryptedNotes` becomes `{ notes, total_count }`; `FromProof`
  threads it and now yields `Some(..)` on empty chunks so the count
  still flows on the final empty chunk.
- `fetch_chunk` returns `total_count`; `ShieldedChunkBatch` and
  `ShieldedSyncResult` carry it.
- wallet `sync_notes_across` sources `total_target` from
  `batch.total_count` (max-seen) instead of calling
  `fetch_shielded_notes_count` — that up-front RPC is removed.

The standalone GetShieldedNotesCount query (PR #3769) stays as the
primitive for showing the total WITHOUT syncing; the sync path no
longer needs it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Count

GetShieldedNotesCount is proved-only (the count is bound by the Merk
value hash, so it is always returned inside a verifiable proof). The
Query impl previously hit `unimplemented!()` — a process-aborting
panic — when a caller explicitly disabled proofs on this public
request type. Return a recoverable `Error::Generic` instead.

Addresses review feedback on the panic; the proved-only design is
intentional (the cheap, non-binding progress denominator is sourced
from the note-fetch proof's total_count, so this standalone query
stays the cryptographically-verified primitive).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@QuantumExplorer
Copy link
Copy Markdown
Member Author

Reviewed

@QuantumExplorer QuantumExplorer merged commit 40f11b2 into v3.1-dev May 29, 2026
12 checks passed
@QuantumExplorer QuantumExplorer deleted the feat/shielded-notes-count-rpc branch May 29, 2026 11:12
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Single-commit delta at 30d3da0 cleanly replaces a process-aborting unimplemented!() with a typed Err(Error::Generic(...)) in the Query<GetShieldedNotesCountRequest>::query impl for prove=false. The change is correctly scoped, the Error::Generic variant exists, and the proved happy-path is untouched. One suggestion-level test-coverage gap remains for the new error branch.

🟡 1 suggestion(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/rs-sdk/src/platform/query.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/query.rs:1194-1199: Add a unit test pinning the new `prove=false` error path for `GetShieldedNotesCount`
  This PR's only behavioral change is replacing `unimplemented!()` with `Err(Error::Generic(...))` when callers build `GetShieldedNotesCountRequest` with `QuerySettings { prove: false, .. }`. That branch is part of the public `Query` surface but is unreachable through the standard `FetchCurrent` flow (which forces `prove=true`), so without a focused unit test exercising `NoParamQuery::query` directly with `prove=false`, a future refactor could silently regress this back to a panic or change the error shape without any signal. The module-level docs already endorse direct `QuerySettings` use for encoder-level unit tests, making this the appropriate place to lock in the intended non-panicking behavior.

Comment on lines +1194 to +1199
if !prove {
return Err(Error::Generic(
"GetShieldedNotesCount requires proofs; unproved queries are not supported"
.to_string(),
));
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Add a unit test pinning the new prove=false error path for GetShieldedNotesCount

This PR's only behavioral change is replacing unimplemented!() with Err(Error::Generic(...)) when callers build GetShieldedNotesCountRequest with QuerySettings { prove: false, .. }. That branch is part of the public Query surface but is unreachable through the standard FetchCurrent flow (which forces prove=true), so without a focused unit test exercising NoParamQuery::query directly with prove=false, a future refactor could silently regress this back to a panic or change the error shape without any signal. The module-level docs already endorse direct QuerySettings use for encoder-level unit tests, making this the appropriate place to lock in the intended non-panicking behavior.

source: ['codex']

QuantumExplorer added a commit that referenced this pull request May 29, 2026
Resolves conflicts created when #3769 (GetShieldedNotesCount) merged to
v3.1-dev as the PROVED query, superseding this branch's earlier UNPROVED
version (eabb145).

Conflict resolution:
- All GetShieldedNotesCount surface (proto, notes_count handler,
  types.rs ShieldedNotesCount doc, query.rs Query impl, and the SDK
  unproved/fetch surface) taken from v3.1-dev — the proved version is
  authoritative. The branch's stale unproved impls (FromUnproved,
  FetchUnproved) are dropped; query.rs keeps the proved-only recoverable
  error on prove=false.
- proto + regenerated gRPC clients now match v3.1-dev exactly.
- Kept this branch's separate note-fetch total_count work: the
  ShieldedEncryptedNotes { notes, total_count } struct and the
  verify_shielded_encrypted_notes extraction are preserved (they don't
  conflict with the proved standalone query).

Verified: dapi-grpc, drive-proof-verifier, dash-sdk, drive-abci,
platform-wallet (--features shielded), and wasm-sdk all check clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants