feat(pq-eth-signer/ts): implement Ethereum transaction signing with ML-DSA#30
Open
shaibuafeez wants to merge 2 commits intomultivmlabs:mainfrom
Open
feat(pq-eth-signer/ts): implement Ethereum transaction signing with ML-DSA#30shaibuafeez wants to merge 2 commits intomultivmlabs:mainfrom
shaibuafeez wants to merge 2 commits intomultivmlabs:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Greptile SummaryImplements the previously-empty Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/edge-case issues that don't affect the primary signing paths. No P0 or P1 bugs found. The RLP encoder, Noble API usage, EIP-55 checksum, and key management are all correct. The two logic comments (multi-dimensional array dependency in EIP-712 and missing bytesN length guard) are genuine edge cases that won't be triggered by any common EIP-712 payload; the interface vs type issue is a style convention. None of these block merge. packages/pq-eth-signer/ts/src/eip712.ts — findTypeDependencies regex and bytesN encoding; packages/pq-eth-signer/ts/src/types.ts — interface to type migration Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant PQSigner
participant transaction.ts
participant eip712.ts
participant address.ts
participant pq-key-encoder
Caller->>PQSigner: generate(options?) / fromSecretKey() / fromPem()
PQSigner->>pq-key-encoder: toSPKI(pubkey) [for address]
pq-key-encoder-->>PQSigner: SPKI bytes
PQSigner->>address.ts: deriveAddress(publicKey, algorithm)
address.ts-->>PQSigner: 0x... checksummed address
Caller->>PQSigner: signTransaction(tx)
PQSigner->>transaction.ts: hashUnsignedTransaction(tx)
transaction.ts-->>PQSigner: keccak256(0x02 || RLP([...fields]))
PQSigner->>PQSigner: sign(txHash) via ml_dsa.sign
PQSigner->>transaction.ts: serializeSignedTransaction(tx, sig)
transaction.ts-->>PQSigner: 0x02 || RLP([...fields, sig])
PQSigner-->>Caller: SignedTransaction { hash, rawTransaction, signature }
Caller->>PQSigner: signTypedData(domain, types, primaryType, message)
PQSigner->>eip712.ts: hashTypedData(...)
eip712.ts->>eip712.ts: domainSeparator + hashStruct
eip712.ts-->>PQSigner: keccak256(0x1901 || ds || structHash)
PQSigner->>PQSigner: sign(digest) via ml_dsa.sign
PQSigner-->>Caller: Uint8Array signature
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/pq-eth-signer/ts/src/types.ts
Line: 7-67
Comment:
**`interface` instead of `type` for plain data structures**
The repo's custom rule requires using `type` instead of `interface` for DTOs and simple data structures that don't use inheritance or extension. All six declarations here (`PQSignerOptions`, `TransactionRequest`, `SignedTransaction`, `ExportedKey`, `EIP712Domain`, `TypedDataField`) are plain data shapes with no extension or `implements` usage and should be `type` aliases.
```suggestion
export type PQSignerOptions = {
/** ML-DSA algorithm level. Defaults to 'ML-DSA-65'. */
algorithm?: SupportedAlgorithm;
/** Optional 32-byte seed for deterministic key generation. */
seed?: Uint8Array;
};
```
The same change applies to the remaining five declarations in this file.
**Rule Used:** Use `type` instead of `interface` for DTOs and sim... ([source](https://app.greptile.com/review/custom-context?memory=2b2a7a55-162e-44b9-8c4c-3f52514f7037))
**Learnt From**
[cytonic-network/ai-frontend#48](https://github.com/cytonic-network/ai-frontend/pull/48)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/pq-eth-signer/ts/src/eip712.ts
Line: 33-53
Comment:
**Multi-dimensional array dependencies not resolved**
`findTypeDependencies` strips only the outermost array suffix with `replace(/\[\d*\]$/, '')`. For a field type like `SomeStruct[][]`, `baseType` becomes `"SomeStruct[]"`, which is not a key in `types`, so `SomeStruct` is never added as a dependency. The resulting EIP-712 type string omits `SomeStruct(...)`, producing a wrong `typeHash` when callers use 2-D struct arrays.
```suggestion
function findTypeDependencies(
typeName: string,
types: Record<string, TypedDataField[]>,
result: Set<string> = new Set(),
): Set<string> {
if (result.has(typeName)) {
return result;
}
const fields = types[typeName];
if (!fields) {
return result;
}
result.add(typeName);
for (const field of fields) {
// Strip ALL array dimensions before looking up the base type
const baseType = field.type.replace(/(\[\d*\])+$/, '');
if (types[baseType]) {
findTypeDependencies(baseType, types, result);
}
}
return result;
}
```
The same single-strip pattern in `encodeValue` (`fieldType.replace(/\[\d*\]$/, '')`) handles nesting correctly there via recursion, so only this function needs the fix.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/pq-eth-signer/ts/src/eip712.ts
Line: 117-123
Comment:
**No length guard for `bytesN` values**
For fixed-size `bytesN` types (e.g. `bytes32`), values are written directly into a 32-byte result buffer with `result.set(bytes, 0)`. If the supplied `Uint8Array` is longer than 32 bytes, `TypedArray.set()` throws a `RangeError` with no informative message. This is most likely to surface via a malformed `EIP712Domain.salt` field.
Consider adding an early guard:
```typescript
if (fieldType.startsWith('bytes')) {
const bytes = value as Uint8Array;
const n = Number(fieldType.slice(5)); // e.g. 32 from "bytes32"
if (!Number.isNaN(n) && bytes.length > n) {
throw new Error(`Value too large for ${fieldType}: got ${bytes.length} bytes, expected \u2264${n}`);
}
result.set(bytes, 0);
return result;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(pq-eth-signer/ts): implement Ethere..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+7
to
+67
| export interface PQSignerOptions { | ||
| /** ML-DSA algorithm level. Defaults to 'ML-DSA-65'. */ | ||
| algorithm?: SupportedAlgorithm; | ||
| /** Optional 32-byte seed for deterministic key generation. */ | ||
| seed?: Uint8Array; | ||
| } | ||
|
|
||
| /** EIP-1559 (type 2) transaction request fields. */ | ||
| export interface TransactionRequest { | ||
| /** Recipient address (0x-prefixed, 20-byte hex). */ | ||
| to: string; | ||
| /** Transfer value in wei. Defaults to 0n. */ | ||
| value?: bigint; | ||
| /** Contract calldata. */ | ||
| data?: Uint8Array; | ||
| /** Sender nonce. */ | ||
| nonce: number; | ||
| /** Chain identifier. */ | ||
| chainId: bigint; | ||
| /** Gas limit. */ | ||
| gasLimit: bigint; | ||
| /** EIP-1559 max fee per gas. */ | ||
| maxFeePerGas: bigint; | ||
| /** EIP-1559 max priority fee per gas. */ | ||
| maxPriorityFeePerGas: bigint; | ||
| } | ||
|
|
||
| /** Result of signing a transaction. */ | ||
| export interface SignedTransaction { | ||
| /** Transaction hash (0x-prefixed hex). */ | ||
| hash: string; | ||
| /** RLP-encoded signed transaction bytes. */ | ||
| rawTransaction: Uint8Array; | ||
| /** Raw ML-DSA signature bytes. */ | ||
| signature: Uint8Array; | ||
| } | ||
|
|
||
| /** Exported key information. */ | ||
| export interface ExportedKey { | ||
| /** Algorithm used. */ | ||
| algorithm: SupportedAlgorithm; | ||
| /** Raw public key bytes. */ | ||
| publicKey: Uint8Array; | ||
| /** Derived Ethereum-style address (0x-prefixed, checksummed). */ | ||
| address: string; | ||
| } | ||
|
|
||
| /** EIP-712 domain separator fields. */ | ||
| export interface EIP712Domain { | ||
| name?: string; | ||
| version?: string; | ||
| chainId?: bigint; | ||
| verifyingContract?: string; | ||
| salt?: Uint8Array; | ||
| } | ||
|
|
||
| /** EIP-712 typed data field descriptor. */ | ||
| export interface TypedDataField { | ||
| name: string; | ||
| type: string; | ||
| } |
There was a problem hiding this comment.
interface instead of type for plain data structures
The repo's custom rule requires using type instead of interface for DTOs and simple data structures that don't use inheritance or extension. All six declarations here (PQSignerOptions, TransactionRequest, SignedTransaction, ExportedKey, EIP712Domain, TypedDataField) are plain data shapes with no extension or implements usage and should be type aliases.
Suggested change
| export interface PQSignerOptions { | |
| /** ML-DSA algorithm level. Defaults to 'ML-DSA-65'. */ | |
| algorithm?: SupportedAlgorithm; | |
| /** Optional 32-byte seed for deterministic key generation. */ | |
| seed?: Uint8Array; | |
| } | |
| /** EIP-1559 (type 2) transaction request fields. */ | |
| export interface TransactionRequest { | |
| /** Recipient address (0x-prefixed, 20-byte hex). */ | |
| to: string; | |
| /** Transfer value in wei. Defaults to 0n. */ | |
| value?: bigint; | |
| /** Contract calldata. */ | |
| data?: Uint8Array; | |
| /** Sender nonce. */ | |
| nonce: number; | |
| /** Chain identifier. */ | |
| chainId: bigint; | |
| /** Gas limit. */ | |
| gasLimit: bigint; | |
| /** EIP-1559 max fee per gas. */ | |
| maxFeePerGas: bigint; | |
| /** EIP-1559 max priority fee per gas. */ | |
| maxPriorityFeePerGas: bigint; | |
| } | |
| /** Result of signing a transaction. */ | |
| export interface SignedTransaction { | |
| /** Transaction hash (0x-prefixed hex). */ | |
| hash: string; | |
| /** RLP-encoded signed transaction bytes. */ | |
| rawTransaction: Uint8Array; | |
| /** Raw ML-DSA signature bytes. */ | |
| signature: Uint8Array; | |
| } | |
| /** Exported key information. */ | |
| export interface ExportedKey { | |
| /** Algorithm used. */ | |
| algorithm: SupportedAlgorithm; | |
| /** Raw public key bytes. */ | |
| publicKey: Uint8Array; | |
| /** Derived Ethereum-style address (0x-prefixed, checksummed). */ | |
| address: string; | |
| } | |
| /** EIP-712 domain separator fields. */ | |
| export interface EIP712Domain { | |
| name?: string; | |
| version?: string; | |
| chainId?: bigint; | |
| verifyingContract?: string; | |
| salt?: Uint8Array; | |
| } | |
| /** EIP-712 typed data field descriptor. */ | |
| export interface TypedDataField { | |
| name: string; | |
| type: string; | |
| } | |
| export type PQSignerOptions = { | |
| /** ML-DSA algorithm level. Defaults to 'ML-DSA-65'. */ | |
| algorithm?: SupportedAlgorithm; | |
| /** Optional 32-byte seed for deterministic key generation. */ | |
| seed?: Uint8Array; | |
| }; |
The same change applies to the remaining five declarations in this file.
Rule Used: Use type instead of interface for DTOs and sim... (source)
Learnt From
cytonic-network/ai-frontend#48
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pq-eth-signer/ts/src/types.ts
Line: 7-67
Comment:
**`interface` instead of `type` for plain data structures**
The repo's custom rule requires using `type` instead of `interface` for DTOs and simple data structures that don't use inheritance or extension. All six declarations here (`PQSignerOptions`, `TransactionRequest`, `SignedTransaction`, `ExportedKey`, `EIP712Domain`, `TypedDataField`) are plain data shapes with no extension or `implements` usage and should be `type` aliases.
```suggestion
export type PQSignerOptions = {
/** ML-DSA algorithm level. Defaults to 'ML-DSA-65'. */
algorithm?: SupportedAlgorithm;
/** Optional 32-byte seed for deterministic key generation. */
seed?: Uint8Array;
};
```
The same change applies to the remaining five declarations in this file.
**Rule Used:** Use `type` instead of `interface` for DTOs and sim... ([source](https://app.greptile.com/review/custom-context?memory=2b2a7a55-162e-44b9-8c4c-3f52514f7037))
**Learnt From**
[cytonic-network/ai-frontend#48](https://github.com/cytonic-network/ai-frontend/pull/48)
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+33
to
+53
| function findTypeDependencies( | ||
| typeName: string, | ||
| types: Record<string, TypedDataField[]>, | ||
| result: Set<string> = new Set(), | ||
| ): Set<string> { | ||
| if (result.has(typeName)) { | ||
| return result; | ||
| } | ||
| const fields = types[typeName]; | ||
| if (!fields) { | ||
| return result; | ||
| } | ||
| result.add(typeName); | ||
| for (const field of fields) { | ||
| const baseType = field.type.replace(/\[\d*\]$/, ''); | ||
| if (types[baseType]) { | ||
| findTypeDependencies(baseType, types, result); | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
findTypeDependencies strips only the outermost array suffix with replace(/\[\d*\]$/, ''). For a field type like SomeStruct[][], baseType becomes "SomeStruct[]", which is not a key in types, so SomeStruct is never added as a dependency. The resulting EIP-712 type string omits SomeStruct(...), producing a wrong typeHash when callers use 2-D struct arrays.
Suggested change
| function findTypeDependencies( | |
| typeName: string, | |
| types: Record<string, TypedDataField[]>, | |
| result: Set<string> = new Set(), | |
| ): Set<string> { | |
| if (result.has(typeName)) { | |
| return result; | |
| } | |
| const fields = types[typeName]; | |
| if (!fields) { | |
| return result; | |
| } | |
| result.add(typeName); | |
| for (const field of fields) { | |
| const baseType = field.type.replace(/\[\d*\]$/, ''); | |
| if (types[baseType]) { | |
| findTypeDependencies(baseType, types, result); | |
| } | |
| } | |
| return result; | |
| } | |
| function findTypeDependencies( | |
| typeName: string, | |
| types: Record<string, TypedDataField[]>, | |
| result: Set<string> = new Set(), | |
| ): Set<string> { | |
| if (result.has(typeName)) { | |
| return result; | |
| } | |
| const fields = types[typeName]; | |
| if (!fields) { | |
| return result; | |
| } | |
| result.add(typeName); | |
| for (const field of fields) { | |
| // Strip ALL array dimensions before looking up the base type | |
| const baseType = field.type.replace(/(\[\d*\])+$/, ''); | |
| if (types[baseType]) { | |
| findTypeDependencies(baseType, types, result); | |
| } | |
| } | |
| return result; | |
| } |
The same single-strip pattern in encodeValue (fieldType.replace(/\[\d*\]$/, '')) handles nesting correctly there via recursion, so only this function needs the fix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pq-eth-signer/ts/src/eip712.ts
Line: 33-53
Comment:
**Multi-dimensional array dependencies not resolved**
`findTypeDependencies` strips only the outermost array suffix with `replace(/\[\d*\]$/, '')`. For a field type like `SomeStruct[][]`, `baseType` becomes `"SomeStruct[]"`, which is not a key in `types`, so `SomeStruct` is never added as a dependency. The resulting EIP-712 type string omits `SomeStruct(...)`, producing a wrong `typeHash` when callers use 2-D struct arrays.
```suggestion
function findTypeDependencies(
typeName: string,
types: Record<string, TypedDataField[]>,
result: Set<string> = new Set(),
): Set<string> {
if (result.has(typeName)) {
return result;
}
const fields = types[typeName];
if (!fields) {
return result;
}
result.add(typeName);
for (const field of fields) {
// Strip ALL array dimensions before looking up the base type
const baseType = field.type.replace(/(\[\d*\])+$/, '');
if (types[baseType]) {
findTypeDependencies(baseType, types, result);
}
}
return result;
}
```
The same single-strip pattern in `encodeValue` (`fieldType.replace(/\[\d*\]$/, '')`) handles nesting correctly there via recursion, so only this function needs the fix.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+117
to
+123
| if (fieldType.startsWith('bytes')) { | ||
| const bytes = value as Uint8Array; | ||
| result.set(bytes, 0); | ||
| return result; | ||
| } | ||
|
|
||
| throw new Error(`Unsupported EIP-712 type: ${fieldType}`); |
There was a problem hiding this comment.
bytesN values
For fixed-size bytesN types (e.g. bytes32), values are written directly into a 32-byte result buffer with result.set(bytes, 0). If the supplied Uint8Array is longer than 32 bytes, TypedArray.set() throws a RangeError with no informative message. This is most likely to surface via a malformed EIP712Domain.salt field.
Consider adding an early guard:
if (fieldType.startsWith('bytes')) {
const bytes = value as Uint8Array;
const n = Number(fieldType.slice(5)); // e.g. 32 from "bytes32"
if (!Number.isNaN(n) && bytes.length > n) {
throw new Error(`Value too large for ${fieldType}: got ${bytes.length} bytes, expected \u2264${n}`);
}
result.set(bytes, 0);
return result;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pq-eth-signer/ts/src/eip712.ts
Line: 117-123
Comment:
**No length guard for `bytesN` values**
For fixed-size `bytesN` types (e.g. `bytes32`), values are written directly into a 32-byte result buffer with `result.set(bytes, 0)`. If the supplied `Uint8Array` is longer than 32 bytes, `TypedArray.set()` throws a `RangeError` with no informative message. This is most likely to surface via a malformed `EIP712Domain.salt` field.
Consider adding an early guard:
```typescript
if (fieldType.startsWith('bytes')) {
const bytes = value as Uint8Array;
const n = Number(fieldType.slice(5)); // e.g. 32 from "bytes32"
if (!Number.isNaN(n) && bytes.length > n) {
throw new Error(`Value too large for ${fieldType}: got ${bytes.length} bytes, expected \u2264${n}`);
}
result.set(bytes, 0);
return result;
}
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the empty
pq-eth-signerTypeScript package with complete ML-DSA Ethereum transaction signing.keccak256(toSPKI(pubkey))last 20 bytes, EIP-55 checksummedpq-key-encoder@noble/post-quantum, transaction serialization, EIP-712, error handling across all 3 ML-DSA levelsMotivation
The
pq-eth-signerpackage existed as an empty stub (export {}, "Coming soon" README). This is the missing bridge between your PQ crypto primitives (pq-oid,pq-key-encoder) and actual Ethereum transaction signing — the piece that lets developers sign EIP-1559 transactions and EIP-712 typed data with post-quantum keys.Design Decisions
#secretKeyfield — explicitexportSecretKey()required, no accidental leaksPackage(s)
pq-eth-signer/tsLanguages
Files Changed
packages/pq-eth-signer/ts/src/errors.ts— error hierarchy (matchespq-key-fingerprintpattern)packages/pq-eth-signer/ts/src/types.ts— TransactionRequest, SignedTransaction, EIP712Domain, etc.packages/pq-eth-signer/ts/src/utils.ts— hex encoding, EIP-55 checksum, byte helperspackages/pq-eth-signer/ts/src/address.ts— keccak256 address derivation from SPKIpackages/pq-eth-signer/ts/src/transaction.ts— EIP-1559 serialization, RLP encodingpackages/pq-eth-signer/ts/src/eip712.ts— EIP-712 typed data hashingpackages/pq-eth-signer/ts/src/signer.ts— PQSigner class (core implementation)packages/pq-eth-signer/ts/src/index.ts— barrel exports (replaces emptyexport {})packages/pq-eth-signer/ts/tests/— 4 test files, 64 tests totalpackages/pq-eth-signer/ts/package.json— dependencies, scripts, exportspackages/pq-eth-signer/ts/README.md— full API reference and usage guideTest Plan
bun test— 64 tests, 0 failures@noble/post-quantum(ml_dsa44.verify,ml_dsa65.verify,ml_dsa87.verify)