-
Notifications
You must be signed in to change notification settings - Fork 37
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.
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
Feat/eth-pubkey #85
Feat/eth-pubkey #85
Conversation
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EthPublicKey
participant PublicKeyNamespace
User->>EthPublicKey: Create instance with public key
EthPublicKey->>PublicKeyNamespace: Call fromAmino()
PublicKeyNamespace-->>EthPublicKey: Return EthPublicKey instance
EthPublicKey->>EthPublicKey: Generate address
EthPublicKey-->>User: Return generated address
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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/core/PublicKey.spec.ts (1)
Line range hint
1-46
: Summary: EthPublicKey support added successfully, consider enhancing test coverage.The changes to add support for Ethereum public keys have been implemented correctly. The import statement and the new test case are well-integrated with the existing code. To further improve the quality of this addition:
- Expand the test suite for
EthPublicKey
with additional test cases as suggested earlier.- Add comments or documentation explaining the source and significance of the test data used.
- Ensure that the
EthPublicKey
class insrc/core/PublicKey.ts
is thoroughly tested, including edge cases and error handling.As this change introduces Ethereum functionality to the project, consider reviewing the overall architecture to ensure it accommodates potential future blockchain integrations smoothly. This might involve creating a more generic interface for different blockchain public keys if not already present.
src/core/PublicKey.ts (1)
374-444
: Add unit tests forEthPublicKey
classThe new
EthPublicKey
class introduces significant functionality for handling Ethereum public keys. To ensure correctness and prevent future regressions, please add unit tests covering all its methods, including edge cases and error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (3)
- package.json (1 hunks)
- src/core/PublicKey.spec.ts (2 hunks)
- src/core/PublicKey.ts (6 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🔇 Additional comments (3)
src/core/PublicKey.spec.ts (2)
2-2
: LGTM: Import statement updated correctly.The
EthPublicKey
import has been added appropriately alongside other public key imports. This change is necessary to support the new test case for Ethereum public keys.
39-46
: LGTM: New test case for EthPublicKey added.The test case for
EthPublicKey
has been implemented correctly, following the structure of other public key tests in the file. It verifies the basic functionality of address generation for Ethereum public keys.Consider adding the following test cases to improve coverage:
- Test with an invalid public key string to ensure proper error handling.
- Test with multiple different valid public keys to ensure consistency.
- Test the serialization and deserialization of
EthPublicKey
if applicable.To verify the correctness of the public key and address used in the test, run the following script:
src/core/PublicKey.ts (1)
426-439
: Ensure correct derivation of Ethereum address from public keyThe method
rawAddress()
computes the Ethereum address by hashing the uncompressed public key and slicing the last 20 bytes. Verify that this implementation aligns with Ethereum's address derivation standards to prevent potential address mismatches.Run the following script to confirm the address derivation:
Replace
YOUR_PUBLIC_KEY_IN_BASE64
andYOUR_EXPECTED_ADDRESS
with actual values to perform the verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary by CodeRabbit
New Features
EthPublicKey
, with methods for conversion and address generation.Tests
PublicKey
suite by adding a test case for theEthPublicKey
class.Chores