-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sdk): asset lock quorum and core locked height verification #2030
base: v1.7-dev
Are you sure you want to change the base?
Conversation
9c21078
to
b3f5f17
Compare
a328081
to
f57e455
Compare
|
||
pub fn new_devnet() -> Self { | ||
QuorumParams { | ||
// FIXME: local devnet uses regtest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs discussion how to handle that.
Ok(()) | ||
} | ||
} | ||
AssetLockProof::Instant(v) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not gonna check if we can verify signature in Drive, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have any hint how to do that client-side, I would be happy to include this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuantumExplorer can you give me some formula to add it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what we discussed, we have no better solution than what's already implemented.
WalkthroughThis pull request introduces several modifications across multiple files, primarily focusing on enhancing the request and response handling within the SDK. Key changes include the expansion of versioned requests and responses in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
packages/rs-sdk/src/platform/transition/put_identity.rs (2)
Line range hint
21-21
: Consider consumingself
input_to_platform
to prevent misuse.The TODO comment suggests that
put_to_platform
might need to consumeself
since the identity is no longer valid after the operation (e.g., identity ID is changed). Consumingself
can prevent accidental use of an invalid identity object.Would you like assistance in refactoring the method to consume
self
or opening a GitHub issue to track this task?
Line range hint
41-58
: Add verification ofasset_lock_proof
before broadcasting.Before broadcasting the state transition, it's important to verify the
asset_lock_proof
to ensure its validity. This can be done by callingasset_lock_proof.verify(sdk).await?
prior to proceeding.Apply this diff to add the verification step:
async fn put_to_platform( &self, sdk: &Sdk, asset_lock_proof: AssetLockProof, asset_lock_proof_private_key: &PrivateKey, signer: &S, settings: Option<PutSettings>, ) -> Result<StateTransition, Error> { + // Verify the asset lock proof + asset_lock_proof.verify(sdk).await?; let (state_transition, _) = self.broadcast_request_for_new_identity( asset_lock_proof, asset_lock_proof_private_key, signer, sdk.version(), )?; // response is empty for a broadcast, result comes from the stream wait for state transition result state_transition.broadcast(sdk, settings).await?; Ok(state_transition) }packages/rs-sdk/tests/fetch/mod.rs (1)
Line range hint
1-1
: Consider addressing the TODO comment in a separate PRThe comment about renaming the test package should be tracked and addressed separately from the current asset lock implementation.
Would you like me to create a GitHub issue to track this TODO?
packages/rs-sdk/src/networks.rs (3)
32-32
: Fix typo in documentationThere's a typo in the Mock variant documentation: "feaults" should be "defaults".
- /// Mock implementation; in practice, feaults to Devnet config for Mock mode. Errors when used in non-mock mode. + /// Mock implementation; in practice, defaults to Devnet config for Mock mode. Errors when used in non-mock mode.
88-90
: Consider documenting Mock implementation detailsThe Mock configuration currently defaults to devnet settings. This implementation detail should be documented to explain why this is appropriate and any limitations.
pub fn new_mock() -> Self { + // Mock configuration uses devnet settings as they are suitable for testing Self::new_devnet() }
64-66
: Consider future extensibility of QuorumParamsThe
QuorumParams
struct currently only containsinstant_lock_quorum_type
. Consider documenting if more parameters will be added in the future, as suggested by the comprehensive network configurations in the comments above.packages/rs-sdk/tests/fetch/asset_lock.rs (4)
17-34
: Consider improving error handling in test helper function.The use of
expect()
in helper functions can make test failures harder to debug. Consider either:
- Propagating the errors with proper context using
?
operator- Using more descriptive expect messages that include the actual values
async fn current_platform_state(sdk: &dash_sdk::Sdk) -> (u32, Vec<u8>) { let req: GetEpochsInfoRequest = GetEpochsInfoRequestV0 { ascending: false, count: 1, prove: true, start_epoch: None, } .into(); let resp: GetEpochsInfoResponse = req .execute(sdk, Default::default()) .await - .expect("get epoch info") + .expect("failed to execute GetEpochsInfoRequest") .into_inner(); - let core_height = resp.metadata().expect("metadata").core_chain_locked_height; - let quorum_hash = resp.proof().expect("proof").quorum_hash.clone(); + let core_height = resp.metadata() + .expect("response metadata missing") + .core_chain_locked_height; + let quorum_hash = resp.proof() + .expect("response proof missing") + .quorum_hash + .clone(); (core_height, quorum_hash) }
56-59
: Document the purpose of hardcoded transaction data.The hardcoded transaction hex string lacks documentation explaining its purpose and structure. This makes it harder to maintain or modify the test in the future.
Add a comment explaining:
- What this transaction represents
- Why this specific transaction was chosen
- The key properties that make it suitable for testing
61-98
: Improve test case documentation and constants.The test cases could be more maintainable and self-documenting:
- Add descriptions for each test case
- Extract magic numbers into named constants
+ const FUTURE_HEIGHT_OFFSET: u32 = 100; + struct TestCase { asset_lock_proof: AssetLockProof, - // expect err that can be retried + /// Whether this test case should result in a retryable error expect_err: bool, + /// Description of what this test case verifies + description: String, } let test_cases = vec![ TestCase { asset_lock_proof: AssetLockProof::Chain(ChainAssetLockProof::new( core_chain_locked_height, out_point, )), expect_err: false, + description: "Valid chain asset lock at current height".to_string(), }, // ... similar changes for other test cases TestCase { asset_lock_proof: AssetLockProof::Chain(ChainAssetLockProof::new( - core_chain_locked_height + 100, + core_chain_locked_height + FUTURE_HEIGHT_OFFSET, out_point, )), expect_err: true, + description: "Invalid chain asset lock with future height".to_string(), },
100-111
: Enhance test failure messages.The current assertion message could be more helpful in identifying which test case failed and why.
for (i, tc) in test_cases.into_iter().enumerate() { let result = tc.asset_lock_proof.verify(&sdk).await; assert_eq!( result.is_err(), tc.expect_err, - "tc {} expeced err = {}, got err = {}: {:?}", + "Test case {} failed: {}\nExpected error: {}\nGot: {}\nError details: {:?}", i, + tc.description, tc.expect_err, result.is_err(), result );packages/rs-sdk/tests/vectors/test_asset_lock_proof/quorum_pubkey-100-4ce7fd81273c2b394c0f32367374fc5b09ba912e017aacb366d2171e9ca6f9d5.json (1)
1-1
: Add documentation for the test vectorSince this is a test vector file, it would be helpful to include metadata about what this value represents and how it's used in testing. Consider structuring the JSON to include:
- Description of the test case
- Expected results
- Any relevant parameters
Example structure:
{ "description": "Test vector for quorum public key verification", "quorumPublicKey": "8aa46461c5a7e1b5da330050d97b3dc928445c3908c2b0f9d3b1b84fd4a7a2ecdd2da5e7480690b0f0f5e10ae51555a7", "expectedResult": true, "testParameters": { "quorumIndex": 100 } }🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: String values must be double quoted.
(parse)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
packages/dapi-grpc/build.rs
(2 hunks)packages/rs-dapi-client/src/executor.rs
(2 hunks)packages/rs-dpp/src/core_types/validator_set/v0/mod.rs
(1 hunks)packages/rs-sdk/src/error.rs
(2 hunks)packages/rs-sdk/src/lib.rs
(1 hunks)packages/rs-sdk/src/networks.rs
(1 hunks)packages/rs-sdk/src/platform/transition.rs
(1 hunks)packages/rs-sdk/src/platform/transition/asset_lock.rs
(1 hunks)packages/rs-sdk/src/platform/transition/put_identity.rs
(1 hunks)packages/rs-sdk/src/platform/types/epoch.rs
(1 hunks)packages/rs-sdk/src/sdk.rs
(12 hunks)packages/rs-sdk/tests/fetch/asset_lock.rs
(1 hunks)packages/rs-sdk/tests/fetch/config.rs
(2 hunks)packages/rs-sdk/tests/fetch/mod.rs
(1 hunks)packages/rs-sdk/tests/vectors/test_asset_lock_proof/quorum_pubkey-100-4ce7fd81273c2b394c0f32367374fc5b09ba912e017aacb366d2171e9ca6f9d5.json
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-sdk/src/platform/types/epoch.rs
🧰 Additional context used
🪛 Biome (1.9.4)
packages/rs-sdk/tests/vectors/test_asset_lock_proof/quorum_pubkey-100-4ce7fd81273c2b394c0f32367374fc5b09ba912e017aacb366d2171e9ca6f9d5.json
[error] 1-1: String values must be double quoted.
(parse)
🔇 Additional comments (15)
packages/rs-sdk/src/platform/transition/asset_lock.rs (1)
1-82
: AssetLockProof verification logic is correctly implemented.
The AssetLockProofVerifier
trait and its implementation accurately verify both Chain and Instant asset lock proofs. Error handling is comprehensive and appropriately addresses different failure scenarios.
packages/rs-sdk/src/sdk.rs (1)
99-100
: Integration of network_type
enhances configurability.
The addition of the network_type
field to Sdk
and SdkBuilder
, along with related methods, successfully improves the SDK's configurability. The with_network_type
method and the validations in the build
method ensure that the network type is properly set, preventing misconfiguration.
Also applies to: 153-153, 531-534, 729-730, 778-778, 805-809, 895-902, 991-996, 1006-1006, 1075-1075
packages/rs-sdk/src/platform/transition.rs (1)
2-2
: Module asset_lock
added appropriately.
The new asset_lock
module is correctly declared and integrates seamlessly with existing modules in the platform::transition
package.
packages/rs-sdk/tests/fetch/mod.rs (1)
8-8
: LGTM! Module declaration aligns with PR objectives
The addition of the asset_lock
module is consistent with the PR's goal of implementing asset lock verification functionality.
packages/rs-sdk/src/lib.rs (1)
68-68
: LGTM! Clean module addition
The networks
module is properly declared as public and follows the existing module organization pattern.
packages/rs-sdk/src/networks.rs (3)
1-25
: Well-documented network configurations
The comprehensive documentation with references to chainparams.cpp provides excellent context for the network configurations.
31-42
: LGTM! Clear network type enumeration
The NetworkType
enum provides a clean abstraction for different network configurations with appropriate documentation.
83-84
: Address FIXME comment about devnet configuration
The FIXME comment indicates that local devnet uses regtest, which needs to be properly documented and potentially discussed.
Let's verify if this is documented elsewhere:
Would you like me to:
- Create a GitHub issue to track this FIXME?
- Help document the devnet/regtest relationship?
✅ Verification successful
Let me gather more context about the network configuration to better understand the relationship between devnet and regtest.
Let me check if there are any configuration files or tests that might provide more context about the devnet and regtest relationship.
Let me check for any test configurations or documentation about the QuorumType::LlmqTest usage.
FIXME comment is accurate and can be resolved
Based on the evidence found in rs-drive-abci/src/config.rs
, the FIXME comment is correct. The local development environment (devnet) indeed uses regtest network configuration with QuorumType::LlmqTest
. This is explicitly shown in the default_local()
configuration where the network is set to Network::Regtest
and uses QuorumType::LlmqTest
for instant locks.
- The comment can be updated to be more descriptive:
// Local devnet uses regtest network configuration with LlmqTest quorum type
instant_lock_quorum_type: QuorumType::LlmqTest,
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation about devnet using regtest
rg -i "devnet.*regtest|regtest.*devnet" --type md
Length of output: 315
Script:
#!/bin/bash
# Search for network configuration related code and comments
rg -i "devnet|regtest" packages/rs-sdk/src/networks.rs -A 5 -B 5
Length of output: 2108
Script:
#!/bin/bash
# Search for devnet configuration in test files and configuration files
rg -i "llmqtest.*devnet|devnet.*llmqtest"
Length of output: 43
Script:
#!/bin/bash
# Search for LlmqTest configuration and usage
rg "LlmqTest" -A 5 -B 5
Length of output: 8560
packages/rs-sdk/src/error.rs (2)
57-75
: LGTM! Well-structured error variants.
The new error variants are well-documented and appropriately designed:
QuorumNotFound
includes all necessary context for debuggingCoreLockedHeightNotYetAvailable
clearly explains the issue with height parameters
150-156
: LGTM! Appropriate retry behavior.
The updated can_retry
implementation correctly includes the new error variants as retryable conditions.
packages/rs-dapi-client/src/executor.rs (1)
125-139
: LGTM! Clean trait implementation.
The VersionedGrpcResponse
implementation for ExecutionResponse
correctly delegates to the inner response while maintaining the execution context.
packages/dapi-grpc/build.rs (2)
Line range hint 50-83
: LGTM! The VERSIONED_REQUESTS array is properly updated.
The array has been expanded to include new request types while maintaining alphabetical order, which aids in maintenance and readability.
90-122
: LGTM! The VERSIONED_RESPONSES array is properly updated.
The array has been expanded with new response types while maintaining alphabetical order. The comment about GetEvonodesProposedEpochBlocksResponse
being used for 2 requests provides helpful context.
packages/rs-sdk/tests/fetch/config.rs (1)
6-6
: LGTM! Network type configuration is properly added.
The addition of .with_network_type(dash_sdk::networks::NetworkType::Devnet)
ensures that tests are executed against the correct network environment.
Also applies to: 185-192
packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (1)
303-303
: LGTM! Simplified pro_tx_hash assignment.
Removed unnecessary cloning of pro_tx_hash
in the test function, which is a good optimization.
Issue being fixed or feature implemented
We want to be able to check if the platform can process an asset lock.
What was done?
Added AssetLockProofVerifier trait with verify() method, implemented on AssetLockProof.
How Has This Been Tested?
Added test.
Note: Requires #2061 to pass the tests.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests