Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to sign transaction details in addition to the existing passport data signing capability. It refactors the codebase to distinguish between different types of signable data by renaming StructToSign to PassportDataToSign and introducing a new TransactionDetailsToSign type. The signData function is similarly split into signPassportData and signTransactionDetailsData for clarity.
Key changes include:
- Renamed types and functions for better clarity between passport and transaction data
- Added new
TransactionDetailsToSigntype and corresponding signing function - Converted
/message-detailsGET endpoint to/sign-message-detailsPOST endpoint with authentication
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/utils/types.ts | Renamed StructToSign to PassportDataToSign and added TransactionDetailsToSign type definition |
| src/http-server/server.ts | Updated imports to use renamed types, refactored passport signing endpoint to use new names, and added new POST endpoint for signing transaction details |
| src/blockchain/sign-data.ts | Renamed signData to signPassportData, added new signTransactionDetailsData function, and extracted common domain payload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| domain: signDataDomainPayload, | ||
| types: { | ||
| PaymentTransaction: [ | ||
| { name: "id", type: "bytes" }, |
There was a problem hiding this comment.
The type for the "id" field is defined as "bytes", but the actual data being passed is an Address type (which is a string). This mismatch between the TypeScript type and the EIP-712 type definition could cause issues. Consider using "address" or "string" type instead, or if a bytes type is needed, the data should be properly encoded as bytes before signing.
| { name: "id", type: "bytes" }, | |
| { name: "id", type: "address" }, |
| const result = await getMessageInfoByCid(req.query.msgCid as string); | ||
|
|
||
| if (!result) { | ||
| return res | ||
| .status(500) | ||
| .json({ error: "Failed to fetch transaction details" }); | ||
| } | ||
| const transactionDetailsToSign: TransactionDetailsToSign = { | ||
| id: req.query.msgCid as Address, | ||
| from: result?.From as Address, | ||
| to: result?.To as Address, | ||
| amount: BigInt(result!.Value), | ||
| }; | ||
|
|
||
| res.json({ | ||
| status: "ok", | ||
| timestamp: new Date().toISOString(), | ||
| data: { | ||
| from: result.From, | ||
| to: result.To, | ||
| amount: result.Value, | ||
| }, | ||
| }); | ||
| }); | ||
| const signature = await signTransactionDetailsData( | ||
| transactionDetailsToSign | ||
| ); | ||
|
|
||
| if (!result) { | ||
| return res | ||
| .status(500) | ||
| .json({ error: "Failed to fetch transaction details" }); | ||
| } |
There was a problem hiding this comment.
The error check for result is performed after the result is already used to create transactionDetailsToSign and sign the data. If getMessageInfoByCid returns null or undefined, this will cause errors on lines 86-88 when trying to access properties of result. The null check on line 95 should be moved to immediately after line 82, before using result.
| const result = await getMessageInfoByCid(req.query.msgCid as string); | ||
|
|
||
| if (!result) { | ||
| return res | ||
| .status(500) | ||
| .json({ error: "Failed to fetch transaction details" }); | ||
| } | ||
| const transactionDetailsToSign: TransactionDetailsToSign = { | ||
| id: req.query.msgCid as Address, |
There was a problem hiding this comment.
The msgCid parameter is being read from req.query but this is a POST endpoint. For POST requests, parameters should be read from req.body as shown in the validation check on line 76. This inconsistency will cause the endpoint to not work correctly.
| timestamp: new Date().toISOString(), | ||
| data: { | ||
| signedStructSignature: signature, | ||
| signedSignature: signature, |
There was a problem hiding this comment.
The property name "signedStructSignature" has been changed to "signedSignature", which is redundant and less descriptive. The word "signed" is repeated twice in "signedSignature". Consider using just "signature" or a more descriptive name.
| status: "ok", | ||
| timestamp: new Date().toISOString(), | ||
| data: { | ||
| signedSignature: signature, |
There was a problem hiding this comment.
The property name "signedSignature" is redundant with the word "signed" repeated twice. Consider using just "signature" or a more descriptive name for consistency.
No description provided.