-
Notifications
You must be signed in to change notification settings - Fork 53
fix(platform): default omitted proved query limits #3509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3.1-dev
Are you sure you want to change the base?
Changes from all commits
8369775
7571f90
8016f64
169c120
69856a7
ee74646
ed6db4e
e08f71d
6d0b9ac
09a7bdd
a4fd818
8d87ff9
c05b969
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,41 @@ pub mod from_request; | |||||||||||||||||||||||
| /// Implementation of unproved verification | ||||||||||||||||||||||||
| pub mod unproved; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Default query limit applied by Platform when proved paginated requests omit `count` or `limit`. | ||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||
| /// Proof verification must mirror this behavior; otherwise a valid proof for a default-bounded | ||||||||||||||||||||||||
| /// server query is reconstructed locally as an unbounded query and looks truncated. | ||||||||||||||||||||||||
| pub(crate) use drive::config::DEFAULT_QUERY_LIMIT; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Parse a proved request's optional `count`/`limit`, applying Platform's default when omitted. | ||||||||||||||||||||||||
| pub(crate) fn proved_request_limit(limit: Option<u32>) -> Result<u16, Error> { | ||||||||||||||||||||||||
| limit.map_or(Ok(DEFAULT_QUERY_LIMIT), |limit| { | ||||||||||||||||||||||||
| u16::try_from(limit).map_err(|_| Error::RequestError { | ||||||||||||||||||||||||
| error: "query limit exceeds u16::MAX".to_string(), | ||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+28
to
+37
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Verifier hard-codes DEFAULT_QUERY_LIMIT while server reads runtime-configurable default_query_limit
source: ['claude', 'codex'] 🤖 Fix this with AI agents
Comment on lines
+24
to
+37
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Verifier hardcodes server's runtime-configurable
source: ['claude-security-auditor', 'claude-rust-quality'] 🤖 Fix this with AI agents
Comment on lines
+31
to
+37
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: The helper only checks source: ['codex-rust-quality', 'claude-general'] 🤖 Fix this with AI agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Needed for #[derive(PlatformSerialize, PlatformDeserialize)] | ||||||||||||||||||||||||
| #[cfg(feature = "mocks")] | ||||||||||||||||||||||||
| use dpp::serialization; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[cfg(test)] | ||||||||||||||||||||||||
| mod tests { | ||||||||||||||||||||||||
| use super::{proved_request_limit, DEFAULT_QUERY_LIMIT}; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn proved_request_limit_defaults_when_omitted() { | ||||||||||||||||||||||||
| assert_eq!(proved_request_limit(None).unwrap(), DEFAULT_QUERY_LIMIT); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn proved_request_limit_preserves_explicit_value() { | ||||||||||||||||||||||||
| assert_eq!(proved_request_limit(Some(7)).unwrap(), 7); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn proved_request_limit_rejects_u32_overflow() { | ||||||||||||||||||||||||
| assert!(proved_request_limit(Some(u16::MAX as u32 + 1)).is_err()); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+57
to
+60
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 Nitpick: Overflow test only asserts
💡 Suggested change
Suggested change
source: ['claude'] |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Nitpick: Previously unbounded omitted-limit FetchMany queries are now silently truncated to 100 verified items
Pre-fix, calling FetchMany with omitted
count/limitfailed proof verification outright. Post-fix, the same call returns up toDEFAULT_QUERY_LIMIT(100) items as a fully verified set with no indication that more rows may exist server-side. SDK consumers that previously distinguished "got an error" from "got the complete set" — e.g. enumerating votes given by an identity to check whether they have voted on a specific contested resource — could now silently make decisions on a truncated set. The proofs are sound; the concern is presentation. Worth either logging when the wirecount/limitwasNoneso the user knows pagination kicked in, or surfacing atruncated/has_moresignal on the FetchMany return types so callers can detect the boundary.source: ['claude-security-auditor']