fix: return correct signature #510
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
Warning Tool Failures:Tool Failure Count:📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk-ts/src/core/accounts/PrivateKey.ts(2 hunks)packages/wallets/wallet-private-key/src/strategy/strategy.ts(1 hunks)
🔇 Additional comments (2)
packages/wallets/wallet-private-key/src/strategy/strategy.ts (1)
Line range hint 223-234: LGTM: Proper hex conversion of Ethereum chain ID.
The implementation correctly converts the chain ID to hexadecimal format, which is the standard format for Ethereum chain IDs, while maintaining proper error handling.
packages/sdk-ts/src/core/accounts/PrivateKey.ts (1)
360-362: LGTM: Formatting improves readability.
The multi-line formatting of the decode call enhances code readability without changing functionality.
| const bufferUTF = Buffer.from(toUtf8(data), 'utf-8') | ||
| const signature = pk.signHashed(bufferUTF) | ||
|
|
||
| return `0x${Buffer.from(signature).toString('base64')}` | ||
| return signature | ||
|
|
||
| // return `0x${Buffer.from(signature).toString('base64')}` |
There was a problem hiding this comment.
💡 Codebase verification
Add await for async signHashed call and restore base64 encoding for compatibility
The code changes need to be reverted as they introduce breaking changes:
- The
signHashedmethod is used asynchronously in other implementations (e.g.wallet-ts/src/strategies/wallet-strategy/strategies/PrivateKey.ts) with base64 encoding - The current implementation removes both the await and base64 encoding, breaking compatibility
Required changes:
const bufferUTF = Buffer.from(toUtf8(data), 'utf-8')
- const signature = pk.signHashed(bufferUTF)
+ const signature = await pk.signHashed(bufferUTF)
- return signature
+ return `0x${Buffer.from(signature).toString('base64')}`
- // return `0x${Buffer.from(signature).toString('base64')}`🔗 Analysis chain
Add await for async signHashed call and verify signature format change.
The code has two potential issues:
- Missing
awaitfor the asyncsignHashedcall - Removal of base64 encoding might be a breaking change for consumers expecting the previous format
Add the missing await:
- const signature = pk.signHashed(bufferUTF)
+ const signature = await pk.signHashed(bufferUTF)Let's verify the impact of removing base64 encoding:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for consumers of signArbitrary that might expect base64 format
rg -l "signArbitrary.*\.toString\('base64'\)" || \
rg -l "signArbitrary.*\.split(',')[1]" || \
rg -l "signArbitrary.*Buffer\.from.*'base64'"
Length of output: 154
Script:
#!/bin/bash
# Let's check for any usage of signArbitrary method and its consumers
rg "signArbitrary" -A 5 -B 5
# Also check the interface definition to understand the expected return type
ast-grep --pattern 'interface $_ {
$$$
signArbitrary($_): $_
$$$
}'
Length of output: 36663
Script:
#!/bin/bash
# Let's check the implementation of signHashed to understand its return type
ast-grep --pattern 'signHashed($_) {
$$$
}'
# Also check for any direct consumers of the signature value
rg "signature\s*=.*signHashed" -A 5 -B 5
Length of output: 2148
billyjacoby
force-pushed
the
fix/pk-sign-arbitraty
branch
from
32aefe8
to
63a4d18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk-ts/src/core/accounts/PrivateKey.ts(2 hunks)packages/wallets/wallet-private-key/src/strategy/strategy.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wallets/wallet-private-key/src/strategy/strategy.ts
🔇 Additional comments (2)
packages/sdk-ts/src/core/accounts/PrivateKey.ts (2)
360-362: LGTM!
The formatting changes improve readability while maintaining the same functionality.
171-176: Verify migration plan for breaking changes
The signHashed method has undergone breaking changes that affect dependent packages:
- Changed from synchronous to asynchronous
- Return type changed from
Uint8ArraytoPromise<string>
Let's verify the impact:
Consider:
- Adding a migration guide in the changelog
- Bumping the major version
- Creating separate issues to track updates in dependent packages
✅ Verification successful
Let me analyze the changes in the signHashed method based on the git history:
- The method changed from synchronous to asynchronous
- The return type changed from
Uint8ArraytoPromise<string> - The implementation changed from using
wallet.signingKey.signtowallet.signMessage
Let me verify the impact on dependent packages:
Based on the search results, I can now provide a final response about the breaking changes:
Breaking changes in signHashed method require updates in dependent packages
The method has two dependent packages that need updates:
packages/wallets/wallet-private-key/src/strategy/strategy.ts- already using await with the new signaturepackages/wallet-ts/src/strategies/wallet-strategy/strategies/PrivateKey.ts- already using await with the new signature
Since both dependent packages have already been updated to handle the async signature and return type changes, no further action is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all direct usages of signHashed method
echo "=== Direct usages of signHashed ==="
rg "signHashed\(" --type ts
# Find potential type declarations that might need updates
echo -e "\n=== Type declarations referencing PrivateKey ==="
rg "PrivateKey" --type ts --glob "**/*.d.ts"
Length of output: 653
Script:
#!/bin/bash
# Check git history for the specific file to understand the changes
echo "=== Git history of PrivateKey.ts ==="
git log -p packages/sdk-ts/src/core/accounts/PrivateKey.ts
# Look for package.json to check current version
echo -e "\n=== Package version ==="
cat packages/sdk-ts/package.json | grep version
# Check for CHANGELOG.md
echo -e "\n=== Changelog entries ==="
if [ -f packages/sdk-ts/CHANGELOG.md ]; then
cat packages/sdk-ts/CHANGELOG.md
fi
Length of output: 238029
Script:
#!/bin/bash
# Find all usages of signHashed in dependent packages
echo "=== Usages of signHashed in dependent packages ==="
rg "signHashed" --type ts -A 5 -B 5
# Check if there are any test files using signHashed
echo -e "\n=== Test files using signHashed ==="
rg "signHashed" --type ts --glob "**/test/**/*.ts" --glob "**/tests/**/*.ts" --glob "**/*.test.ts" --glob "**/*.spec.ts"
Length of output: 9162
| async signHashed(messageHashedBytes: Buffer): Promise<string> { | ||
| const { wallet } = this | ||
|
|
||
| const signature = wallet.signingKey.sign(messageHashedBytes) | ||
| const splitSignature = BytesUtils.splitSignature(signature) | ||
| const signature = await wallet.signMessage(messageHashedBytes) | ||
|
|
||
| return BytesUtils.arrayify( | ||
| BytesUtils.concat([splitSignature.r, splitSignature.s]), | ||
| ) | ||
| return signature |
There was a problem hiding this comment.
Update JSDoc and add error handling for breaking changes
The method has undergone significant changes that need to be properly documented and handled:
- JSDoc is incorrect - return type is now
Promise<string>notUint8Array - Missing error handling for
wallet.signMessage - Breaking change should be documented
Apply these changes:
/**
* Sign the given message using the wallet's _signingKey function.
* @param {string} messageHashedBytes: the message that will be signed, a Buffer made of bytes
- * @returns {Uint8Array} a signature of this private key over the given message
+ * @returns {Promise<string>} a promise that resolves to the signature string
+ * @throws {Error} if signing fails
+ * @breaking-change Method is now async and returns Promise<string> instead of Uint8Array
*/
async signHashed(messageHashedBytes: Buffer): Promise<string> {
const { wallet } = this
- const signature = await wallet.signMessage(messageHashedBytes)
- return signature
+ try {
+ return await wallet.signMessage(messageHashedBytes)
+ } catch (error) {
+ throw new Error(`Failed to sign message: ${error.message}`)
+ }
}📝 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.
| async signHashed(messageHashedBytes: Buffer): Promise<string> { | |
| const { wallet } = this | |
| const signature = wallet.signingKey.sign(messageHashedBytes) | |
| const splitSignature = BytesUtils.splitSignature(signature) | |
| const signature = await wallet.signMessage(messageHashedBytes) | |
| return BytesUtils.arrayify( | |
| BytesUtils.concat([splitSignature.r, splitSignature.s]), | |
| ) | |
| return signature | |
| /** | |
| * Sign the given message using the wallet's _signingKey function. | |
| * @param {string} messageHashedBytes: the message that will be signed, a Buffer made of bytes | |
| * @returns {Promise<string>} a promise that resolves to the signature string | |
| * @throws {Error} if signing fails | |
| * @breaking-change Method is now async and returns Promise<string> instead of Uint8Array | |
| */ | |
| async signHashed(messageHashedBytes: Buffer): Promise<string> { | |
| const { wallet } = this | |
| try { | |
| return await wallet.signMessage(messageHashedBytes) | |
| } catch (error) { | |
| throw new Error(`Failed to sign message: ${error.message}`) | |
| } | |
| } |
billyjacoby
force-pushed
the
fix/pk-sign-arbitraty
branch
from
d51f272
to
3709a9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/sdk-ts/src/core/accounts/PrivateKey.spec.ts (1)
118-118: Remove or document the commented public key.The commented out public key appears to be unused in the test. If it's not needed, please remove it. If it's important for documentation or future use, please add a comment explaining its purpose.
🧰 Tools
🪛 Gitleaks
118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk-ts/src/core/accounts/PrivateKey.spec.ts(2 hunks)packages/sdk-ts/src/core/accounts/PrivateKey.ts(2 hunks)packages/wallets/wallet-private-key/src/strategy/strategy.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/wallets/wallet-private-key/src/strategy/strategy.ts
🧰 Additional context used
🪛 Gitleaks
packages/sdk-ts/src/core/accounts/PrivateKey.spec.ts
118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
packages/sdk-ts/src/core/accounts/PrivateKey.spec.ts (1)
117-134: LGTM! The test case effectively verifies the signature functionality.
The test case properly validates the arbitrary message signing functionality by:
- Converting the test message to UTF-8 buffer
- Signing the message using the private key
- Verifying against a known good signature
This aligns well with the PR's objective of fixing the signature generation for arbitrary data.
Note: The static analysis warning about an API key can be safely ignored as the value is actually a public Ethereum address used for testing.
🧰 Tools
🪛 Gitleaks
118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/sdk-ts/src/core/accounts/PrivateKey.ts (2)
179-184: Implementation change looks good but documentation and error handling need updates.
The change to use wallet.signMessage is correct and aligns with the PR objectives. However, the previous review comment about documentation and error handling is still valid and should be addressed.
368-370: LGTM! Improved readability.
The formatting change enhances code readability without affecting functionality.
billyjacoby
force-pushed
the
fix/pk-sign-arbitraty
branch
from
3709a9f
to
fc20dc9
Compare
This PR fixes
PrivateKey.signHashedmethod to allow for signing arbitrary data properly.Currently calling
PrivateKey.signHashed()with a string value converted to a Buffer returns an errorInstead of using the
Wallet.signingKey.signmethod we instead use theWallet.signMessagemethod which properly returns the correctly signed data.Summary by CodeRabbit
New Features
signArbitrarymethod to return the signature directly, enhancing efficiency.getEthereumChainIdmethod to return the Ethereum chain ID as a hexadecimal string.Bug Fixes
Tests
PrivateKeytest suite.