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

Create Repository Structure #1

Merged
merged 15 commits into from
Mar 30, 2023

Conversation

bd21
Copy link
Contributor

@bd21 bd21 commented Mar 17, 2023

No description provided.

Copy link

@chatton chatton left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! I just a had a few comments/questions

I see .DS_Store is being included, perhaps we should include a .gitignore file with this as an entry.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link

@womensrights womensrights left a comment

Choose a reason for hiding this comment

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

Thank you for doing all this work on the repo structure. I have left some comments and suggestions.

- A release and tag for _App A_ ought not be blocked on changes in other unrelated or upstream apps
- Version management must be able to be handled independently. i.e. An _App A_ can upgrade to `ibc-go v8` and release a tag against it, while _App B_ may remain unsupported for `v8`.
- Teams with general write access to the repo should not be authorized to write to apps that they do not maintain (only default branch/tags/etc). Of course, PRs welcome :-)

Choose a reason for hiding this comment

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

Will it then be up to the specific app developers to communicate the versioning and compatibility of their apps with ibc-go releases or should there be some information collated for all apps in the repo? i.e. as a user, is there an easy way for me to come to this repo and see which apps will work with which release

Choose a reason for hiding this comment

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

Would it be helpful if this issue is implemented for ibc-go v8.0.0? cc @damiannolan

Copy link
Member

Choose a reason for hiding this comment

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

Will it then be up to the specific app developers to communicate the versioning and compatibility of their apps with ibc-go releases or should there be some information collated for all apps in the repo? i.e. as a user, is there an easy way for me to come to this repo and see which apps will work with which release

This is a great call out. This will need to be coordinated and collated for the repo, such as with a version matrix on the README. But we can keep the release cycles separate for different ibc-apps similar to how the Cosmos SDK repo does for different modules.



## New contributer approval process
- [ ] Submit a Github issue titled "I should be a maintainer because..."

Choose a reason for hiding this comment

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

I think it would be good to give a bit of an indication of why someone would be granted write access. Some thoughts I have:

  • Actively developing an ibc app or middleware that is / will be in this repo
  • Has previously made a substantial contribution to an app housed in the repo and plans to contribute more going forwards
  • Already has write access to an app that will move into the repo


IBC Modules are self-contained applications that enable packets to be sent to and received from other IBC-enabled chains. IBC application developers do not need to concern themselves with the low-level details of clients, connections, and proof verification.

IBC Middleware are self-contained modules that sit between core IBC and an underlying IBC application. This allows developers to customize lower-level packet handling. Multiple middleware modules can be chained together.

Choose a reason for hiding this comment

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

Maybe link to the middleware spec?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the spec? Would it not make more sense to point to ibc-go middleware docs?
Ditto for apps btw

Copy link
Member

Choose a reason for hiding this comment

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

Agreed the docs are probably more user friendly

An [example IBC app](./examples/hello-world/)


## List of Apps

Choose a reason for hiding this comment

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

Maybe this should go earlier in the read me? Not a big point but I can imagine if you already know about ibc you might just want to know which apps and middleware are in the repo asap

🌌🌌🌌🌌 How to Use this repo
=============================

If you'd like to include software in this repo, please see [contributing](../ibc-apps/CONTRIBUTING.md).

Choose a reason for hiding this comment

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

Should there be some information here about how to actually import the modules in the repo? Users would be not just contributors but chain devs

@@ -1,2 +1,64 @@
# ibc-apps
IBC applications and middleware for Cosmos SDK chains

![IBC-APPS Header](ibc-apps.png)

Choose a reason for hiding this comment

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

This is just a general comment which probably needs a bit more discussion - do we want their to be a feeling of coherence between ibc-apps and ibc-go? Right now the readme's have a different flow of content and different descriptions of ibc for example. If we want a unified feel, I would suggest we align on this and that isn't to say ibc-go just sets the tone but we can adapt both in parallel?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should aim for a unified feel in terms of readme, documentation etc. Could be good to have teams sync on this given that the README revamp for ibc-go is still ongoing. cc @crodriguezvega

🌌🌌🌌 What is it?
==================

### What is IBC?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we explain this here? Feel like one sentence about IBC and one about ibc-go would be enough with references for further prerequisite readings if the reader needs it. Given the comment above wrt coherency between ibc-go and ibc-apps, how about we use this:

The Inter-Blockchain Communication protocol (IBC) allows blockchains to talk to each other. This end-to-end, connection-oriented, stateful protocol provides reliable, ordered, and authenticated communication between heterogeneous blockchains.

This IBC implementation in Golang is built as a Cosmos SDK module. To understand more about how to use the ibc-go module as well as about the IBC protocol, please check out the Interchain Developer Academy section on IBC, or our docs."

Could be rephrased but likely good to have something similar in both repos


IBC applications and middleware for Cosmos SDK blockchains

🌌 Why have an ibc-apps repo?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This paragraph and the next talk about IBC and ibc-go without first defining them, how about we swap the order?

  1. What is IBC? (short and succinct)
  2. Why have an ibc-apps repo?
  3. Who's it for?
  4. How to use this repo?
  5. Bonus content

@bd21
Copy link
Contributor Author

bd21 commented Mar 27, 2023

Thanks for putting this together! I just a had a few comments/questions

I see .DS_Store is being included, perhaps we should include a .gitignore file with this as an entry.

Should be removed now

@bd21
Copy link
Contributor Author

bd21 commented Mar 27, 2023

hey @tmsdkeys @womensrights @chatton, I've granted you write access to the feature branch. I don't have any more strong opinions about the layout or wording of this repository. Feel free to edit as needed - let's merge this tomorrow on Tuesday.

Invitaiton: https://github.com/bd21/ibc-apps/invitations

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Ack on the general structure


IBC Modules are self-contained applications that enable packets to be sent to and received from other IBC-enabled chains. IBC application developers do not need to concern themselves with the low-level details of clients, connections, and proof verification.

IBC Middleware are self-contained modules that sit between core IBC and an underlying IBC application. This allows developers to customize lower-level packet handling. Multiple middleware modules can be chained together.
Copy link
Member

Choose a reason for hiding this comment

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

Agreed the docs are probably more user friendly

Copy link
Member

Choose a reason for hiding this comment

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

Should we call this directory apps instead of modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the differentiation between modules vs. middleware, both of which are Apps

Choose a reason for hiding this comment

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

What about having the base apps and the middleware both under an apps directory? What top level directories will be shared between the apps?

@agouin
Copy link
Member

agouin commented Mar 30, 2023

Merging this so we can move forward with bootstrapping the repo. Please open new PRs if necessary for unresolved comments.

@agouin agouin merged commit 8e9967c into cosmos:main Mar 30, 2023
Copy link

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks for getting this work started!


## New contributer approval process
- [ ] Submit a Github issue titled "I should be a maintainer because..."
- [ ] After approval, write privileges will be granted to a member of an external team.

Choose a reason for hiding this comment

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

Is this the team who opened the issue?

## New contributer approval process
- [ ] Submit a Github issue titled "I should be a maintainer because..."
- [ ] After approval, write privileges will be granted to a member of an external team.
- [ ] Merging PRs will require approval from more than one team

Choose a reason for hiding this comment

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

What's the intention behind this requirement? Trying to gauge what issue it is trying to solve

- [ ] After approval, write privileges will be granted to a member of an external team.
- [ ] Merging PRs will require approval from more than one team

Privileges will be revoked in case of failure to comply with the [Code of Conduct](../CODE_OF_CONDUCT.md)

Choose a reason for hiding this comment

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

Who makes this judgement? In my experience, violations of code of conduct are usually not so straight forward and hard to enforce when no one is given the authority to make the judgement

Choose a reason for hiding this comment

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

What about having the base apps and the middleware both under an apps directory? What top level directories will be shared between the apps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants