# Regen Cosmos SDK Architecture Review

**Date:** Fri May 7, 2021 4pm CET / 10am ET / 7am PT
**Zoom Link:** https://us02web.zoom.us/j/86444484091?pwd=T2tabFhqaFVSYkwyM0cycmdITjJCdz09
**Attendees:**

## Agenda Items

Preliminary agenda is generated from issues tagged with the github label ["Status: Needs Architecture Review"](https://github.com/cosmos/cosmos-sdk/issues?q=is%3Aissue+is%3Aopen+label%3A%22Status%3A+Needs+Architecture+Review%22). New items are encouraged to be added prior to the start of the meeting.

(10minutes each)
- [x] Change sdk.Msg.GetSigners() return type to []string [#9239](https://github.com/cosmos/cosmos-sdk/issues/9239)
- [x] Specify & implement SIGN_MODE_TEXTUAL [#6513](https://github.com/cosmos/cosmos-sdk/issues/6513)
- [x] Custom Protobuf Service code generator [#8270](https://github.com/cosmos/cosmos-sdk/issues/8270)
- [ ] Add ADR033 Router [#9238](https://github.com/cosmos/cosmos-sdk/issues/9238)
- [ ] Implement ADR032: Typed Events [#7563](https://github.com/cosmos/cosmos-sdk/issues/7563) (only if marko is present)
- [ ] Lets go through all ongoing ADR and try to advance them? (10min)
- [ ] Review the Keyring migration [#7108](https://github.com/cosmos/cosmos-sdk/issues/7108)

## Notes
- Change sdk.Msg.GetSigners() return type to []string (signer addresses)
- Aaron: When we switched to proto, we initially switched to address as bytes, but this caused problems as in many places it would default encode as base64
- As a result we switched to strings for encoding addresses (bringing bech32 in the protocol layer). So universally in proto msg's, "Signers" are strings, but in the GetSigners() method, it returns a type of AccAddress.
- Currently to do this, we need pass in the global bech32 prefix (which we're trying to deprecate)
- My current thinking is to put global bech32 prefixes into the antehandler
- Bez: So you want to move the validation from the stateless check into the antehanlder right?
- I don't think there's any other option really if we don't want to rely on a global config
- Aaron: The alternative is you add the configuration to GetSigners..
- Bez: I'm in line with this proposal, thinks it makes sense
- Shahan: Being new to the SDK, From what i've seen- when the antehandler is modified there's a sequence of parameters to the antehandler already
- Bez: There's a series of decorators (middleware), and you can essentially construct an antehandler with whatever things you need
- Shahan: When dealing w/ atomic swaps or multichain addresses (where initial prefix would be different), wouldn't passing a single parameter to the antehandler be more difficult than passing the context?
- Aaron: I don't think you'll ever have the antehandler processing different bech32 prefixes. Any msg's coming from other chains would be getting validated through IBC or elsewhere
- Bez: The only cons are that you don't know you have a malformed transaction (from bad address) until it gets to CheckTx
- Aaron: But you could just have hte client check GetSigners
- Robert: Signers check is not the only way that a transaction can be malformed

- Specify & implement SIGN_MODE_TEXTUAL
- Aaron: Last time we had this discussion, we said we were going to use protobuf JSON, which seemed like the best short term solution for Ledger integration
- In this issue, there is a complex problem with JSON encoding for Duration & Timestamps (which is not deterministic)
- The specification says that fractional digits can be used
- As a result, different encoders may use it differently
- There's a few approaches that we could consider:
1. We disallow duration & timestamp and always use int64 (then in JSON you just see int64 and this defeats the nice UX of seeing timestamp in the JSON)
2. Another approach is to use a custom JSON serializer using reflection that takes protobuf definitions
3. Look through the contents and make sure any Duration has 9 digits... but this seems not great
- Bez: Don't we rely on a canonical protobuf serializer?
- Aaron: Yes, but that works on arbitrary JSON and doesn't recognize specific schemas
- Bez: So we would need to add a check to see if its Timestamp or Duration then we would need to canonicalize it
- Amaury: We already have ADR027, which is s a canonical serializer for binary, so in my opinion this is OK
- Cory: Is it important for CosmJS folks to input into this conversation?
- Aaron: Potentially, but maybe moreso important to bring in the Ledger team?
- Aaron: In some ways i see no way around this in the long term. Without some sortof static analyzer that prevents it, any chain could theoretically still use Duration & TImestamp themselves
- Bez: I think doing a bit of reflection in the serializer is OK
- Amaury: Me to, I agree with this
- Robert: Maybe going too far down this route might create too much complexity
- All the serialization that will be in the signing will affect all libraries, so we should really aim for simplicity.

- Custom Protobuf Service Codegenerator
- Tyler: There were a few things-
- Context refactor being one
- Adding a new invoker
- Aaron: So the changes to context is just simply adding a few methods to sdk.Context to make it implement Context.Context
- sdk.Context already contains a Context.Context underneath, and this is just about adding a few methods (hopefully not super controversial)
- What's the issue w/ the Invoker?
- Tyler: I think it's just that there's other changes, not necessary issues
- Aaron: You and i can probably sync on this, but i think the only change is that the sdk.Context should implement Context.Context
- Add ADR033 Router #9238
- Marie: This is more or less related to the group module which is using ADR33 and the new ADR33 module wiring in regen ledger
- We need to make a decision on that before we can merge the group module into the SDK
- Right now in regen ledger we have 2 types of module wiring (regular SDK one, new ADR33 one)
- We'd like to merge them into a single module wiring and doing this we could try to simplify the app.go wiring (which is quite a huge file at the moment)
- We need to discuss a few things on this design in particular
- Aaron: I'm wondering- maybe somebody could share screen just for some of the examples we have (in the discussion)
- ~~ screen sharing & further discussion ~~
- Aaron: There are 2 high level goals in #9182
1. Make ap wiring much simpler
2. Make app wiring completely declaritive (and therefore app configuration mutable by governance)
- Aaron:
- Maybe we could talk about creating a group on this just within our team would be good?
-

## Follow-ups
- #9239 can proceed as proposed (returning []string signer addresses)
-