feat: get proofs for tokens #2433
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive enhancements to token-related functionality across multiple packages in the platform. The changes primarily focus on expanding token management capabilities, including new methods for querying token balances, token information, and token statuses. The modifications span protocol buffer definitions, state transition proofs, drive queries, and verification methods, providing a more robust and flexible token ecosystem. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Platform
participant Drive
participant TokenVerification
Client->>Platform: GetProofsRequest with token queries
Platform->>Drive: Construct token-related path queries
Drive->>TokenVerification: Verify token balance/info/status
TokenVerification-->>Drive: Verification result
Drive-->>Platform: Proof results
Platform-->>Client: Proof response
Possibly related PRs
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (20)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (4)
157-177: Refactor repetitive error handling foridentity_idandtoken_idThe parsing and error handling for
identity_idandtoken_idin theidentity_token_balancesprocessing block are repeated multiple times. Consider refactoring this logic into a helper function to reduce code duplication and improve maintainability.
179-199: Refactor repetitive code inidentity_token_infosprocessingSimilar to the previous comment, the
identity_token_infosblock contains repeated parsing and error handling foridentity_idandtoken_id. Extracting this into a shared utility function would enhance code reuse and clarity.
201-213: Refactor repetitive code intoken_statusesprocessingThe
token_statusesblock also repeats the parsing and error handling logic fortoken_id. Consider creating a common function to handle this pattern across all token-related queries.
165-175: Ensure consistency in error messagesThe error messages for invalid
identity_idandtoken_idshould be consistent. Currently, some messages specify the field name (e.g.,"identity_id must be a valid identifier..."), while others use"id must be a valid identifier...". Standardize these messages for clarity.packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs (3)
296-296: Address the TODO comment regarding action groupingThere's a
//todo group actionscomment indicating that action grouping logic is pending implementation. Completing this implementation will ensure efficient handling of token actions.Do you want me to help implement the action grouping logic or open a new issue to track this task?
296-504: Refactor handling ofTokenTransitionvariantsThe match block for
TokenTransitionvariants is extensive and contains complex logic for each case. Consider refactoring each variant's handling into separate helper functions to improve readability and maintainability.
356-504: Simplify error messages in token transition handlingSome error messages within the token transition match arms could be streamlined for clarity. For example, in the
TokenTransition::EmergencyActioncase, the error message construction is complex. Simplifying these messages can improve code clarity.packages/rs-drive/src/query/token_status_drive_query.rs (1)
6-11: Consider enhancing documentation.While the basic documentation is present, it would be beneficial to add more details about:
- The purpose and use cases of this query
- The expected format/constraints of the token_id
- Any potential error conditions
packages/rs-drive/src/query/identity_token_info_drive_query.rs (1)
6-13: Enhance struct documentation.Consider adding more comprehensive documentation:
- Explain the relationship between identity and token
- Document any constraints on the identifiers
- Add examples of typical usage
packages/rs-drive/src/query/identity_token_balance_drive_query.rs (1)
6-13: Consider documentation improvements and potential refactoring.
Documentation could be enhanced with:
- Expected balance format/type
- Use cases and examples
- Error conditions
Consider extracting common patterns:
- The identity_id + token_id combination appears in multiple query structs
- Could benefit from a shared trait or macro implementation
Example of a potential shared trait:
trait IdentityTokenQuery { fn identity_id(&self) -> &Identifier; fn token_id(&self) -> &Identifier; }packages/rs-drive/src/drive/group/fetch/fetch_action_signers/v0/mod.rs (1)
Line range hint
66-70: Consider logging the conversion error for debugging.While using
_to discard the error is acceptable here, logging the actual conversion error could be helpful for debugging purposes in production environments.- value.try_into().map_err(|_| { + value.try_into().map_err(|e| { + tracing::debug!("Failed to convert signed power to u32: {:?}", e); Error::Drive(DriveError::CorruptedDriveState( "signed power should be encodable on a u32 integer".to_string(), )) })?,packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0_methods.rs (1)
Line range hint
79-93: Consider enhancing error messaging.While the implementation is correct, the error message could be more specific about why no minting recipient was found.
Consider this enhancement:
token_configuration .new_tokens_destination_identity() .ok_or(ProtocolError::Token( - TokenError::TokenNoMintingRecipient.into(), + TokenError::TokenNoMintingRecipient { + reason: "Neither issued_to_identity_id nor new_tokens_destination_identity is set".to_string(), + }.into(), ))packages/rs-drive/src/prove/prove_multiple_state_transition_results/v0/mod.rs (2)
123-135: Consider extracting common query extension logic.The filter_map blocks for token queries contain duplicated code patterns. Consider extracting this into a helper function to improve maintainability.
+ fn extend_path_queries<T>( + path_queries: &mut Vec<PathQuery>, + queries: &[T], + construct_query: impl Fn(&T) -> PathQuery, + ) { + path_queries.extend(queries.iter().filter_map(|query| { + let mut path_query = construct_query(query); + path_query.query.limit = None; + Some(path_query) + })); + }Usage:
- if !token_balance_queries.is_empty() { - path_queries.extend( - token_balance_queries - .iter() - .filter_map(|token_balance_query| { - let mut path_query = token_balance_query.construct_path_query(); - path_query.query.limit = None; - Some(path_query) - }), - ); - } + if !token_balance_queries.is_empty() { + extend_path_queries(&mut path_queries, token_balance_queries, |q| q.construct_path_query()); + }Also applies to: 137-145, 147-159
128-130: Consider improving error handling in path query construction.The comments suggest that serialization failures are rare but possible. Consider adding error logging or metrics to track these failures in production.
- let mut path_query = token_balance_query.construct_path_query(); + let mut path_query = token_balance_query.construct_path_query(); + if cfg!(debug_assertions) { + log::debug!("Constructed path query for token balance: {:?}", path_query); + }Also applies to: 140-141, 153-154
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/v0_methods.rs (1)
Line range hint
99-110: Consider documenting performance implications of cloning.The method clones all note fields, which could be expensive for large notes. Consider documenting this in the method comment or providing a reference-only alternative for performance-critical paths.
- /// Returns all notes (public, shared, and private) as cloned values in a tuple. + /// Returns all notes (public, shared, and private) as cloned values in a tuple. + /// + /// Note: This method clones all fields. For large notes or performance-critical paths, + /// consider using individual getter methods that return references.packages/dapi-grpc/protos/platform/v0/platform.proto (1)
467-469: Consider adding documentation to clarify usage patterns.The file has multiple ways to retrieve token information (direct RPCs vs GetProofs). Consider adding documentation to explain:
- When to use GetProofs vs direct token RPCs
- The trade-offs between these approaches
- Example use cases for each approach
packages/rs-drive/src/query/mod.rs (1)
144-154: LGTM! New token-related modules are well-organized.The new modules are properly feature-gated and follow the established naming conventions. Consider enhancing the documentation to briefly describe the specific token-related information each query returns.
/// A query to get the identity's token balance #[cfg(any(feature = "server", feature = "verify"))] pub mod identity_token_balance_drive_query; -/// A query to get the identity's token info +/// A query to get the identity's token info (e.g., ownership, permissions) #[cfg(any(feature = "server", feature = "verify"))] pub mod identity_token_info_drive_query; -/// A query to get the token's status +/// A query to get the token's status (e.g., active, locked, transferred) #[cfg(any(feature = "server", feature = "verify"))] pub mod token_status_drive_query;packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/v0/v0_methods.rs (1)
98-110: LGTM! Consider adding more detailed documentation.The new
notesmethod provides a non-consuming alternative tonotes_owned, which is a good addition. Consider enhancing the documentation to explain:
- The relationship between
notesandnotes_owned- The performance implications of cloning
/// Returns all notes (public, shared, and private) as cloned values in a tuple. +/// This method clones the notes instead of consuming them, unlike `notes_owned`. +/// Note: Cloning may have performance implications for large encrypted notes. fn notes( &self, ) -> ( Option<String>, Option<(SenderKeyIndex, RecipientKeyIndex, Vec<u8>)>, Option<( RootEncryptionKeyIndex, DerivationEncryptionKeyIndex, Vec<u8>, )>, );packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs (2)
329-332: Consider improving error handling for token destination.The error message could be more specific about the configuration requirements.
- None => token_configuration.new_tokens_destination_identity().ok_or(ProtocolError::NotSupported("either the mint destination must be set or the contract must have a destination set for new tokens".to_string()))?, + None => token_configuration.new_tokens_destination_identity().ok_or_else(|| { + ProtocolError::NotSupported( + "Token mint requires either an explicit destination identity or a default destination configured in the contract".to_string() + ) + })?,
358-360: Consider documenting the MAX token amount usage.The use of
TokenAmount::MAXfor destroyed funds should be documented to explain why we don't know the exact amount.TokenEvent::DestroyFrozenFunds( destroy.frozen_identity_id(), - TokenAmount::MAX, // we do not know how much will be destroyed + TokenAmount::MAX, // Using MAX as the exact amount of frozen funds to be destroyed is unknown at this point destroy.public_note().cloned(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/dapi-grpc/protos/platform/v0/platform.proto(1 hunks)packages/rs-dpp/src/state_transition/proof_result.rs(2 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0/v0_methods.rs(3 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0_methods.rs(2 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/v0/v0_methods.rs(2 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/v0_methods.rs(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs(4 hunks)packages/rs-dpp/src/tokens/emergency_action.rs(1 hunks)packages/rs-dpp/src/tokens/errors.rs(1 hunks)packages/rs-dpp/src/tokens/token_event.rs(1 hunks)packages/rs-drive-abci/src/query/proofs/v0/mod.rs(12 hunks)packages/rs-drive-abci/tests/strategy_tests/token_tests.rs(2 hunks)packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs(1 hunks)packages/rs-drive/src/drive/group/fetch/fetch_action_signers/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/info/queries.rs(1 hunks)packages/rs-drive/src/prove/prove_multiple_state_transition_results/mod.rs(4 hunks)packages/rs-drive/src/prove/prove_multiple_state_transition_results/v0/mod.rs(4 hunks)packages/rs-drive/src/query/identity_token_balance_drive_query.rs(1 hunks)packages/rs-drive/src/query/identity_token_info_drive_query.rs(1 hunks)packages/rs-drive/src/query/mod.rs(1 hunks)packages/rs-drive/src/query/token_status_drive_query.rs(1 hunks)packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs(4 hunks)packages/rs-drive/src/verify/tokens/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_balance_for_identity_id/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_balance_for_identity_id/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_info_for_identity_id/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_info_for_identity_id/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_status/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_status/v0/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs(2 hunks)packages/token-history-contract/schema/v1/token-history-contract-documents.json(3 hunks)
🔇 Additional comments (37)
packages/rs-dpp/src/tokens/token_event.rs (1)
167-167: LGTM! Property key renamed for consistency.The renaming of the property key from "amount" to "destroyedAmount" in the
DestroyFrozenFundsvariant aligns with the schema changes, maintaining consistency across the codebase.Let's verify that this is the only occurrence of this property key in the codebase:
✅ Verification successful
Verified: Property key renaming is correctly implemented and isolated
The renaming from "amount" to "destroyedAmount" is properly implemented in the DestroyFrozenFunds variant while maintaining appropriate "amount" property keys in other token operations (mint, burn, transfer).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of the old property key name in token-related files rg '"amount"' --type rust | grep -v 'mint\|burn\|transfer'Length of output: 3627
Script:
#!/bin/bash # Search for DestroyFrozenFunds variant and surrounding context ast-grep --pattern 'DestroyFrozenFunds { $$$ }' # Search for both property keys in token-related files echo "=== Searching for destroyedAmount ===" rg '"destroyedAmount"' --type rust echo "=== Searching for amount in token files ===" rg '"amount"' packages/rs-dpp/src/tokens/Length of output: 902
Script:
#!/bin/bash # Get full context around the property key usages in token_event.rs rg -B 5 -A 5 '"amount"|"destroyedAmount"' packages/rs-dpp/src/tokens/token_event.rsLength of output: 2134
packages/token-history-contract/schema/v1/token-history-contract-documents.json (3)
463-463: LGTM! Index property renamed for consistency.The index property in
destroyFrozenFunds.indiceshas been updated from "amount" to "destroyedAmount", maintaining consistency with the property name change.
Line range hint
501-505: LGTM! Property definition updated with clear description.The property definition has been renamed to "destroyedAmount" with an appropriate description indicating it represents "the amount that was frost burned".
511-511: LGTM! Required field updated accordingly.The required field list has been updated to use "destroyedAmount" instead of "amount", completing the consistent renaming across the schema.
packages/rs-drive/src/verify/tokens/verify_token_balance_for_identity_id/v0/mod.rs (1)
14-51: Well-implemented verification method with proper error handlingThe
verify_token_balance_for_identity_id_v0function is correctly implemented, handling proof verification and error cases appropriately. The use of pattern matching onproved_key_valuesensures that both the presence and absence of the token balance are accounted for.packages/rs-drive/src/verify/tokens/verify_token_status/v0/mod.rs (1)
15-54: Correct implementation of token status verificationThe
verify_token_status_v0function properly verifies the token status using proofs. Error handling for unexpected cases is appropriately managed, and deserialization of theTokenStatusis correctly performed.packages/rs-drive/src/verify/tokens/verify_token_info_for_identity_id/v0/mod.rs (1)
15-55: Accurate implementation of token info verificationThe method
verify_token_info_for_identity_id_v0is accurately implemented, effectively handling the verification of token information associated with an identity ID. The function properly manages proofs and deserializes theIdentityTokenInfoas expected.packages/rs-drive-abci/src/query/proofs/v0/mod.rs (7)
7-9: LGTM: Added imports for token request typesThe new imports
IdentityTokenBalanceRequest,IdentityTokenInfoRequest, andTokenStatusRequestare correctly added to support the new token-related functionalities.
19-21: LGTM: Imported token query typesThe imports for
IdentityTokenBalanceDriveQuery,IdentityTokenInfoDriveQuery, andTokenStatusDriveQueryare appropriately included for handling token-related proofs.
32-34: ExtendedGetProofsRequestV0with token fieldsThe addition of
identity_token_balances,identity_token_infos, andtoken_statusesfields toGetProofsRequestV0correctly extends the request structure to handle token-related queries.
220-222: Included token queries in proof generationAdding
&token_balance_queries,&token_info_queries, and&token_status_queriestoprove_multiple_state_transition_resultscorrectly integrates the new token queries into the proof generation process.
263-265: Add tests foridentity_token_balancesIn the
test_invalid_identity_idstest case, consider adding scenarios that include invalididentity_token_balancesto ensure proper validation and error handling of token balance requests.
289-291: Add tests foridentity_token_infosEnhance the
test_invalid_identity_prove_request_typetest by including invalididentity_token_infosto verify that the system correctly handles token info requests with invalid identifiers.
318-320: Add tests fortoken_statusesIn the
test_invalid_contract_idstest, consider adding cases for invalidtoken_statusesto ensure comprehensive testing of token status requests.packages/rs-drive/src/verify/tokens/mod.rs (1)
Line range hint
1-10: Well-organized module structure!The new modules for singular token operations (balance, info, status) complement the existing plural form modules, following a consistent and logical organization pattern.
packages/rs-drive/src/query/token_status_drive_query.rs (1)
13-18: Clean implementation!The implementation is well-structured and follows Rust best practices with proper encapsulation and error handling.
packages/rs-drive/src/query/identity_token_info_drive_query.rs (1)
15-23: Well-implemented query construction!The path query construction is clean and follows the established pattern consistently.
packages/rs-drive/src/query/identity_token_balance_drive_query.rs (1)
15-23: Clean implementation of path query construction!The implementation follows the established pattern consistently.
packages/rs-dpp/src/tokens/emergency_action.rs (1)
21-23: LGTM! Clean and idiomatic implementation.The new
paused()method is well-implemented using idiomatic Rust pattern matching. It provides a clear and concise way to check the token's pause status.packages/rs-dpp/src/state_transition/proof_result.rs (1)
15-19: LGTM! Well-structured enum variants.The new token-related variants in
StateTransitionProofResultare well-organized and follow a consistent naming pattern. The use ofBTreeMapfor collections and appropriate types for balances and identifiers aligns with Rust best practices.Also applies to: 23-23
packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs (1)
36-36: LGTM! Consistent version initialization.The new verification methods for action signers and token operations are well-integrated into the existing structure. The initialization to version 0 is consistent with other methods in the codebase.
Also applies to: 45-47
packages/rs-drive/src/prove/prove_multiple_state_transition_results/mod.rs (3)
7-9: LGTM! Clean imports addition.The new imports for token-related queries are well-organized and follow the existing import pattern.
27-34: LGTM! Well-documented parameters.The documentation for the new token-related parameters is clear, comprehensive, and follows the existing documentation style.
48-50: LGTM! Consistent parameter addition.The new token-related query parameters are added consistently in both the function signature and the version-specific function call.
Also applies to: 65-67
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0_methods.rs (2)
2-4: LGTM! Clean imports addition.The new imports are well-organized and follow the existing import pattern.
Also applies to: 11-11
Line range hint
45-48: LGTM! Clear method signature.The
recipient_idmethod signature is well-defined and follows the trait's existing pattern.packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs (1)
52-52: LGTM! Consistent version field additions.The new version fields for token and group verification methods follow the existing pattern and maintain consistency with the codebase.
Also applies to: 63-65
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0/v0_methods.rs (1)
78-94: Consider adding test coverage.The
recipient_idimplementation looks correct, but it would benefit from comprehensive test coverage to ensure proper handling of different scenarios:
- When
issued_to_identity_idis set- When
new_tokens_destination_identityis used as fallback- When both are missing (error case)
Would you like me to help generate test cases for these scenarios?
packages/dapi-grpc/protos/platform/v0/platform.proto (2)
449-461: Well-structured message types for token-related requests!The new message types are well-designed with:
- Consistent use of bytes type for IDs
- Clear and descriptive field names
- Sequential field numbering
- Appropriate granularity of data
467-469: Clean integration of new fields into GetProofsRequestV0!The new repeated fields are well-integrated with:
- Sequential field numbers continuing from existing fields
- Consistent naming with their referenced message types
- Appropriate use of repeated fields for collections
packages/rs-drive/src/query/mod.rs (1)
135-142: LGTM! Module visibility changes are well-structured.The modules are properly gated behind feature flags and include descriptive documentation.
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/v0/v0_methods.rs (1)
208-225: Implementation looks good!The implementation correctly clones all fields and matches the trait declaration.
packages/rs-drive-abci/tests/strategy_tests/token_tests.rs (2)
70-72: LGTM! Good cleanup of unnecessary mutability.Removing
mutfromtoken_configurationimproves code quality by enforcing immutability after initialization.
218-220: LGTM! Consistent with previous change.Similar to the previous instance, removing
mutfromtoken_configurationis a good practice.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs (2)
6-16: LGTM! Well-organized imports.The new imports are correctly organized and necessary for the added functionality.
162-188: LGTM! Well-structured trait methods.The new trait methods are well-defined with clear purposes and appropriate return types.
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs (1)
116-118: LGTM! Consistent with existing pattern.The new fields for token-related proofs are correctly initialized as empty vectors, following the established pattern.
packages/rs-drive/src/verify/tokens/verify_token_info_for_identity_id/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/tokens/verify_token_info_for_identity_id/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/tokens/verify_token_info_for_identity_id/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/tokens/verify_token_balance_for_identity_id/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/tokens/verify_token_balance_for_identity_id/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/tokens/verify_token_balance_for_identity_id/mod.rs
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
New Features
Improvements
Bug Fixes
These changes significantly enhance the platform's token management and verification capabilities, providing more robust and flexible token-related functionality.
Checklist:
For repository code-owners and collaborators only