docs: add QGB verify command docs #1287

rach-id · 2023-01-24T17:05:42Z

Overview

We could add more documentation after executing the command to give an example output, but I guess it's better to do it after we have mocked QGB deployed so that we have the same output as users.

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

x/qgb/README.md

rootulp · 2023-01-24T17:45:38Z

x/qgb/README.md

+
+It currently supports three sub-commands:
+
+- `msg`: Takes a transaction hash, in hex format, and verifies that the message that has been posted using it has been committed to, by the QGB contract. It only supports one message for now.


[question] how is this different from the tx subcommand? Does it verify that the blob shares associated with a BlobTx were committed to? If so should this use blob instead of msg?

the tx verifies that the transaction carrying the blob was committed to, in the tx namespace.

If so should this use blob instead of msg

100%, thanks a lot for spotting these. I prepared these PRs before the renaming and I missed renaming some places, will change now.

#1289 updating the command too

Co-authored-by: Rootul P <[email protected]>

x/qgb/README.md

Co-authored-by: Rootul P <[email protected]>

rootulp · 2023-01-24T21:52:50Z

x/qgb/README.md

+```shell
+$ celestia-appd verify --help          


[non-blocking] This code snippet may become out of date with the true --help docs so instead of copy + pasting the entire help output, I think it may be preferable to include the text that isn't the code snippet and at the bottom say something like:

See the following command for more details

celestia-appd verify --help

and then ensure that these docs make it into the appropriate --help docs

What do you think is better for docs, providing the commands but without an output, or the commands + their output (that might be outdated after some time)?

I personally think the latter is better, and we should just update the docs to reflect the new flags/arguments later

What do you think is better for docs, providing the commands but without an output, or the commands + their output (that might be outdated after some time)?

In this scenario: providing the commands but without an output. I think this because the goal of the README is to give developers a high level overview of the QGB module. Developers aren't necessarily users of the QGB module CLI. Users of the QGB CLI should be able to get information on CLI flags via --help and I expect them to try that before finding this README.

Also (repeated from above): the command output becomes a maintenance burden but I defer to you @sweexordious

I guess we can leave it as it is for now, with the help output. And, if it becomes a burden later, we can remove it.
Thanks guys for your help 👍 👍

rahulghangas · 2023-01-25T12:38:00Z

x/qgb/README.md

+```shell
+$ celestia-appd verify --help          


I personally think the latter is better, and we should just update the docs to reflect the new flags/arguments later

rahulghangas · 2023-01-25T12:38:52Z

x/qgb/README.md

+
+It currently supports three sub-commands:
+
+- `blob`: Takes a transaction hash, in hex format, and verifies that the blob paid for by the transaction has been committed to by the QGB contract. It only supports one blob for now.


Should this specify that a prefix is needed?

Which prefix? A transaction hash in hex format is something like: 7826D5310CD007F82B918359D74A853B4ECB6633E9F231C5477B7F21D68D642D if I am not missing something

Nvm, I was wrong. Maybe we should still specify that no prefix is required

rahulghangas · 2023-01-25T12:39:05Z

x/qgb/README.md

+
+- `blob`: Takes a transaction hash, in hex format, and verifies that the blob paid for by the transaction has been committed to by the QGB contract. It only supports one blob for now.
+- `shares`: Takes a range of shares and a height, and verifies that these shares have been committed to by the QGB contract.
+- `tx`: Takes a transaction hash, in hex format, and verifies that it has been committed to by the QGB contract.


Same as above

evan-forbes

nice, 👍

might also want to link to nmt/rsmt2d/specs, and eventually a diagram would likely be useful to show precising what inclusion proofs are being used for which proof since we're using two or three different proofs

x/qgb/README.md

evan-forbes · 2023-01-31T23:09:37Z

is this mergable @sweexordious ?

rach-id · 2023-01-31T23:29:14Z

## Overview Closes #1254 We could add more documentation after executing the command to give an example output, but I guess it's better to do it after we have mocked QGB deployed so that we have the same output as users. ## Checklist  - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --------- Co-authored-by: Rootul P <[email protected]>

docs: add QGB verify command docs

601461c

rach-id added documentation Improvements or additions to documentation x/qgb labels Jan 24, 2023

rach-id requested a review from rahulghangas January 24, 2023 17:05

rach-id self-assigned this Jan 24, 2023

rach-id requested a review from evan-forbes as a code owner January 24, 2023 17:05

MSevey requested a review from a team January 24, 2023 17:06

rootulp reviewed Jan 24, 2023

View reviewed changes

rach-id and others added 2 commits January 24, 2023 19:02

Update x/qgb/README.md

f10c8e2

Co-authored-by: Rootul P <[email protected]>

Update x/qgb/README.md

9245044

Co-authored-by: Rootul P <[email protected]>

MSevey requested a review from a team January 24, 2023 18:03

rach-id added 2 commits January 24, 2023 19:05

chore: msg -> blob

dc61ae6

chore: msg -> blob

3106431

rootulp previously approved these changes Jan 24, 2023

View reviewed changes

x/qgb/README.md Outdated Show resolved Hide resolved

x/qgb/README.md Outdated Show resolved Hide resolved

rach-id dismissed rootulp’s stale review via 438bcc1 January 24, 2023 21:29

rach-id and others added 2 commits January 24, 2023 22:29

Update x/qgb/README.md

438bcc1

Co-authored-by: Rootul P <[email protected]>

Update x/qgb/README.md

de21d45

Co-authored-by: Rootul P <[email protected]>

MSevey requested a review from a team January 24, 2023 21:29

rach-id requested a review from rootulp January 24, 2023 21:45

rootulp approved these changes Jan 24, 2023

View reviewed changes

rahulghangas approved these changes Jan 25, 2023

View reviewed changes

evan-forbes approved these changes Jan 30, 2023

View reviewed changes

x/qgb/README.md Show resolved Hide resolved

rach-id mentioned this pull request Jan 30, 2023

Document the share inclusion proofs #1313

Closed

rach-id merged commit 381b8ef into celestiaorg:main Jan 31, 2023

rach-id deleted the qgb_verify_docs branch January 31, 2023 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add QGB verify command docs #1287

docs: add QGB verify command docs #1287

rach-id commented Jan 24, 2023 •

edited

Loading

rootulp Jan 24, 2023

rach-id Jan 24, 2023

rach-id Jan 24, 2023

rootulp Jan 24, 2023

rach-id Jan 24, 2023 •

edited

Loading

rahulghangas Jan 25, 2023

rootulp Jan 25, 2023

rach-id Jan 25, 2023

rahulghangas Jan 25, 2023

rahulghangas Jan 25, 2023

rach-id Jan 25, 2023

rahulghangas Jan 30, 2023

rahulghangas Jan 25, 2023

evan-forbes left a comment •

edited

Loading

evan-forbes commented Jan 31, 2023

rach-id commented Jan 31, 2023


		It currently supports three sub-commands:

		- `msg`: Takes a transaction hash, in hex format, and verifies that the message that has been posted using it has been committed to, by the QGB contract. It only supports one message for now.


		It currently supports three sub-commands:

		- `blob`: Takes a transaction hash, in hex format, and verifies that the blob paid for by the transaction has been committed to by the QGB contract. It only supports one blob for now.

docs: add QGB verify command docs #1287

docs: add QGB verify command docs #1287

Conversation

rach-id commented Jan 24, 2023 • edited Loading

Overview

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

evan-forbes commented Jan 31, 2023

rach-id commented Jan 31, 2023

rach-id commented Jan 24, 2023 •

edited

Loading

rach-id Jan 24, 2023 •

edited

Loading

evan-forbes left a comment •

edited

Loading