ADR 033: Inter-Module Communication

[aaronc]

ref: #7093 #7421

This ADR introduces a system for permissioned inter-module communication leveraging the protobuf Query and Msg service definitions defined in ADR 021 and ADR 031 which provides:

• stable module interfaces to eventually replace the keeper paradigm based on protobuf
• stronger inter-module object capabilities guarantees
• module accounts and sub-account authorization (Multiple Accounts for ModuleAccount #4657)

It is intended serve as a foundational piece in the roadmap towards Cosmos SDK v1.0.

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[robert-zaremba]

Quick review with few ideas. In the section below, I'm describing a paradigm for inter-module communication rather than specification details.

Protobuf Msg Services and hiding Keepers

I really like the Protobuf Msg Services (ADR 31) concept and limit / remove the support for keepers. Keeper is a module primitive which works on the data level, without getting a bigger context. I don't see any benefits of exposing it outside, only disadvantages:

• hacks with different keeper interfaces
• difficulties with creating permissions

Essentially there is no way how we can protect a module without hiding keepers. So, ideally I would see a keeper as a Data Access Layer object, which would imply that keepers shouldn't call other services (note: if this make sense then we should update the examples).

Encapsulation

When thinking about modules, I'm considering them as independent blocks. More like a smart-contracts. With that in mind, modules should have a well defined interface, which will enable the communication with the modules.

For authorization we need to know:

1. transaction context (signer, etc...)
2. who is calling the service method (eg other module)

The former one can be easily passed as a call chain. For the latter one we have few options: module key (as described in a draft) or using a runtime information (I'm describing it below).

Extracting a call stack from the runtime

All popular languages allow to get a call stack and inspect who is calling the service. Using a runtime information removes a burden of passing and verifying module keys (at the end of the day this keys are available to the developer, so it's only about the developer culture to use them correctly).

On the other hand usage of module key allows module execution on different machines.

[aaronc]

Extracting a call stack from the runtime

All popular languages allow to get a call stack and inspect who is calling the service. Using a runtime information removes a burden of passing and verifying module keys (at the end of the day this keys are available to the developer, so it's only about the developer culture to use them correctly).

I see what you're getting at here @robert-zaremba and it might be the most secure way of doing ocaps. I imagine you're thinking of runtime.Callers. While maybe there's a way we could make this work, I imagine it might be somewhat error prone and hacky in its own way. If you have some more concrete idea of how this would work though, maybe it's work exploring...

The ModuleKey approach doesn't guarantee complete encapsulation of course, but I think it can guarantee encapsulation up to and including the usage of reflection, just not for any code that actually manipulates the stack which I think could be sniffed out pretty easily with some linting tools. There is maybe no perfect encapsulation without static analysis I'd say. The more important short-term goal in my mind is a stable inter-module interface which will allow many different modules to evolve somewhat independently.

[robert-zaremba]

I see what you're getting at here @robert-zaremba and it might be the most secure way of doing ocaps. I imagine you're thinking of runtime.Callers. While maybe there's a way we could make this work, I imagine it might be somewhat error prone and hacky in its own way. If you have some more concrete idea of how this would work though, maybe it's work exploring...

It shouldn't be very hacky, but some runtime inspection will be required. This is how it could work:

1. callstack = get_callstack()
2. module_pkgs = get_current_module_pkgs()  // list of current modle packages 
                                            // each module have to declare it by itself
3. pkg := strip_top(callstack, module_pkgs) // pops packages from the callstack until reaching a package 
                                // which is outside of the current module and protobuf staff and cosmos_core.
				// cosmos_core is a list of cosmos-sdk packages not being modules
4. if ! pkg_authorized?(pkg) 
	return

[aaronc]

@robert-zaremba so if something like that were to work I don't think it's incompatible with module keys. Modules will need reference to something at a minimum to get their module account address. And they will need something for ClientConn to use the generated clients (unless you're suggesting something completely different).

That said I do think the call stack approach would be more complex and error prone to implement and doesn't provide any security guarantees over the module key approach because it is vulnerable to stack manipulation as well. Basically if an attacker is going to use stack manipulation they can just bypass the call stack check you're proposing altogether. So I would conclude that the only defense against that is static analysis. The best runtime defense we can get is against reflection attacks.

[robert-zaremba]

Yes, it's more error prone. On the other hand it's more convenient, because we don't need ModuleKey. We don't need to authorize the module using a signature. If module needs an address, then the module can create it. The app developer can decide that a module will have different addresses, and use a bank module to balance assets between them.

At the end of the day, everything is in the power of the developer. IIUC, developer can manipulate the keys if he wants - he can use the same key in different modules. No need to hack the runtime.

[aaronc]

Yes, it's more error prone. On the other hand it's more convenient, because we don't need ModuleKey. We don't need to authorize the module using a signature. If module needs an address, then the module can create it. The app developer can decide that a module will have different addresses, and use a bank module to balance assets between them.

But in this model modules can't create addresses at will. The app assigns the module addresses and that's what it gets. So module keys seem necessary for that.

At the end of the day, everything is in the power of the developer. IIUC, developer can manipulate the keys if he wants - he can use the same key in different modules. No need to hack the runtime.

An app developer could hack module keys so that two modules use the same key if they wanted. But a module developer couldn't. This system creates that sort of isolation. It's important to distinguish between the two types of developers - module and app.

[aaronc]

Based on discussions in #8134 (specifically #8134 (comment)), I think it is likely better to make this design primarily asynchronous to support future parallel execution models.

Here's what I'm thinking:

• queries would be synchronous but would only read the last block's committed state - we would need to make sure this is explicitly clear
• msg's are always asynchronous and could possibly include a "continuation" where the caller is called again after the callee returns. (I'm not really seeing a use case for asynchronous queries with continuations currently...)

[aaronc]

• msg's are always asynchronous and could possibly include a "continuation" where the caller is called again after the callee returns. (I'm not really seeing a use case for asynchronous queries with continuations currently...)

This approach won't really quite work, but I did a bit of a spike on a parallel design and came up with something that I think will (#8134 (reply in thread)). I am currently leaning to moving in that direction.

[PaddyMc]

Has a decision been made wrt to Inter-Module Communication and this ADR?

[alessio]

Has a decision been made wrt to Inter-Module Communication and this ADR?

ADR committee (@aaronc @alexanderbez me): are we positive with this? Can we just approve this and move on, please?

[aaronc]

Thanks @alessio. I think it makes sense to move forward with this for now. A parallel approach could be complementary and opt-in when we get there.

[alessio]

A parallel approach could be complementary and opt-in when we get there.

Music to my ears! :) @alexanderbez are you OK with merging this?

[aaronc]

@robert-zaremba ?

[alexanderbez]

It's been sometime since I thoroughly reviewed this. I recall having some questions and just overall misunderstandings on some of the things this is really trying to solve. In addition, I think this might be too complicated and more engineering than what we might need. Perhaps if I spent more time really digging into it that would change...

That being said, a considerable amount of thought, time and discussion has went into this from many people and I think it has a strong foundation, so I'm giving my 👍

[alessio]

Habemus consensus. @robert-zaremba mind lifting your block, please?

[robert-zaremba]

I'm in favor of it from the beginning, I requested changes related to cosmetic updates, typos and adding more details. Before merging, let's:

• consider suggestions - there are few typos and style updates.
• maybe we can add a new section: Further Discussion to list related topics (as already listed in some comments).

[robert-zaremba]

So how about creating required clients in the constructor and storing them as a private fields in the structure? It has few benefits:

• we clearly signal the dependencies
• we don't recreate the objects (caching).

[robert-zaremba]

Maybe we can "deeper reuse of protobuf generated code"? I don't think it's negative, sounds more like neutral or positive.

[robert-zaremba]

Why it was merged without checking / answering the comments?

[alessio]

Why it was merged without checking / answering the comments?

The ADR committee approved it, thus it makes no sense to keep it open. Changes can be carried out in ensuing PRs.
If you have concerns please mail @aaronc @alexanderbez @alessio - we'll respond in a timely fashion. Thanks!

[clevinson]

@robert-zaremba sorry about that. I had forgotten about your last comments, and was mostly acting on the fact that it had sufficient approvals

[robert-zaremba]

@clevinson I created #8676 with cosmetic changes to improve (hopefully) reading of this ADR.