refactor: move proof retrieval from DAPI to Drive ABCI #2535
Conversation
WalkthroughThis pull request refactors and introduces Drive-related functionality while removing legacy proof handling from platform endpoints. It removes the dependency on the token history contract from configuration files and shifts the proof logic from Platform clients to a new Drive service implementation with updated gRPC clients and proto definitions. Additionally, build scripts, Rust modules, and auxiliary functions have been updated to support the Drive feature, resulting in streamlined request handling and consolidated control flows. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant DPC as DrivePromiseClient
participant PG as ProtoLoader (getDriveDefinition)
participant GS as gRPC Server (DriveInternal)
C->>DPC: Call getProofs(request)
DPC->>PG: Load Drive proto definitions
DPC->>GS: Send GetProofs RPC request
GS-->>DPC: Return GetProofsResponse
DPC-->>C: Return response to client
Possibly related PRs
Suggested labels
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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 (
|
shumkov
changed the title
|
Can someone please explain the rationale for this refactor (for example, will it have any effect on proof availability or any other effect for clients)? |
|
I believe this is related to #2524, but the description is minimal. |
![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
🧹 Nitpick comments (3)
packages/dapi-grpc/scripts/build.sh (1)
19-104: Consider adding Drive client generation for Java, Objective-C, and Python.While JavaScript/TypeScript client generation for Drive has been implemented, there are no sections for generating Java, Objective-C, or Python clients for Drive as there are for Core and Platform services. If these clients will be needed in the future, consider adding these sections.
packages/dapi-grpc/clients/drive/v0/web/drive_pb_service.d.ts (1)
16-19: Consider replacing the static-only classStatic analysis notes that
DriveInternalcontains only static members. Converting this to a simpler structure (e.g., a namespace or standalone methods) might reduce complexity and avoid the no-static-only-class lint.🧰 Tools
🪛 Biome (1.9.4)
[error] 16-19: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (1)
62-312: Large match block highlights potential for modularizationThis match statement handles each state transition type thoroughly, constructing path queries for each scenario. However, it’s quite large and could become challenging to maintain. Consider extracting each transition branch into a dedicated helper function or module to reduce complexity and improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/dashmate/templates/platform/gateway/envoy.yaml.dotis excluded by!**/*.dotyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (45)
.pnp.cjs(0 hunks)packages/dapi-grpc/.gitignore(1 hunks)packages/dapi-grpc/Cargo.toml(2 hunks)packages/dapi-grpc/build.rs(5 hunks)packages/dapi-grpc/clients/drive/v0/nodejs/DrivePromiseClient.js(1 hunks)packages/dapi-grpc/clients/drive/v0/nodejs/drive_protoc.js(1 hunks)packages/dapi-grpc/clients/drive/v0/web/drive_pb.d.ts(1 hunks)packages/dapi-grpc/clients/drive/v0/web/drive_pb.js(1 hunks)packages/dapi-grpc/clients/drive/v0/web/drive_pb_service.d.ts(1 hunks)packages/dapi-grpc/clients/drive/v0/web/drive_pb_service.js(1 hunks)packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java(1 hunks)packages/dapi-grpc/clients/platform/v0/nodejs/PlatformPromiseClient.js(0 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h(0 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m(0 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h(0 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m(0 hunks)packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py(0 hunks)packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts(0 hunks)packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts(0 hunks)packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js(0 hunks)packages/dapi-grpc/lib/getDriveDefinition.js(1 hunks)packages/dapi-grpc/node.js(2 hunks)packages/dapi-grpc/protos/drive/v0/drive.proto(1 hunks)packages/dapi-grpc/protos/platform/v0/platform.proto(2 hunks)packages/dapi-grpc/scripts/build.sh(10 hunks)packages/dapi-grpc/scripts/patch-protobuf-js.sh(2 hunks)packages/dapi-grpc/src/lib.rs(1 hunks)packages/dapi/lib/dpp/DriveStateRepository.js(0 hunks)packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js(1 hunks)packages/dapi/lib/grpcServer/handlers/platform/getStatusHandlerFactory.js(1 hunks)packages/dapi/lib/grpcServer/handlers/platform/platformHandlersFactory.js(3 hunks)packages/dapi/lib/grpcServer/handlers/platform/waitForStateTransitionResultHandlerFactory.js(1 hunks)packages/dapi/package.json(0 hunks)packages/dapi/scripts/api.js(3 hunks)packages/js-grpc-common/lib/loadPackageDefinition.js(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/mod.rs(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/test/tokens.rs(3 hunks)packages/rs-drive-abci/src/query/proofs/mod.rs(2 hunks)packages/rs-drive-abci/src/query/proofs/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/query/service.rs(3 hunks)packages/rs-drive-abci/src/server.rs(2 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/mocks/v2_test.rs(1 hunks)
💤 Files with no reviewable changes (12)
- .pnp.cjs
- packages/dapi/lib/dpp/DriveStateRepository.js
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js
- packages/dapi-grpc/clients/platform/v0/nodejs/PlatformPromiseClient.js
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts
- packages/dapi/package.json
- packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py
- 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/web/platform_pb.d.ts
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h
🧰 Additional context used
🧬 Code Graph Analysis (14)
packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/mod.rs (1)
packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/v0/mod.rs (1)
vote_owned(11-11)
packages/js-grpc-common/lib/loadPackageDefinition.js (1)
packages/dapi-grpc/lib/getDriveDefinition.js (2)
protoPath(6-6)includeDirs(7-9)
packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/v0/mod.rs (1)
packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/mod.rs (1)
vote_owned(44-48)
packages/dapi-grpc/clients/drive/v0/web/drive_pb.js (3)
packages/dapi-grpc/clients/drive/v0/nodejs/drive_protoc.js (1)
proto(16-16)packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js (1)
proto(16-16)packages/dapi-grpc/clients/platform/v0/web/platform_pb.js (1)
proto(16-16)
packages/rs-drive-abci/src/query/service.rs (1)
packages/dapi-grpc/clients/drive/v0/web/drive_pb.d.ts (2)
GetProofsRequest(7-21)GetProofsResponse(29-48)
packages/rs-drive-abci/src/server.rs (4)
packages/dapi-grpc/build.rs (1)
new(317-338)packages/rs-drive-abci/src/query/service.rs (1)
new(71-73)packages/rs-drive-abci/src/abci/app/check_tx.rs (2)
new(41-43)platform(31-33)packages/rs-drive-abci/src/abci/app/full.rs (2)
new(30-36)platform(40-42)
packages/dapi-grpc/clients/drive/v0/nodejs/DrivePromiseClient.js (6)
packages/js-grpc-common/lib/loadPackageDefinition.js (1)
grpc(1-1)packages/dapi-grpc/clients/platform/v0/nodejs/PlatformPromiseClient.js (4)
grpc(1-1)require(2-2)require(4-18)require(20-20)packages/dapi-grpc/lib/getDriveDefinition.js (1)
require(3-3)packages/dapi/lib/grpcServer/handlers/platform/waitForStateTransitionResultHandlerFactory.js (2)
require(1-8)require(10-15)packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js (1)
require(1-5)packages/dapi-grpc/node.js (1)
DrivePromiseClient(4-4)
packages/dapi/lib/grpcServer/handlers/platform/platformHandlersFactory.js (1)
packages/dapi/scripts/api.js (5)
platformHandlersFactory(37-39)rpcClient(71-74)blockchainListener(111-111)platformClient(68-68)driveClient(69-69)
packages/dapi/scripts/api.js (1)
packages/dapi-grpc/node.js (2)
PlatformPromiseClient(3-3)DrivePromiseClient(4-4)
packages/dapi-grpc/node.js (2)
packages/dapi-grpc/lib/getDriveDefinition.js (1)
require(3-3)packages/dapi/scripts/api.js (3)
require(5-9)require(11-18)require(28-28)
packages/dapi-grpc/scripts/build.sh (1)
packages/rs-drive-abci/src/main.rs (1)
run(102-159)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (3)
packages/dapi-grpc/clients/drive/v0/web/drive_pb.d.ts (2)
GetProofsRequest(7-21)GetProofsResponse(29-48)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs (4)
data_contract_id(177-177)data_contract_id(250-252)token_id(182-182)token_id(285-287)packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/v0/mod.rs (1)
pro_tx_hash(6-6)
packages/dapi-grpc/clients/drive/v0/nodejs/drive_protoc.js (3)
packages/dapi-grpc/clients/drive/v0/web/drive_pb.js (1)
proto(16-16)packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js (1)
proto(16-16)packages/dapi-grpc/clients/platform/v0/web/platform_pb.js (1)
proto(16-16)
packages/rs-drive-abci/src/query/proofs/mod.rs (1)
packages/dapi-grpc/clients/drive/v0/web/drive_pb.d.ts (2)
GetProofsRequest(7-21)GetProofsResponse(29-48)
🪛 Biome (1.9.4)
packages/dapi-grpc/clients/drive/v0/web/drive_pb.js
[error] 134-134: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 289-289: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 294-294: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 290-290: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 291-291: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 294-294: Shouldn't redeclare 'value'. Consider to delete it or rename it.
'value' is defined here:
(lint/suspicious/noRedeclare)
packages/dapi-grpc/clients/drive/v0/nodejs/drive_protoc.js
[error] 134-134: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 289-289: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 294-294: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 290-290: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 291-291: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 294-294: Shouldn't redeclare 'value'. Consider to delete it or rename it.
'value' is defined here:
(lint/suspicious/noRedeclare)
packages/dapi-grpc/clients/drive/v0/web/drive_pb_service.d.ts
[error] 16-19: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 Buf (1.47.2)
packages/dapi-grpc/protos/drive/v0/drive.proto
5-5: import "platform/v0/platform.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- 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-abci) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- 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 JS packages / Build JS
🔇 Additional comments (60)
packages/dapi-grpc/.gitignore (1)
3-3: New drive directory added to .gitignoreThe addition of
src/drive/*/to .gitignore is consistent with the rest of the file and aligns with the PR objective of moving proof retrieval logic to Drive ABCI. This ensures generated Drive-related files are properly excluded from version control.packages/dapi/lib/grpcServer/handlers/platform/getStatusHandlerFactory.js (1)
218-218:❓ Verification inconclusive
Number to string conversion for setLocal method
The change converts the numeric timestamp from
Date.now()to a string before passing it totime.setLocal(). This suggests that the method now expects a string parameter rather than a number.
🏁 Script executed:
#!/bin/bash # Check the method signature in the GetStatusResponse proto definition to verify it expects a string rg -A 2 -B 2 "message Time" --type proto rg "setLocal" --type jsLength of output: 1273
Ensure consistent type conversion for all setLocal calls
The conversion using
Date.now().toString()on line 218 looks correct given thatsetLocalis now expected to receive a string. However, note that elsewhere in the same file (e.g., the call on the cached response)setLocalis still being invoked with a numeric timestamp (Date.now()). Please verify whether this inconsistency is intentional or if the conversion should be applied there as well.packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/v0/mod.rs (1)
11-11: Added method to take ownership of VoteAdding the
vote_ownedmethod to the trait is a good design choice. This complements the existingvotemethod that returns a reference, allowing callers to take ownership of the Vote when needed, which is useful for proof handling logic being moved to Drive ABCI.packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/mod.rs (1)
44-48: Implemented vote_owned for MasternodeVoteTransitionThe implementation correctly implements the new trait method by pattern matching on the enum variant and returning the owned Vote. This maintains consistency with other accessor methods while following Rust's ownership principles.
packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/test/tokens.rs (1)
276-276:❓ Verification inconclusive
Verify the new parameter in token_mint calls
The
token_mintmethod has been updated with an additional boolean parameter set tofalse. This change appears to be related to the proof retrieval refactoring mentioned in the PR objectives.Also applies to: 290-290, 305-305
🏁 Script executed:
#!/bin/bash # Let's explore the token_mint method signature to understand the meaning of the new parameter # First, find the token_mint method definition rg -A 10 "fn token_mint.*?" --type rustLength of output: 11282
Action Required: Verify the Updated Boolean Parameter in
token_mintCallsThe change in the
token_mintmethod now introduces an extra boolean argument (shown asfalsein the test file at lines 276, 290, and 305). Our search confirms that the updated signatures in the drive module include two boolean parameters (one being the preexisting option, and the new one likely governing behavior such as saturation/proof retrieval as indicated in the PR objectives). Please ensure that thefalsevalue is correct and consistent with the intended changes from the proof retrieval refactoring.
- Verify in:
packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/test/tokens.rsat lines 276, 290, and 305- The updated signature in the drive token modules (e.g., in
packages/rs-drive/src/drive/tokens/mint/mod.rs)packages/dapi-grpc/lib/getDriveDefinition.js (1)
1-19: New Drive gRPC service definition handler looks goodThis new file properly implements the Drive service definition loader, which is consistent with the PR's objective of moving proof retrieval from DAPI to Drive ABCI. The implementation follows the pattern used for other service definitions in the codebase.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs (1)
9-9:✅ Verification successful
Simplified version handling for proofs_query
Changed
proofs_querytype fromFeatureVersionBoundstoFeatureVersion, which simplifies version handling for proof queries. This aligns with the refactoring goal of streamlining the proof handling process.
🏁 Script executed:
#!/bin/bash # Let's check if this change is consistently applied across the codebase # Look for instances where proofs_query is defined or initialized rg "proofs_query.*FeatureVersion" --type rustLength of output: 179
Verified: proofs_query refactor to FeatureVersion is consistent.
After executing the shell script, the only instance found was in
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs, confirming that the change fromFeatureVersionBoundstoFeatureVersionhas been applied consistently. No additional occurrences were found.packages/js-grpc-common/lib/loadPackageDefinition.js (1)
11-12: Enhanced flexibility with includeDirs parameterThe
loadPackageDefinitionfunction has been updated to accept an optionalincludeDirsparameter, which allows for more flexible proto file loading. This change supports the new Drive service integration by enabling the specification of additional directories for proto file resolution.Also applies to: 14-14, 21-21
packages/dapi-grpc/Cargo.toml (2)
18-19: New feature added for Drive internal endpointsThe addition of the
drivefeature enables support for internal Drive endpoints that will be used by DAPI. This is aligned with the PR goal of moving proof retrieval from DAPI to Drive ABCI.
30-30: Updated server feature to include driveThe server feature now includes the new drive feature, ensuring that server implementations will have access to the Drive functionality. This change is necessary for the refactoring of proof retrieval.
packages/rs-platform-version/src/version/mocks/v2_test.rs (1)
150-150: Simplified proofs_query versioningThe
proofs_queryfield has been simplified from aFeatureVersionBoundsstructure to a simple integer value. This change aligns with the restructuring of proof handling and simplifies the versioning for proof queries.packages/dapi-grpc/src/lib.rs (1)
49-52: New module added for DAPI internal useThe addition of the
dapimodule that re-exports the platform module functionality provides a clear separation of concerns while ensuring the Drive feature has access to the necessary platform components.packages/dapi-grpc/clients/drive/v0/nodejs/DrivePromiseClient.js (2)
1-43: New DrivePromiseClient implementation looks goodThe implementation includes necessary imports and properly initializes the gRPC client based on the protocol (HTTP/HTTPS). The class follows the same pattern as other gRPC client implementations in the codebase.
44-102: Client implementation for getProofs methodThe
DrivePromiseClientimplementation correctly sets up the promise-based client and provides thegetProofsmethod that will now handle proof retrieval in Drive ABCI instead of DAPI. The method includes proper JSON to Protobuf conversion and error handling.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs (1)
12-12: Simplified versioning forproofs_queryThe change from a complex
FeatureVersionBoundsstructure to a simple integer value (0) aligns with the PR objective of moving proof retrieval from DAPI to Drive ABCI. This simplification makes the versioning approach more consistent with the new implementation and reduces complexity.packages/rs-drive-abci/src/query/service.rs (2)
11-12: Added imports for new Drive functionalityThese imports support the implementation of the DriveInternal trait, which is essential for moving proof functionality from DAPI to Drive ABCI.
743-756: Implementedget_proofsas part of DriveInternal traitThis implementation correctly adds proof retrieval functionality to Drive ABCI, leveraging the existing
handle_blocking_queryinfrastructure to process the request and invoke the appropriate query method. This is the core change that fulfills the PR objective of moving proof retrieval from DAPI to Drive ABCI.The implementation ensures consistent error handling and metrics tracking by using the same handling pattern as other query endpoints, which promotes code reliability and maintainability.
packages/dapi-grpc/protos/platform/v0/platform.proto (1)
9-68: RemovedgetProofsendpoint from Platform serviceThe removal of the
getProofsmethod from the Platform service is consistent with moving this functionality to Drive ABCI. This change ensures that clients will now use the appropriate Drive ABCI endpoint for proof retrieval.Note that while the Proof message definition (lines 71-78) is still maintained, the actual service method for retrieving proofs has been moved to the Drive service.
packages/dapi-grpc/scripts/patch-protobuf-js.sh (2)
5-5: Extended file search to include Drive directoriesThe script now searches for files in the Drive client directories, which is necessary to support the new Drive ABCI service functionality. This ensures that all generated JavaScript files are properly patched.
30-30: Added path correction for platform importsThis replacement operation ensures that the Drive-related generated code correctly references Platform-related code, maintaining interoperability between the services. This is necessary infrastructure work to support the architectural change.
packages/dapi-grpc/node.js (3)
4-4: New Drive service imports added correctly.The imports for the Drive client, messages, and definition are properly added following the same pattern as the existing Core and Platform services.
Also applies to: 8-8, 12-12
39-49: LGTM: Drive messages import structure follows established pattern.The import structure for the Drive messages mirrors the existing pattern used for Core and Platform, maintaining code consistency.
54-54: Exports correctly updated for new Drive functionality.The module exports have been properly updated to include the new Drive-related functions and messages, making them available to consumers of this module.
Also applies to: 58-58, 63-63, 67-67
packages/dapi-grpc/scripts/build.sh (4)
11-12: Good refactoring: Common path variable extracted.Creating a common
PROTOS_PATHvariable improves maintainability and reduces repetition in Docker mount commands.
19-21: Drive path variables defined consistently.The Drive path variables follow the same naming convention and structure as the existing Core and Platform variables.
Also applies to: 24-24
71-104: New Drive JavaScript client generation section implemented correctly.The section for generating the JavaScript client for the DriveInternal service follows the same pattern as the existing sections for Core and Platform services. The implementation includes all necessary steps: cleaning directories, running the Docker container with appropriate mounts and flags, and generating the necessary message classes.
113-113: PROTOS_PATH consistently added to all Docker commands.The new
PROTOS_PATHis consistently mounted and included in all Docker commands throughout the script, ensuring access to all proto files during code generation.Also applies to: 119-119, 136-136, 147-147, 154-154, 165-165, 172-172, 183-183, 191-191, 202-202, 210-210, 221-221, 229-229, 240-240, 248-248
packages/dapi/lib/grpcServer/handlers/platform/waitForStateTransitionResultHandlerFactory.js (1)
118-122: Simplified proof retrieval logic.The refactored code now directly fetches the proof using
fetchProofForStateTransitioninstead of first creating a state transition object. This simplification aligns with the PR objective of moving proof retrieval from DAPI to Drive ABCI.The new approach:
- Directly passes the transaction to
fetchProofForStateTransition- Uses the returned
stateTransitionProofobject to set metadata and proof- Eliminates unnecessary intermediate steps
packages/dapi-grpc/clients/drive/v0/web/drive_pb.d.ts (3)
1-6: Well-structured TypeScript definitions with appropriate imports.The new Drive TypeScript definitions correctly import necessary dependencies and define the package and proto file information.
7-27: GetProofsRequest class properly defines state transition handling.The
GetProofsRequestclass is well-implemented with appropriate methods for:
- Getting and setting state transition data in various formats
- Serialization and deserialization
- Object conversion
The class follows standard gRPC TypeScript definition patterns.
29-55: GetProofsResponse class reuses Platform components appropriately.The
GetProofsResponseclass correctly reuses existing Platform components (ProofandResponseMetadata) rather than duplicating definitions. The implementation includes all necessary methods for handling these fields.packages/dapi/lib/grpcServer/handlers/platform/platformHandlersFactory.js (4)
63-64: Updated function parameters to support the refactored architectureThe parameters have been updated to accept both
platformClientanddriveClientinstances, which aligns with the PR objective of moving proof retrieval from DAPI to Drive ABCI.
71-72: Altered parameter ordering to accommodate the new platformClient parameterThe parameter order has been updated to include the new
platformClientbefore the existingdriveClient, maintaining a consistent parameter structure.
98-98: Simplified fetchProofForStateTransition factory callThe factory call now only requires the
driveClientparameter, which aligns with the architectural refactoring to move proof retrieval logic from DAPI to Drive ABCI.
146-146: Switched to use platformClient for status retrievalThe
getStatusHandlernow usesplatformClientinstead ofdriveClient, properly separating responsibilities between the platform and drive services.packages/dapi-grpc/clients/drive/v0/web/drive_pb.js (1)
1-421: Added Protocol Buffer definitions for Drive API communicationThis is a generated file defining the message types needed for the new Drive API endpoints, specifically for proof retrieval. The file introduces
GetProofsRequestandGetProofsResponsemessage types that will facilitate communication with the Drive service.There are some static analysis warnings about variable declarations in switch clauses that should be wrapped in blocks. However, since this is auto-generated code (as indicated by the "GENERATED CODE -- DO NOT EDIT!" comment), these issues should not be manually fixed in this file.
🧰 Tools
🪛 Biome (1.9.4)
[error] 134-134: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 289-289: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 294-294: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 290-290: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 291-291: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 294-294: Shouldn't redeclare 'value'. Consider to delete it or rename it.
'value' is defined here:
(lint/suspicious/noRedeclare)
packages/dapi/scripts/api.js (3)
16-16: Added import for Drive Promise ClientAdded
DrivePromiseClientimport to support the new architecture where proof retrieval is handled by the Drive service.
68-69: Added client instantiation for platform and drive servicesCreated separate client instances for both Platform and Drive services, using the same host and port configuration. This supports the architectural separation of concerns outlined in the PR objectives.
142-144: Updated platformHandlersFactory parametersModified the parameters passed to
platformHandlersFactoryto include bothplatformClientanddriveClient, aligning with the refactored function signature.packages/dapi-grpc/clients/drive/v0/nodejs/drive_protoc.js (1)
1-421:✅ Verification successful
Added Node.js Protocol Buffer definitions for Drive API
This is a generated file that defines the message types needed for Node.js clients to communicate with the Drive service. It mirrors the web version but is specifically for Node.js environments.
Similar to the web version, there are static analysis warnings about variable declarations in switch clauses. As this is auto-generated code, these issues should not be manually fixed.
It's worth confirming that both the web and Node.js protocol buffer definitions are in sync and generated from the same source proto file to ensure consistent behavior across environments.
🏁 Script executed:
#!/bin/bash # Verify that both web and Node.js protocol buffer definitions are in sync md5sum packages/dapi-grpc/clients/drive/v0/web/drive_pb.js packages/dapi-grpc/clients/drive/v0/nodejs/drive_protoc.js grep -n "proto.org.dash.platform.drive.v0.GetProofsRequest" packages/dapi-grpc/clients/drive/v0/web/drive_pb.js packages/dapi-grpc/clients/drive/v0/nodejs/drive_protoc.jsLength of output: 7655
Synchronized Protocol Buffer Definitions Confirmed
Both the Node.js and web versions of the Drive API protocol buffer definitions are identical (verified via MD5 hash and symbol occurrences). As these files are auto-generated, the static analysis warnings (e.g., variable declarations in switch clauses) are expected and should not be manually addressed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 134-134: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 289-289: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 294-294: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 290-290: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 291-291: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 294-294: Shouldn't redeclare 'value'. Consider to delete it or rename it.
'value' is defined here:
(lint/suspicious/noRedeclare)
packages/dapi-grpc/protos/drive/v0/drive.proto (1)
1-18:❓ Verification inconclusive
New DriveInternal service implementation looks good, but the import path needs verification
The implementation of the new
DriveInternalservice aligns well with the PR objective to move proof retrieval from DAPI to Drive ABCI. The service definition, request and response message structures are clear and well-commented.However, the static analysis shows that the imported file "platform/v0/platform.proto" might not exist. Let's verify this:
🏁 Script executed:
#!/bin/bash # Check if the imported platform proto file exists ls -la protos/platform/v0/platform.proto # Check what imports are available in the proto directory structure find protos -name "*.proto" | sortLength of output: 202
Review: Verify and Update Import Path in DriveInternal Service
The new
DriveInternalservice implementation is well-structured and meets the PR objectives regarding proof retrieval from DAPI. However, the static analysis indicates that the imported file"platform/v0/platform.proto"is not found in the expected location. This may be due to an incorrect relative path, file relocation, or the file being missing entirely.
- File Affected:
packages/dapi-grpc/protos/drive/v0/drive.proto
- Line: 3 (
import "platform/v0/platform.proto";)- Action Required: Please manually verify whether:
- The referenced file exists elsewhere in the repository.
- The intended import path should be updated based on the current project structure.
Once confirmed, update the import path accordingly to ensure the file is correctly linked.
🧰 Tools
🪛 Buf (1.47.2)
5-5: import "platform/v0/platform.proto": file does not exist
(COMPILE)
packages/dapi-grpc/clients/drive/v0/web/drive_pb_service.js (1)
1-61: Web client implementation for DriveInternal service looks goodThe gRPC-web client implementation follows the established pattern in the codebase and provides appropriate error handling. The service name, method definition, and client implementation are correctly structured for interacting with the newly defined Drive gRPC service.
packages/rs-drive-abci/src/query/proofs/mod.rs (2)
1-6: Updated imports to support the new Drive proofs serviceThe import statements have been properly updated to use the new
GetProofsRequestandGetProofsResponsetypes from the Drive v0 module, which aligns with the objective of moving proof functionality from DAPI to Drive ABCI.
13-27: Simplified query_proofs function with better error handlingThe refactored
query_proofsfunction now accepts the entire request object instead of destructuring it, and has a cleaner version checking mechanism. This approach reduces code complexity and makes future version additions more straightforward.packages/dapi-grpc/build.rs (5)
50-59: New Drive module configuration added correctlyThe build script has been appropriately extended to support the new Drive proto module, setting up the necessary configuration to generate Rust code from the proto definitions.
76-76: Updated VERSIONED_REQUESTS array sizeThe size of the VERSIONED_REQUESTS array has been reduced by one to account for moving the
GetProofsRequestfrom the Platform service to the new Drive service, which is consistent with the refactoring objective.
123-123: Updated VERSIONED_RESPONSES array sizeThe size of the VERSIONED_RESPONSES array has been reduced by one to account for moving the
GetProofsResponsefrom the Platform service to the new Drive service, which is consistent with the refactoring objective.
221-232: New configure_drive function implemented correctlyThe
configure_drivefunction correctly sets up the necessary attributes for the Drive proto messages, ensuring they have the same serialization/deserialization capabilities and mockability as the rest of the codebase.
347-352: New includes method added to MappingConfigThe
includesmethod has been added to theMappingConfigstruct, allowing for including additional proto paths. This enhances the flexibility of the build system and supports the modular architecture being developed.packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java (1)
3044-3074:✅ Verification successful
Renumbering of method IDs after removal of
getProofsmethodThese changes are part of the renumbering of method IDs following the removal of the
getProofsmethod from the Platform service. The adjustment ensures proper method dispatching after the API refactoring that moved proof retrieval functionality from DAPI to Drive ABCI.
🏁 Script executed:
#!/bin/bash # Verify that the getProofs method has been completely removed from the Java client echo "Checking for any remaining references to getProofs in the Platform gRPC Java client..." grep -n "getProofs" packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java || echo "No references to getProofs found in the Java client" echo "Checking if the getProofs functionality is available in the new Drive service..." grep -n "getProofs" packages/dapi-grpc/protos/drive/v0/drive.proto || echo "Could not find getProofs in the Drive proto"Length of output: 706
Verification complete: Renumbering and removal of
getProofsconfirmed
- All references to
getProofshave been successfully removed from the Platform gRPC Java client.- The
getProofsRPC is properly defined in the Drive proto (line 8), aligning with the refactoring and the subsequent method ID renumbering.packages/rs-drive-abci/src/server.rs (2)
23-25: Good use ofArcfor concurrencyCreating
query_serviceanddrive_internalviaArcensures safe concurrent access to shared resources and is consistent with the existing pattern forplatformusage.
38-45: Separation of services looks well-structuredAdding both
DriveInternalServerandPlatformServerto the gRPC server neatly splits responsibilities for drive operations and platform endpoints. This design makes the architecture more modular and maintainable.packages/dapi-grpc/clients/drive/v0/web/drive_pb_service.d.ts (3)
1-14: No issues foundThese definitions for service requests, responses, and imports appear correct for gRPC-Web usage.
21-48: No issues foundThe type definitions for
ServiceError,Status, and the various gRPC stream interfaces follow standard usage patterns in gRPC-Web.
49-62: Definitions and overloading ofgetProofsare correctProviding multiple overloads for
getProofswith optionalmetadataaligns with typical gRPC-Web patterns, and theUnaryResponsetype is properly referenced.packages/rs-drive-abci/src/query/proofs/v0/mod.rs (2)
6-52: Imports and helper function look consistent• The newly added imports for data contracts, identity transitions, and drive queries are aligned with the new proof-based logic.
• The helpercontract_ids_to_non_historical_path_queryfunction is straightforward and properly sets query limits toNone.
330-352: Helper function for historical document queries is well-defined
create_token_historical_document_queryproperly loads the system data contract and creates aSingleDocumentDriveQuerywith up-to-date platform version. The usage ofkeeps_history()is consistent with the token configuration logic shown in the main match block.packages/dapi/lib/externalApis/drive/fetchProofForStateTransitionFactory.js (3)
11-11: Function signature simplified appropriately.The removal of the
dppparameter aligns with the PR objective of moving proof retrieval logic from DAPI to Drive ABCI. This simplifies the interface and reduces dependencies in the DAPI layer.
14-14: Type change from AbstractStateTransition to Uint8Array is correct.This change reflects the architectural shift where DAPI now passes raw state transition bytes rather than processing the state transition object. This aligns with the goal of moving the state transition parsing and processing logic to Drive ABCI.
19-20: Significant simplification improves architecture.The removal of complex logic for handling different state transition types greatly simplifies this function. Instead of DAPI needing to understand and process the state transition content, it now just passes the raw bytes to Drive ABCI. This change:
- Reduces complexity in the DAPI layer
- Centralizes proof generation logic in Drive ABCI
- Makes adding new state transitions easier (as mentioned in PR objectives)
- Creates a cleaner separation of responsibilities between services
The implementation correctly matches the PR objective of "transmitting only state transition bytes from DAPI to Drive ABCI."
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (1)
92-264: Consider breaking down the token transition handling logicThe transition handling match statement is quite long and complex. Consider extracting the token transition handling (particularly lines 176-262) into a separate function for improved readability and maintainability.
match transition { BatchedTransitionRef::Document(document_transition) => { // ... document transition handling ... } BatchedTransitionRef::Token(token_transition) => { - //todo group actions - let data_contract_id = token_transition.data_contract_id(); - let token_id = token_transition.token_id(); - - let Some(contract_fetch_info) = self.drive.get_contract_with_fetch_info( - data_contract_id.to_buffer(), - false, - None, - platform_version, - )? - else { - return Ok(QueryValidationResult::new_with_error(QueryError::Proof( - ProofError::UnknownContract(format!( - "unknown contract with id {}", - data_contract_id - )), - ))); - }; - - let contract = &contract_fetch_info.contract; - - let token_config = contract.expected_token_configuration( - token_transition.base().token_contract_position(), - )?; - - let keeps_historical_document = token_config.keeps_history(); - - match token_transition { - // ... token transition matching ... - } + self.handle_token_transition(token_transition, owner_id, platform_version)? } } // Add this new method to the Platform impl fn handle_token_transition( &self, token_transition: &TokenTransition, owner_id: Identifier, platform_version: &PlatformVersion, ) -> Result<PathQuery, Error> { //todo group actions let data_contract_id = token_transition.data_contract_id(); let token_id = token_transition.token_id(); let Some(contract_fetch_info) = self.drive.get_contract_with_fetch_info( data_contract_id.to_buffer(), false, None, platform_version, )? else { return Ok(QueryValidationResult::new_with_error(QueryError::Proof( ProofError::UnknownContract(format!( "unknown contract with id {}", data_contract_id )), ))); }; let contract = &contract_fetch_info.contract; let token_config = contract.expected_token_configuration( token_transition.base().token_contract_position(), )?; let keeps_historical_document = token_config.keeps_history(); match token_transition { // ... token transition matching ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/dapi-grpc/clients/drive/v0/nodejs/DrivePromiseClient.js(1 hunks)packages/dapi-grpc/src/lib.rs(1 hunks)packages/dapi/test/integration/dpp/DriveStateRepository.spec.js(0 hunks)packages/rs-drive-abci/src/query/proofs/v0/mod.rs(1 hunks)
💤 Files with no reviewable changes (1)
- packages/dapi/test/integration/dpp/DriveStateRepository.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/dapi-grpc/src/lib.rs
- packages/dapi-grpc/clients/drive/v0/nodejs/DrivePromiseClient.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (5)
packages/rs-drive/src/drive/votes/cleanup/remove_contested_resource_vote_poll_end_date_query_operations/v1/mod.rs (1)
drive(24-113)packages/rs-drive/src/drive/votes/cleanup/remove_contested_resource_vote_poll_info_operations/v0/mod.rs (1)
drive(17-43)packages/rs-drive/src/drive/votes/cleanup/remove_contested_resource_vote_poll_documents_operations/v0/mod.rs (1)
drive(24-88)packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/mod.rs (1)
pro_tx_hash(10-14)packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/v0/mod.rs (1)
pro_tx_hash(6-6)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (wasm-dpp) / Detect immutable structure changes
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- 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 JS packages / Build JS
🔇 Additional comments (5)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (5)
73-89: Proper validation of batch transitionsGood implementation of defensive programming by ensuring that batch state transitions contain exactly one batched transition. This validation improves error handling and makes the expected behavior clear.
54-327: Well-structured state transition handlingThe refactored code now handles various types of state transitions in a clean, structured way. This aligns with the PR objective of streamlining the process of introducing new state transitions by moving proof logic from DAPI to Drive ABCI. The pattern matching approach makes the code more maintainable and extensible.
330-352: Good extraction of token historical document query creationExtracting the
create_token_historical_document_queryfunction improves code reusability and readability, as this functionality is used multiple times in the token transition handling.
6-44: Comprehensive imports for state transition handlingThe updated imports properly cover all the necessary components for handling various state transitions, including data contracts, document types, token transitions, and masternode votes. This demonstrates thorough planning for the refactored implementation.
294-311:✅ Verification successful
Verify completeness of masternode vote transition handling
The implementation currently handles resource votes for masternode vote transitions. Make sure this covers all possible vote types that can be used in the system.
🏁 Script executed:
#!/bin/bash # Find all vote types in the codebase to ensure we're handling them all rg "enum Vote" --type rust -A 10Length of output: 2672
Masternode Vote Transition Handling Verified
Our verification confirms that theVoteenum in the system (as found inpackages/rs-dpp/src/voting/votes/mod.rs) only contains theResourceVotevariant. This means that the current masternode vote transition handling inpackages/rs-drive-abci/src/query/proofs/v0/mod.rsis complete and properly covers the defined vote type. If additional vote types are introduced in the future, please update this section accordingly.
|
@riongull Hello! I've updated the description. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs (3)
129-161: DataContractCreateAction proof verification.
This block correctly verifies the contract from the proof, confirming root hash and contract integrity. It’s a thorough approach. If it grows more complex, consider factoring out repeated logic into helper methods.
205-396: DocumentAction sub-block covers multiple transition types extensively.
This large match branch validates created, replaced, transferred, purchased, deleted, or price-updated documents. While thorough, it may be beneficial to consolidate repeated verification patterns (e.g., root hash checks, optional document checks) into helper functions to improve readability.
541-541: Unaddressed TODO inIdentityCreditWithdrawalAction.
You’ve noted that full document verification is pending. Please clarify if you plan to implement it soon or if it remains out of scope.Would you like me to open a follow-up PR or issue to handle this verification?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/dapi/test/integration/grpcServer/handlers/platform/waitForStateTransitionResultHandlerFactory.spec.js(4 hunks)packages/dapi/test/unit/externalApis/drive/fetchProofForStateTransitionFactory.spec.js(0 hunks)packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs(5 hunks)
💤 Files with no reviewable changes (1)
- packages/dapi/test/unit/externalApis/drive/fetchProofForStateTransitionFactory.spec.js
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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) / Formatting
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (17)
packages/dapi/test/integration/grpcServer/handlers/platform/waitForStateTransitionResultHandlerFactory.spec.js (5)
44-44: Appropriate variable additionGood addition of the
stateTransitionBytesvariable which will be reused multiple times in the test. This improves code readability and reduces redundancy.
68-69: Clean implementation of state transition bytes handlingThese changes align with the PR's goal of moving proof retrieval logic to Drive ABCI. By extracting the state transition bytes first and then deriving the base64 encoding from it, the code becomes more maintainable and better reflects how the actual implementation will work.
187-188: Simplified response constructionThe response handling has been appropriately simplified to directly set the proof and metadata on the
GetProofsResponseobject without wrapping them in a versioned response. This aligns with the refactoring goal of streamlining the proof handling process.
279-279: Updated request constructionThe
GetProofsRequestnow correctly usesstateTransitionBytesdirectly, which is consistent with the PR's objective of having DAPI transmit only state transition bytes to Drive ABCI. This change makes the test more accurate to the new implementation.
1-370: Overall test adaptation looks goodThe changes to this test file correctly adapt to the architectural shift of moving proof retrieval from DAPI to Drive ABCI. The test now accurately reflects the updated flow where raw state transition bytes are passed to Drive, which handles the proof generation internally. This should make introducing new state transitions simpler as mentioned in the PR objectives.
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs (12)
18-18: New import forGetProofsRequestlooks consistent with the refactor.
This import aligns with the updated approach of sending serialized state transition bytes directly to Drive. No concerns here.
22-22: ImportingPlatformSerializableis appropriate for serialization logic.
This import is clearly needed to serialize state transitions prior to proof queries.
104-108: Serializing state transitions for proof queries.
The call toserialize_to_bytes()is straightforward and necessary for sending transitions. Consider verifying large or complex transitions in a separate test if needed.Would you like to confirm handling of potentially large state transitions?
109-111: Creating theGetProofsRequestwith serialized bytes.
This succinct approach matches the refactor’s goal of sending only state transition bytes. Looks good.
113-116: Querying proofs throughquery_proofs.
This appears to integrate neatly with the new Drive gRPC service. No issues at first glance.
118-123: Validating the query result in test code.
Panic is standard in tests to indicate unexpected outcomes. This is acceptable for a test environment.
125-127: Retrieving the proof from the response.
Expecting the proof to be present matches the logic that a valid response must contain it. This is consistent with the new request structure.
162-195: DataContractUpdateAction proof verification.
The code properly checks the updated contract against the proof. This is well-structured for verifying contract changes.
197-203: Panic if batch transition is empty.
This early check ensures at least one transition is present in the batch, which helps avoid silently ignoring empty transitions.
397-476: TokenAction logic verifying token history documents.
This chunk consistently confirms the existence of the expected token history document. The approach aligns with the newly centralized proof querying.
477-479: Panic onBumpIdentityDataContractNonce.
This placeholder looks intentional for the test. Verify if or when you plan to handle this action; otherwise, tests will continue to fail here if it's introduced in production code.Would you like to track an issue for implementing this transition’s verification, or is it intentionally unsupported?
646-646: Direct reference to the contested document resource contract.
Accessingcontested_document_resource_vote_poll.contract.as_ref()is concise and clearly indicates which contract instance is under scrutiny. Implementation looks fine.
| let state_transition: StateTransition = | ||
| StateTransition::deserialize_from_bytes(&state_transition_bytes)?; |
There was a problem hiding this comment.
Won't this give an internal error if we can't deserialize it?
There was a problem hiding this comment.
You right! I will fix it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (1)
157-157: Resolve the TODO for Group Actions.The code still contains a placeholder in the form of a TODO comment (
//todo group actions). This should be addressed to finalize the refactor.
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (1)
64-72: Consider adding error details to the response for better user debugging.While the deserialization error handling is properly returning a QueryError, it doesn't provide specific details about why the deserialization failed, which could be challenging for clients to debug.
match StateTransition::deserialize_from_bytes(&state_transition_bytes) { Ok(state_transition) => state_transition, Err(e) => { return Ok(QueryValidationResult::new_with_error(QueryError::Protocol( - e, + ProtocolError::StateTransitionDeserializationError { + error: format!("Failed to deserialize state transition: {}", e), + source: Some(Box::new(e)), + }, ))) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (3)
packages/dapi-grpc/clients/drive/v0/web/drive_pb.d.ts (2)
GetProofsRequest(7-21)GetProofsResponse(29-48)packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/mod.rs (1)
pro_tx_hash(10-14)packages/rs-dpp/src/state_transition/state_transitions/identity/masternode_vote_transition/accessors/v0/mod.rs (1)
pro_tx_hash(6-6)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (6)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (6)
59-60: Improved error handling for state transition deserialization.The request structure has been simplified from multiple fields to just
state_transition_bytes, aligning with the new API design where proof logic is moved from DAPI to Drive ABCI. This makes it easier to introduce new state transitions as mentioned in the PR objectives.
81-96: Clear validation for batch transitions.The implementation correctly validates that batch state transitions must contain exactly one batched transition. This provides clear error messages and prevents potential ambiguity in how batch proofs are handled.
184-270: Well-structured token transition handling based on history configuration.The implementation elegantly handles different token transition types (Burn, Mint, Transfer, Freeze, Unfreeze, etc.) by checking whether historical records should be kept for each operation and constructing the appropriate query.
274-319: Comprehensive support for different identity operations.The implementation now handles various identity-related state transitions (Create, TopUp, CreditWithdrawal, Update, CreditTransfer) by constructing appropriate path queries for each type. This is a significant improvement that supports the PR's goal of streamlining the process of introducing new state transitions.
338-360: Well-implemented helper function for token historical documents.The
create_token_historical_document_queryfunction provides a clean abstraction for creating historical document queries for token transitions, which reduces code duplication and improves maintainability.
329-332: Clean response construction using the new GetProofsResponse type.The response construction is straightforward and aligns with the new API design, providing both the proof and metadata as expected by clients.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (1)
156-156: Resolve the TODO for Group ActionsThe code still contains a placeholder in the form of a TODO comment (
//todo group actions). This issue was previously flagged in an earlier review.Please either implement the actual action grouping logic or document the planned approach clearly so that the intended behavior is evident for future reference.
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (1)
1-359: Consider further modularization of the query_proofs_v0 functionThe function has grown quite large (270+ lines) with multiple nested match statements for different transition types. Consider breaking it down into smaller functions for each state transition type to improve maintainability.
This could be implemented by creating separate functions like:
fn create_path_query_for_data_contract_transition( &self, transition: &DataContractTransition, platform_version: &PlatformVersion ) -> Result<PathQuery, Error> { // Implementation } fn create_path_query_for_batch_transition( &self, transition: &BatchTransition, platform_version: &PlatformVersion ) -> Result<PathQuery, Error> { // Implementation } // Similar functions for other transition types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (2)
packages/rs-drive/src/drive/votes/cleanup/remove_contested_resource_vote_poll_documents_operations/v0/mod.rs (1)
drive(24-88)packages/dapi-grpc/clients/drive/v0/web/drive_pb.d.ts (2)
GetProofsRequest(7-21)GetProofsResponse(29-48)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (9)
packages/rs-drive-abci/src/query/proofs/v0/mod.rs (9)
6-44: Well organized imports for different state transitionsThe updated import statements are well organized and clearly reflect the refactoring effort to support various state transition types directly in the Drive ABCI layer. The imports now include comprehensive support for data contracts, tokens, and identity operations.
46-52: LGTM: Well-structured utility functionThis is a clean, focused utility function that appropriately handles contract IDs for path query construction. The removal of the limit ensures complete retrieval of relevant data.
55-62: Function signature improved to focus on state transition bytesThe updated function signature now accepts only the state transition bytes, which aligns with the PR's objective of moving proof retrieval from DAPI to Drive ABCI. This simplifies the API by requiring only what's necessary.
81-95: Good validation for batch transitionsThe validation ensures that batch state transitions contain exactly one batched transition, which is important for proper proof generation. The error messages are clear and helpful.
155-269: Comprehensive token transition handlingThe implementation provides thorough handling of various token transition types (Burn, Mint, Transfer, Freeze, Unfreeze, etc.), with appropriate conditional logic based on whether historical documents are kept. The code correctly differentiates between operations that modify balances and those that affect token information.
273-318: Well-structured identity and vote transition handlingThe implementation provides appropriate path queries for each identity operation type (Create, TopUp, CreditWithdrawal, Update, CreditTransfer) and MasternodeVote transitions. The code efficiently handles each case with specific query types.
321-331: Clean proof retrieval and response constructionThe proof retrieval using
grove_get_proved_path_queryand subsequent response construction with metadata is clean and aligns well with the overall architecture.
337-359: Well-designed helper function for token historical document queriesThe
create_token_historical_document_queryfunction provides a clean abstraction for creating path queries for token historical documents, reducing code duplication in the token transition handling section.
63-71:❓ Verification inconclusive
Consider returning internal server error for deserialization failures
The function currently returns a protocol error when deserialization fails. Based on a previous review comment by QuantumExplorer, this might be better handled as an internal server error since it could be unexpected from the client's perspective.
Previous comment from QuantumExplorer:
Won't this give an internal error if we can't deserialize it?
🏁 Script executed:
#!/bin/bash # Search for similar error handling patterns in the codebase rg "deserialize_from_bytes.*return.*QueryValidationResult::new_with_error" --type rust -C 3Length of output: 93
Action: Revisit Error Handling for Deserialization Failures
In
packages/rs-drive-abci/src/query/proofs/v0/mod.rs(lines 63–71), the code currently wraps deserialization errors as protocol errors:return Ok(QueryValidationResult::new_with_error(QueryError::Protocol(e)))However, as raised previously by QuantumExplorer, a failure during deserialization is arguably unexpected from the client’s perspective and might be better represented as an internal server error. This change could more accurately convey that the error originates from processing on the server side rather than from an erroneous client request.
Recommendations:
- Assess the Error Classification: Consider whether deserialization failures should indeed trigger an internal server error. If so, replacing the protocol error with an internal server error variant (e.g., using something like
QueryError::InternalServer(e)if available) would align better with typical error-handling practices.- Ensure Consistency: Double-check similar error-handling cases in the codebase to verify that adopting an internal server error here will maintain consistent behavior across the system.
- Update Code if Needed: Modify the error handling accordingly so that it better reflects the nature of the failure.
|
Approved, good work. |
QuantumExplorer
changed the title
Issue being fixed or feature implemented
To introduce a new state transition a developer needs to implement specific structures and methods in WASM DPP and then update quite ugly functions in DAPI. It requires knowledge of JavaScript.
What was done?
fetchProofForStateTransitionFactoryfunction logic to Drive ABCI'sget_proofquery function.getProofsendpoint to send just state transition bytes from DAPI to Drive ABCI and move it to a new drive internal grpc service.How Has This Been Tested?
With test suite and RS SDK tests
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit