feat: [ADR-072] Sign Mode Unified #22714
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new signing mode called Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Signer
participant SDK
participant MerkleTree
User->>SDK: Construct Transaction
SDK->>Signer: Request Signing
Signer->>SDK: Provide Metadata
SDK->>MerkleTree: Create Merkle Structure
MerkleTree-->>SDK: Return Merkle Tree
SDK->>User: Present Transaction for Approval
User->>SDK: Approve Transaction
SDK->>Signer: Sign Transaction
Signer-->>SDK: Return Signed Transaction
SDK->>User: Broadcast Transaction
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
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: 1
🧹 Outside diff range and nitpick comments (4)
docs/architecture/adr-076-sign-mode-unified.md (4)
47-47: Improve sentence clarityThe sentence can be made more concise by removing the wordy phrase "in order to".
-The metadata file is a JSON file that contains all the types and messages definitions used in the SDK, is meant to be generated at build time and stored in the node, using the protobuf definition files as source. This file (in fact, chunks of its encoded version) are sent to the signer in order to be used to decode and show the transaction's fields without the need to store locally all definitions of types and messages. +The metadata file is a JSON file that contains all the types and messages definitions used in the SDK, is meant to be generated at build time and stored in the node, using the protobuf definition files as source. This file (in fact, chunks of its encoded version) are sent to the signer to decode and show the transaction's fields without the need to store locally all definitions of types and messages.🧰 Tools
🪛 LanguageTool
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...encoded version) are sent to the signer in order to be used to decode and show the transact...(IN_ORDER_TO_PREMIUM)
288-290: Add blank line before list itemFor better readability, add a blank line before the list item.
### Backwards Compatibility + **Preservation of Existing Sign Modes:** * Existing sign modes remain supported to ensure that legacy wallets and clients continue to function without interruption.🧰 Tools
🪛 Markdownlint (0.35.0)
289-289: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
327-327: Add newline at end of fileAdd a newline character at the end of the file to follow Markdown best practices.
https://polkadot-fellows.github.io/RFCs/approved/0078-merkleized-metadata.html +🧰 Tools
🪛 Markdownlint (0.35.0)
327-327: null
Files should end with a single newline character(MD047, single-trailing-newline)
168-170: Consider using a different type for coin amountIn the metadata example, the
amountfield of theCointype is defined as a string (type 1). Consider using a more appropriate numeric type for coin amounts to ensure proper validation and prevent potential issues with numeric operations.{ "name": "amount", - "type": 1 + "type": { + "def": { + "Primitive": "Uint256" + } + } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
docs/architecture/adr-076-sign-mode-unified.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/architecture/adr-076-sign-mode-unified.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
docs/architecture/adr-076-sign-mode-unified.md
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...encoded version) are sent to the signer in order to be used to decode and show the transact...
(IN_ORDER_TO_PREMIUM)
🪛 Markdownlint (0.35.0)
docs/architecture/adr-076-sign-mode-unified.md
289-289: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
327-327: null
Files should end with a single newline character
(MD047, single-trailing-newline)
| @@ -0,0 +1,327 @@ | |||
| # ADR 075: Sign Mode Unified | |||
There was a problem hiding this comment.
Fix inconsistency in ADR number
The ADR number in the title (075) doesn't match the filename (076). Please ensure consistency between the title and filename.
-# ADR 075: Sign Mode Unified
+# ADR 076: Sign Mode Unified📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ADR 075: Sign Mode Unified | |
| # ADR 076: Sign Mode Unified |
| `SIGN_MODE_UNIFIED` proposes using CBOR (Concise Binary Object Representation) for transaction encoding, offering several critical advantages: | ||
|
|
||
| * **Deterministic Encoding**: CBOR's canonical form ensures consistent merkleization of transaction data, which is essential for verification and proof generation. | ||
| * **Human-readable Diagnostic Notation**: While CBOR is binary, it provides a diagnostic notation format that maintains transparency and reduces blind signing risks. |
There was a problem hiding this comment.
Human readability was a design goal (and powerful feature) of AminoJSON (and Textual). The user is presented with a literal representation of the tx bytes which are being signed over. From https://www.rfc-editor.org/rfc/rfc8949.html
JSON: {_ "a": 1, "b": [_ 2, 3]}
CBOR: 0xbf61610161629f0203ffff
I don't think we can say that CBOR is human readable, its use in a wallet UX would be more akin to blind signing. I'm not a security expert but isn't this a bit of step backwards for verifiability?
There was a problem hiding this comment.
Hey, thanks for pointing that out, true!
We are proposing CBOR because it enables metadata merkleization, a central feature of this new sign mode. While CBOR is not human-readable in its raw form, any UI can easily decode it to present the data to users. Blind signing would only occur if a wallet implements a "lazy" UI that doesn't decode the CBOR string, which would likely make such a wallet less appealing to users.
This approach doesn't represent a step backwards in terms of verifiability, because:
- CBOR transactions can always be decoded for human readability. We rely on widely-tested codec libraries for this, just as we do with other formats.
- Since the transaction contains the metadata's root hash, the signer can verify that the decoded transaction chunks comply with the metadata sent by the node. This provides a "first-order" verification.
- Finally, since the sign doc contains the root hash (meaning the user is signing both
tx_blob + metadata_root_hash), validators can verify that the transaction was displayed (and signed) using the consensual metadata tree, providing a "second-order" verification.
There was a problem hiding this comment.
We are proposing CBOR because it enables metadata merkleization, a central feature of this new sign mode.
What specifically about CBOR enables metadata merkleization as opposed to say JSON? It's simply an encoding format.
There was a problem hiding this comment.
The original question here was about human readability. Given the amount of detail that the previous spec for textual put into it human readability (or even amino JSON), I find "CBOR transactions can always be decoded for human readability" to be a pretty underwhelming answer.
| Validator->>Validator: Compare with local values | ||
| ``` | ||
|
|
||
| ## Decision |
There was a problem hiding this comment.
Were any alternatives considered? The simplest which comes to mind is a constrained JSON spec. Given that proto is the defacto IDL (it remains so in this design since the JSON metadata is derived from it) proto reflect could drive marshaling pretty easily. Downside being that clients still have a strong dependency on a proto tool chain, I think.
| * *Metadata Digest*: A compact representation of the complete metadata set used for transaction construction. When hashed (digest hash), this struct serves as the root of the merkle tree, enabling efficient verification of metadata integrity. | ||
| * *Merkleized Structure*: The metadata is organized into a merkle tree, allowing for efficient proofs and verification. | ||
|
|
||
| ### Metadata file |
There was a problem hiding this comment.
This is a powerful abstraction and to me seems like a viable pathway to completely decouple a wallet or execution environment from protobuf. Did I get that right? If so we should push that as a big pro! I think proto has been troublesome on front ends, hardware wallets and VMs.
There was a problem hiding this comment.
Yes, that's right, the wallet / signers should be completely decoupled from the protobuf definition files
There was a problem hiding this comment.
First of all, I assume there are valid reasons for abandoning the SIGN_MODE_TEXTUAL effort, but what are they? It would be good to document all of these.
I do want to note that in quickly reviewing the original SIGN_MODE_TEXTUAL ADR 050 and its Annexes for comparison, a lot of attention was given towards details of the client user experience. Things like line length and display of integers and non-ASCII characters are carefully considered. This ADR, on the other hand, has only this to say regarding human readability:
* **Human-readable Diagnostic Notation**: While CBOR is binary, it provides a diagnostic notation format that maintains transparency and reduces blind signing risks.
Most of the ADR seems to focus on how schema metadata can be embedded in a node in a provable way and even goes as far as constructing a new merkle tree format. I would note that all nodes currently do make protobuf descriptors available for querying, but not in a provable way. We did actually discuss storing the protobuf descriptors in state and decided against it 1) because this introduces a separate build step (also proposed in this ADR) and 2) introduces non-determinism if a user so much as adds a comment to a .proto file in a separate compilation of a node's binary. Even if we did embed schema metadata in a provable way, why do we need a separate merkle tree structure besides the one already provided for proving state? Putting that aside, I'm really not sure why this is an issue at all. The way that we prove that the user signed what they intended to sign is by generating an equivalent sign doc in a textual format (currently amino JSON) and verifying the signature. We do not need to verify that the same metadata was used because a human readable format is inherently sign describing. So why is this provable and signed metadata needed?
A second requirement here is an incompletely specified mapping between protobuf and this new schema format and CBOR encoding. This is a pretty big deal in and of itself and I am not sure what benefits it brings. If you read through the SIGN_MODE_TEXTUAL documents, there are a lot of details on how to map the SDK's data to a human readable format. Even Amino JSON does pretty good at this. A naive mapping from protobuf to CBOR will not produce a good end user experience all by itself. This is especially true since some users are starting to encode addresses as bytes rather than strings in transactions. Both Amino JSON and SIGN_MODE_TEXTUAL provide detailed ways to map protobuf data to a human readable representation and this new mapping would need to repeat a lot of that work or risk being a downgrade.
Overall, it seems like there would be a lot of work to make this work in terms of specification, build tooling and client tooling. The end result would introduce a new required build step and there are no details as to whether the human readable format would be an upgrade or downgrade from Amino JSON. I appreciate the effort the authors put into this document and I also appreciate the efforts that people put into SIGN_MODE_TEXTUAL. From what I'm reading here I don't see any compelling reasons for why it would be worth moving to this new sign mode unified approach over the current status quo of Amino JSON.
julienrbrt
assigned aaronc, kocubinski, JulianToledano, facundomedica, tac0turtle, julienrbrt and testinginprod
Hey @aaronc, thanks for your input. |
|
If moving away from textual is an already established direction where is that documented? In a quick issue search I didn't find anything. A technical proposal should justify its design decisions, especially when there are existing solutions. Specifically, I asked in my comment above:
This proposal should address these points IMHO. |
|
|
||
| Key components of `SIGN_MODE_UNIFIED` include: | ||
|
|
||
| * *Metadata struct*: A JSON file containing the definitions of all data types for each message of each module, down to primitive types. |
There was a problem hiding this comment.
Do I understand correctly that this is referring to a JSON document, independent of its encoding (I.e. the in-memory data structure)? If so, I suggest avoiding the term "JSON file" as it indicates to something on disk, using some series of bytes. The difference matters a lot, especially as Go by default randomizes map entry order and we have a lot of genesis files where mismatch and confusion is caused by different encodings of the same document.
There was a problem hiding this comment.
Yes, thanks for pointing that out. This is referring to a .json document meant to be stored on disk. This document is the one that the adr mentions that should be created from the .proto files.
The on-memory representation of this struct uses this document as source but then it encodes it (with the proposed codecs, json, cbor, etc) to make a canonical representation and later to build the merkle tree out of it.
Will enhance the docs to reenforce this idea
| @@ -0,0 +1,327 @@ | |||
| # ADR 075: Sign Mode Unified | |||
|
|
|||
There was a problem hiding this comment.
What does this sign mode unify? I think both Amino JSON and Textual are designed to be self-descriptive documents. The big selling point of this is using on-chain state to mitigate the limitations of the other approach. I would use a great new name for great new tech.
There was a problem hiding this comment.
Name ideas are welcome! 😃
| Key components of `SIGN_MODE_UNIFIED` include: | ||
|
|
||
| * *Metadata struct*: A JSON file containing the definitions of all data types for each message of each module, down to primitive types. | ||
| * *Metadata Digest*: A compact representation of the complete metadata set used for transaction construction. When hashed (digest hash), this struct serves as the root of the merkle tree, enabling efficient verification of metadata integrity. |
There was a problem hiding this comment.
A "digest" is usually some sort of summary or even the hash which I think is misleading here. What we have here is a serialization of the document above. I think "Metadata Serialization" or "Metadata file" (or similar) would be better.
| * Requires significant development effort to implement the new sign mode, including updates to multiple components such as transaction builders, signers, and validators and HW wallet apps. | ||
|
|
||
| * **Tooling and Documentation Updates:** | ||
| * Requires updates to existing tools, documentation, and developer guides to accommodate the new sign mode, which may involve additional resources and time. |
There was a problem hiding this comment.
Not a show stopper but I think it's worth mentioning that the sign doc is not human readable anymore
There was a problem hiding this comment.
Added using plain json + spec as an alternative to cbor to keep human readability
|
Thanks everyone for your inputs and comments. Will address them and come back with some modifications. Main key points to analyze are:
|
Fix md format
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
docs/architecture/adr-076-sign-mode-unified.md (5)
222-229: Enhance CBOR section with concrete examplesWhile the advantages of CBOR are well-explained, the section would benefit from concrete examples showing:
- How the diagnostic notation looks for common transaction types
- How wallets should implement the decoding for human readability
47-47: Improve sentence clarityThe sentence is wordy and can be simplified.
-The metadata file is a JSON file that contains all the types and messages definitions used in the SDK, is meant to be generated at build time and stored in the node, using the protobuf definition files as source. This file (in fact, chunks of its encoded version) are sent to the signer in order to be used to decode and show the transaction's fields without the need to store locally all definitions of types and messages. +The metadata file is a JSON file containing all type and message definitions used in the SDK. It is generated at build time from protobuf definition files and stored in the node. Chunks of its encoded version are sent to the signer to decode and display transaction fields, eliminating the need for local storage of type and message definitions.🧰 Tools
🪛 LanguageTool
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...encoded version) are sent to the signer in order to be used to decode and show the transact...(IN_ORDER_TO_PREMIUM)
290-290: Fix conjunction usageThe sentence structure needs improvement.
-This would achieve deterministic encoding across different signer implementations while maintaining readability. However, this approach would require all wallet implementations of this sign mode to ensure their JSON encoder complies with the canonical encoding rules, otherwise transactions would be rejected by validators. +This would achieve deterministic encoding across different signer implementations while maintaining readability. However, this approach would require all wallet implementations of this sign mode to ensure their JSON encoder complies with the canonical encoding rules; otherwise, transactions would be rejected by validators.🧰 Tools
🪛 LanguageTool
[typographical] ~290-~290: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...er complies with the canonical encoding rules, otherwise transactions would be rejected by valid...(THUS_SENTENCE)
282-282: Add explicit deprecation timelineConsider adding a specific timeline for the deprecation of legacy sign modes, as suggested in previous discussions.
-Finally, all legacy sign modes (`SIGN_MODE_LEGACY_AMINO`, `SIGN_MODE_DIRECT`, `SIGN_MODE_TEXTUAL`) will be deprecated and removed from the SDK. +Finally, all legacy sign modes (`SIGN_MODE_LEGACY_AMINO`, `SIGN_MODE_DIRECT`, `SIGN_MODE_TEXTUAL`) will be marked as deprecated in 2024 and removed from the SDK in 2026.
349-349: Add missing newline at end of fileAdd a newline character at the end of the file to comply with standard formatting practices.
🧰 Tools
🪛 Markdownlint (0.37.0)
349-349: null
Files should end with a single newline character(MD047, single-trailing-newline)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/adr-076-sign-mode-unified.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/architecture/adr-076-sign-mode-unified.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
docs/architecture/adr-076-sign-mode-unified.md
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...encoded version) are sent to the signer in order to be used to decode and show the transact...
(IN_ORDER_TO_PREMIUM)
[typographical] ~290-~290: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...er complies with the canonical encoding rules, otherwise transactions would be rejected by valid...
(THUS_SENTENCE)
🪛 Markdownlint (0.37.0)
docs/architecture/adr-076-sign-mode-unified.md
349-349: null
Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (1)
docs/architecture/adr-076-sign-mode-unified.md (1)
1-1: Fix inconsistency in ADR number
The ADR number in the title (075) doesn't match the filename (076).
-# ADR 075: Sign Mode Unified
+# ADR 076: Sign Mode Unified
Anyone has suggestions or ideas on how multisig can be handled in this signmode ? |
|
guys any thoughts on eze's question? |
Description
ADR for proposing a new sign mode,
SIGN_MODE_UNIFIED, in replacement toSIGN_MODE_TEXTUAL.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
SIGN_MODE_UNIFIED, a new signing mode to simplify the signing process and enhance user experience.SIGN_MODE_UNIFIED.