Skip to content
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

Merged
merged 10 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/architecture/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ When writing ADRs, follow the same best practices for writing RFCs. When writing
* [ADR 063: Core Module API](./adr-063-core-module-api.md)
* [ADR 067: Simulator v2](./adr-067-simulator-v2.md)
* [ADR 069: `x/gov` modularity, multiple choice and optimisic proposals](./adr-069-gov-improvements.md)
* [ADR 074: Messages with implicit signers](./adr-074-implicit-msg-signers.md)

### Draft

Expand Down
131 changes: 131 additions & 0 deletions docs/architecture/adr-074-implicit-msg-signers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
# ADR 074: Messages with Implicit Signers

## Changelog

* 2024-06-10: Initial draft

## Status

PROPOSED Not Implemented
Copy link
Contributor

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.

- 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.

Suggested change
PROPOSED Not Implemented
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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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 ...

This can be used for both simple inter-module message passing and simpler messages in transactions.

## Context

Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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...

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Committable 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 of Msgs. Originally this was via a GetSigners method on t...

Comment on lines +18 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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 of Msgs. Originally this was via a GetSigners method on t...

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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...

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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 ...

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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...

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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 into x/auth to check if an address belongs to a module or not? ...

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Committable 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...

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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 new MsgV2 standard whereby the signer of the message is im...

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Tools
Markdownlint

64-64: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The 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:
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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...

```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;
Copy link
Member

@kocubinski kocubinski Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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 for MsgV2 instances will need to extract the sender from the `context.Co...

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.

Copy link
Member

Choose a reason for hiding this comment

The 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

Two possible directions that have been proposed are:
1. allowing for the omission of the `cosmos.msg.v2.is_msg` option and assuming any `Msg`s registered that do not include `cosmos.msg.v1.signer` are `MsgV2` instances. The pitfall is that this could be incorrect if `Msg` v1 behavior is actually decided but the user forgot the `cosmos.msg.v1.signer` option.
2. allow `Msg` v1 instances to be wrapped in a `MsgV2` envelope as well to simplify things client-side. In this scenario we would need to either a) check that the signer in the envelope and the signer in the message are the same or b) allow the signer in the message to be empty and then set it inside the state machine before it reaches the module. While this may be easier for some clients, it may introduce unexpected behavior with Ledger signing via Amino JSON or SIGN_MODE_TEXTUAL.

Both of these are seem as quality of life improvements for some users, but not strictly necessary and could have some pitfalls so further discussion is needed.

## References

* [ADR 033](./adr-033-protobuf-inter-module-comm.md)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
* [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

Loading