feat(EIP712): deduce primaryType if not provided
#1632
Conversation
WalkthroughThe pull request introduces significant changes to the VeChain signing functionality across multiple packages. The primary modifications involve simplifying the signing methods, particularly Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Signer
participant Crypto
Caller->>Signer: signPayload(payload)
Signer->>Crypto: Sign payload
Crypto-->>Signer: Return signature
Signer-->>Caller: Return signed payload
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (3)
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
CodeRabbit Configuration File (
|
Test CoverageSummary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/network/src/provider/utils/rpc-mapper/methods/eth_signTypedData_v4/eth_signTypedData_v4.ts (1)
77-78: Update function documentation to reflect parameter order.The JSDoc comment for the method needs to be updated to reflect that
primaryTypeis now optional and comes aftermessage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/aws-kms-adapter/jest.config.js(1 hunks)packages/aws-kms-adapter/src/KMSVeChainSigner.ts(4 hunks)packages/aws-kms-adapter/tests/KMSVeChainSigner.solo.test.ts(1 hunks)packages/aws-kms-adapter/tests/KMSVeChainSigner.unit.test.ts(2 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_signTypedData_v4/eth_signTypedData_v4.ts(1 hunks)packages/network/src/signer/signers/types.d.ts(1 hunks)packages/network/src/signer/signers/vechain-abstract-signer/vechain-abstract-signer.ts(1 hunks)packages/network/src/signer/signers/vechain-private-key-signer/vechain-private-key-signer.ts(1 hunks)packages/network/tests/signer/signers/vechain-private-key-signer/vechain-private-key-signer.unit.test.ts(3 hunks)
🔇 Additional comments (9)
packages/aws-kms-adapter/jest.config.js (1)
17-17: LGTM! Coverage threshold adjustment is reasonable.The reduction in branch coverage threshold from 100% to 90% aligns with the introduction of optional parameters and their associated conditional logic.
packages/network/src/provider/utils/rpc-mapper/methods/eth_signTypedData_v4/eth_signTypedData_v4.ts (1)
77-78: LGTM! Parameter reordering aligns with making primaryType optional.The change to move
typedData.messagebeforetypedData.primaryTypeis consistent with the PR objective of makingprimaryTypeoptional.packages/aws-kms-adapter/tests/KMSVeChainSigner.unit.test.ts (1)
149-150: LGTM! Test cases updated correctly.The test cases have been properly updated to reflect the new parameter order, maintaining comprehensive test coverage for both success and error scenarios.
Also applies to: 164-165
packages/aws-kms-adapter/tests/KMSVeChainSigner.solo.test.ts (1)
302-303: LGTM! Integration test updated correctly.The integration test has been properly updated to reflect the new parameter order while maintaining the same test coverage and validation.
packages/network/tests/signer/signers/vechain-private-key-signer/vechain-private-key-signer.unit.test.ts (1)
294-295: Add test coverage for omitted primaryType parameter.While the test cases are updated to use the new parameter order, there's no test coverage for the scenario where
primaryTypeis omitted. This is important to verify the new optional parameter behavior.Consider adding the following test case:
test('signTypedData - without primaryType', async () => { const signer = new VeChainPrivateKeySigner( Hex.of(eip712TestCases.valid.privateKey).bytes, provider ); const actual = await signer.signTypedData( eip712TestCases.valid.domain, eip712TestCases.valid.types, eip712TestCases.valid.data ); expect(actual).toBe(eip712TestCases.valid.signature); });Also applies to: 318-319, 327-328, 348-349
packages/network/src/signer/signers/vechain-abstract-signer/vechain-abstract-signer.ts (1)
349-349: LGTM!The abstract method signature is correctly updated to match the interface changes.
packages/aws-kms-adapter/src/KMSVeChainSigner.ts (3)
160-160: Documentation improvements look good!The grammatical fixes enhance clarity:
- "bytes payload" → "bytes' payload"
- "to by signed" → "to be signed"
Also applies to: 315-315
191-194: Good refactoring!The direct return of concatenated bytes simplifies the code while maintaining the same functionality.
402-407: Verify the impact of optional primaryType.The changes to make primaryType optional align with the PR objective. However, we should verify that all consumers of this method are updated to handle the new parameter order.
✅ Verification successful
All signTypedData implementations and calls are consistent with optional primaryType parameter
The codebase scan shows that all implementations and calls to signTypedData maintain consistent parameter ordering with primaryType as the last parameter. The interface already defines primaryType as optional, and this pattern is followed throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to signTypedData to ensure they're updated for the new parameter order rg -A 5 "signTypedData.*\(" --type tsLength of output: 17824
packages/network/src/signer/signers/vechain-private-key-signer/vechain-private-key-signer.ts
Outdated
Show resolved
Hide resolved
freemanzMrojo
changed the title
primaryType if not provided
freemanzMrojo
changed the title
primaryType if not providedprimaryType if not provided
Summary by CodeRabbit
New Features
primaryTypeparameter for typed data signingBug Fixes
Refactor
Chores