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

Documentation update #340

Closed
wants to merge 4 commits into from

Conversation

jkilpatr
Copy link
Contributor

@jkilpatr jkilpatr commented May 4, 2021

This pr depends on #327 as well as a as of yet unopened pr for validator set rewards.

This dramatically increases the documentation scope as well as adds a top level index for the readme.

jkilpatr added 2 commits May 4, 2021 16:25
This brings this spec in line with the current state of what's actually
implemmented and it's nuances.
Copy link
Contributor

@jtremback jtremback left a comment

Choose a reason for hiding this comment

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

This documentation is not in the format that we are using, and much of the documentation of messages is not detailed enough. However, we may be able to extract some good stuff, for example the bootstrapping documentation. I recommend this not be merged, and instead be used as source material for the ongoing documentation efforts in the module spec folder

@jkilpatr
Copy link
Contributor Author

jkilpatr commented May 5, 2021

This documentation is not in the format that we are using, and much of the documentation of messages is not detailed enough. However, we may be able to extract some good stuff, for example the bootstrapping documentation. I recommend this not be merged, and instead be used as source material for the ongoing documentation efforts in the module spec folder

This documentation is objectively more complete than the current set, what's the timeline for 'properly formated' documentation? if it's more than a week why are we deciding that not having up to date documentation is better for that time period than having 'poorly formatted' documentation?

@@ -0,0 +1,89 @@
# Gravity messages
Copy link
Member

Choose a reason for hiding this comment

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

This is contained within the module/spec. When we go to render the doucmentation, the spec in the module will be used. Can we merge any information from here into there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is the spec being worked on? I'll compare them on review.

docs/design/mint-lock.md Outdated Show resolved Hide resolved

This is identical to Ethereum -> Cosmos for Ethereum based assets. Please see that section and refer to the above section for details on how the overall process differs for Cosmos assets.

## Relaying rewards
Copy link
Member

Choose a reason for hiding this comment

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

should this be added into the rewards section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's intended to be the EThereum -> Cosmos flow for Cosmos based assets but I wasn't sure how much duplication was really needed.

@@ -0,0 +1,81 @@
# Gravity parameters
Copy link
Member

@tac0turtle tac0turtle May 6, 2021

Choose a reason for hiding this comment

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

This is already within the spec, why was it added here?
Please merge this into there

Copy link
Contributor Author

@jkilpatr jkilpatr May 6, 2021

Choose a reason for hiding this comment

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

ah I see it. Not open at the time of this PR's creation.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/cosmos/gravity-bridge/blob/main/module/x/gravity/spec/07_params.md

this was created a while ago. It lists the params but does not explain them.

@tac0turtle
Copy link
Member

tac0turtle commented May 6, 2021

Love the initiative, but it would be nice to collaborate more on how to structure things. There is already a spec folder in the module/x/gravity module. Lets merge what we can into there and keep the additional docs here.

Nit: it would be nice to get some diagrams.

jkilpatr added 2 commits May 6, 2021 14:42
This patch overhauls the documentation to cover a lot more info and
provide a lot more details on different subjects
This clarifies where the 5% number comes from in validator set
creation, also notes that this should probably be a parameter.
@jkilpatr jkilpatr force-pushed the jkilpatr/more-docs-rebased-2 branch from 4065bb3 to 438e8c3 Compare May 6, 2021 18:43
@okwme okwme closed this Aug 27, 2021
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.

4 participants