1661 en 2 #1747
1661 en 2 #1747
Conversation
# Conflicts: # packages/core/src/vcdm/hash/Keccak256.ts
WalkthroughThis pull request introduces modifications to several files in the VeChain SDK core package, focusing on documentation updates, error handling, and method signature refinements. The changes primarily affect the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
Documentation and Community
|
Test CoverageSummary
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/examples/accounts/keystore.ts (1)
8-8: Consider adding async/sync documentation note.While the private key generation is now synchronous, it might be helpful to add a comment clarifying that subsequent keystore operations (
encrypt/decrypt) remain asynchronous. This helps developers understand the mixed sync/async nature of the operations.const privateKey = Secp256k1.generatePrivateKey(); +// Note: While key generation is synchronous, keystore operations below are asynchronousdocs/examples/transactions/blockref-expiration.ts (1)
40-40: LGTM! Consider grouping related operations.The synchronous key generation is correctly implemented. For better readability, consider grouping the key generation with the signing operation since they're closely related.
-// 3 - Create private key - -const privateKey = Secp256k1.generatePrivateKey(); - -// 4 - Sign transaction +// 3 - Sign transaction with a new private key +const privateKey = Secp256k1.generatePrivateKey(); const signedTransaction = Transaction.of(body).sign(privateKey);packages/network/tests/provider/helpers/provider-internal-wallets/base-wallet/provider-internal-base-wallet.unit.test.ts (1)
172-173: Consider using a more descriptive variable name.The variable name
delegatorPrivateKeycould be more descriptive to indicate its test purpose, such astestDelegatorPrivateKeyormockDelegatorPrivateKey.- delegatorPrivateKey: Hex.of(Secp256k1.generatePrivateKey()) + delegatorPrivateKey: Hex.of(Secp256k1.generatePrivateKey()) // Mock private key for testing
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/sdk-vite-integration/package.json(1 hunks)docs/accounts.md(1 hunks)docs/certificates.md(1 hunks)docs/cryptography.md(1 hunks)docs/examples/accounts/keystore.ts(1 hunks)docs/examples/certificates/sign_verify.ts(1 hunks)docs/examples/cryptography/secp256k1.ts(1 hunks)docs/examples/transactions/blockref-expiration.ts(1 hunks)docs/examples/transactions/multiple-clauses.ts(1 hunks)docs/examples/transactions/sign-decode.ts(1 hunks)docs/examples/transactions/tx-dependency.ts(1 hunks)docs/transactions.md(4 hunks)packages/core/src/secp256k1/Secp256k1.ts(2 hunks)packages/core/tests/keystore/keystore.unit.test.ts(6 hunks)packages/core/tests/secp256k1/Secp256k1.unit.test.ts(1 hunks)packages/core/tests/secp256k1/fixture.ts(0 hunks)packages/network/tests/provider/helpers/provider-internal-wallets/base-wallet/provider-internal-base-wallet.unit.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/tests/secp256k1/fixture.ts
✅ Files skipped from review due to trivial changes (1)
- apps/sdk-vite-integration/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/tests/keystore/keystore.unit.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
🔇 Additional comments (13)
docs/examples/certificates/sign_verify.ts (1)
7-7: LGTM! Clean update to synchronous private key generation.The change aligns with the broader refactoring of
Secp256k1.generatePrivateKey()to be synchronous, while maintaining the correct flow of the certificate signing example.docs/examples/transactions/sign-decode.ts (1)
43-43: LGTM! Consistent with transaction flow.The synchronous private key generation aligns well with the transaction signing and encoding flow, which are also synchronous operations.
docs/examples/transactions/multiple-clauses.ts (1)
49-49: LGTM! Correctly updated to use synchronous private key generation.The change aligns with the updated
Secp256k1.generatePrivateKey()API.docs/examples/cryptography/secp256k1.ts (1)
8-8: LGTM! Example updated to reflect the new synchronous API.The example correctly demonstrates the usage of
Secp256k1.generatePrivateKey()in its synchronous form.docs/examples/transactions/tx-dependency.ts (1)
60-60: LGTM! Correctly updated transaction dependency example.The example has been properly updated to use the synchronous
generatePrivateKey()method while maintaining the correct transaction dependency logic.packages/network/tests/provider/helpers/provider-internal-wallets/base-wallet/provider-internal-base-wallet.unit.test.ts (1)
172-173: LGTM! Test updated to use synchronous private key generation.The test has been properly updated to use the synchronous
generatePrivateKey()method.packages/core/src/secp256k1/Secp256k1.ts (2)
97-115: LGTM! Clean implementation of synchronous private key generation.The changes correctly simplify the method by making it synchronous while maintaining proper error handling and clear error messages.
180-191: LGTM! Good improvement to the API.Adding a default value of 32 for
bytesLengthis a sensible choice as it matches the common use case for generating private keys while maintaining flexibility for other use cases.docs/certificates.md (1)
50-50: LGTM! Documentation correctly updated.The example code has been properly updated to reflect the synchronous nature of
generatePrivateKey.docs/cryptography.md (1)
54-54: LGTM! Documentation correctly updated.The example code has been properly updated to reflect the synchronous nature of
generatePrivateKey.packages/core/tests/secp256k1/Secp256k1.unit.test.ts (2)
10-38: LGTM! Well-organized test constants.The reorganization of test constants into HASHES, KEYS, and SIGNATURES improves code readability and maintainability.
79-101: LGTM! Comprehensive test coverage for private key generation.The tests properly verify both successful generation and error handling cases, including mocking hardware crypto support failure.
docs/transactions.md (1)
50-50: LGTM! Method signatures updated consistently across examples.All instances of
Secp256k1.generatePrivateKey()have been correctly updated from async to sync calls across the transaction examples.Let's verify that all examples in the transactions documentation have been updated:
Also applies to: 102-102, 217-217, 280-280
✅ Verification successful
Verified: Documentation examples consistently use sync
generatePrivateKey()All instances in the transactions documentation and related example files have been correctly updated to use the synchronous version of
Secp256k1.generatePrivateKey(). While there is one async usage in the Cloudflare integration SDK, it's outside the scope of these documentation changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining async calls to generatePrivateKey in transaction examples rg -l "await.*Secp256k1\.generatePrivateKey\(\)" "docs/examples/transactions/"Length of output: 78
Script:
#!/bin/bash # First, find all transaction-related markdown and code files echo "=== Transaction-related files ===" fd -e md -e js -e ts "transaction" echo -e "\n=== Checking for remaining async calls ===" rg "await.*Secp256k1\.generatePrivateKey\(\)" echo -e "\n=== Checking sync usage pattern ===" rg "Secp256k1\.generatePrivateKey\(\)" -A 1Length of output: 9774
| @@ -130,7 +130,7 @@ Through the use of mnemonics and keystore, VeChainSDK ensures secure and user-fr | |||
| ```typescript { name=keystore, category=example } | |||
| // 1 - Create private key using Secp256k1 | |||
|
|
|||
| const privateKey = await Secp256k1.generatePrivateKey(); | |||
| const privateKey = Secp256k1.generatePrivateKey(); | |||
There was a problem hiding this comment.
💡 Codebase verification
Architecture diagram needs updating to reflect sync signature
All code examples correctly use the synchronous generatePrivateKey(), but the architecture diagram in docs/diagrams/architecture/secp256k1.md still shows it as async. Specific location:
docs/diagrams/architecture/secp256k1.md: UpdatePromise~Uint8Array~ async generatePrivateKey()to reflect the new sync signature
🔗 Analysis chain
LGTM! Method signature updated correctly.
The change from async to sync call for Secp256k1.generatePrivateKey() is consistent with the method signature refinement.
Let's verify the consistency of this change across all examples:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining async calls to generatePrivateKey
rg -l "await.*Secp256k1\.generatePrivateKey\(\)" docs/
Length of output: 56
Script:
#!/bin/bash
# Search for any mention of generatePrivateKey with context
rg -C 2 "generatePrivateKey" docs/
Length of output: 4785
Description
en_2 recommendation implemented.
The
Addressatpackages/core/src/vcdm/Address.tsremoves the uselessisCompressedparameter in theofPrivateKeymethod.Fixes # EN_2
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
yarn test:soloyarn test:unityarn test:examplesTest Configuration:
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
Refactor
ofPrivateKeymethod in Address class.generatePrivateKeymethod to be synchronous in Secp256k1 class.Tests