feat(sdk): fetch many and return metadata and proof to client#2331
Conversation
WalkthroughThe changes involve modifications to two files: Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/rs-sdk/src/platform/fetch_many.rs (2)
235-236: Avoid unnecessary cloning ofrequestIn the
fetch_many_with_metadata_and_proofmethod,request.clone()is used when callingparse_proof_with_metadata_and_proof. If possible, consider passing a reference torequestinstead of cloning to improve performance and reduce overhead.
Line range hint
336-366: Refactorfetch_manyimplementation forDocumentto use base methodIn the implementation of
FetchMany<Identifier, Documents> for Document, thefetch_manymethod is re-implemented separately and does not utilizefetch_many_with_metadata_and_proof, unlike the base trait. To maintain consistency and reduce code duplication, consider refactoring this method to call the basefetch_manyimplementation or modify it to usefetch_many_with_metadata_and_proof.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/rs-dapi-client/src/executor.rs(1 hunks)packages/rs-sdk/src/platform/fetch_many.rs(4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/rs-dapi-client/src/executor.rs (1)
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-dapi-client/src/executor.rs:38-38
Timestamp: 2024-11-12T14:57:47.419Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, for the structs `ExecutionError<E>` and `ExecutionResponse<R>`, only the serde derives (`Serialize`, `Deserialize`) should be conditional based on the "mocks" feature; the other derives (`Debug`, `Clone`, `Eq`, `PartialEq`) should always be present.
🔇 Additional comments (2)
packages/rs-dapi-client/src/executor.rs (1)
79-79: Verify the impact of removing the Eq trait.
The removal of the Eq trait could be a breaking change if any code relies on full equality comparison of ExecutionResponse. This needs verification as the PR objectives state "no breaking changes".
✅ Verification successful
Removing Eq trait is safe in this context
The verification shows that ExecutionResponse is only used for equality comparison in test code (mock_dapi_client.rs), where PartialEq is sufficient. The struct is primarily used for returning responses from API calls, and there's no evidence of it being used in contexts requiring full equality (like HashSet or BTreeSet). The removal of Eq is not a breaking change as PartialEq satisfies all current usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential usage of equality comparison on ExecutionResponse
# Look for direct equality comparisons
rg -A 2 "ExecutionResponse.*==|assert.*ExecutionResponse"
# Look for trait bounds requiring Eq
ast-grep --pattern 'where $type: Eq'
# Look for derive(Eq) on types containing ExecutionResponse
ast-grep --pattern '#[derive($$$Eq$$$)]
struct $name {
$$$
$field: ExecutionResponse<$type>,
$$$
}'
Length of output: 206
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of ExecutionResponse
# Find all files containing ExecutionResponse
rg --type rust "ExecutionResponse" -l
# Look for any comparisons or assertions in test files
rg --type rust -g "*test*.rs" "ExecutionResponse"
# Look for any collections (HashSet, BTreeSet) that might require Eq
rg --type rust "ExecutionResponse.*HashSet|ExecutionResponse.*BTreeSet"
# Look for the struct definition and its usage
ast-grep --pattern 'struct ExecutionResponse'
Length of output: 573
Script:
#!/bin/bash
# Let's examine how ExecutionResponse is used in the codebase
# Look at the struct definition and its usage context
rg -B 3 -A 10 "struct ExecutionResponse" packages/rs-dapi-client/src/executor.rs
# Check if it's used in any function signatures or return types
ast-grep --pattern 'fn $name($$$) -> $ret<ExecutionResponse<$type>>'
# Look for any pattern matching or destructuring
rg --type rust "match.*ExecutionResponse|if let.*ExecutionResponse"
# Check test files specifically for the struct usage
cat packages/rs-dapi-client/tests/mock_dapi_client.rs
Length of output: 2209
packages/rs-sdk/src/platform/fetch_many.rs (1)
149-152: Refactored fetch_many to delegate to fetch_many_with_metadata_and_proof
The fetch_many method now correctly calls fetch_many_with_metadata_and_proof, enhancing code reuse and maintaining consistency across methods.
| retries, | ||
| }) | ||
| .map(|(o, metadata, proof)| ExecutionResponse { | ||
| inner: (o.unwrap_or_default(), metadata, proof), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential None value without using unwrap_or_default()
On line 245, o.unwrap_or_default() is used when constructing the ExecutionResponse. This could mask situations where no data is returned (o is None), possibly leading to confusion for the caller. Consider handling the None case explicitly, perhaps by returning an error or conveying that no data was found.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Issue being fixed or feature implemented
Clients sometimes want to see the metadata or the proof data, this was implemented for fetch, but was not implemented for fetch_many.
What was done?
Returns metadata and proof for fetch_many
How Has This Been Tested?
Not tested
Breaking Changes
No breaking changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
FetchManytrait for enhanced data retrieval, including fetching with metadata and proof.fetch_manymethod to return additional metadata alongside the fetched objects.Bug Fixes
fetch_manymethod to accommodate new features.Refactor
ExecutionResponsestruct.