feat: add wallet contract #2314
Conversation
HashEngineering
requested review from
shumkov and
QuantumExplorer
as code owners
WalkthroughThe pull request introduces a new package named 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (18)
packages/wallet-contract/.eslintrc (4)
4-4: Consider keeping the no-plusplus rule enabled.Disabling
no-plusplusmight lead to subtle bugs in certain cases due to automatic semicolon insertion (ASI) in JavaScript. Consider using+=1or-=1instead, which are more explicit and safer alternatives.- "no-plusplus": 0, + "no-plusplus": ["error", { "allowForLoopAfterthoughts": true }]
9-9: Reconsider disabling class-methods-use-this.Completely disabling
class-methods-use-thismight lead to instance methods that should be static. Consider enabling it with exceptions where necessary.- "class-methods-use-this": "off", + "class-methods-use-this": ["error", { + "exceptMethods": [ + // Add specific method names that need to be excluded + ] + }]
15-17: Consider environment configuration for BigInt.Instead of declaring BigInt as a global, consider using the
envconfiguration with a modern ECMAScript version that includes BigInt support.- "globals": { - "BigInt": true - } + "env": { + "es2020": true + }
1-18: Add missing TypeScript-related configurations.Since this is a new contract implementation that will likely handle critical wallet operations, consider adding TypeScript support to catch type-related issues early.
Add these configurations to enhance type safety:
{ "extends": [ "airbnb-base", + "plugin:@typescript-eslint/recommended" ], + "parser": "@typescript-eslint/parser", + "plugins": ["@typescript-eslint"], // ... rest of your config }packages/wallet-contract/src/error.rs (2)
5-14: Enhance error documentation for better maintainability.While the error structure is well-designed, the documentation could be more descriptive:
pub enum Error { - /// Platform expected some specific versions + /// Error returned when a method is called with an unsupported platform version. + /// This typically occurs during feature version validation when processing + /// wallet-related operations. #[error("platform unknown version on {method}, received: {received}")] UnknownVersionMismatch { - /// method + /// Name of the method where version mismatch occurred method: String, - /// the allowed versions for this method + /// List of platform versions that are supported by this method known_versions: Vec<FeatureVersion>, - /// requested core height + /// The platform version that was actually received in the request received: FeatureVersion, },
4-17: Consider adding error variants for wallet-specific operations.Given that this contract will handle encrypted transaction metadata and username request information, consider adding error variants for:
- Encryption/decryption failures
- Username validation errors
- Storage operation failures
- Invalid metadata format
This will help in providing more specific error handling for wallet operations.
packages/wallet-contract/test/bootstrap.js (3)
7-9: Consider adding error handling for WebAssembly loading.The WebAssembly DPP loading is critical for tests. Consider wrapping it in a try-catch block to handle potential loading failures gracefully.
const { default: loadWasmDpp, } = require('@dashevo/wasm-dpp'); + +async function loadWasmDppSafely() { + try { + await loadWasmDpp(); + } catch (error) { + console.error('Failed to load WebAssembly DPP:', error); + throw error; + } +}
17-23: Simplify sinon sandbox initialization.The current sandbox initialization logic can be simplified by always creating a new sandbox.
beforeEach() { - if (!this.sinon) { - this.sinon = sinon.createSandbox(); - } else { - this.sinon.restore(); - } + this.sinon = sinon.createSandbox(); },
30-30: Add a comment explaining global expect usage.Consider documenting why expect is made globally available.
+// Make Chai's expect globally available for all test files global.expect = expect;packages/wallet-contract/src/v1/mod.rs (2)
4-14: Add documentation for the document types module.While the structure is clean, adding documentation comments would help explain:
- The purpose of each constant
- The expected format/constraints of each field
- Any security considerations for handling encrypted metadata
Example documentation:
/// Module containing document type definitions for transaction metadata pub mod document_types { /// Transaction metadata document type and its properties pub mod tx_metadata { /// Name identifier for transaction metadata documents pub const NAME: &str = "tx_metadata"; /// Property keys used in transaction metadata documents pub mod properties { /// Index key for identifying the metadata entry pub const KEY_INDEX: &str = "keyIndex"; /// Index for the encryption key used to secure the metadata pub const ENCRYPTION_KEY_INDEX: &str = "encryptionKeyIndex"; /// The actual encrypted metadata content pub const ENCRYPTED_METADATA: &str = "encryptedMetadata"; } } }
16-19: Consider adding schema validation and improving error handling.While the current implementation works, consider these improvements:
- Add schema validation after loading to ensure it matches expected structure
- Provide more specific error types for different failure scenarios
Example implementation:
use serde_json::Value; use thiserror::Error; #[derive(Error, Debug)] pub enum SchemaError { #[error("Failed to parse schema JSON: {0}")] InvalidJson(#[from] serde_json::Error), #[error("Schema validation failed: {0}")] ValidationError(String), } pub fn load_documents_schemas() -> Result<Value, Error> { let schema: Value = serde_json::from_str(include_str!("../../schema/v1/wallet-contract-documents.json")) .map_err(Error::InvalidSchemaJson)?; // Validate required structure if !schema.get("definitions") .and_then(|d| d.get("tx_metadata")) .and_then(|t| t.get("properties")) .is_some() { return Err(Error::InvalidSchemaJson( serde_json::Error::custom("Missing required schema structure") )); } Ok(schema) }packages/wallet-contract/schema/v1/wallet-contract-documents.json (1)
56-57: Document deletion constraints for identityVerify.While the schema indicates that documents are mutable and can be deleted (
documentsMutableandcanBeDeleted), there's no documentation about:
- Who can delete/modify these documents
- The impact of deletion on dependent systems
- Any cooldown periods between modifications
Consider adding documentation that covers:
- Access control for modifications and deletions
- The impact of changes on the identity verification process
- Any rate limiting or cooldown periods between modifications
packages/wallet-contract/test/unit/walletContract.spec.js (6)
64-64: Consider assigningundefinedinstead of usingdeletefor better performanceUsing the
deleteoperator can impact performance because it alters the object's structure, leading to de-optimization in JavaScript engines. Assigningundefinedto the property is a more performant alternative when you need to unset a property.Apply this diff to modify the code:
- delete rawTxMetadataDocument.keyIndex; + rawTxMetadataDocument.keyIndex = undefined;🧰 Tools
🪛 Biome
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
81-81: Consider assigningundefinedinstead of usingdeletefor better performanceSimilar to line 64, replacing
deletewith an assignment toundefinedcan improve performance by avoiding de-optimization.Apply this diff:
- delete rawTxMetadataDocument.encryptionKeyIndex; + rawTxMetadataDocument.encryptionKeyIndex = undefined;🧰 Tools
🪛 Biome
[error] 81-81: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
98-98: Consider assigningundefinedinstead of usingdeletefor better performanceAgain, for consistency and to enhance performance, consider assigning
undefinedinstead of deleting the property.Apply this diff:
- delete rawTxMetadataDocument.encryptedMetadata; + rawTxMetadataDocument.encryptedMetadata = undefined;🧰 Tools
🪛 Biome
[error] 98-98: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
184-184: Consider assigningundefinedinstead of usingdeletefor better performanceIn the
verifyIdentitytests, replacingdeletewith an assignment toundefinedhelps maintain object shape and performance.Apply this diff:
- delete rawIdentityVerifyDocument.normalizedLabel; + rawIdentityVerifyDocument.normalizedLabel = undefined;🧰 Tools
🪛 Biome
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
231-231: Consider assigningundefinedinstead of usingdeletefor better performanceAs before, assigning
undefinedcan be a better choice than usingdeletefor performance optimization.Apply this diff:
- delete rawIdentityVerifyDocument.normalizedParentDomainName; + rawIdentityVerifyDocument.normalizedParentDomainName = undefined;🧰 Tools
🪛 Biome
[error] 231-231: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
278-278: Consider assigningundefinedinstead of usingdeletefor better performanceContinuing the pattern, consider this change for consistency and performance benefits.
Apply this diff:
- delete rawIdentityVerifyDocument.url; + rawIdentityVerifyDocument.url = undefined;🧰 Tools
🪛 Biome
[error] 278-278: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.toml(1 hunks)config/local_seed.json(1 hunks)packages/wallet-contract/.eslintrc(1 hunks)packages/wallet-contract/.mocharc.yml(1 hunks)packages/wallet-contract/Cargo.toml(1 hunks)packages/wallet-contract/LICENSE(1 hunks)packages/wallet-contract/README.md(1 hunks)packages/wallet-contract/lib/systemIds.js(1 hunks)packages/wallet-contract/package.json(1 hunks)packages/wallet-contract/schema/v1/wallet-contract-documents.json(1 hunks)packages/wallet-contract/src/error.rs(1 hunks)packages/wallet-contract/src/lib.rs(1 hunks)packages/wallet-contract/src/v1/mod.rs(1 hunks)packages/wallet-contract/test/.eslintrc(1 hunks)packages/wallet-contract/test/bootstrap.js(1 hunks)packages/wallet-contract/test/unit/walletContract.spec.js(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- config/local_seed.json
- packages/wallet-contract/.mocharc.yml
- packages/wallet-contract/Cargo.toml
- packages/wallet-contract/LICENSE
- packages/wallet-contract/lib/systemIds.js
- packages/wallet-contract/package.json
- packages/wallet-contract/test/.eslintrc
🧰 Additional context used
🪛 LanguageTool
packages/wallet-contract/README.md
[style] ~28-~28: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ... sh # TODO ... ## Contributing Feel free to dive in! [Open an issue](https://github...
(FEEL_FREE_TO_STYLE_ME)
🪛 Biome
packages/wallet-contract/test/unit/walletContract.spec.js
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 81-81: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 98-98: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 231-231: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 278-278: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (17)
packages/wallet-contract/.eslintrc (3)
1-2: LGTM: Good choice of base configuration.
Extending airbnb-base is a solid choice as it provides a comprehensive set of rules that enforce consistent code style and catch common JavaScript pitfalls.
5-8: LGTM: Consistent line endings.
Enforcing consistent line endings with eol-last is good for version control systems and cross-platform compatibility.
10-13: LGTM: Enforcing curly braces.
Enforcing curly braces for all control statements helps prevent errors and improves code readability.
packages/wallet-contract/src/error.rs (2)
1-3: LGTM! Good choice of error handling approach.
The use of thiserror for deriving error implementations is a good practice, and the explicit import of FeatureVersion shows proper version management.
15-16: Verify the long-term need for JSON schema validation.
The PR objectives mention a planned transition from CBOR to ProtoBuf for storage. Please confirm if JSON schema validation will still be needed after this transition.
Let's verify the usage of JSON schemas in the codebase:
✅ Verification successful
Let me analyze the JSON schema usage further by checking its specific usage in the codebase:
Based on the analysis of the JSON schema usage in the codebase, I can now provide a final response:
JSON schema validation remains necessary even with ProtoBuf transition
The JSON schema validation is used across multiple contracts for document validation, including the wallet contract. This validation occurs at the application level for document structure verification, which is separate from the storage serialization format (CBOR/ProtoBuf). The transition to ProtoBuf for storage does not eliminate the need for JSON schema validation as they serve different purposes:
- ProtoBuf: Used for efficient data serialization in storage
- JSON Schema: Used for document structure validation and ensuring data integrity at the application level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for JSON schema related files and usage
# to understand the scope of JSON schema validation
# Look for JSON schema files
echo "Looking for JSON schema files:"
fd -e json -e jsonschema
# Check for JSON schema related code
echo -e "\nChecking for JSON schema related code:"
rg -l "schema|json" --type rust
Length of output: 81504
Script:
#!/bin/bash
# Check for JSON schema validation usage in the codebase
echo "Checking JSON schema validation usage:"
rg -A 5 "InvalidSchemaJson" --type rust
# Check for JSON schema validation in the wallet contract
echo -e "\nChecking wallet contract JSON schema usage:"
rg -A 10 "schema" packages/wallet-contract/
Length of output: 7199
packages/wallet-contract/test/bootstrap.js (2)
1-6: LGTM: Testing dependencies are well-structured.
The combination of Sinon for mocking and Chai with Dirty-Chai for assertions provides a robust testing foundation.
11-12: LGTM: Chai plugins are properly configured.
The setup correctly integrates both dirty-chai and sinon-chai plugins.
packages/wallet-contract/src/v1/mod.rs (3)
1-2: LGTM! Clean and minimal imports.
The imports are specific and necessary for the functionality provided.
4-14: Verify schema property names.
Let's ensure the constant values match the schema definition.
✅ Verification successful
Property names match the schema definition
The constant values in the code exactly match the property names defined in the schema:
KEY_INDEX: "keyIndex"ENCRYPTION_KEY_INDEX: "encryptionKeyIndex"ENCRYPTED_METADATA: "encryptedMetadata"
These property names are correctly used throughout the codebase, including tests and schema definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the property names in the code match the schema definition
# Expected: Property names in schema should match the constants
# Get the schema content and check for property names
cat packages/wallet-contract/schema/v1/wallet-contract-documents.json | jq '.definitions.tx_metadata.properties | keys'
Length of output: 173
Script:
#!/bin/bash
# Let's first check if the schema file exists and see its content
cat packages/wallet-contract/schema/v1/wallet-contract-documents.json
Length of output: 3649
Script:
#!/bin/bash
# Let's also search for any other schema files that might contain tx_metadata
fd -e json -e yaml -e yml . packages/wallet-contract/schema
Length of output: 127
Script:
#!/bin/bash
# Search for any references to these property names in the codebase
rg -i "keyIndex|encryptionKeyIndex|encryptedMetadata" packages/wallet-contract
Length of output: 2630
1-19: Security considerations for encrypted metadata handling.
Since this code handles encrypted transaction metadata in a wallet context:
- Ensure encryption/decryption operations are performed in a secure context
- Consider adding input validation for metadata size limits
- Implement proper access controls for metadata operations
✅ Verification successful
Let me analyze the encryption and access control related code more specifically for the wallet contract context by examining the implementation details.
Let me check the schema file that defines the validation rules for these fields.
Security review for encrypted metadata handling is satisfied
The implementation has appropriate security measures in place:
-
The schema enforces proper validation:
- Encrypted metadata has size limits (32-4096 bytes)
- Required key indices with minimum value constraints
- Strict schema validation with
additionalProperties: false
-
The metadata structure follows security best practices:
- Uses two-factor key derivation (keyIndex + encryptionKeyIndex)
- Enforces minimum size for encrypted data (32 bytes) ensuring proper encryption
- Documentation indicates AES-CBC-256 encryption
-
Access control is inherently handled through the platform's identity system via
$ownerIdindices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security-sensitive operations in the codebase
# Look for encryption-related code and access controls
# Search for encryption-related code
echo "Checking for encryption-related code:"
rg -l "encrypt|decrypt" --type rust
# Search for access control implementations
echo "Checking for access control implementations:"
rg -l "authorize|permission|access" --type rust
Length of output: 36967
Script:
#!/bin/bash
# Check the wallet contract implementation for encryption and access control
# Look for encryption key usage and metadata handling
# Check wallet contract implementation details
echo "Checking wallet contract implementation:"
rg -A 10 "encryptedMetadata|encryptionKeyIndex|keyIndex" packages/wallet-contract/src/
# Check for any validation or size limit implementations
echo "Checking for validation implementations:"
ast-grep --pattern 'fn validate($$$) { $$$ }' packages/wallet-contract/src/
Length of output: 1183
Script:
#!/bin/bash
# Check the schema file for metadata validation rules
echo "Checking wallet contract schema:"
cat packages/wallet-contract/schema/v1/wallet-contract-documents.json
# Look for any additional validation logic in the codebase
echo "Checking for additional validation logic:"
rg -A 5 "tx_metadata" packages/wallet-contract/src/
Length of output: 4438
Cargo.toml (2)
31-31: LGTM! The wallet-contract package addition follows workspace conventions.
The new package follows the established naming pattern and is correctly placed in the workspace members list.
31-31: Verify the wallet-contract package configuration.
Let's ensure the wallet-contract package is properly configured with necessary dependencies and metadata.
✅ Verification successful
The wallet-contract package is properly configured with consistent dependencies
The wallet-contract package has all the necessary dependencies aligned with other contract packages in the codebase:
- Has the required metadata fields (name, description, version, edition, etc.)
- Contains the same core dependencies as other contracts:
- thiserror = "1.0.64"
- platform-version (local)
- serde_json = "1.0"
- platform-value (local)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the wallet-contract package setup and dependencies
# Test 1: Check if the package's Cargo.toml exists and contains required fields
echo "Checking wallet-contract package configuration..."
cat packages/wallet-contract/Cargo.toml 2>/dev/null || echo "Error: wallet-contract Cargo.toml not found"
# Test 2: Verify package dependencies align with other contract packages
echo -e "\nComparing dependencies with other contract packages..."
for contract in packages/*-contract/Cargo.toml; do
if [ -f "$contract" ]; then
echo "=== $contract ==="
grep "^\[dependencies\]" -A 10 "$contract" 2>/dev/null
fi
done
Length of output: 3538
packages/wallet-contract/src/lib.rs (3)
1-7: LGTM! Clean module organization and imports.
The module structure follows good practices with separate error handling and versioned implementations.
9-17:
Security concern: Verify the OWNER_ID initialization.
The OWNER_ID_BYTES is initialized to all zeros, which could be a security risk if used in production. This might allow unauthorized access if not properly updated.
Additionally, please add documentation explaining:
- The significance and generation method of
ID_BYTES - Whether
OWNER_ID_BYTESshould be configured during deployment
1-37: Verify implementation completeness against PR objectives.
The current implementation provides the basic contract structure but appears to be missing key functionality mentioned in the PR objectives:
- Storage structures for encrypted transaction metadata
- Username request information handling
- Implementation of the transition from CBOR to ProtoBuf
Please confirm if these features will be added in subsequent PRs or if they should be included here.
packages/wallet-contract/schema/v1/wallet-contract-documents.json (2)
109-116:
Review uniqueUsernameIndex for potential collisions.
The current uniqueUsernameIndex only includes normalizedLabel, which might not prevent duplicate usernames across different parent domains. Consider including both normalizedLabel and normalizedParentDomainName in the unique index.
Let's check if there are any existing username-related tests:
#!/bin/bash
# Search for username-related tests
rg -i "username|duplicate|collision" --type jsApply this diff to strengthen uniqueness constraints:
{
"name": "uniqueUsernameIndex",
"properties": [
{
"normalizedLabel": "asc"
- }
+ },
+ {
+ "normalizedParentDomainName": "asc"
+ }
]
}38-45: Document the encryption format in encryptedMetadata.
The description mentions AES-CBC-256 but doesn't specify:
- The format of the encrypted data (CBOR/ProtoBuf)
- The IV/nonce handling
- The padding scheme used
Let's check if there's any encryption-related documentation:
Consider adding detailed encryption documentation that covers:
- The exact encryption scheme (AES-CBC-256)
- The format of the plaintext before encryption (CBOR/ProtoBuf)
- How the IV/nonce is handled
- The padding scheme used
| exports.mochaHooks = { | ||
| beforeAll: loadWasmDpp, | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make beforeAll hook async to handle WebAssembly loading.
The beforeAll hook should be async to properly handle the WebAssembly loading promise.
exports.mochaHooks = {
- beforeAll: loadWasmDpp,
+ beforeAll: async function() {
+ await loadWasmDpp();
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exports.mochaHooks = { | |
| beforeAll: loadWasmDpp, | |
| exports.mochaHooks = { | |
| beforeAll: async function() { | |
| await loadWasmDpp(); | |
| }, | |
There was a problem hiding this comment.
@shumkov, thank you for pointing that out. Since loadWasmDpp is already async, assigning it directly to beforeAll is sufficient.
(^.^)?
✏️ Learnings added
Learnt from: shumkov
PR: dashpay/platform#2314
File: packages/wallet-contract/test/bootstrap.js:14-16
Timestamp: 2024-11-06T07:27:01.446Z
Learning: In `packages/wallet-contract/test/bootstrap.js`, for Mocha tests in Node.js, async functions like `loadWasmDpp` can be assigned directly to `beforeAll` without wrapping them in another async function.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
packages/wallet-contract/src/lib.rs
Outdated
| pub fn load_definitions(platform_version: &PlatformVersion) -> Result<Option<Value>, Error> { | ||
| match platform_version.system_data_contracts.withdrawals { | ||
| 1 => Ok(None), | ||
| version => Err(Error::UnknownVersionMismatch { | ||
| method: "dpns_contract::load_definitions".to_string(), | ||
| known_versions: vec![1], | ||
| received: version, | ||
| }), | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message in load_definitions.
The error message references "dpns_contract" instead of "wallet_contract", which appears to be a copy-paste error.
- method: "dpns_contract::load_definitions".to_string(),
+ method: "wallet_contract::load_definitions".to_string(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn load_definitions(platform_version: &PlatformVersion) -> Result<Option<Value>, Error> { | |
| match platform_version.system_data_contracts.withdrawals { | |
| 1 => Ok(None), | |
| version => Err(Error::UnknownVersionMismatch { | |
| method: "dpns_contract::load_definitions".to_string(), | |
| known_versions: vec![1], | |
| received: version, | |
| }), | |
| } | |
| } | |
| pub fn load_definitions(platform_version: &PlatformVersion) -> Result<Option<Value>, Error> { | |
| match platform_version.system_data_contracts.withdrawals { | |
| 1 => Ok(None), | |
| version => Err(Error::UnknownVersionMismatch { | |
| method: "wallet_contract::load_definitions".to_string(), | |
| known_versions: vec![1], | |
| received: version, | |
| }), | |
| } | |
| } |
packages/wallet-contract/src/lib.rs
Outdated
| pub fn load_definitions(platform_version: &PlatformVersion) -> Result<Option<Value>, Error> { | ||
| match platform_version.system_data_contracts.withdrawals { | ||
| 1 => Ok(None), | ||
| version => Err(Error::UnknownVersionMismatch { | ||
| method: "dpns_contract::load_definitions".to_string(), | ||
| known_versions: vec![1], | ||
| received: version, | ||
| }), | ||
| } | ||
| } | ||
| pub fn load_documents_schemas(platform_version: &PlatformVersion) -> Result<Value, Error> { | ||
| match platform_version.system_data_contracts.withdrawals { | ||
| 1 => v1::load_documents_schemas(), | ||
| version => Err(Error::UnknownVersionMismatch { | ||
| method: "dpns_contract::load_documents_schemas".to_string(), | ||
| known_versions: vec![1], | ||
| received: version, | ||
| }), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and consider version handling.
Please add documentation explaining:
- The purpose and functionality of these contract functions
- The relationship with the withdrawals version
- Expected return values and their significance
Also, consider:
- Adding constants for supported versions instead of magic numbers
- Adding logging for version mismatches to aid debugging
+/// Version 1 is currently the only supported version
+const SUPPORTED_VERSION: u32 = 1;
+
+/// Loads contract definitions based on the platform version.
+/// Returns None for version 1 as no definitions are needed.
pub fn load_definitions(platform_version: &PlatformVersion) -> Result<Option<Value>, Error> {
match platform_version.system_data_contracts.withdrawals {
- 1 => Ok(None),
+ SUPPORTED_VERSION => Ok(None),
version => Err(Error::UnknownVersionMismatch {
method: "wallet_contract::load_definitions".to_string(),
- known_versions: vec![1],
+ known_versions: vec![SUPPORTED_VERSION],
received: version,
}),
}
}Committable suggestion skipped: line range outside the PR's diff.
| "keyIndex": { | ||
| "type": "integer", | ||
| "minimum": 0, | ||
| "description": "The index of the owners identity public key used to derive the encryption key.", | ||
| "position": 0 | ||
| }, |
There was a problem hiding this comment.
Add maximum bounds for key indices.
The keyIndex and encryptionKeyIndex properties should have maximum values to prevent potential resource exhaustion attacks.
Apply this diff:
"keyIndex": {
"type": "integer",
"minimum": 0,
+ "maximum": 2147483647,
"description": "The index of the owners identity public key used to derive the encryption key.",
"position": 0
},
"encryptionKeyIndex": {
"type": "integer",
"minimum": 0,
+ "maximum": 2147483647,
"description": "The secondary index used to derive the encryption key that is used to encrypt and decrypt encryptedData.",
"position": 1
},Also applies to: 32-37
| "url": { | ||
| "position": 2, | ||
| "type": "string", | ||
| "description": "The identity verification URL to be stored.", | ||
| "maxLength": 128, | ||
| "pattern": "^https?://.*", | ||
| "format": "uri" | ||
| } |
There was a problem hiding this comment.
Enforce HTTPS-only URLs for security.
The current URL pattern allows both HTTP and HTTPS protocols. For security reasons, especially when dealing with identity verification, only HTTPS URLs should be allowed.
Apply this diff:
"url": {
"position": 2,
"type": "string",
"description": "The identity verification URL to be stored.",
"maxLength": 128,
- "pattern": "^https?://.*",
+ "pattern": "^https://.*",
"format": "uri"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "url": { | |
| "position": 2, | |
| "type": "string", | |
| "description": "The identity verification URL to be stored.", | |
| "maxLength": 128, | |
| "pattern": "^https?://.*", | |
| "format": "uri" | |
| } | |
| "url": { | |
| "position": 2, | |
| "type": "string", | |
| "description": "The identity verification URL to be stored.", | |
| "maxLength": 128, | |
| "pattern": "^https://.*", | |
| "format": "uri" | |
| } |
| it('should be defined', async () => { | ||
| delete rawIdentityVerifyDocument.normalizedLabel; | ||
|
|
||
| const document = dpp.document.create(dataContract, identityId, 'domain', rawIdentityVerifyDocument); |
There was a problem hiding this comment.
Correct the document type in dpp.document.create
In the verifyIdentity tests, the document is being created with the type 'domain' instead of 'verifyIdentity'. This could lead to incorrect validation against the wrong document schema.
Apply this diff to fix the document type:
- const document = dpp.document.create(dataContract, identityId, 'domain', rawIdentityVerifyDocument);
+ const document = dpp.document.create(dataContract, identityId, 'verifyIdentity', rawIdentityVerifyDocument);Please make this change in all instances within the verifyIdentity tests where 'domain' is used instead of 'verifyIdentity'.
Also applies to: 201-201, 217-217, 233-233, 248-248, 263-263, 280-280, 295-295, 310-310, 324-324
config/local_seed.json
Outdated
| @@ -0,0 +1,5 @@ | |||
| { | |||
There was a problem hiding this comment.
oh, that is a mistake
| @@ -0,0 +1,29 @@ | |||
| { | |||
| "name": "@dashevo/wallet-contract", | |||
| "version": "1.4.1", | |||
packages/wallet-contract/Cargo.toml
Outdated
| [package] | ||
| name = "wallet-contract" | ||
| description = "Wallet data contract schema and tools" | ||
| version = "1.4.1" |
| ], | ||
| "additionalProperties": false | ||
| }, | ||
| "identityVerify": { |
There was a problem hiding this comment.
What is it for? How is it related to the wallet?
There was a problem hiding this comment.
In the DashPay app, when a user requests a contested username, we allow the user to specify a social media post to "prove" an identity.
If two people wanted bob each can specify a url to a social media post that says something like "I am the real bob and my identity id is Abs19....47k."
Masternode owners using DashPay can view this see this URL in the username request details page of the Username Voting screen.
This contact might not be the best place for it. If it isn't, I am not sure if it should be in a new contract or the dashpay contract.
| exports.mochaHooks = { | ||
| beforeAll: loadWasmDpp, | ||
|
|
|
Thank you. I started this PR when 1.4.1 the latest version, so some of these things are out of date. I will need to rebase against 1.6-dev and then make required changes. |
HashEngineering
force-pushed
the
feat/add-wallet-contract
branch
from
69f98e7
to
2e16647
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
packages/wallet-contract/README.md (2)
6-6: Enhance package descriptionThe current description "JSON Contracts for Dash Wallet apps" is quite brief. Consider expanding it to include the key functionalities mentioned in the PR objectives, specifically about transaction metadata storage and username request information.
-JSON Contracts for Dash Wallet apps +JSON Contracts for Dash Wallet apps that provide encrypted transaction metadata storage and username request information management to assist masternode voters in user verification.
29-29: Consider more formal contributing guidanceWhile the current contributing section is welcoming, it could be more professional and informative.
-Feel free to dive in! [Open an issue](https://github.com/dashpay/platform/issues/new/choose) or submit PRs. +We welcome contributions! Please [open an issue](https://github.com/dashpay/platform/issues/new/choose) for bug reports or feature requests. For code contributions, please submit a pull request following our contribution guidelines.packages/wallet-contract/src/lib.rs (1)
9-17: Add documentation for contract identifiers.Please add documentation comments explaining:
- The purpose and significance of these identifiers
- How they are used in the contract
- Why OWNER_ID_BYTES is set to zeros (if intentional)
packages/wallet-contract/schema/v1/wallet-contract-documents.json (2)
38-44: Consider adding encryption algorithm version.Since the metadata is using AES-CBC-256 encryption, it would be beneficial to add a version field to support future encryption algorithm changes without breaking existing data.
"encryptedMetadata": { "type": "array", "byteArray": true, "minItems": 32, "maxItems": 4096, "description": "encrypted metadata using AES-CBC-256", + "version": { + "type": "integer", + "minimum": 1, + "maximum": 1, + "description": "Version of the encryption algorithm (1 = AES-CBC-256)", + "position": 3 + }, "position": 2 }
55-85: Consider moving identity verification to a separate contract.Based on the PR comments discussion, this functionality might be better suited in a separate identity verification contract or the DashPay contract since:
- It's specifically for username dispute resolution in DashPay
- It's not directly related to wallet functionality
- It would improve separation of concerns
This would make the wallet contract more focused and maintainable.
packages/wallet-contract/test/unit/walletContract.spec.js (3)
12-26: Consider enhancing the error assertion helper function.The
expectJsonSchemaErrorhelper could be more reusable and provide better error messages by:
- Adding an optional parameter for expected keyword
- Adding an optional parameter for expected instancePath
- Including the actual error message in the assertion output
-const expectJsonSchemaError = (validationResult, errorCount = 1) => { +const expectJsonSchemaError = (validationResult, { + errorCount = 1, + expectedKeyword = null, + expectedPath = null +} = {}) => { const errors = validationResult.getErrors(); expect(errors) .to .have .length(errorCount); const error = validationResult.getErrors()[0]; expect(error) .to .be .instanceof(JsonSchemaError); + + if (expectedKeyword) { + expect(error.keyword, `Expected error keyword "${expectedKeyword}" but got "${error.keyword}"`) + .to.equal(expectedKeyword); + } + + if (expectedPath) { + expect(error.instancePath, `Expected error path "${expectedPath}" but got "${error.instancePath}"`) + .to.equal(expectedPath); + } return error; };
40-40: Consider adding a named constant for the contract version.The
BigInt(1)value appears to be a contract version number. Consider extracting it to a named constant for better code clarity and maintainability.+const CONTRACT_VERSION = BigInt(1); -dataContract = dpp.dataContract.create(identityId, BigInt(1), walletContractDocumentsSchema); +dataContract = dpp.dataContract.create(identityId, CONTRACT_VERSION, walletContractDocumentsSchema);
64-64: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. Consider using undefined assignment instead.-delete rawTxMetadataDocument.keyIndex; +rawTxMetadataDocument.keyIndex = undefined;Apply similar changes to other instances where
deleteis used.Also applies to: 81-81, 98-98
🧰 Tools
🪛 Biome
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.toml(1 hunks)packages/wallet-contract/.eslintrc(1 hunks)packages/wallet-contract/.mocharc.yml(1 hunks)packages/wallet-contract/Cargo.toml(1 hunks)packages/wallet-contract/LICENSE(1 hunks)packages/wallet-contract/README.md(1 hunks)packages/wallet-contract/lib/systemIds.js(1 hunks)packages/wallet-contract/package.json(1 hunks)packages/wallet-contract/schema/v1/wallet-contract-documents.json(1 hunks)packages/wallet-contract/src/error.rs(1 hunks)packages/wallet-contract/src/lib.rs(1 hunks)packages/wallet-contract/src/v1/mod.rs(1 hunks)packages/wallet-contract/test/.eslintrc(1 hunks)packages/wallet-contract/test/bootstrap.js(1 hunks)packages/wallet-contract/test/unit/walletContract.spec.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/wallet-contract/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (10)
- Cargo.toml
- packages/wallet-contract/.eslintrc
- packages/wallet-contract/.mocharc.yml
- packages/wallet-contract/LICENSE
- packages/wallet-contract/lib/systemIds.js
- packages/wallet-contract/package.json
- packages/wallet-contract/src/error.rs
- packages/wallet-contract/src/v1/mod.rs
- packages/wallet-contract/test/.eslintrc
- packages/wallet-contract/test/bootstrap.js
🧰 Additional context used
🪛 LanguageTool
packages/wallet-contract/README.md
[style] ~28-~28: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ... sh # TODO ... ## Contributing Feel free to dive in! [Open an issue](https://github...
(FEEL_FREE_TO_STYLE_ME)
🪛 Biome
packages/wallet-contract/test/unit/walletContract.spec.js
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 81-81: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 98-98: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 231-231: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 278-278: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (6)
packages/wallet-contract/src/lib.rs (2)
1-8: LGTM! Clean module structure and imports.
The module organization and imports are well-structured and appropriate for the contract's functionality.
14-14:
Security concern: OWNER_ID_BYTES initialized to zeros.
Using an all-zero identifier for ownership could pose security risks if used in production. Please verify if this is intentional or a placeholder.
packages/wallet-contract/schema/v1/wallet-contract-documents.json (3)
38-45: Verify encrypted metadata size constraints.
The current size constraints for encryptedMetadata (32-4096 bytes) need verification:
- Minimum size (32 bytes) seems appropriate for AES-CBC-256 encryption
- Maximum size (4096 bytes) might need adjustment based on:
- Expected payload sizes
- Network bandwidth considerations
- Storage costs
86-116: LGTM: Well-designed indices for efficient querying.
The indices are well-structured for:
- Querying by owner ID
- Ensuring username uniqueness
- Supporting hierarchical domain queries
60-67: Add validation for homograph attack mitigation.
While the schema includes patterns to prevent homograph attacks, consider adding server-side validation to ensure:
- The replacement of "o", "i", and "l" with "0" and "1" is correctly applied
- The normalization process is consistent across the platform
✅ Verification successful
Let's search for any normalization, homograph prevention, or label transformation implementations in the codebase to verify if there's consistent handling of these cases.
Let's search for the implementation of label normalization and validation to verify if homograph attack mitigation is properly implemented.
Let's search for the actual implementation of homograph attack mitigation and its tests to verify if it's properly implemented and validated.
Based on the search results, I can now provide a definitive response about the homograph attack mitigation implementation:
Server-side validation for homograph attack mitigation is properly implemented.
The codebase shows comprehensive protection through:
-
Schema validation in
normalizedLabelandnormalizedParentDomainNamefields with pattern^[a-hj-km-np-z0-9][a-hj-km-np-z0-9-]{0,61}[a-hj-km-np-z0-9]$that explicitly excludes homograph-prone characters. -
Server-side implementation in
convert_to_homograph_safe_chars()function that:- Converts input to lowercase
- Replaces 'o' with '0'
- Replaces 'l' and 'i' with '1'
-
Data trigger validation in
dpns/v0/mod.rsthat enforces the normalized form matches the conversion rules by comparingnormalized_labelwithconvert_to_homograph_safe_chars(label). -
Consistent implementation across platform components including domain registration, queries, and validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other username normalization implementations
rg -A 5 "normaliz(e|ing) (username|label)"
Length of output: 44
Script:
#!/bin/bash
# Search for any normalization related functions or implementations
rg -A 5 "normaliz" --type ts --type js
# Search for homograph attack prevention patterns
rg -A 5 "homograph"
# Search for label transformation or validation patterns
rg -A 5 "label.*transform|transform.*label"
# Look for any string validation or sanitization patterns
ast-grep --pattern 'function $_ ($$$) {
$$$
.replace(/[oil]/g, $$$)
$$$
}'
Length of output: 197038
Script:
#!/bin/bash
# Search for label normalization or transformation implementations
ast-grep --pattern 'function $_ ($$$) {
$$$
let $_ = $_.toLowerCase()
$$$
}'
# Look for any string conversion functions
ast-grep --pattern 'function convertToHomographSafeChars($$$) {
$$$
}'
# Search for any label validation functions
rg -A 5 "validateLabel|validateNormalizedLabel"
# Search for any test cases related to label normalization
rg -A 5 "test.*label.*normalization|test.*normalized.*label"
Length of output: 102503
Script:
#!/bin/bash
# Search for the implementation of homograph safe conversion
rg -A 10 "convert_to_homograph_safe_chars"
# Look for any tests related to homograph attack prevention
rg -A 5 "test.*homograph|homograph.*test"
# Search for data trigger validation implementations
rg -A 10 "data_triggers.*dpns"
Length of output: 162228
packages/wallet-contract/test/unit/walletContract.spec.js (1)
186-186: Fix incorrect document type in verifyIdentity tests.
This issue was previously identified. The document type should be 'verifyIdentity' instead of 'domain'.
Also applies to: 201-201, 216-216, 233-233, 248-248, 263-263, 280-280, 295-295, 310-310, 324-324
packages/wallet-contract/README.md
Outdated
| # Wallet Contract | ||
|
|
||
| [](https://github.com/dashpay/platform/actions/workflows/release.yml) | ||
| [](https://npmjs.org/package/@dashevo/dpns-contract) |
There was a problem hiding this comment.
Fix incorrect NPM badge package reference
The NPM badge is currently referencing @dashevo/dpns-contract instead of @dashevo/wallet-contract.
-[](https://npmjs.org/package/@dashevo/dpns-contract)
+[](https://npmjs.org/package/@dashevo/wallet-contract)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [](https://npmjs.org/package/@dashevo/dpns-contract) | |
| [](https://npmjs.org/package/@dashevo/wallet-contract) |
| ```sh | ||
| npm install @dashevo/wallet-contract | ||
| ``` |
There was a problem hiding this comment.
💡 Codebase verification
Package installation command needs to be updated
The package @dashevo/wallet-contract is not published on npm registry yet, despite being defined in package.json. The installation command in the README won't work for users. This needs to be either:
- Published to npm registry before the documentation is updated, or
- Updated with alternative installation instructions (e.g., installing from GitHub repository)
🔗 Analysis chain
Verify package installation command
The installation command references a package that might not be published yet.
Let me check the package.json to understand if this is a local package that needs to be published.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the package exists on npm registry
# Test: Check if the package exists and get its details
npm view @dashevo/wallet-contract --json || echo "Package not found"
Length of output: 1022
Script:
#!/bin/bash
# Check package.json in the wallet-contract directory
cat packages/wallet-contract/package.json
Length of output: 779
packages/wallet-contract/src/lib.rs
Outdated
| pub fn load_documents_schemas(platform_version: &PlatformVersion) -> Result<Value, Error> { | ||
| match platform_version.system_data_contracts.withdrawals { | ||
| 1 => v1::load_documents_schemas(), | ||
| version => Err(Error::UnknownVersionMismatch { | ||
| method: "dpns_contract::load_documents_schemas".to_string(), | ||
| known_versions: vec![1], | ||
| received: version, | ||
| }), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for schema loading.
The function should handle potential errors from v1::load_documents_schemas(). Consider adding error context to help with debugging.
pub fn load_documents_schemas(platform_version: &PlatformVersion) -> Result<Value, Error> {
match platform_version.system_data_contracts.withdrawals {
- 1 => v1::load_documents_schemas(),
+ 1 => v1::load_documents_schemas().map_err(|e| Error::InvalidSchemaJson {
+ method: "wallet_contract::load_documents_schemas".to_string(),
+ error: e.to_string(),
+ }),
version => Err(Error::UnknownVersionMismatch {
method: "wallet_contract::load_documents_schemas".to_string(),
known_versions: vec![1],Committable suggestion skipped: line range outside the PR's diff.
| it('should be not shorter than 32 bytes', async () => { | ||
| rawTxMetadataDocument.encryptedMetadata = crypto.randomBytes(31); | ||
|
|
||
| const document = dpp.document.create(dataContract, identityId, 'txMetadata', rawTxMetadataDocument); | ||
| const validationResult = document.validate(dpp.protocolVersion); | ||
| const error = expectJsonSchemaError(validationResult); | ||
|
|
||
| expect(error.keyword) | ||
| .to | ||
| .equal('maxItems'); | ||
| expect(error.instancePath) | ||
| .to | ||
| .equal('/encryptedMetadata'); | ||
| }); |
There was a problem hiding this comment.
Fix incorrect error assertions in byte length tests.
The tests for byte length constraints are using maxItems keyword, which is incorrect for byte array length validation. The schema likely uses minBytes and maxBytes or similar keywords.
expect(error.keyword)
.to
- .equal('maxItems');
+ .equal('minBytes');
expect(error.instancePath)
.to
.equal('/encryptedMetadata');Apply similar fix to the maxBytes test.
Also applies to: 127-140
| it('should be less than 128 chars', async () => { | ||
| rawIdentityVerifyDocument.normalizedParentDomainName = 'a'.repeat(129); | ||
|
|
||
| const document = dpp.document.create(dataContract, identityId, 'domain', rawIdentityVerifyDocument); | ||
| const validationResult = document.validate(dpp.protocolVersion); | ||
| const error = expectJsonSchemaError(validationResult, 2); | ||
|
|
||
| expect(error.keyword) | ||
| .to | ||
| .equal('pattern'); | ||
| expect(error.instancePath) | ||
| .to | ||
| .equal('/url'); | ||
| }); |
There was a problem hiding this comment.
Fix incorrect property in url length test.
The test for url length is incorrectly modifying normalizedParentDomainName instead of url.
-rawIdentityVerifyDocument.normalizedParentDomainName = 'a'.repeat(129);
+rawIdentityVerifyDocument.url = 'a'.repeat(129);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should be less than 128 chars', async () => { | |
| rawIdentityVerifyDocument.normalizedParentDomainName = 'a'.repeat(129); | |
| const document = dpp.document.create(dataContract, identityId, 'domain', rawIdentityVerifyDocument); | |
| const validationResult = document.validate(dpp.protocolVersion); | |
| const error = expectJsonSchemaError(validationResult, 2); | |
| expect(error.keyword) | |
| .to | |
| .equal('pattern'); | |
| expect(error.instancePath) | |
| .to | |
| .equal('/url'); | |
| }); | |
| it('should be less than 128 chars', async () => { | |
| rawIdentityVerifyDocument.url = 'a'.repeat(129); | |
| const document = dpp.document.create(dataContract, identityId, 'domain', rawIdentityVerifyDocument); | |
| const validationResult = document.validate(dpp.protocolVersion); | |
| const error = expectJsonSchemaError(validationResult, 2); | |
| expect(error.keyword) | |
| .to | |
| .equal('pattern'); | |
| expect(error.instancePath) | |
| .to | |
| .equal('/url'); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
packages/wallet-contract/Cargo.toml (1)
9-13: Consider standardizing dependency specifications.A few suggestions to improve dependency management:
- Consider using workspace-level version management for
thiserrorandserde_json- The
serde_jsondependency might need specific features enabled for contract serialization- Consider adding typical testing dependencies (e.g.,
rstest,mockall) for contract testingExample improvement:
[dependencies] -thiserror = "1.0.64" +thiserror.workspace = true platform-version = { path = "../rs-platform-version" } -serde_json = { version = "1.0" } +serde_json = { workspace = true, features = ["preserve_order"] } platform-value = { path = "../rs-platform-value" } +[dev-dependencies] +rstest.workspace = true +mockall.workspace = truepackages/wallet-contract/README.md (1)
27-29: Enhance contributing guidelinesConsider expanding the contributing section with more specific guidance:
## Contributing -Feel free to dive in! [Open an issue](https://github.com/dashpay/platform/issues/new/choose) or submit PRs. +We welcome contributions! Here's how you can help: + +1. Check existing issues or create a new one to discuss your ideas +2. Fork the repository and create a feature branch +3. Write tests for any new functionality +4. Ensure your code follows our coding standards +5. Submit a pull request with a clear description of your changes + +For more details, see our [contribution guidelines](../CONTRIBUTING.md).🧰 Tools
🪛 LanguageTool
[style] ~28-~28: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ...sh # TODO ...## Contributing Feel free to dive in! [Open an issue](https://github...(FEEL_FREE_TO_STYLE_ME)
packages/wallet-contract/src/lib.rs (2)
9-12: Document the significance of ID_BYTES.Please add documentation explaining:
- The purpose and significance of this byte sequence
- How it was generated
- Any implications for changing it
18-27: Extract common version handling logic.Both functions share similar version handling logic. Consider extracting this into a common helper function to improve maintainability and reduce code duplication.
+/// Checks if the given version is supported +fn check_version(version: u32, method: &str) -> Result<(), Error> { + const SUPPORTED_VERSION: u32 = 1; + if version != SUPPORTED_VERSION { + return Err(Error::UnknownVersionMismatch { + method: method.to_string(), + known_versions: vec![SUPPORTED_VERSION], + received: version, + }); + } + Ok(()) +} pub fn load_definitions(platform_version: &PlatformVersion) -> Result<Option<Value>, Error> { - match platform_version.system_data_contracts.withdrawals { - 1 => Ok(None), - version => Err(Error::UnknownVersionMismatch { - method: "wallet_contract::load_definitions".to_string(), - known_versions: vec![1], - received: version, - }), - } + check_version(platform_version.system_data_contracts.withdrawals, + "wallet_contract::load_definitions")?; + Ok(None) } pub fn load_documents_schemas(platform_version: &PlatformVersion) -> Result<Value, Error> { - match platform_version.system_data_contracts.withdrawals { - 1 => v1::load_documents_schemas(), - version => Err(Error::UnknownVersionMismatch { - method: "wallet_contract::load_documents_schemas".to_string(), - known_versions: vec![1], - received: version, - }), - } + check_version(platform_version.system_data_contracts.withdrawals, + "wallet_contract::load_documents_schemas")?; + v1::load_documents_schemas() }Also applies to: 28-37
packages/wallet-contract/test/unit/walletContract.spec.js (4)
28-48: Consider adding more comprehensive contract validation.While the basic contract creation test is good, consider adding tests for:
- Schema validation
- Contract version validation
- Invalid identityId scenarios
64-64: Consider using undefined assignment instead of delete operator.For better performance, consider replacing delete operations with undefined assignments:
-delete rawTxMetadataDocument.keyIndex; +rawTxMetadataDocument.keyIndex = undefined;Apply similar changes to other delete operations in the txMetadata tests.
Also applies to: 81-81, 98-98
🧰 Tools
🪛 Biome
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
184-184: Replace delete operations in verifyIdentity tests.For better performance, consider replacing delete operations with undefined assignments:
-delete rawIdentityVerifyDocument.normalizedLabel; +rawIdentityVerifyDocument.normalizedLabel = undefined;Apply similar changes to other delete operations in the verifyIdentity tests.
Also applies to: 231-231, 278-278
🧰 Tools
🪛 Biome
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
198-211: Enhance pattern validation tests.The pattern validation tests could be more specific:
- Add test cases with specific invalid characters
- Test edge cases (e.g., spaces at start/end)
- Document the expected pattern in test descriptions
Example enhancement:
it('should not allow special characters except hyphen and underscore', async () => { rawIdentityVerifyDocument.normalizedLabel = 'invalid@label'; // ... rest of the test });Also applies to: 260-273, 307-320
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/wallet-contract/Cargo.toml(1 hunks)packages/wallet-contract/README.md(1 hunks)packages/wallet-contract/package.json(1 hunks)packages/wallet-contract/src/lib.rs(1 hunks)packages/wallet-contract/test/unit/walletContract.spec.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wallet-contract/package.json
🧰 Additional context used
🪛 LanguageTool
packages/wallet-contract/README.md
[style] ~28-~28: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ... sh # TODO ... ## Contributing Feel free to dive in! [Open an issue](https://github...
(FEEL_FREE_TO_STYLE_ME)
🪛 Biome
packages/wallet-contract/test/unit/walletContract.spec.js
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 81-81: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 98-98: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 184-184: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 231-231: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 278-278: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (6)
packages/wallet-contract/Cargo.toml (1)
1-8: LGTM! Package metadata is well-structured.
The package metadata follows Rust conventions and aligns with the project's versioning (1.5.0).
Let's verify the version alignment with other packages:
packages/wallet-contract/src/lib.rs (2)
1-8: LGTM! Well-structured module organization.
The module declarations and imports are clean, following Rust best practices with specific imports and clear module organization.
14-14:
Verify the zero initialization of OWNER_ID_BYTES.
An all-zero identifier in a production system seems unusual and could potentially cause security issues. Please verify if this is intentional or if it should be initialized with a specific value.
packages/wallet-contract/test/unit/walletContract.spec.js (3)
1-27: LGTM! Well-structured test setup.
The test setup and helper functions are well-organized. The expectJsonSchemaError helper function is particularly well-designed for reuse across test cases.
127-140: Fix incorrect assertion in maxBytes test.
The test for maximum byte length is using the wrong assertion keyword.
186-186: Fix incorrect document type in verifyIdentity tests.
The document type should be 'verifyIdentity' instead of 'domain'.
Also applies to: 201-201, 216-216, 233-233, 248-248, 263-263, 280-280, 295-295, 310-310, 324-324
| ## Usage | ||
|
|
||
| ```sh | ||
| # TODO ... | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace TODO with comprehensive documentation
Based on the contract's purposes, please replace the TODO with detailed documentation covering:
-## Usage
-
-```sh
-# TODO ...
-```
+## Usage
+
+### Transaction Metadata Storage
+
+```javascript
+// Example: Storing encrypted transaction metadata
+const metadata = {
+ txId: "your-tx-id",
+ encryptedData: "encrypted-payload"
+};
+
+await walletContract.storeTransactionMetadata(metadata);
+
+// Example: Retrieving transaction metadata
+const storedMetadata = await walletContract.getTransactionMetadata(txId);
+```
+
+### Username Request Management
+
+```javascript
+// Example: Storing username request information
+const requestInfo = {
+ username: "alice",
+ normalizedLabel: "alice",
+ parentDomain: "dash",
+ socialMediaUrl: "https://twitter.com/alice/status/123"
+};
+
+await walletContract.storeUsernameRequest(requestInfo);
+
+// Example: Retrieving username requests for voting
+const requests = await walletContract.getUsernameRequests();
+```
+
+### Error Handling
+
+```javascript
+try {
+ await walletContract.storeTransactionMetadata(metadata);
+} catch (error) {
+ if (error.code === 'INVALID_METADATA') {
+ // Handle validation errors
+ }
+ // Handle other errors
+}
<!-- This is an auto-generated comment by CodeRabbit -->
| [](https://github.com/dashpay/platform/actions/workflows/release.yml) | ||
| [](https://npmjs.org/package/@dashevo/wallet-contract) | ||
|
|
||
| JSON Contracts for Dash Wallet apps |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the package description
The current description is too brief. Consider expanding it to better reflect the contract's key functionalities:
-JSON Contracts for Dash Wallet apps
+JSON Contracts for Dash Wallet apps, providing secure storage for encrypted transaction metadata and username request information to support masternode voting processes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JSON Contracts for Dash Wallet apps | |
| JSON Contracts for Dash Wallet apps, providing secure storage for encrypted transaction metadata and username request information to support masternode voting processes. |
Issue being fixed or feature implemented
Add a Wallet system contract:
What was done?
Add Wallet system contract. This is called "wallet" because it stores wallet-level
Preliminary documentation on the contract:
Transaction Metadata: https://docs.google.com/document/d/1aLDZmum_MOWF_QVkFbkAVR-m3Wx-FqINiVhCRYBMZXw/edit?tab=t.0
Identity Verify - This document stores a normalizedLabel and normalizedParentDomainName and a url to a social media post or some kind of post that helps prove the online identity of a platform identity. It supports one index to allow matching an identity with a particular username (normalizedLabel, normalizedParentDomainName).
This documentation will need to be converted to a DIP.
How Has This Been Tested?
This was not tested. My local systems would not run the
yarn setuporyarn startsuccessfully to execute the tests, perhaps due to configuration issues.Breaking Changes
There should be no breaking changes.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
wallet-contractpackage for managing wallet data, including a JSON schema for documents.txMetadataandidentityVerifyto enhance document structure.Documentation
Bug Fixes