feat: ETH sign_typed_data_v4 #75
Conversation
WalkthroughThe pull request introduces several updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (1)
packages/kos-sdk/Cargo.toml (1)
52-52: LGTM! Consider using a more flexible version constraint.The addition of the
alloy-dyn-abidependency with the "eip712" feature aligns well with the PR objective of implementing ETH sign_typed_data_v4. This will enable support for Ethereum's EIP-712 standard for typed structured data hashing and signing.Consider using a more flexible version constraint to automatically receive compatible updates:
-alloy-dyn-abi = { version = "0.8.5", features = ["eip712"] } +alloy-dyn-abi = { version = "^0.8.5", features = ["eip712"] }This change allows for minor version updates (0.8.x) which typically include bug fixes and backwards-compatible changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
- packages/kos-sdk/Cargo.toml (1 hunks)
- packages/kos-sdk/src/chains/ethereum/mod.rs (3 hunks)
- packages/kos-sdk/src/wallet.rs (1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/kos-sdk/src/chains/ethereum/mod.rs (1)
761-808: Test implementation looks good, consider adding more test cases.The test for
sign_type_data_v4provides good coverage with a realistic EIP-712 typed data example. It correctly verifies both the hexadecimal representation and the length of the signature.As suggested in a previous review, consider adding test cases for invalid input scenarios to ensure
sign_type_data_v4handles errors gracefully.packages/kos-mobile/src/lib.rs (1)
70-83: Consider adding documentation forsign_typed_data_v4Adding documentation comments to the new
sign_typed_data_v4function will improve code maintainability and help other developers understand its purpose and usage.You might add something like:
/// Signs typed data using EIP-712 standard for Ethereum chains. /// /// # Arguments /// /// * `account` - The KOSAccount containing chain information and private key. /// * `data` - The JSON string representing the typed data to be signed. /// /// # Returns /// /// A vector of bytes containing the signature. /// /// # Errors /// /// Returns `KOSError::UnsupportedChain` if the chain is not Ethereum.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/kos-mobile/src/lib.rs (3 hunks)
- packages/kos-sdk/src/chains/ethereum/mod.rs (3 hunks)
🔇 Additional comments (4)
packages/kos-sdk/src/chains/ethereum/mod.rs (1)
169-176: LGTM! Implementation looks correct and previous issues addressed.The
sign_type_data_v4function correctly implements EIP-712 signing. It properly parses the input data, computes the signing hash, and signs the digest. The error handling has been improved, and theserde_jsonimport issue has been resolved.packages/kos-mobile/src/lib.rs (3)
6-6: ImportETHchain moduleThe addition of
use kos_sdk::chains::ETH;correctly allows access to Ethereum-specific functionalities needed for the newsign_typed_data_v4function.
392-448: Testshould_sign_typed_data_v4accurately verifies new functionalityThe test function
should_sign_typed_data_v4effectively checks the signing process for typed data. It uses a known mnemonic and validates both the signature length and its expected value.
395-395: Verify thatchain_idcorresponds to EthereumEnsure that the
chain_idvalue3used in the test corresponds to the Ethereum chain (Chain::ETH). IfChain::ETHuses a differentchain_id, the test may not execute as expected.Run the following script to confirm the
chain_idfor Ethereum:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/kos-mobile/src/lib.rs (1)
395-450: Test looks good, consider adding error scenarios.The
should_sign_typed_data_v4test is well-implemented and covers the basic functionality of thesign_typed_data_v4function. It uses a known input and checks for a specific output, which is excellent for regression testing.Consider adding more test cases to cover error scenarios, such as:
- Attempting to sign with a non-ETH chain.
- Providing invalid typed data structure.
- Using an invalid private key.
These additional tests would improve the overall test coverage and robustness of the implementation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/kos-mobile/src/lib.rs (3 hunks)
🔇 Additional comments (2)
packages/kos-mobile/src/lib.rs (2)
70-84: LGTM! Implementation looks correct and past review comment addressed.The
sign_typed_data_v4function is well-implemented. It correctly checks for the Ethereum chain and handles the potentialNonefromwallet.get_keypair()as suggested in a past review comment.
Line range hint
1-450: Overall, the changes look good and improve the functionality.The addition of the
sign_typed_data_v4function and its corresponding test enhances the Ethereum-specific functionality of the library. The implementation addresses a previous review comment and follows good practices. The changes are focused and don't introduce any apparent regressions to existing functionality.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Dependency Updates