feat(sdk): fetch last distribution claim moment#2566
Conversation
## Walkthrough
This set of changes introduces support for the `get_token_perpetual_distribution_last_claim` request and response across multiple Rust packages. It adds a new gRPC transport implementation for this method, defines the associated types and enums, and integrates the request into the SDK's fetch and query traits. The proof verifier gains logic to parse and verify responses, while mock support and testing scaffolding are also established. Public API exports are updated to include the new types and error handling.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `packages/rs-dapi-client/src/transport/grpc.rs` | Added gRPC transport request implementation for `get_token_perpetual_distribution_last_claim` using the corresponding request and response types. |
| `packages/rs-drive-proof-verifier/src/lib.rs` | Publicly re-exported `ProtocolError` from the `dpp` crate. |
| `packages/rs-drive-proof-verifier/src/proof.rs` | Declared new public module `token_perpetual_distribution_last_claim`. |
| `packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs` | Added `FromProof` implementation for `RewardDistributionMoment` handling versioning, token and identity ID validation, proof verification, and conversion to public enum. |
| `packages/rs-drive-proof-verifier/src/types.rs` | Removed redundant import of `ProtocolError` under mocks and imported it directly from `dpp`. |
| `packages/rs-drive-proof-verifier/src/error.rs` | Added `TokenConfigurationFailure(String)` variant to `ContextProviderError` enum for token configuration retrieval errors. |
| `packages/rs-drive-proof-verifier/src/provider.rs` | Added `get_token_configuration` method to `ContextProvider` trait and implementations, including mock stub returning `None`. |
| `packages/rs-sdk/src/mock/requests.rs` | Added `MockResponse` trait implementation for `RewardDistributionMoment` using macro. |
| `packages/rs-sdk/src/mock/sdk.rs` | Integrated `GetTokenPerpetualDistributionLastClaimRequest` into mock expectations loader. |
| `packages/rs-sdk/src/mock/provider.rs` | Extended `GrpcContextProvider` with `token_configurations_cache` and implemented `get_token_configuration` method with cache lookup and warning on miss. |
| `packages/rs-sdk/src/platform/fetch.rs` | Implemented `Fetch` trait for `RewardDistributionMoment` associating it with `GetTokenPerpetualDistributionLastClaimRequest`. |
| `packages/rs-sdk/src/platform/query.rs` | Added `TokenLastClaimQuery` struct with `token_id` and `identity_id` fields; implemented `Query` trait for `GetTokenPerpetualDistributionLastClaimRequest` constructing request with proof requirement and contract info set to `None`. |
| `packages/rs-sdk/tests/fetch/tokens/mod.rs` | Added module declaration for `token_perpetual_distribution_last_claim`. |
| `packages/rs-sdk/tests/fetch/tokens/token_perpetual_distribution_last_claim.rs` | Added new test file with commented-out async tests for fetching last claim with and without proof, and for never-claimed identity. |
| `packages/rs-sdk/src/platform.rs` | Changed `query` module visibility from private to public. |
| `packages/rs-sdk/examples/read_contract.rs` | Added cache size parameter for token configurations when creating `GrpcContextProvider` in `setup_sdk`. |
| `packages/rs-sdk/src/sdk.rs` | Added `DEFAULT_TOKEN_CONFIG_CACHE_SIZE` constant and `token_config_cache_size` field to `SdkBuilder`; passed new cache size to `GrpcContextProvider::new` in build method. |
| `packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_moment/mod.rs` | Added derives `PlatformSerialize` and `PlatformDeserialize` with attribute `#[platform_serialize(unversioned)]` to `RewardDistributionMoment` enum. |
| `packages/rs-dpp/src/data_contract/associated_token/token_configuration/mod.rs` | Removed trailing blank line after `TokenConfiguration` enum definition; no functional changes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant SDK
participant DAPI
participant ProofVerifier
Client->>SDK: Create TokenLastClaimQuery
SDK->>DAPI: Send GetTokenPerpetualDistributionLastClaimRequest (gRPC)
DAPI-->>SDK: GetTokenPerpetualDistributionLastClaimResponse (with proof or value)
SDK->>ProofVerifier: maybe_from_proof_with_metadata(request, response, ...)
ProofVerifier-->>SDK: (Option<RewardDistributionMoment>, metadata, proof)
SDK-->>Client: Return last claim info and metadataPossibly related PRs
Suggested labels
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/rs-dapi-client/src/transport/grpc.rs (1)
601-608: gRPC transport implementation is correctly defined.The implementation of the transport request for
GetTokenPerpetualDistributionLastClaimRequestfollows the established pattern using theimpl_transport_request_grpc!macro with appropriate types and default settings.Minor nitpick: The comment on line 601 includes "Request" in the method name which is slightly inconsistent with other similar comments in this file. Consider aligning it with other comments for better consistency.
-// rpc getTokenPerpetualDistributionLastClaimRequest(GetTokenPerpetualDistributionLastClaimRequest) returns (GetTokenPerpetualDistributionLastClaimResponse); +// rpc getTokenPerpetualDistributionLastClaim(GetTokenPerpetualDistributionLastClaimRequest) returns (GetTokenPerpetualDistributionLastClaimResponse);packages/rs-drive-proof-verifier/src/types.rs (1)
637-653: Well-designed enum with appropriate variants.The
TokenPerpetualDistributionLastClaimenum is well-defined with variants that cover different ways of representing the last claim moment (timestamp, block height, epoch, or raw data). The documentation is clear, and the necessary derive attributes for serialization are included.Consider adding helper methods for common operations, such as converting between variant types or extracting the value in a standardized format.
impl TokenPerpetualDistributionLastClaim { /// Attempts to convert to a timestamp in milliseconds pub fn as_timestamp_ms(&self) -> Option<u64> { match self { Self::TimestampMs(ms) => Some(*ms), // Add conversions from other formats if needed _ => None, } } /// Attempts to convert to a block height pub fn as_block_height(&self) -> Option<u64> { match self { Self::BlockHeight(height) => Some(*height), // Add conversions from other formats if needed _ => None, } } // Add more helper methods as needed }packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (2)
31-33: Minor: consolidate duplicateusepaths
get_token_perpetual_distribution_last_claim_response_v0is imported twice (lines 4‑8 and 31‑33).
You can fold theResultalias into the firstuseto keep the prelude concise:- get_token_perpetual_distribution_last_claim_response_v0::{ - self, last_claim_info::PaidAt, - }, + get_token_perpetual_distribution_last_claim_response_v0::{ + self, last_claim_info::PaidAt, Result as RespResult, + },This removes the second standalone
usestatement.
131-136: Early‑exit with clearer error message when proof verification is impossibleWhen
contract_infois missing the function errors deep inside the proof path.
Returning a dedicated error before attempting verification makes the cause obvious.Consider introducing a specialised
Error::MissingContractInfovariant and using it here; this avoids embedding a long explanatory string in the generic variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/rs-dapi-client/src/transport/grpc.rs(1 hunks)packages/rs-drive-proof-verifier/src/lib.rs(1 hunks)packages/rs-drive-proof-verifier/src/proof.rs(1 hunks)packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs(1 hunks)packages/rs-drive-proof-verifier/src/types.rs(3 hunks)packages/rs-sdk/src/mock/requests.rs(2 hunks)packages/rs-sdk/src/mock/sdk.rs(1 hunks)packages/rs-sdk/src/platform/fetch.rs(1 hunks)packages/rs-sdk/src/platform/query.rs(2 hunks)packages/rs-sdk/tests/fetch/tokens/mod.rs(1 hunks)packages/rs-sdk/tests/fetch/tokens/token_perpetual_distribution_last_claim.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/rs-sdk/src/mock/sdk.rs (1)
Learnt from: lklimek
PR: dashpay/platform#2232
File: packages/rs-sdk/src/mock/sdk.rs:90-95
Timestamp: 2024-10-10T10:30:19.883Z
Learning: In `packages/rs-sdk/src/mock/sdk.rs`, the `load_expectations` method in `MockDashPlatformSdk` remains asynchronous (`async`) for backward compatibility, even though it now delegates to the synchronous `load_expectations_sync` method.
🧬 Code Graph Analysis (1)
packages/rs-dapi-client/src/transport/grpc.rs (1)
packages/rs-drive-abci/src/query/service.rs (1)
get_token_perpetual_distribution_last_claim(769-779)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (9)
packages/rs-sdk/tests/fetch/tokens/token_perpetual_distribution_last_claim.rs (1)
1-2: Implementation needed: Add actual test cases for the token perpetual distribution last claim functionality.This file currently contains only a placeholder comment without any actual test implementation. Since the PR introduces a new feature for fetching the last distribution claim moment, comprehensive test coverage is essential.
According to the PR description, the feature hasn't been tested yet. Consider implementing test cases that cover:
- Basic fetch functionality for existing claims
- Behavior when no claim exists
- Error handling scenarios
- Proof verification logic
Would you like me to help generate a skeleton implementation for these test cases?
packages/rs-drive-proof-verifier/src/proof.rs (1)
5-5: LGTM: Module declaration follows existing pattern.The new module declaration for
token_perpetual_distribution_last_claimfollows the established pattern in this file and properly integrates the new proof verification functionality.packages/rs-sdk/tests/fetch/tokens/mod.rs (1)
3-3: LGTM: Module declaration properly integrates the new test file.The new module declaration correctly integrates the token perpetual distribution last claim test file into the test suite.
packages/rs-drive-proof-verifier/src/lib.rs (1)
11-11: LGTM: Appropriate re-export of the ProtocolError type.The re-export of
ProtocolErrorfrom thedppcrate makes it available to consumers of this library without requiring them to import it directly. This is consistent with the library's export pattern and necessary for the new proof verification functionality.packages/rs-sdk/src/mock/sdk.rs (1)
235-239: LGTM: Mock expectation handler follows existing pattern.The new match arm for handling
GetTokenPerpetualDistributionLastClaimRequestcorrectly follows the established pattern in theload_expectations_syncmethod. This enables proper mocking support for the new request type.packages/rs-sdk/src/platform/fetch.rs (1)
296-299: Implementation looks correct and follows established patterns.This implementation of the
Fetchtrait forTokenPerpetualDistributionLastClaimcorrectly associates it with the corresponding request type. It follows the same pattern as other implementations in this file.packages/rs-sdk/src/mock/requests.rs (2)
35-36: Import additions look good.The addition of
TokenPerpetualDistributionLastClaimto the imports list is correctly placed.
451-451: Mock implementation looks correct.Using the
impl_mock_response!macro forTokenPerpetualDistributionLastClaimis the right approach and follows the standard pattern used for other types in this file.packages/rs-drive-proof-verifier/src/types.rs (1)
41-41: Import looks good.The addition of
ProtocolErrorfrom thedppcrate is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (1)
102-108:⚠️ Potential issueLossy cast from
u32tou16can select the wrong tokenThe
token_contract_positionarrives asu32from the protobuf but the contract'stokens()map is keyed byu16. Blindly casting (as u16) truncates values > 65,535 without error, which will lead to wrong orNonelook-ups.- let pos = cti.token_contract_position as u16; + let pos_u32 = cti.token_contract_position; + if pos_u32 > u16::MAX as u32 { + return Err(Error::RequestError { + error: format!( + "token_contract_position {} exceeds u16::MAX", + pos_u32 + ), + }); + } + let pos = pos_u32 as u16;
🧹 Nitpick comments (4)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (4)
190-190: Add context tounreachable!macroWhile the current implementation handles all variants, providing more context in the unreachable macro would improve debugging if enum variants are added in the future.
- unreachable!("`result` covers all variants; qed") + unreachable!("Unhandled RespResult variant - all known variants (Proof, LastClaim) are covered above")
48-57: Consider adding documentation for trait method parametersThe
maybe_from_proof_with_metadatamethod has several parameters but lacks documentation describing what each one does and what the return values represent.Consider adding documentation to clarify the purpose of each parameter and the return value structure:
+ /// Parses and optionally verifies a token perpetual distribution last claim response + /// + /// # Parameters + /// * `request` - The original request for token perpetual distribution last claim + /// * `response` - The response from the platform containing last claim data + /// * `_network` - The Dash network being used + /// * `platform_version` - Version information for the platform + /// * `provider` - Provider for accessing contracts and verification context + /// + /// # Returns + /// A tuple containing: + /// * Optional TokenPerpetualDistributionLastClaim - The parsed last claim data if present + /// * ResponseMetadata - Metadata from the response + /// * Proof - The proof data (empty for unproved responses) fn maybe_from_proof_with_metadata<'a, I: Into<Self::Request>, O: Into<Self::Response>>(
131-136: Improve error message for missing distribution typeThe current error could be more specific about what the caller needs to provide.
- return Err(Error::RequestError { - error: "cannot verify GroveDB proof without contract_info / distribution_type" - .into(), - }); + return Err(Error::RequestError { + error: "cannot verify GroveDB proof without contract_info in request; add contract_id and token_contract_position".into(), + });
83-124: Consider refactoring to avoid nested blocks and reduce complexityThe token and identity ID extraction code could be refactored into helper functions to improve readability and maintainability.
+ fn extract_identifier(bytes: &[u8], field_name: &str) -> Result<[u8; 32], Error> { + <[u8; 32]>::try_from(bytes).map_err(|_| { + Error::RequestError { + error: format!("{field_name} must be 32 bytes"), + } + }) + } + let (token_id, identity_id, distribution_type) = match request .version .ok_or(Error::EmptyVersion)? { RequestVersion::V0(req_v0) => { - let tok = <[u8; 32]>::try_from(req_v0.token_id.as_slice()).map_err(|_| { - Error::RequestError { - error: "token_id must be 32 bytes".into(), - } - })?; - let ide = <[u8; 32]>::try_from(req_v0.identity_id.as_slice()).map_err(|_| { - Error::RequestError { - error: "identity_id must be 32 bytes".into(), - } - })?; + let tok = extract_identifier(req_v0.token_id.as_slice(), "token_id")?; + let ide = extract_identifier(req_v0.identity_id.as_slice(), "identity_id")?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs(1 hunks)packages/rs-sdk/src/platform.rs(1 hunks)packages/rs-sdk/src/platform/query.rs(2 hunks)packages/rs-sdk/tests/fetch/tokens/token_perpetual_distribution_last_claim.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-sdk/src/platform/query.rs
- packages/rs-sdk/tests/fetch/tokens/token_perpetual_distribution_last_claim.rs
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Rust crates security audit
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/rs-sdk/src/platform.rs (1)
16-16: Appropriately exposing the query moduleMaking the
querymodule public is necessary for exposing the newTokenLastClaimQueryimplementation and related functionality to external code, which supports the token perpetual distribution last claim feature.packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (2)
1-192: Well-structured implementation with comprehensive error handlingThe overall implementation of the
FromProoftrait forTokenPerpetualDistributionLastClaimis well-structured with thorough error handling, proper verification of both GroveDB and Tendermint proofs, and appropriate handling of both proved and direct response paths.
165-165:Details
✅ Verification successful
Potential data loss in epoch conversion
The
RewardDistributionMoment::EpochBasedMomentvalue is cast from what appears to be a larger integer type tou32without bounds checking.Verify the actual type of the epoch value in
RewardDistributionMoment::EpochBasedMomentand ensure that casting tou32doesn't lose data:
🏁 Script executed:
#!/bin/bash # Find the definition of RewardDistributionMoment to check epoch type rg -A 10 "enum RewardDistributionMoment" --type rustLength of output: 1703
🏁 Script executed:
#!/bin/bash # Locate the definition of EpochIndex to determine its underlying type rg -n --type rust "type EpochIndex" rg -n --type rust "struct EpochIndex"Length of output: 277
Epoch conversion is safe; no data loss
TheEpochIndextype is defined as au16(packages/rs-dpp/src/block/epoch/mod.rs:12), so casting it tou32cannot overflow. You can keep the existing conversion:TokenPerpetualDistributionLastClaim::Epoch(e as u32)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (1)
85-85: Use a more descriptive error for missingresult.The error
NoProofInResultdoesn't accurately describe this situation - you're checking if theresultfield exists in the response, not if there's a proof in the result.- let result = resp_v0.result.clone().ok_or(Error::NoProofInResult)?; + let result = resp_v0.result.clone().ok_or(Error::RequestError { + error: "missing result in response".into(), + })?;
🧹 Nitpick comments (3)
packages/rs-sdk/src/mock/provider.rs (1)
228-238: Consider fetching token configurations like data contracts.The implementation for
get_token_configurationonly checks the cache and returnsNoneon a miss, unlikeget_data_contractwhich attempts to fetch missing contracts using the SDK when available.Consider implementing a similar pattern to fetch missing token configurations when SDK is available:
fn get_token_configuration( &self, token_id: &Identifier, ) -> Result<Option<TokenConfiguration>, ContextProviderError> { if let Some(config) = self.token_configurations_cache.get(token_id) { Ok(Some((*config).clone())) } else { - tracing::warn!("token config cache miss"); - Ok(None) + let sdk_guard = self.sdk.load(); + + let sdk = match sdk_guard.as_ref() { + Some(sdk) => sdk, + None => { + tracing::warn!("token config cache miss and no sdk provided, skipping fetch"); + return Ok(None); + } + }; + + // Implement token configuration fetching logic here or return None + // for now if fetching is not yet supported + tracing::warn!("token config cache miss, fetching not yet implemented"); + Ok(None) } }packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (1)
90-104: Avoid redundant token ID validation.The code validates the token ID twice - once when extracting it from the request at lines 58-65, and again when creating the identifier at lines 91-94.
let maybe_distribution_type = { - let token_id_identifier = Identifier::from_vec(req_v0.token_id.clone()) - .map_err(|_| Error::RequestError { - error: "token_id must be 32 bytes".into(), - })?; + // Create identifier from already-validated token_id + let token_id_identifier = Identifier::new(token_id); let maybe_token_config = provider.get_token_configuration(&token_id_identifier)?; let maybe_dist_type = maybe_token_config .as_ref() .and_then(|cfg| cfg.distribution_rules().perpetual_distribution()) .map(|perp| perp.distribution_type().clone()); maybe_dist_type };packages/rs-drive-proof-verifier/src/provider.rs (1)
43-60: Documentation contains incorrect return type description.The documentation comment mentions returning an
Arc<TokenConfiguration>but the actual return type isOption<TokenConfiguration>without Arc-wrapping./// # Returns /// /// * `Ok(Option<TokenConfiguration>)`: On success, returns the token configuration if it exists, or `None` if it does not. - /// We use Arc to avoid copying the token configuration. + /// Unlike data contracts, token configurations are not Arc-wrapped to avoid copying. /// * `Err(Error)`: On failure, returns an error indicating why the operation failed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/mod.rs(0 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_moment/mod.rs(1 hunks)packages/rs-drive-proof-verifier/src/error.rs(1 hunks)packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs(1 hunks)packages/rs-drive-proof-verifier/src/provider.rs(5 hunks)packages/rs-drive-proof-verifier/src/types.rs(2 hunks)packages/rs-sdk/examples/read_contract.rs(1 hunks)packages/rs-sdk/src/mock/provider.rs(6 hunks)packages/rs-sdk/src/mock/requests.rs(2 hunks)packages/rs-sdk/src/platform/fetch.rs(2 hunks)packages/rs-sdk/src/platform/query.rs(2 hunks)packages/rs-sdk/src/sdk.rs(4 hunks)packages/rs-sdk/tests/fetch/tokens/token_perpetual_distribution_last_claim.rs(1 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-dpp/src/data_contract/associated_token/token_configuration/mod.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-sdk/tests/fetch/tokens/token_perpetual_distribution_last_claim.rs
- packages/rs-sdk/src/mock/requests.rs
- packages/rs-drive-proof-verifier/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-sdk/src/platform/fetch.rs
- packages/rs-sdk/src/platform/query.rs
🧰 Additional context used
🧠 Learnings (1)
packages/rs-drive-proof-verifier/src/error.rs (1)
Learnt from: lklimek
PR: dashpay/platform#2380
File: packages/rs-drive-proof-verifier/src/error.rs:122-125
Timestamp: 2024-12-06T14:15:49.367Z
Learning: All error variants in `ContextProviderError` within `packages/rs-drive-proof-verifier/src/error.rs` are designed to take `String` as their parameter.
🧬 Code Graph Analysis (2)
packages/rs-sdk/src/mock/provider.rs (1)
packages/rs-drive-proof-verifier/src/provider.rs (5)
new(204-208)get_token_configuration(57-60)get_token_configuration(99-104)get_token_configuration(134-140)get_token_configuration(314-320)
packages/rs-drive-proof-verifier/src/provider.rs (1)
packages/rs-sdk/src/mock/provider.rs (3)
get_token_configuration(228-238)get_quorum_public_key(166-188)get_data_contract(190-226)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (19)
packages/rs-sdk/examples/read_contract.rs (1)
77-77: LGTM: Added token config cache parameterThe new parameter for token configuration cache size aligns with the broader changes in the SDK builder and mock provider. This ensures that the example correctly initializes the
GrpcContextProviderwith the necessary caching support for token configurations.packages/rs-drive-proof-verifier/src/error.rs (1)
115-117: LGTM: Error variant for token configuration failuresThe new
TokenConfigurationFailureerror variant follows the established pattern of other variants in this enum, taking aStringparameter for the error message. This addition properly supports error handling for the token configuration retrieval operations.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_moment/mod.rs (2)
4-4: LGTM: Platform serialization support importsAdded the necessary imports from
platform_serialization_deriveto support serialization and deserialization of theRewardDistributionMomentenum.
12-26: LGTM: Enhanced serialization support for RewardDistributionMomentAdded
PlatformSerializeandPlatformDeserializetraits to enable platform-specific serialization mechanisms for this enum. The#[platform_serialize(unversioned)]attribute aligns with serialization requirements for this data type.packages/rs-sdk/src/sdk.rs (4)
50-51: LGTM: New constant for token config cache sizeAdded a new constant
DEFAULT_TOKEN_CONFIG_CACHE_SIZEwith value 100, which matches the existing pattern for other cache size constants in this file.
737-739: LGTM: Added token_config_cache_size field to SdkBuilderThis field properly supports the token configuration cache needed for the new token perpetual distribution claim feature, consistent with existing caching fields.
791-793: LGTM: Initialized token_config_cache_size in Default implementationThis initialization ensures the field is properly set to the default constant value when creating a new SdkBuilder instance.
1067-1067: LGTM: Updated GrpcContextProvider initializationThe GrpcContextProvider constructor now receives the token configuration cache size parameter, ensuring that token configurations can be properly cached when using the fallback context provider.
packages/rs-sdk/src/mock/provider.rs (4)
35-39: Properly documented cache field.The documentation for the new
token_configurations_cacheis clear and follows the same style as the existing cache fields, which maintains consistency across the codebase.
71-71: This parameter adds consistency with other cache sizes.The new parameter
token_config_cache_sizefollows the established pattern for configuring cache sizes.
80-80: Cache initialization is consistent with existing caches.The initialization uses the same approach as other caches, maintaining a consistent implementation pattern.
62-62: Good use of compiler directive to suppress warning.The
#[allow(clippy::too_many_arguments)]directive is appropriate here given the new parameter addition pushed the function over Clippy's threshold.packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (3)
1-27: Well-organized imports with logical grouping.The imports are organized in a clean and logical manner, separating gRPC-related imports from DPP and Drive components.
29-41: Comprehensive documentation explaining implementation cases.The implementation documentation clearly outlines the three cases handled by this method, which is important for understanding the logic flow.
131-136: Good preparation for future functionality.The error message clearly indicates that non-proof LastClaim responses are not yet supported, signaling that this functionality might be implemented in the future.
packages/rs-drive-proof-verifier/src/provider.rs (4)
3-3: Appropriate import of TokenConfiguration.Adding the TokenConfiguration import is necessary for the new trait method.
99-104: Implementation follows established pattern.The forwarding implementation for
get_token_configurationcorrectly delegates to the inner reference, matching the pattern used for other methods.
134-140: Mutex implementation is consistent with other methods.The implementation for
std::sync::Mutex<T>correctly acquires the lock and delegates to the inner implementation.
314-320: Clear documentation for mock implementation.The comment explaining that "Token configuration files are never generated" is helpful for understanding why this implementation always returns
None.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (1)
74-74: Use a more descriptive error for missingresultThe error
NoProofInResultdoesn't accurately describe this situation - you're checking for a missingresultfield, not necessarily a missing proof.- let result = resp_v0.result.clone().ok_or(Error::NoProofInResult)?; + let result = resp_v0.result.clone().ok_or(Error::RequestError { + error: "missing result in last claim response".into(), + })?;
🧹 Nitpick comments (6)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs (6)
54-56: Duplicate error message for token_id validationThe same error message for token_id validation appears twice in the code. Consider extracting this into a constant or creating a helper function to avoid duplication.
+ const TOKEN_ID_SIZE_ERROR: &str = "token_id must be 32 bytes"; // Then use this constant in both error locations: - .map_err(|_| Error::RequestError { - error: "token_id must be 32 bytes".into(), - })?; + .map_err(|_| Error::RequestError { + error: TOKEN_ID_SIZE_ERROR.into(), + })?;Also applies to: 80-82
72-72: Avoid unnecessary cloning of response fieldsThe
clone()calls on response fields could be inefficient, especially if these structures are large. Consider using references where possible.- let metadata = resp_v0 - .metadata - .clone() - .ok_or(Error::EmptyResponseMetadata)?; + let metadata = resp_v0 + .metadata + .as_ref() + .ok_or(Error::EmptyResponseMetadata)? + .clone(); - let result = resp_v0.result.clone().ok_or(Error::NoProofInResult)?; + let result = resp_v0.result.as_ref().ok_or(Error::NoProofInResult)?.clone();Also applies to: 74-74
79-82: Avoid unnecessary cloning of request dataThe
clone()call onreq_v0.token_idcould be inefficient. Use references instead of cloning data.- let token_id_identifier = Identifier::from_vec(req_v0.token_id.clone()) + let token_id_identifier = Identifier::from_vec(&req_v0.token_id) .map_err(|_| Error::RequestError { error: "token_id must be 32 bytes".into(), })?;
96-104: Document the purpose of thefalseparameterThe boolean parameter
falsein theverify_token_perpetual_distribution_last_paid_timecall is not self-explanatory. Consider adding a comment to clarify its purpose or use a named constant to make the code more readable.+ // false parameter indicates we don't need to enforce existence of the last paid time let (root_hash, moment_opt) = Drive::verify_token_perpetual_distribution_last_paid_time( &proof_msg.grovedb_proof, token_id, identity_id, &distribution_type, false, platform_version, )?;
118-120: Plan for supporting non-proof LastClaim responsesThe code currently returns an error for non-proof LastClaim responses, but it would be helpful to add a TODO comment with information about when this will be supported or what's needed to implement it.
RespResult::LastClaim(_) => Err(Error::RequestError { - error: "Non-proof LastClaim response is not supported in rs-sdk".into(), + error: "Non-proof LastClaim response is not yet supported in rs-sdk".into(), + // TODO: Implement support for direct LastClaim responses in a future update }),
33-123: Add more comprehensive documentationThe implementation could benefit from more detailed documentation explaining:
- When will this method return None vs Some(RewardDistributionMoment)
- What happens if an identity has never claimed
- What's the relationship between the
RewardDistributionMomentand the proofThe method has a brief doc comment but would benefit from more detailed documentation covering edge cases and return value semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-proof-verifier/src/proof/token_perpetual_distribution_last_claim.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (wasm-dpp) / Detect immutable structure changes
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
2 participants
Issue being fixed or feature implemented
Implemented Fetch for getting the last distribution claim moment
What was done?
Add a new struct in SDK query.rs TokenLastClaimQuery with token ID and identity ID, implement Query, Fetch, and FromProof for GetTokenPerpetualDistributionLastClaimRequest.
How Has This Been Tested?
Not yet
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores