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: add ADR 057 App Wiring Part I #11873

Merged
merged 14 commits into from
Jul 21, 2022
Merged

docs: add ADR 057 App Wiring Part I #11873

merged 14 commits into from
Jul 21, 2022

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented May 4, 2022

Description

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label May 4, 2022
@alexanderbez alexanderbez self-assigned this May 4, 2022
@aaronc aaronc changed the title docs: add ADR 057 App Wiring docs: add ADR 057 App Wiring Part I May 4, 2022
@aaronc aaronc marked this pull request as ready for review May 4, 2022 18:03
@aaronc aaronc requested a review from a team as a code owner May 4, 2022 18:03
@ValarDragon
Copy link
Contributor

ValarDragon commented May 4, 2022

Awesome work on writing this up, and for all the excellent thought that went into this! I think it'll be a huge win for Cosmos!

My attempt at breaking down the changes: Feels to me like there are 5 changes / decisions happening here:

  • Simplifying order specification.
  • bundling module, store key and keeper together. Alias it all under a single name ("staking") to get the replacability in configurations we depend upon
  • make modules expect other modules by common name. (Rather than typed positional arguments), or otherwise framing of dependency injection
  • using yaml specification layer

I don't know what the current rollout plan is, but I also feel like the native go instantiation can be massively improved in incremental ways using the same ideas.

  • orderings can easily be made to have an api like what's here. We just move beginblock / end block code to being optional for a module, and decide on whether we want them included by default (and then have partial ord api) or must be specified to be included.
  • absolutely can bundle modulename and keeper, and automate the store key passing rather easily (just always make a kvstore, transient and memstore key)
  • Dependency injection (or otherwise automating the wiring of modules via common name) is awesome! I think we can do this in the native golang side as well. I am unclear in the YAML format, how we imagine 'name overriding'. E.g. in osmosis the staking keeper thats passed into gov, is actually the superfluid keeper. Whereas the staking keeper passed in elsewhere, is the "true" staking keeper. We do something similar around bech32ibc and bank. I need to read more into the container package that was built, to understand if allows for this sort of overriding!
  • My primary concern with the yaml approach is in how this lands. I don't see how we iteratively get to the desired state, vs just one massive breaking update, that requires updating every existing module, ecosystem wide. This also why I'd like to keep improving the golang side using these ideas, as then we can get module updates more iteratively

@ValarDragon
Copy link
Contributor

ValarDragon commented May 4, 2022

Also presumably as part of this, for special keepers like Gov or Params, where we want each module to register parameters / proposals directly, we have to do one of:

  • Make all module bundles have an optional interface they can implement, specifying their parameters / proposals
  • Make gov proposals / gov parameters a native concept at the YAML config level

I imagine we're going in the former direction? But either direction we want to go, that seems like another component that can be independently spun out / opt-in applied to existing go code. (Giving governance / params an ability to iterate over every module and check if that module registers things they care about. Whether that be through run-time interface casting, dependency injection, or something in the YML config)

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

TBH I really don't have strong feelings on this one way or another as the contents in this ADR are all pretty abstract. Is there a demo or something I can look at to see how this all works and what it looks like?

As long as dev UX becomes easier with little overall cost, I say 👍

@aaronc
Copy link
Member Author

aaronc commented May 5, 2022

  • Dependency injection (or otherwise automating the wiring of modules via common name) is awesome! I think we can do this in the native golang side as well. I am unclear in the YAML format, how we imagine 'name overriding'. E.g. in osmosis the staking keeper thats passed into gov, is actually the superfluid keeper. Whereas the staking keeper passed in elsewhere, is the "true" staking keeper. We do something similar around bech32ibc and bank. I need to read more into the container package that was built, to understand if allows for this sort of overriding!

@ValarDragon can you say a bit more about what you're doing? I'm not sure the current container package allows this, but sounds like a use case we need to accommodate.

  • My primary concern with the yaml approach is in how this lands. I don't see how we iteratively get to the desired state, vs just one massive breaking update, that requires updating every existing module, ecosystem wide. This also why I'd like to keep improving the golang side using these ideas, as then we can get module updates more iteratively

I think incremental migration is a requirement for this to work properly. I will add something in this ADR to that effect and will start spec'ing out some ideas as to how that could work.

@tac0turtle
Copy link
Member

@ValarDragon any chance you can assist with

can you say a bit more about what you're doing? I'm not sure the current container package allows this, but sounds like a use case we need to accommodate.

@tac0turtle
Copy link
Member

do we want to merge this as is then come back to editing based on the many discussions we have had @aaronc

@aaronc
Copy link
Member Author

aaronc commented Jul 18, 2022

That's fine with me

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

merging as is, we will be amending the adr in followup prs.

Many of the above discussions have been held in the community call and in breakout calls

* dependency injection `In` and `Out` structs which support `optional` dependencies
* grouped-dependencies (many-per-container) through the `AutoGroupType` tag interface
* module-scoped dependencies via `ModuleKey`s (where each module gets a unique dependency)
* one-per-module dependencies through the `OnePerModuleType` tag interface
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's better distinguish one-per-module dependencies from module-scoped dependencies by adding more description. The former is module dependency, the latter is app dependency.

to share resources across different versions of the same module which might have a different protobuf package
versions (ex. `cosmos.bank.module.v2.Module`).

An example app config in YAML might look like this:
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we add a note that this is is a potential update, and we focus on the proto based config?

var appConfigYaml []byte

func main() {
app.Run(app.ParseYamlConfig(appConfigYaml))
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should update the example to use go struct?

docs/architecture/adr-057-app-wiring-1.md Show resolved Hide resolved
@robert-zaremba
Copy link
Collaborator

Before merging I suggest to add some todo list of things to be updated in the ADR.

@julienrbrt
Copy link
Member

merging as is, we will be amending the adr in followup prs.

Many of the above discussions have been held in the community call and in breakout calls

I've added a task here for coming back to this in a follow up: #11899.
Putting automerge.

@julienrbrt julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 21, 2022
@mergify mergify bot merged commit f02c28a into main Jul 21, 2022
@mergify mergify bot deleted the aaronc/adr-app-wiring branch July 21, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: ADR An issue or PR relating to an architectural decision record Type: ADR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants