Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: return correct signature #510

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
+27 −8
Open

fix: return correct signature #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8478577
fix: return correct signature
billyjacoby Oct 31, 2024
c723b31
typo: fix dev import
billyjacoby Oct 31, 2024
3709a9f
chore: add initial test case
billyjacoby Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions packages/sdk-ts/src/core/accounts/PrivateKey.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { toUtf8 } from '../..'
import { generateArbitrarySignDoc } from '../tx'
import { PrivateKey } from './PrivateKey'

Expand Down Expand Up @@ -113,6 +114,25 @@ describe('PrivateKey', () => {
).toBe(true)
})

it('returns the correct signature for signing an arbitrary message', () => {
// const pubKey = '0x697e62225Dd22A5afcAa82901089b2151DAEB706'
const message = 'this is a test message'

const verifiedSignature =
'0xfd0879e35cec78b87ae7e647ebb743093ea15edcb88eb388806adaaff5f645527dab0cfac030b23d702206d6e0840a7bae5d2239518ba20b73c6636f75f150401b'

const privateKey = PrivateKey.fromHex(pk)

const privKeySignature = privateKey.sign(
Buffer.from(toUtf8(message), 'utf-8'),
)

expect(Buffer.from(privKeySignature).toString('hex')).toEqual(
verifiedSignature,
)
//
})

it('returns true when verifying signature for a public key and a cosmos message', () => {
//
})
Expand Down
13 changes: 6 additions & 7 deletions packages/sdk-ts/src/core/accounts/PrivateKey.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,12 @@ export class PrivateKey {
* @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
*/
signHashed(messageHashedBytes: Buffer): Uint8Array {
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
Comment on lines +179 to +184
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update JSDoc and add error handling for breaking changes

The method has undergone significant changes that need to be properly documented and handled:

  1. JSDoc is incorrect - return type is now Promise<string> not Uint8Array
  2. Missing error handling for wallet.signMessage
  3. 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.

Suggested change
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}`)
}
}

}

/**
Expand Down Expand Up @@ -368,7 +365,9 @@ export class PrivateKey {
}

const decodedExtension =
InjectiveTypesV1Beta1TxExt.ExtensionOptionsWeb3Tx.decode(extension.value)
InjectiveTypesV1Beta1TxExt.ExtensionOptionsWeb3Tx.decode(
extension.value,
)

const ethereumChainId = Number(
decodedExtension.typedDataChainID,
Expand Down
2 changes: 1 addition & 1 deletion packages/wallets/wallet-private-key/src/strategy/strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ export class PrivateKeyWallet
try {
const signature = await pk.signHashed(Buffer.from(toUtf8(data), 'utf-8'))

return `0x${Buffer.from(signature).toString('base64')}`
return signature
} catch (e: unknown) {
throw new MetamaskException(new Error((e as any).message), {
code: UnspecifiedErrorCode,
Expand Down