-
Notifications
You must be signed in to change notification settings - Fork 52
docs: update the privval extension CIP to match the expected implementation #342
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
docs: update the privval extension CIP to match the expected implementation #342
Conversation
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.
Summary of Changes
Hello @rach-id, 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 updates the privval extension CIP (CIP-040) to ensure its specification accurately reflects the intended and implemented behavior. The core changes revolve around refining the SignRawBytesRequest message structure and, more significantly, overhauling the mechanism for constructing the bytes that are signed, moving towards a more robust protobuf-based approach with a domain separator.
Highlights
- Sign Bytes Construction Revision: The method for constructing the bytes to be signed has been fundamentally changed. Previously, it involved simple concatenation of
chain_id,unique_id, andraw_bytes. Now, it requires prepending a domain separator ("COMET::RAW_BYTES::SIGN") to the protobuf-encodedSignRawBytesRequestmessage. A Go code example is provided to illustrate this new canonical construction. - Field Definition Clarifications: The descriptions for
chain_idandraw_byteshave been refined.raw_bytesis now explicitly stated to be the actual data, not a digest, and its construction refers to the new sign bytes section. - Documentation Streamlining: The detailed 'Encoding Specifications' section has been removed, as the new protobuf-based sign bytes construction implicitly handles the encoding. The 'Implementation Requirements' have also been updated to reflect the revised sign bytes generation process.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 updates the CIP for the privval extension to match the intended implementation. The changes are significant, moving from manual byte concatenation to a more robust protobuf-based serialization for signing, including a domain separator. This is a great improvement for security and clarity.
My review focuses on improving the clarity and correctness of the provided Go code snippet in the CIP and fixing a minor technical typo in the backwards compatibility section. The suggestions aim to make the reference implementation more self-contained and consistent.
|
after this is merged we should send it out to as many validators as possible |
cmwaters
left a comment
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.
Amendments look good to me
|
|
||
| prefix := []byte(RawBytesSignBytesPrefix) | ||
|
|
||
| signRequest := &privval.SignRawBytesRequest{ |
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.
Just a semantic nit, it would be clearer to have the prefix as part of the SignRawBytesRequest, so a user can easily see all the fields that get marshalled and won't miss the prefix that gets prepended at the end
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.
I see, but this request is going on the wire, I don't see a use for a field that's going to be always the same value to be added on each request.
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.
It was just to indicate that there is a prefix and that the sign raw bytes isn't all of the signed raw bytes. As I said this is a semantic nit
Overview
Updates the existing CIP to match what we're implementing