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

docs: add QGB verify command docs #1287

Merged
merged 7 commits into from
Jan 31, 2023
Merged

docs: add QGB verify command docs #1287

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
601461c
docs: add QGB verify command docs
rach-id Jan 24, 2023
f10c8e2
Update x/qgb/README.md
rach-id Jan 24, 2023
9245044
Update x/qgb/README.md
rach-id Jan 24, 2023
dc61ae6
chore: msg -> blob
rach-id Jan 24, 2023
3106431
chore: msg -> blob
rach-id Jan 24, 2023
438bcc1
Update x/qgb/README.md
rach-id Jan 24, 2023
de21d45
Update x/qgb/README.md
rach-id Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions x/qgb/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,42 @@ if k.CheckLatestAttestationNonce(ctx) && k.GetLatestAttestationNonce(ctx)+1 != n
}
```

## Verification command

The QGB verification command is part of the `Celestia-appd` binary. It allows to verify that a set of shares has been posted to a specific QGB contract.
rach-id marked this conversation as resolved.
Show resolved Hide resolved

```shell
$ celestia-appd verify --help
Comment on lines +125 to +126
Copy link
Collaborator

Choose a reason for hiding this comment

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

[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

Copy link
Member Author

@rach-id rach-id Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 👍 👍


Verifies that a transaction hash, a range of shares, or a message referenced by its transaction hash were committed to by the QGB contract

Usage:
celestia-appd verify [command]

Available Commands:
msg Verifies that a message, referenced by its transaction hash, in hex format, has been committed to by the QGB contract. Only supports one message for now
shares Verifies that a shares range has been committed to by the QGB contract
rach-id marked this conversation as resolved.
Show resolved Hide resolved
tx Verifies that a transaction hash, in hex format, has been committed to by the QGB contract

Flags:
-h, --help help for verify

Global Flags:
--home string directory for config and data (default "/home/midnight/.celestia-app")
--log_format string The logging format (json|plain) (default "plain")
--log_level string The logging level (trace|debug|info|warn|error|fatal|panic) (default "info")
--trace print out full stack trace on errors

Use "celestia-appd verify [command] --help" for more information about a command.

```

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

#1289 updating the command too

- `shares`: Takes a shares range and a height, and verifies that these former have been committed to by the QGB contract.
rach-id marked this conversation as resolved.
Show resolved Hide resolved
- `tx`: Takes a transaction hash, in hex format, and verifies that it has been committed to by the QGB contract.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above


## Useful links

The smart contract implementation is in [quantum-gravity-bridge](https://github.com/celestiaorg/quantum-gravity-bridge/).
Expand Down