-
Notifications
You must be signed in to change notification settings - Fork 57
Refactor message signing #1490
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
Refactor message signing #1490
Conversation
Refactored SignMessageSceneViewModel to use chain-specific signature schemes and digest signing, improving support for multiple chains. Added Chain+SignatureScheme extension to map chains to their signature schemes. Updated ChainSigner to delegate message signing to GemChainSigner and handle both raw and typed messages. Updated core submodule.
Summary of ChangesHello @0xh3rman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the message signing infrastructure to enhance multi-chain compatibility and streamline the signing process. It introduces a more modular approach by defining chain-specific signature schemes and centralizing message signing delegation, ensuring robust and flexible handling of various blockchain message types. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request refactors the message signing logic to better support multiple chains. The changes in SignMessageSceneViewModel and ChainSigner are a good step towards centralizing and simplifying the signing process. The introduction of the Chain+SignatureScheme extension helps in abstracting signature algorithms.
My review includes two main points. First, I've suggested an improvement to the SignMessageSceneViewModel to further abstract away chain-specific signing logic, which would improve maintainability. Second, I've pointed out a potential robustness issue in the new Chain+SignatureScheme extension and suggested a more resilient design for the future. Overall, this is a positive refactoring.
Features/WalletConnector/Sources/WalletConnector/ViewModels/SignMessageSceneViewModel.swift
Outdated
Show resolved
Hide resolved
Packages/GemstonePrimitives/Sources/Extensions/Chain+SignatureScheme.swift
Outdated
Show resolved
Hide resolved
Replaced usage of SignMessageDecoder and CryptoSigner with the new MessageSigner class in both the service and view model. Updated related tests and renamed SignMessageDecoderTests to MessageSignerTests. This change streamlines message signing and preview logic under a single class.
Needs gemwalletcom/core#840