-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: ADR 074: Msg v2 #20618
docs: ADR 074: Msg v2 #20618
Changes from 7 commits
3d4fa1b
40d185c
27d32f1
3dbf951
36ea153
0570bf7
5baa208
7251368
07bb232
5699eb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,127 @@ | ||||||||||||||
# ADR 074: Messages with Implicit Signers | ||||||||||||||
|
||||||||||||||
## Changelog | ||||||||||||||
|
||||||||||||||
* 2024-06-10: Initial draft | ||||||||||||||
|
||||||||||||||
## Status | ||||||||||||||
|
||||||||||||||
PROPOSED Not Implemented | ||||||||||||||
|
||||||||||||||
## 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
ToolsLanguageTool
|
||||||||||||||
This can be used for both simple inter-module message passing and simpler messages in transactions. | ||||||||||||||
|
||||||||||||||
## Context | ||||||||||||||
|
||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 and Committable suggestion
Suggested change
ToolsLanguageTool
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 returned
ToolsLanguageTool
Comment on lines
+18
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 returned Committable suggestion
Suggested change
ToolsLanguageTool
|
||||||||||||||
Originally this was via a `GetSigners` method on the `sdk.Msg` interface which returned | ||||||||||||||
instances of `sdk.AccAddress` which itself relied on a global variable for decoding | ||||||||||||||
the addresses from bech32 strings. This was a messy situation. In addition, the implementation | ||||||||||||||
for `GetSigners` was different for each `Msg` type and clients would need to do a custom | ||||||||||||||
implementation for each `Msg` type. These were improved somewhat with the introduction of | ||||||||||||||
the `cosmos.msg.v1.signer` protobuf option which allowed for a more standardised way of | ||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
ToolsLanguageTool
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comma for clarity. - With embedded message signers there will always need
+ With embedded message signers, there will always need Committable suggestion
Suggested change
ToolsLanguageTool
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 message Committable suggestion
Suggested change
ToolsLanguageTool
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
ToolsLanguageTool
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 most
ToolsLanguageTool
|
||||||||||||||
There are many use cases for inter-module message passing which are still relevant, the most | ||||||||||||||
immediate of which is a more robust denom management system in `x/bank` `v2` which is being explored | ||||||||||||||
in [ADR 071](https://github.com/cosmos/cosmos-sdk/pull/20316). | ||||||||||||||
|
||||||||||||||
## Alternatives | ||||||||||||||
|
||||||||||||||
Alternatives that have been considered are extending the current `x/tx` signer extraction system | ||||||||||||||
to inter-module message passing as defined in [ADR 033](./adr-033-protobuf-inter-module-comm.md). | ||||||||||||||
|
||||||||||||||
## 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 implied Committable suggestion
Suggested change
ToolsLanguageTool
|
||||||||||||||
by the credentials of the party sending it. These messages will be distinct from the existing messages | ||||||||||||||
and define new semantics with the understanding that signers are implicit. | ||||||||||||||
|
||||||||||||||
In the case of messages passed internally by a module or `x/account` instance, the signer of a message | ||||||||||||||
will simply be the main root address of the module or account sending the message. An interface for | ||||||||||||||
safely passing such messages to the message router will need to be defined. | ||||||||||||||
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ToolsMarkdownlint
|
||||||||||||||
message MsgV2 { | ||||||||||||||
string signer = 1; | ||||||||||||||
google.protobuf.Any msg = 2; | ||||||||||||||
} | ||||||||||||||
``` | ||||||||||||||
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if we could find a way for clients to deduct this without parsing options. 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would they do that? The other option I can think of would be scoping this to the MsgServer instead of per message. |
||||||||||||||
|
||||||||||||||
Here is an example comparing a v1 an v2 message: | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
ToolsLanguageTool
|
||||||||||||||
```protobuf | ||||||||||||||
// v1 | ||||||||||||||
message MsgSendV1 { | ||||||||||||||
option (cosmos.msg.v1.signer) = "from_address"; | ||||||||||||||
string from_address = 1 ; | ||||||||||||||
string to_address = 2; | ||||||||||||||
repeated Coin amount = 3; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// v2 | ||||||||||||||
message MsgSendV2 { | ||||||||||||||
option (cosmos.msg.v2.is_msg) = true; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intuitively I think there is a logic path whereby absence of That would allow for removing the requirement of yet another annotation |
||||||||||||||
// from address is implied by the signer | ||||||||||||||
string to_address = 1; | ||||||||||||||
repeated Coin amount = 2; | ||||||||||||||
} | ||||||||||||||
``` | ||||||||||||||
|
||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 is Committable suggestion
Suggested change
ToolsLanguageTool
|
||||||||||||||
passed in. An interface in `core` which will be present on the `appmodule.Environment` will be defined for this purpose: | ||||||||||||||
```go | ||||||||||||||
type GetSenderService interface { | ||||||||||||||
GetSender(ctx context.Context) []byte | ||||||||||||||
} | ||||||||||||||
``` | ||||||||||||||
|
||||||||||||||
Sender addresses that are returned by the service will be simple `[]byte` slices and any bech32 conversion will be | ||||||||||||||
done by the framework. | ||||||||||||||
|
||||||||||||||
## Consequences | ||||||||||||||
|
||||||||||||||
### Backwards Compatibility | ||||||||||||||
|
||||||||||||||
This design does not depreciate the existing method of embedded signers in `Msg`s and is totally compatible with it. | ||||||||||||||
|
||||||||||||||
### Positive | ||||||||||||||
|
||||||||||||||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||
### Negative | ||||||||||||||
|
||||||||||||||
* There will be two message designs and developers will need to pick between them. | ||||||||||||||
|
||||||||||||||
### Neutral | ||||||||||||||
|
||||||||||||||
## Further Discussions | ||||||||||||||
|
||||||||||||||
Further discussions should take place on GitHub. | ||||||||||||||
|
||||||||||||||
## References | ||||||||||||||
|
||||||||||||||
* [ADR 033](./adr-033-protobuf-inter-module-comm.md) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure the file ends with a single newline character. + Committable suggestion
Suggested change
ToolsMarkdownlint
|
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.
Please consider revising the status description for clarity.
This change clarifies that the ADR is proposed but not yet implemented.
Committable suggestion