-
Notifications
You must be signed in to change notification settings - Fork 278
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 qgb module readme #1052
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blocking feedback but I think the links in the doc could use link text to make the markdownlint rule pass
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
db4822c
to
1a75526
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like keeping what used to be in the spec folder just in the main readme a ton!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incorporating the suggested changes earlier, I still have some nit picks tho that I think we should handle before merging. We don't have to use only the suggestions listed here, but I do think we can clean up a few minor things.
x/qgb/README.md
Outdated
|
||
#### Data commitment panics | ||
|
||
During EndBlock, if the block height corresponds to a `DataCommitmentWindow`, it will generate a new data commitment, during which, the state machine can panic, if it finds invalid state, in the following case: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During EndBlock, if the block height corresponds to a `DataCommitmentWindow`, it will generate a new data commitment, during which, the state machine can panic, if it finds invalid state, in the following case: | |
During EndBlock, if the block height corresponds to a `DataCommitmentWindow`, it will generate a new data commitment. Note that the state machine can panic if invalid state is introduced. |
I would also then either remove
- An unexpected behavior happened while getting the current data commitment:
or clarify with more precise details of what could cause that. Like if the last nonce's key is missing or something not sure. Also, if we only have a single entry, I'm not sure we need a list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove the data commitment panic section because currently, the GetDataCommitment
will never return an error => no panics.
x/qgb/README.md
Outdated
|
||
#### Valset panics | ||
|
||
Similar to data commitments, when checking if the state machine needs to generate a new valset, it might panic, if it finds invalid state, in the following cases: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to data commitments, when checking if the state machine needs to generate a new valset, it might panic, if it finds invalid state, in the following cases: | |
Note that, similar to data commitments, the state machine can panic if the required valset is not stored in the state db. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't it make sense to keep the list? we have three cases where the state machine can panic...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my mistake! I didn't see the other list items between the code until I rendered the file 🙈
Overview
Closes #501
Checklist