###### tags: `Cosmos SDK`

# SDK Architecture Review - 2021-06-04

The **SDK Architecture Review** meeting is a bi-weekly meeting focused on architecture discussions of ADRs and features currently in development (any issues tagged with the label "Status: Needs Architecture Review").

## Agenda

- [x] Working Group Checkin
- https://github.com/cosmos/cosmos-sdk/wiki/Architecture-Design-Process
- [x] Add support for transaction tips
- https://github.com/cosmos/cosmos-sdk/issues/9406
- [x] Standardize Bank Module Funds Movement Events
- https://github.com/cosmos/cosmos-sdk/issues/9375
- [x] Add ADR-043 NFT Module
- https://github.com/cosmos/cosmos-sdk/pull/9329
- https://github.com/cosmos/cosmos-sdk/pull/9455
- [ ] adr-40: use prefix store instead of multistore #9355
- https://github.com/cosmos/cosmos-sdk/pull/9355/files
- low-level SC access: https://github.com/cosmos/cosmos-sdk/pull/9451

## Notes

- Working Group Checkin
- We have 6 different WGs that have started, and let's start with quick updates from each of these groups
- Consensus fees:
- Aaron: Probably not an update from this group from the past week. There's some relevant stuff that will be addressed later in the arch call
- Robert: At Regen we've been discussing a proposal to do something similar to what the WG had initially discussed (min fee governed by governance). Main efforts for the WG are around mempool prioritization
- Gov/Group Module Alignment:
- Marie: We have setup a meeting to biweekly. We've decided to focus on use cases so it can inform the design & architecture we use. Ongoing repo from Calum @ Internchain, which we should probably link in the discussion
- Aaron: In the last meeting we talked about doing a two phase approach. We have a design that is our more refined design, but in hte midterm we should do the minimal integration btw the gov & group module.
- Module Wiring:
- With Aaron we are working on two proof of concepts. One based on reflection mechanism and uberwig library? Other is based on codegeneration and another library.
- Neither of these are a redesign of how hte SDK works, but htey do improve radically how an app is built, in the way code boilerplate can be drastically reduced without major changes in module construction
- x/auth x/bank vesting redesign
- Nothing new this week
- Proto v2 Migration & Codegen
- Marko yesterday found a library that can be used for fast marshalling (https://github.com/planetscale/vtprotobuf)
- Tyler is starting to work on plugin work
- Offline Signatures
- Robert: I've messaged frodji but still waiting to hear back.
- Add Support for Transaction Tips #9406
- Aaron: There's a request to support basic transaction tips as a way to support multidenom fees
- The alternative is if we had a price feed we could accept fees at a known fee amount
- There's complexity in doing that conversion with a price feed
- The tip mechanism allows another transaction relayer / submitter pay for hte fees in the native fee token of the chain. It's then up to the transaction submitter to calculate if its sufficient for them
- The design is to have `SIGN_MODE_DIRECT_AUX` and `SIGN_MODE_TEXTUAL_AUX` which is for signers that are not necessarily signing over the fee
- They're not signing over the auth info, just signing over the tip if they are the tip payer
- The signer who is paying hte fee would use sign mode direct and sign over the whole auth info
- This is a pretty high priority feature for Hub & Gravity Dex/Bridge
- Charly: THere's an ICS general fee payment spec on hte IBC repo, and it deal with fees but more from the relayer side
- Aaron: I haven't had a chance to look at that spec
- Charly: Even if its just for the UX of the user, it would probably be good to align on strategy for both of these
- In the ICS model, there's two payments (on receive packet, and on ACK)
- Aaron: Sounds like these two are compatible, but we should still ensure there's alignment
- Charly: Maybe we should have a dedicated session to discuss these
- Aaron: How about we schedule a follow-up, including myself and someone from AiB working on meta-transactions
- Charly: I'll set up a chat
- Standardize Bank Module Funds Movement Events
- Tyler: One of his proposals was to get rid of "on coin sent / on coin received" and just emit a single transfer event
- Marko: With state streaming, do we need events for every change? I'm a bit hesitant to overload events with a lot of data. If there's a lot of want for overloading events and state streaming doesn't meet the needs, then this would be fine.
- ADR-043 NFT Module
- Haifeng: I have spent some time in the past couple days reading through all the comments in the ADR, and I just left a comment 3 hrs ago in repsonse to Aaron, about some confusion in the usage of hte data field in the NFT struct
- I sense a worry in Aaron's response of an incomplete module that doesn't add as much concrete functionality
- The iris NFT module is a module setup with a working implementation that supports most of the similar ERC721 standard, with the exception of transfer functionlaity
- Maybe we should focus the conversation in improving the irismod NFT module conversation
- In the beginning I thought there were 2 motivations that drived the ADR:
- Cosmos SDK / Cosmos Ecosystem is in need of a basic working NFT module
- Zaki commented on wanting a very simple base NFT that handles storage of NFT objects and this should be compatible with IBC to make transfering NFTs across chains easier
- He even suggested different levels:
- Base NFT / Collectibles / etc.
- We basically from iristeam tried to create a specification for a base NFT. It was only based on feedback that we added transfer capability
- Haifeng: I sense that what aaron is looking for is something that works out of hte box
- If that's the direction to go, then maybe we should formalize what's already in that module so its not just storage capabilities
- Aaron: Thanks Haifeng. I see a PR for "implement NFT Module". Is that related to this ADR?
- Haifeng: Yes it is. It has a proto definition containign hte 3 fields, and a keeper implementing hte transfer function, and the query capabilities.
- Aaron: I think its not totally true that it doesn't have minting & burning. In the implementation you have a "SetNFT" function.
- The current pattern that keepers are designed with is soemthing we want to be careful about.
- In Auth we have a "SetAccount" method, which isn't very good at encapsulation. This means that any module that gets referenced to the auth keeper could call this and create new accounts.
- In this keeper there is a SetNFT module, which means any module htat has keeper access would have full control.
- I think we should be giving more specific control to how modules have control
- Aaron: My suggestion is that we think of the bank module as the core module for tracking ownership of assets, and an NFT in that case is simply a denom which has a fixed supply of 1. If we do add supply that NFT then we could
- If we went to that approach, the main thing we would need to add is some kind of metadata field
- Cory: So how would it work to have querying of multiple denoms?
- Aaron: We could query by denom prefix to get more data around an NFT class
- Ira: We had a module that actually clearly separated the use case of franctionalizing versus NFT type use cases
- When something is a coin or a denom we wanted all the compatibility of the bank module
- As long as you have some concept of ownership that's clear, as this is what I would look for if i was trying to adopt or build an NFT in my own chain.
- Haifeng: I understand the need for fractionalized NFTs, but they do have EIPs as a "one ERC for one problem" model.
- It would not be hard to come up with an NFT that comes up w/ fractional fungibility. If we have one small standard, we can iterate on it.
- If we want to create an NFT taht will cover all these use cases then it will become too complicated
- Robert: First to this comparison to module & smart contracts. These aren't the same. Cosmos modules are more powerful, as you create the protocol. Ethereum you are not able to create the protocol. Good example is how authz operates at the protocol layer.
- To second Aaron's case for tracking all tokens with the bank module: If you use bank module to send tokens, we automatically have support for all the tools. It will change everything for free.
- I agree that NFT (as the name suggests) is not about fractionalization. We don't need to provide fracitnoalization at the module level, as another module can implement the fractionalization
- Marko: So it sounds like Aaron you're proposing using Bank for all minting & burning of other modules. In my eyes as of right now, we should see how to get the first version into the SDK and iterate ontop of that (like we've done with other modules).
- There's not a need to refactor anything right now, but we could also do a migration script once all of that is ready.
- Charly: It seems to me that there are almost two conversations that don't have to happen at the same time. Basic NFT conversation doesn't need to happen at the same time at this bank module.
- Aaron: I hear the concern that there's a timeline. In terms of the design of this refactor- I've already outlined what this refactor does. THe hard thing about refactoring the bank module is vesting accounts. Maybe there's a way to do the denom refactor without dealing w/ vesting accounts... The complexity is not in defining hte interface with other modules, its more that the bank module has an interplay with auth module.
- I'm also not saying the bank module has to do all that this NFT module does. I just don't think that we should have a SetNFT module.
- My main concern is that we go through a design of a core SDK module with core control over which other modules are able to do what things. I know from the context of other conversations, having some rules around transfer is desirable.
- Haifeng: Is it scoping / restrictions that you mean when you speak of rules?
- Aaron: This is another big difference btw Eth & Cosmos, Eth has all the authorization built in to smart contracts.
- Haifeng: When I think about the NFT transfers, I think that the most important rule is that only the owner of hte NFT can transfer
- Ira (message): On Aaron's comments about DenomManager ... that is a function that Provenance has through its Marker module which controls mint,burn, etc roles for a denom and also manages the send enable flag. These permissions can be assigned to other addresses or a smart contract for mint/burn/and a facilitated transfer option that is permissioned.
- Marko: This to me sounds like we need to understand how we securely write modules
- Aaron: We do have an approach that we mapped out with intermodule access.
- Marko: When would we be able to get that in the SDK?
- Aaron: Moar broadly, why are we wanting to introduce this to the SDK before we have a clean API available for it?
- Marko: I agree that we shouldn't be putting too much burdon on maintainers
- Robert: It seems like ideally from the group like this it would be good to understand the difficulty of integrating these with bank

## Follow Up

...