# Regen Cosmos SDK Architecture Review

**Date:** Dec 3, 2020 – 4pm CET / 10am ET / 7am PT
**Zoom Link**: https://us02web.zoom.us/j/86444484091?pwd=T2tabFhqaFVSYkwyM0cycmdITjJCdz09
**Attendees:**

## Agenda Items
- [x] [ADR033](https://github.com/cosmos/cosmos-sdk/pull/7459) in depth discussion & walk through
- the following accepted ADRs are necessary prior context:
- [ADR021](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-021-protobuf-query-encoding.md) Protobuf Query Encoding
- [ADR031](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-031-msg-service.md) Protobuf Msg Services
- [ADR028](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-028-public-key-addresses.md) Public Key Addresses
- [Alternatives analysis](https://hackmd.io/@tendermint/By8gYcJrP), including [RAID analysis](https://hackmd.io/@tendermint/By8gYcJrP#ADR-033-RAID-Analysis), and [impact assessment](https://hackmd.io/@tendermint/By8gYcJrP#Impact-assessment-on-adoption-by-Cosmos-SDK-application-developers) (from Tendermint).

## Notes
- Starting with a code walkthrough of Regen Ledger ADR033 (bank refactor)
- Let's start with proto files
- You would have a query service definition (defined in ADR21)
- You would have a msg service definition (defined in ADR31)
- Bank PoC has a few specific functions:
- CreateDenom
- Mint
- Send
- Move
- etc.
- These query & msg services are already there for any stargate module
- Modules then implement
- QueryServer interface
- For CLI you would implement QueryClient inetrface
- Now going into x/bank
- there's a grpc_query "QueryServer" , and then we can test (`x/bank/keeper/keeper_test.go`)
- All that ADR33 does is say:
- if you're writing a module, with these services already writen (& generated server/client interfaces)
- We just leverage that same interface for modules to communicate with eachother
- Looking at x/ecocredit module
- `module/module.go` looks a bit different now
- `RegisterServices()` takes a configurator that can register Msg & query serviers
- ADR33 allows for declaring dependencies (`RequireServer`), and setting a modulekey
- `x/ecocredit/server/server.go` gets the module key from teh configurator
- Submodule accounts
- In our group module, we have dynamic multisigs
- This involves creating new derived keys
- Questions:
- Frojdi: How are we distinguising btw internal methods and external methods that clients can call?
- Aaron: One option is in the congifurator defining an `InternalServer`, so a module could define explicitly its own internal methods not to be explosed to clients
- The other option would be to use plugins, but that is a bit more involved. The reason for considering plugins is that you could have plugins defined in CosmWasm
- Alessio: We've explored a bit go plugins, but have some security concerns
- the binary and the shared objects then create a larger attack surface
- Aaron: Currently cosmwasm runs in a sandboxed vm, and ideally if you did a cosmwasm native module (with auditing) you could compile it to binary
- Adding more shared modules should be a governance decision, not a default
- PaddyMc: How does this improve the security?
- Aaron: in the bank SendKeeper, you just "set balance"
- there are lots of places where we've exposed `SetBalance` publicly
- In ADR33, the msg system checks "did hte signer sign this message"
- If we're thinking about minting other tokens, we will need to think about permissions for all of this
- There's also the ability to have authorizationmiddleware which says something like
- "This module is an admin module for this method"
- "this can do X only if ibc is in the denom namespace"
- Paddy: it does rely more heavily on protobuffers
- Aaron: We or someone else in the space will have to write our own codegenerator (replacement for gogoproto)
- Marko: With code generation- do you see us as able to easily implement customtypes?
- Aaron: What i'd like to see, is to get rid of Anys in our generated code, and replace them with actual interfaces, in protov2 this is actually possible
- Jack: Having done a few stargate migrations, the module writing patter now is:
- Write a msg & query service, generate types, write the server package, and the rest can be generated
- This developer experience looks like a big improvement
- Reducing hte many different ways that modules access data looks good
- Alessio: I agree that this functionlaity is already here. There doesn't seem to be a strong argument against it. My concern is the moving target of gogoproto.. are we setting ourselves up for another second large architectural change to come?
- One of the motivations for us proposing a slightly different approach (sticking to Go for the time being), was to reduce the amount of large refactors
- Aaron: The thing that could change for module developers here, is that app.go could be a lot more succinct, 100 lines of imports, frequent copy & past
- We could make a decision to not break existing keepers, and add this wiring as an option, and maybe most keeper interfaces don't need to change much
- Jack: This looks like a preview of sdk 1.0... where we start with ADR33, and tehn get into refactoring app.go, maccperms, etc.
- Paddy: PaddyMc the maccperms stuff seems awesome and makes it much cleaner to interact with, but it doesn't directly depend on protobufs
- Aaron: One of the thingswe were thinking of using early on, was CapnProto, which has a memory arena that doesn't need to decode into structs, it can just pass bytes
- Alessio: Roadmap wise, you state that this is an additive change. Would you be happy for us to maintain the alternative way in the longterm (to 1.0?)
- Aaron: Maybe there's a consideration to be made on a module by module basis. If there's significant module changes, there could be a v1 and v2 module...
- We haven't actually migrated params away from Amino, and there are specific things params is doing using reflection
- If we migrated params to proto, we would want to not use reflection... and the answer is that we need a new design for a paramstore, maybe we just have a params-v2? or for Bank, maybe we just have a bank-v2 ?
- Whether we use hte Keeper constructor, or ADR33 bank module wiring, there will need to be changes regardless
- Alessio: Because we see changes coming in the proto landscape, maybe this could be deployed in parallel with a pure go option?
- Aaron: for folks who want to stick to go, they shouldn't be using msg services & query services for clients. If people want to have a pure amino SDK... we can evaluate it
- Alessio: I can assure you that there is not.
- Aaron: The examples that ive seen with pure go, reuqire a lot of boilerplate and plumming, i'm happy to look at it
- The downside is you lose the ability for CosmWasm to be a first class citizen
- Aaron: This is not grpc, its "protobuf services". The documentation in the protospec documents that there are a number of alternative implementations of protobuf services. We're just saying "this is a DSL for defining an RPC".
- Jack: One thing Zaki and i discussed recently is that staking module, minting, etc. haven't been touched in a while
- In the leadup to 1.0 it would be nice to audit all of those modules, refactoring them into this code pattern seems to be a good way to do that
- The developer experience given in app.go based on this is a quantum leap in terms of DevX
- Aaron: One part of wiring worth calling out--
- These keepers currently all require getting subspaces for params, that need to be wired by module
- In ADR33 you give each module its own paramspace when configuring, and the rest works by default
- Frodji: i think the dependency here is more with on the grpc interface, where the go context is expected
- Aaron writing a codegenerating is not that difficult, and i've already done the work there
- Few other things that are in our form of gogoproto: castrepeated, some thing with oneOfs, would like to get rid of Any's in the future
- Marko: If we're able to separate whats in the struct vs what's on teh generated sidecode, we could separate these out as a separate sideprocess
- Frojdi: About hte auth layer - I think its great, but is it required really, if we can't provide virtualized execution?
- Aaron: There's always a certain risk that a module creates a RNG...
- Aaron: I would argue this is not overblown if you want CosmWasm calling a lot of these methods. There are cases set up for things where CosmWasm
- Frojdi: One last question for gogoproto itself, my question is- would it kill a little the purpose of having protobuf if we have clients in different languages?
- Jack: As long as we have service definition files, each language can choose how to generate those idiomatically, and its up to us when implementing to determine what that compatibility looks like
- Frojdi: So that means other protogenerators would need to implement their own executable to create code which is compatible with the SDK
- Jack: as long as request & response struct types are correct, everything else doesn't matter
- Jack: In peggy we're working on a Rust grpc client to migrate the peggy codebase, and we're able to use the proto files in the SDK
- Ilker: I am a pretty new SDk developer, and like to thin from that perspective of new SDK developers. If we decide to move to something else, this kind of brings new changes to model API again, right?
- Jack: I think particularly for starport, this is going to make things easier for you. This is going to make it easier for you to generate scaffolding. As someone who has been working against tip of the SDK for 2 years, yes there seems like no end of changes to the SDK. But it seems like this is the 1.0 of hte SDK, the form it will take in a few years, and they do make doing that code generation stuff there.
- Aaron: We can try to avoid breaking changes for go interfaces. One of the core reasons for using protobufs as a format, is that it is a serialization format that allows for backwards compatibility in a much more seamless way.

## Follow-ups
- Folks are asked to write revised reflections directly on https://github.com/cosmos/cosmos-sdk/pull/7459