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.

Sign up for GitHub

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

Jump to bottom

IBC architecture document #70

Merged
merged 37 commits into from
Apr 25, 2019
Merged

IBC architecture document #70

merged 37 commits into from
Apr 25, 2019

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Apr 11, 2019 •
edited
Loading

Update per suggestions from @ancazamfir and @milosevic:

  • Write motivation section
  • Clarify protocol purpose, interfaces, and dataflow
  • Take some inspiration from the TCP specification (here)

Also fix a few links.

Feedback welcome, both on clarity of this writing and any conceptual explanations you think are missing.

@cwgoes cwgoes added tao Transport, authentication, & ordering layer. meta Issues or proposals about the ICS process. wip Issues or pull requests which are in progress. labels Apr 11, 2019
@cwgoes cwgoes removed the wip Issues or pull requests which are in progress. label Apr 15, 2019
@cwgoes cwgoes changed the title Update IBC architecture document IBC architecture document Apr 15, 2019
@cwgoes cwgoes mentioned this pull request Apr 15, 2019
Closed
@cwgoes cwgoes added the ready-for-review Pull requests which are ready for review. label Apr 16, 2019
@cwgoes cwgoes marked this pull request as ready for review April 16, 2019 11:41
@cwgoes cwgoes requested a review from ebuchman April 16, 2019 11:42
vshvsh
vshvsh reviewed Apr 22, 2019
View reviewed changes
docs/ibc/1_IBC_ARCHITECTURE.md Show resolved Hide resolved
docs/ibc/1_IBC_ARCHITECTURE.md Outdated Show resolved Hide resolved
@vshvsh
Copy link

vshvsh commented Apr 22, 2019

The one thing that's not easily understandable from this set of documents is why you presume future IBC topology to be complex and not, say, complete graph: reading how permissionless it is you certainly don't understand where that hub and spoke model in examples comes from.

It's be nice to have list of reasons to limit connections to only well-known well-behaving networks or a discriminating hub - namely expensive on-chain storage, reducing counterparty risk and so on. I'm not sure that speculations on how and why IBC network will shape itself should be in this PR though.

@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 23, 2019

It's be nice to have list of reasons to limit connections to only well-known well-behaving networks or a discriminating hub - namely expensive on-chain storage, reducing counterparty risk and so on. I'm not sure that speculations on how and why IBC network will shape itself should be in this PR though.

I agree that it's worthwhile to reason about future network topologies, but I think it is out of scope of this document and the core protocol specification generally - but it could well be in scope for "example topology" documents and analysis in the future, in this repo if you like - some discussion has started at #59.

@cwgoes cwgoes mentioned this pull request Apr 23, 2019
Closed
@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 24, 2019

I believe all comments have been addressed.

@cwgoes cwgoes requested a review from gamarin2 April 24, 2019 11:37
liamsi
liamsi reviewed Apr 24, 2019
View reviewed changes
Copy link

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

I think this gives a really good overview on the motivation and on IBC 👍

I think the flow diagram (in the Diagram section) could still be clearer (maybe by boxing the different layers similarly to how they are structured under the "Protocol Relations" section.

Other than that: great work!

gamarin2
gamarin2 approved these changes Apr 24, 2019
View reviewed changes
Copy link

@gamarin2 gamarin2 left a comment

Choose a reason for hiding this comment

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

Just a nit, otherwise LGTM

docs/ibc/README.md Outdated Show resolved Hide resolved
gamarin2 and others added 3 commits April 24, 2019 15:02
liamsi
liamsi approved these changes Apr 24, 2019
View reviewed changes
Copy link

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Amazing work!

@cwgoes cwgoes merged commit 11b0a68 into master Apr 25, 2019
@cwgoes cwgoes deleted the cwgoes/ibc-architecture-document-v2 branch April 25, 2019 11:22
@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 25, 2019 •
edited
Loading

Merging now, but more comments / suggestions always welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes left review comments

@milosevic milosevic milosevic approved these changes

@vshvsh vshvsh vshvsh left review comments

@liamsi liamsi liamsi approved these changes

@gamarin2 gamarin2 gamarin2 approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@jackzampolin jackzampolin Awaiting requested review from jackzampolin

@jaekwon jaekwon Awaiting requested review from jaekwon

@mossid mossid Awaiting requested review from mossid

@sunnya97 sunnya97 Awaiting requested review from sunnya97

@zmanian zmanian Awaiting requested review from zmanian

Assignees
No one assigned
Labels
meta Issues or proposals about the ICS process. ready-for-review Pull requests which are ready for review. tao Transport, authentication, & ordering layer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cwgoes @vshvsh @zmanian @melekes @milosevic @liamsi @jackzampolin @gamarin2