docs: ADR 074: Msg v2 #20618
docs: ADR 074: Msg v2 #20618
Conversation
WalkthroughA new architectural decision record (ADR 074) has been introduced, proposing the Changes
Sequence Diagram(s)sequenceDiagram
Participant User
Participant SDK Module A
Participant SDK Module B
Participant Authentication Service
User->>SDK Module A: Sends message with credentials
SDK Module A->>Authentication Service: Verify credentials
Authentication Service-->>SDK Module A: Return sender information
SDK Module A->>SDK Module B: Send MsgV2 with implicit signer
SDK Module B->>Authentication Service: Validate implicit signer
Authentication Service-->>SDK Module B: Confirmation
SDK Module B-->>User: Acknowledge message processing
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
docs/architecture/adr-074-implicit-msg-signers.md (1)
108-108: Ensure file ends with a single newline.The file should end with a single newline character to comply with POSIX standards and ensure proper processing by UNIX/Linux utilities.
Tools
Markdownlint
108-108: null (MD047, single-trailing-newline)
Files should end with a single newline characterdocs/architecture/README.md (1)
Line range hint
28-28: Add an article for grammatical correctness.- The spec is much more compressed and streamlined summary of everything as + The spec is a much more compressed and streamlined summary of everything as
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- docs/architecture/README.md (1 hunks)
- docs/architecture/adr-074-implicit-msg-signers.md (1 hunks)
Additional context used
Path-based instructions (2)
docs/architecture/adr-074-implicit-msg-signers.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"docs/architecture/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/architecture/adr-074-implicit-msg-signers.md
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
[uncategorized] ~21-~21: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... aGetSignersmethod on thesdk.Msginterface which returned instances of `sdk.AccAdd...
[typographical] ~30-~30: Consider adding a comma after the introductory adverb. (STILL_COMMA)
Context: ...cy on the global bech32 configuration. Still this design introduces a fair amount of...
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...d consistent way. With embedded message signers there will always need to be a step of ...
[typographical] ~37-~37: At the start of a sentence, a comma is usually required for the expression ‘In any “case,”’. (COMMA_OF_IN_ANY_CASE)
Context: ...formance overhead is introduced. In any case why should an inter-module message pass...
[style] ~40-~40: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...here be a lookup intox/authto check if an address belongs to a module or not? ...
[style] ~42-~42: You can shorten this phrase to improve clarity and avoid wordiness. (NNS_THAT_ARE_JJ)
Context: ... message passing because we do not want an implementation that is overly complex. There are many use cases for inter-mod...
[uncategorized] ~54-~54: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...have decided to introduce a newMsgV2standard whereby the signer of the message is im...
[style] ~74-~74: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_NEED_TO_VB)
Context: ...ing handlers forMsgV2instances will need to extract the sender from the `context.Co...docs/architecture/README.md
[uncategorized] ~28-~28: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ...hitecture of something new. The spec is much more compressed and streamlined summary...
Markdownlint
docs/architecture/adr-074-implicit-msg-signers.md
64-64: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
76-76: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
108-108: null (MD047, single-trailing-newline)
Files should end with a single newline character
Additional comments not posted (1)
docs/architecture/README.md (1)
92-92: Confirm the status of ADR 074.Verification successful
The ADR 074 is correctly referenced in the README and matches the actual ADR file in the repository.
docs/architecture/README.mdcorrectly lists ADR 074.docs/architecture/adr-074-implicit-msg-signers.mdexists and matches the reference.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the status of ADR 074 in the repository. # Test: Search for ADR 074 in the ADR directory. Expect: Status should match the README. rg --type md 'ADR 074' ./docs/architecture/Length of output: 248
| ``` | ||
|
|
||
| Because the `cosmos.msg.v1.signer` annotation is required currently, `MsgV2` types should set the message option | ||
| `cosmos.msg.v2.is_msg` to `true` instead. |
There was a problem hiding this comment.
Wondering if we could find a way for clients to deduct this without parsing options. 🤔
There was a problem hiding this comment.
How would they do that? The other option I can think of would be scoping this to the MsgServer instead of per message.
Nothing would change with existing functionality. But if someone uses MsgV2 there is no custom signer or regular signer in the message. It would always be in the envelope. |
Added an example. In |
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (1)
docs/architecture/adr-074-implicit-msg-signers.md (1)
9-9: Clarify the status of the ADR.The status "PROPOSED Not Implemented" might be confusing. Consider using a more standard term like "PROPOSED" or "DRAFT" to clearly indicate the current state of the ADR.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/architecture/adr-074-implicit-msg-signers.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/architecture/adr-074-implicit-msg-signers.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/architecture/adr-074-implicit-msg-signers.md
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
[uncategorized] ~21-~21: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... aGetSignersmethod on thesdk.Msginterface which returned instances of `sdk.AccAdd...
[typographical] ~30-~30: Consider adding a comma after the introductory adverb. (STILL_COMMA)
Context: ...cy on the global bech32 configuration. Still this design introduces a fair amount of...
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...d consistent way. With embedded message signers there will always need to be a step of ...
[typographical] ~37-~37: At the start of a sentence, a comma is usually required for the expression ‘In any “case,”’. (COMMA_OF_IN_ANY_CASE)
Context: ...formance overhead is introduced. In any case why should an inter-module message pass...
[style] ~40-~40: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...here be a lookup intox/authto check if an address belongs to a module or not? ...
[style] ~42-~42: You can shorten this phrase to improve clarity and avoid wordiness. (NNS_THAT_ARE_JJ)
Context: ... message passing because we do not want an implementation that is overly complex. There are many use cases for inter-mod...
[uncategorized] ~54-~54: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...have decided to introduce a newMsgV2standard whereby the signer of the message is im...
[misspelling] ~74-~74: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’. (EN_A_VS_AN)
Context: ...ead. Here is an example comparing a v1 an v2 message: ```protobuf // v1 message M...
[style] ~92-~92: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_NEED_TO_VB)
Context: ...ing handlers forMsgV2instances will need to extract the sender from the `context.Co...
Markdownlint
docs/architecture/adr-074-implicit-msg-signers.md
64-64: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
75-75: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
94-94: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
126-126: null (MD047, single-trailing-newline)
Files should end with a single newline character
| This can be used for both simple inter-module message passing and simpler messages in transactions. | ||
|
|
||
| ## Context | ||
|
|
There was a problem hiding this comment.
Add a comma for clarity.
- Historically operations in the SDK have been modelled with the `sdk.Msg` interface and
+ Historically, operations in the SDK have been modelled with the `sdk.Msg` interface andCommittable 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.
| Historically, operations in the SDK have been modelled with the `sdk.Msg` interface and |
Tools
LanguageTool
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
| ## Context | ||
|
|
||
| Historically operations in the SDK have been modelled with the `sdk.Msg` interface and | ||
| the account signing the message has to be explicitly extracted from the body of `Msg`s. |
There was a problem hiding this comment.
Add a comma for clarity.
- Originally this was via a `GetSigners` method on the `sdk.Msg` interface which returned
+ Originally, this was via a `GetSigners` method on the `sdk.Msg` interface which returnedCommittable suggestion was skipped due to low confidence.
Tools
LanguageTool
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
|
|
||
| * Allows for a simple inter-module communication design which can be used soon for the `bank` `v2` redesign. | ||
| * Allows for simpler client implementations for messages in the future. | ||
|
|
There was a problem hiding this comment.
the framework doesnt need to decode the entire message in order to get the signer now. We could do things in which we decode only the first field since we know the location every time
|
Are we good to merge this? Any other updates or reviews needed? |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
docs/architecture/README.md (1)
Line range hint
28-28: Consider adding the article "a" before "much more compressed and streamlined summary."- The spec is much more compressed and streamlined summary of everything as it stands today. + The spec is a much more compressed and streamlined summary of everything as it stands today.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/architecture/README.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/architecture/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/architecture/README.md
[uncategorized] ~28-~28: You might be missing the article “a” here. (AI_EN_LECTOR_MISSING_DETERMINER_A)
Context: ...hitecture of something new. The spec is much more compressed and streamlined summary...
Additional comments not posted (1)
docs/architecture/README.md (1)
93-93: Ensure the link to ADR 074 is functional and the description matches the PR summary.Verification successful
The link to ADR 074 is functional, and the description "Messages with implicit signers" matches the content of the ADR document.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the functionality of the link to ADR 074 and its consistency with the PR summary. # Test: Check if the file `adr-074-implicit-msg-signers.md` exists. fd --type file 'adr-074-implicit-msg-signers.md'Length of output: 98
Script:
#!/bin/bash # Description: Verify the content of `adr-074-implicit-msg-signers.md` to ensure it matches the description in the README. # Display the first few lines of the ADR 074 document to check its title and summary. head -n 20 docs/architecture/adr-074-implicit-msg-signers.mdLength of output: 676
yup yup, pr owner has full ownership to merge after approvals. New policy. reviewers should try to avoid merging on behalf of the owner |
| ```protobuf | ||
| // v1 | ||
| message MsgSendV1 { | ||
| string from_address = 1 [(cosmos.msg.v1.signer) = true]; |
There was a problem hiding this comment.
Does this work? cosmos.msg.v1.signer is a message annotation of type string, not a field annotation of type bool right
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/architecture/adr-074-implicit-msg-signers.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/architecture/adr-074-implicit-msg-signers.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/architecture/adr-074-implicit-msg-signers.md
[uncategorized] ~14-~14: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...he credentials of the party sending it, and unlike the current design not part of t...
[uncategorized] ~14-~14: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...arty sending it, and unlike the current design not part of the message body. This can ...
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
[typographical] ~30-~30: Consider adding a comma after the introductory adverb. (STILL_COMMA)
Context: ...cy on the global bech32 configuration. Still this design introduces a fair amount of...
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...d consistent way. With embedded message signers there will always need to be a step of ...
[typographical] ~37-~37: At the start of a sentence, a comma is usually required for the expression ‘In any “case,”’. (COMMA_OF_IN_ANY_CASE)
Context: ...formance overhead is introduced. In any case why should an inter-module message pass...
[style] ~40-~40: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...here be a lookup intox/authto check if an address belongs to a module or not? ...
[style] ~42-~42: You can shorten this phrase to improve clarity and avoid wordiness. (NNS_THAT_ARE_JJ)
Context: ... message passing because we do not want an implementation that is overly complex. There are many use cases for inter-mod...
[uncategorized] ~54-~54: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...have decided to introduce a newMsgV2standard whereby the signer of the message is im...
[uncategorized] ~74-~74: “an” (indefinite article before a vowel sound) seems less likely than “and” (in addition to, following this). (AI_HYDRA_LEO_CP_AN_AND)
Context: ...ead. Here is an example comparing a v1 an v2 message: ```protobuf // v1 message M...
[style] ~92-~92: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_NEED_TO_VB)
Context: ...ing handlers forMsgV2instances will need to extract the sender from the `context.Co...
Markdownlint
docs/architecture/adr-074-implicit-msg-signers.md
64-64: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
75-75: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
94-94: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
126-126: null (MD047, single-trailing-newline)
Files should end with a single newline character
Additional comments not posted (5)
docs/architecture/adr-074-implicit-msg-signers.md (5)
9-9: Clarify the ADR's current status.The status "PROPOSED Not Implemented" might be confusing. Consider using a more standard term like "PROPOSED" or "DRAFT" to clearly indicate the current state of the ADR.
18-18: Add a comma after 'Historically' for better readability.- Historically operations in the SDK have been modelled with the `sdk.Msg` interface and + Historically, operations in the SDK have been modelled with the `sdk.Msg` interface andTools
LanguageTool
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
20-20: Insert a comma after 'Originally' for better readability.- Originally this was via a `GetSigners` method on the `sdk.Msg` interface which returned + Originally, this was via a `GetSigners` method on the `sdk.Msg` interface which returnedTools
LanguageTool
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
30-30: Add a comma after 'Still' for clarity.- Still this design introduces a fair amount of complexity. + Still, this design introduces a fair amount of complexity.Tools
LanguageTool
[typographical] ~30-~30: Consider adding a comma after the introductory adverb. (STILL_COMMA)
Context: ...cy on the global bech32 configuration. Still this design introduces a fair amount of...
37-37: Add a comma after 'In any case' for clarity.- In any case why should an inter-module message passing system need to do so much conversion, parsing, etc. just to check if a message + In any case, why should an inter-module message passing system need to do so much conversion, parsing, etc. just to check if a messageTools
LanguageTool
[typographical] ~37-~37: At the start of a sentence, a comma is usually required for the expression ‘In any “case,”’. (COMMA_OF_IN_ANY_CASE)
Context: ...formance overhead is introduced. In any case why should an inter-module message pass...
|
|
||
| // v2 | ||
| message MsgSendV2 { | ||
| option (cosmos.msg.v2.is_msg) = true; |
There was a problem hiding this comment.
Intuitively I think there is a logic path whereby absence of cosmos.msg.v1.signer is permissible if we're in a msg v2 code path, in other words assume there has been an envelope.
That would allow for removing the requirement of yet another annotation cosmos.msg.v2.is_msg on messages.
There was a problem hiding this comment.
Actionable comments posted: 13
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/architecture/adr-074-implicit-msg-signers.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/architecture/adr-074-implicit-msg-signers.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/architecture/adr-074-implicit-msg-signers.md
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
[typographical] ~30-~30: Consider adding a comma after the introductory adverb. (STILL_COMMA)
Context: ...cy on the global bech32 configuration. Still this design introduces a fair amount of...
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...d consistent way. With embedded message signers there will always need to be a step of ...
[typographical] ~37-~37: At the start of a sentence, a comma is usually required for the expression ‘In any “case,”’. (COMMA_OF_IN_ANY_CASE)
Context: ...formance overhead is introduced. In any case why should an inter-module message pass...
[style] ~40-~40: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...here be a lookup intox/authto check if an address belongs to a module or not? ...
[style] ~42-~42: You can shorten this phrase to improve clarity and avoid wordiness. (NNS_THAT_ARE_JJ)
Context: ... message passing because we do not want an implementation that is overly complex. There are many use cases for inter-mod...
[uncategorized] ~54-~54: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...have decided to introduce a newMsgV2standard whereby the signer of the message is im...
[uncategorized] ~74-~74: “an” (indefinite article before a vowel sound) seems less likely than “and” (in addition to, following this). (AI_HYDRA_LEO_CP_AN_AND)
Context: ...ead. Here is an example comparing a v1 an v2 message: ```protobuf // v1 message M...
[style] ~93-~93: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_NEED_TO_VB)
Context: ...ing handlers forMsgV2instances will need to extract the sender from the `context.Co...
Markdownlint
docs/architecture/adr-074-implicit-msg-signers.md
64-64: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
75-75: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
95-95: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
127-127: null (MD047, single-trailing-newline)
Files should end with a single newline character
|
|
||
| In the case of messages passed externally in transactions, `MsgV2` instances will need to be wrapped | ||
| in a `MsgV2` envelope: | ||
| ```protobuf |
There was a problem hiding this comment.
Ensure that fenced code blocks are surrounded by blank lines for proper formatting.
+
message MsgV2 {
string signer = 1;
google.protobuf.Any msg = 2;
}
+
+
// v1
message MsgSendV1 {
option (cosmos.msg.v1.signer) = "from_address";
string from_address = 1 ;
string to_address = 2;
repeated Coin amount = 3;
}
+
+
type GetSenderService interface {
GetSender(ctx context.Context) []byte
}
+ Also applies to: 75-75, 95-95
Tools
Markdownlint
64-64: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
|
|
||
| ## Status | ||
|
|
||
| PROPOSED Not Implemented |
There was a problem hiding this comment.
Please consider revising the status description for clarity.
- PROPOSED Not Implemented
+ PROPOSED (Not Implemented)This change clarifies that the ADR is proposed but not yet implemented.
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.
| PROPOSED Not Implemented | |
| PROPOSED (Not Implemented) |
|
|
||
| ## Decision | ||
|
|
||
| We have decided to introduce a new `MsgV2` standard whereby the signer of the message is implied |
There was a problem hiding this comment.
Consider adding a comma after 'standard'.
- We have decided to introduce a new `MsgV2` standard whereby the signer of the message is implied
+ We have decided to introduce a new `MsgV2` standard, whereby the signer of the message is impliedCommittable 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.
| We have decided to introduce a new `MsgV2` standard whereby the signer of the message is implied | |
| We have decided to introduce a new `MsgV2` standard, whereby the signer of the message is implied |
Tools
LanguageTool
[uncategorized] ~54-~54: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...have decided to introduce a newMsgV2standard whereby the signer of the message is im...
|
|
||
| ## References | ||
|
|
||
| * [ADR 033](./adr-033-protobuf-inter-module-comm.md) |
There was a problem hiding this comment.
Ensure the file ends with a single newline character.
+ 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 033](./adr-033-protobuf-inter-module-comm.md) | |
| * [ADR 033](./adr-033-protobuf-inter-module-comm.md) | |
Tools
Markdownlint
127-127: null (MD047, single-trailing-newline)
Files should end with a single newline character
|
Added some comments on further discussions from our call and going ahead with merging. |
There was a problem hiding this comment.
Actionable comments posted: 9
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/architecture/adr-074-implicit-msg-signers.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/architecture/adr-074-implicit-msg-signers.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/architecture/adr-074-implicit-msg-signers.md
[uncategorized] ~14-~14: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...he credentials of the party sending it, and unlike the current design not part of t...
[uncategorized] ~14-~14: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...arty sending it, and unlike the current design not part of the message body. This can ...
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
[typographical] ~30-~30: Consider adding a comma after the introductory adverb. (STILL_COMMA)
Context: ...cy on the global bech32 configuration. Still this design introduces a fair amount of...
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...d consistent way. With embedded message signers there will always need to be a step of ...
[typographical] ~37-~37: At the start of a sentence, a comma is usually required for the expression ‘In any “case,”’. (COMMA_OF_IN_ANY_CASE)
Context: ...formance overhead is introduced. In any case why should an inter-module message pass...
[style] ~40-~40: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...here be a lookup intox/authto check if an address belongs to a module or not? ...
[style] ~42-~42: You can shorten this phrase to improve clarity and avoid wordiness. (NNS_THAT_ARE_JJ)
Context: ... message passing because we do not want an implementation that is overly complex. There are many use cases for inter-mod...
[misspelling] ~74-~74: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’. (EN_A_VS_AN)
Context: ...ead. Here is an example comparing a v1 an v2 message: ```protobuf // v1 message M...
[style] ~93-~93: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_NEED_TO_VB)
Context: ...ing handlers forMsgV2instances will need to extract the sender from the `context.Co...
[uncategorized] ~124-~124: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...ifMsgv1 behavior is actually decided but the user forgot the `cosmos.msg.v1.sign...
[typographical] ~125-~125: It appears that a comma is missing. (DURING_THAT_TIME_COMMA)
Context: ...to simplify things client-side. In this scenario we would need to either a) check that t...
[grammar] ~127-~127: Did you mean “seemed” or “seeming”? (BE_VB_OR_NN)
Context: ... SIGN_MODE_TEXTUAL. Both of these are seem as quality of life improvements for som...
[uncategorized] ~127-~127: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...y necessary and could have some pitfalls so further discussion is needed. ## Refer...
Markdownlint
docs/architecture/adr-074-implicit-msg-signers.md
125-125: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
64-64: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
75-75: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
95-95: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
124-124: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
131-131: null (MD047, single-trailing-newline)
Files should end with a single newline character
Additional comments not posted (2)
docs/architecture/adr-074-implicit-msg-signers.md (2)
9-9: Clarify the status of the ADR for better understanding.- PROPOSED Not Implemented + PROPOSED (Not Implemented)This change clarifies that the ADR is proposed but not yet implemented.
Likely invalid or redundant comment.
54-54: Consider adding a comma after 'standard'.- We have decided to introduce a new `MsgV2` standard whereby the signer of the message is implied + We have decided to introduce a new `MsgV2` standard, whereby the signer of the message is impliedLikely invalid or redundant comment.
| defining who the signer of a message was and its implementation in the `x/tx` module which | ||
| extracts signers dynamically and allowed removing the dependency on the global bech32 | ||
| configuration. | ||
|
|
There was a problem hiding this comment.
Add a comma after the introductory adverb.
- Still this design introduces a fair amount of complexity.
+ Still, this design introduces a fair amount of complexity.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.
| Still, this design introduces a fair amount of complexity. |
Tools
LanguageTool
[typographical] ~30-~30: Consider adding a comma after the introductory adverb. (STILL_COMMA)
Context: ...cy on the global bech32 configuration. Still this design introduces a fair amount of...
| ## Abstract | ||
|
|
||
| This ADR introduces a new `MsgV2` standard where the signer of the message is implied by the | ||
| credentials of the party sending it, and unlike the current design not part of the message body. |
There was a problem hiding this comment.
Add a comma for improved readability and clarity.
- credentials of the party sending it, and unlike the current design not part of the message body.
+ credentials of the party sending it, and unlike the current design, not part of the message body.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.
| credentials of the party sending it, and unlike the current design not part of the message body. | |
| credentials of the party sending it, and unlike the current design, not part of the message body. |
Tools
LanguageTool
[uncategorized] ~14-~14: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...he credentials of the party sending it, and unlike the current design not part of t...
[uncategorized] ~14-~14: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...arty sending it, and unlike the current design not part of the message body. This can ...
| messages in a performant and consistent way. With embedded message signers there will always need | ||
| to be a step of extracting the signer and then checking with the module sending is actually | ||
| authorized to perform the operation. With dynamic signer extraction, although the system is | ||
| more consistent, more performance overhead is introduced. In any case why should an inter-module |
There was a problem hiding this comment.
Add a comma after "In any case".
- In any case why should an inter-module message passing system need to do so much conversion, parsing, etc. just to check if a message
+ In any case, why should an inter-module message passing system need to do so much conversion, parsing, etc. just to check if a messageCommittable 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.
| more consistent, more performance overhead is introduced. In any case why should an inter-module | |
| more consistent, more performance overhead is introduced. In any case, why should an inter-module |
Tools
LanguageTool
[typographical] ~37-~37: At the start of a sentence, a comma is usually required for the expression ‘In any “case,”’. (COMMA_OF_IN_ANY_CASE)
Context: ...formance overhead is introduced. In any case why should an inter-module message pass...
| is authenticated? In addition, we have the complexity where modules can actually have many valid | ||
| addresses. How are we to accommodate this? Should there be a lookup into `x/auth` to check if an | ||
| address belongs to a module or not? All of these thorny questions are delaying the delivery of | ||
| inter-module message passing because we do not want an implementation that is overly complex. |
There was a problem hiding this comment.
Consider rephrasing to avoid redundancy.
- There are many use cases for inter-module message passing which are still relevant, the most
+ There are many relevant use cases for inter-module message passing, the mostCommittable suggestion was skipped due to low confidence.
Tools
LanguageTool
[style] ~42-~42: You can shorten this phrase to improve clarity and avoid wordiness. (NNS_THAT_ARE_JJ)
Context: ... message passing because we do not want an implementation that is overly complex. There are many use cases for inter-mod...
| more consistent, more performance overhead is introduced. In any case why should an inter-module | ||
| message passing system need to do so much conversion, parsing, etc. just to check if a message | ||
| is authenticated? In addition, we have the complexity where modules can actually have many valid | ||
| addresses. How are we to accommodate this? Should there be a lookup into `x/auth` to check if an |
There was a problem hiding this comment.
Consider using 'whether' instead of 'if' for formal writing.
- Should there be a lookup into `x/auth` to check if an address belongs to a module or not?
+ Should there be a lookup into `x/auth` to check whether an address belongs to a module or not?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.
| addresses. How are we to accommodate this? Should there be a lookup into `x/auth` to check if an | |
| addresses. How are we to accommodate this? Should there be a lookup into `x/auth` to check whether an |
Tools
LanguageTool
[style] ~40-~40: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...here be a lookup intox/authto check if an address belongs to a module or not? ...
| Still this design introduces a fair amount of complexity. For instance, inter-module message | ||
| passing ([ADR 033](./adr-033-protobuf-inter-module-comm.md)) has been in discussion for years | ||
| without much progress and one of the main blockers is figuring out how to properly authenticate | ||
| messages in a performant and consistent way. With embedded message signers there will always need |
There was a problem hiding this comment.
Add a comma for clarity.
- With embedded message signers there will always need
+ With embedded message signers, there will always needCommittable 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.
| messages in a performant and consistent way. With embedded message signers there will always need | |
| messages in a performant and consistent way. With embedded message signers, there will always need |
Tools
LanguageTool
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...d consistent way. With embedded message signers there will always need to be a step of ...
| Because the `cosmos.msg.v1.signer` annotation is required currently, `MsgV2` types should set the message option | ||
| `cosmos.msg.v2.is_msg` to `true` instead. | ||
|
|
||
| Here is an example comparing a v1 an v2 message: |
There was a problem hiding this comment.
Correct the typo to improve clarity.
- Here is an example comparing a v1 an v2 message:
+ Here is an example comparing a v1 and v2 message: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.
| Here is an example comparing a v1 an v2 message: | |
| Here is an example comparing a v1 and v2 message: |
Tools
LanguageTool
[misspelling] ~74-~74: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’. (EN_A_VS_AN)
Context: ...ead. Here is an example comparing a v1 an v2 message: ```protobuf // v1 message M...
|
|
||
| Historically operations in the SDK have been modelled with the `sdk.Msg` interface and | ||
| the account signing the message has to be explicitly extracted from the body of `Msg`s. |
There was a problem hiding this comment.
Add commas after introductory adverbs for better readability.
- Historically operations in the SDK have been modelled with the `sdk.Msg` interface and
+ Historically, operations in the SDK have been modelled with the `sdk.Msg` interface and
- Originally this was via a `GetSigners` method on the `sdk.Msg` interface which returned
+ Originally, this was via a `GetSigners` method on the `sdk.Msg` interface which returnedCommittable 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.
| Historically operations in the SDK have been modelled with the `sdk.Msg` interface and | |
| the account signing the message has to be explicitly extracted from the body of `Msg`s. | |
| Historically, operations in the SDK have been modelled with the `sdk.Msg` interface and | |
| the account signing the message has to be explicitly extracted from the body of `Msg`s. |
Tools
LanguageTool
[formatting] ~18-~18: Consider adding a comma after ‘Historically’ for more clarity. (CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
Context: ... messages in transactions. ## Context Historically operations in the SDK have been modelle...
[typographical] ~20-~20: Consider adding a comma after ‘Originally’ for more clarity. (RB_LY_COMMA)
Context: ...itly extracted from the body ofMsgs. Originally this was via aGetSignersmethod on t...
| } | ||
| ``` | ||
|
|
||
| Modules defining handlers for `MsgV2` instances will need to extract the sender from the `context.Context` that is |
There was a problem hiding this comment.
Avoid repetitive phrasing for better readability.
- Modules defining handlers for `MsgV2` instances will need to extract the sender from the `context.Context` that is
+ Modules defining handlers for `MsgV2` instances must extract the sender from the `context.Context` that isCommittable 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.
| Modules defining handlers for `MsgV2` instances will need to extract the sender from the `context.Context` that is | |
| Modules defining handlers for `MsgV2` instances must extract the sender from the `context.Context` that is |
Tools
LanguageTool
[style] ~93-~93: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_NEED_TO_VB)
Context: ...ing handlers forMsgV2instances will need to extract the sender from the `context.Co...
* main: feat(tx): port simappv2 changes (#20648) build(deps): Bump github.com/spf13/cobra from 1.8.0 to 1.8.1 (#20682) chore(proto): fix comments (#20679) (serverv2/cometbft) Commands support both json and yaml output (#20670) fix(x/staking,x/auth): regenerate mock to fix test (#20684) docs: ADR 074: Msg v2 (#20618) fix: nested multisig signatures using CLI (#20438) chore: fix spelling errors (#20674) fix: align Dockerfile for build-push-action (#20672) fix: avoid build fail when make localnet-build-env (#20671) build(deps): Bump bufbuild/buf-setup-action from 1.32.2 to 1.33.0 (#20669) chore: make function comment match function names (#20666) chore(consensus): add cometInfo to consensus (#20615) chore: fix typos (#20662) fix: Properly parse json in the wait-tx command. (#20631) fix(sims): check before sending RotateConsPubKey (#20659) test(types/address): add unit tests for the file types/address.go (#20237)
Description
This ADR introduces a new
MsgV2standard where the signer of the message is implied by thecredentials of the party sending it, and unlike the current design not part of the message body.
This can be used for both simple inter-module message passing and simpler messages in transactions.
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
MsgV2standard for message passing and authentication, simplifying inter-module communication within the SDK.