-
Notifications
You must be signed in to change notification settings - Fork 253
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
feat: add QGB query attestation CLI command #1892
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.
Overall code LGTM but I think it would be beneficial if this PR contains tests that verify the new feature. I attempted to use it via $ ./scripts/single-node.sh
:
$ ./build/celestia-appd query qgb attestation 1 | jq
{
"nonce": 1,
"members": [
{
"power": 4294967296,
"evm_address": "0x966e6f22781EF6a6A82BBB4DB3df8E225DfD9488"
}
],
"height": 1,
"time": "2023-06-07T19:50:41.120241Z"
}
$ ./build/celestia-appd query qgb attestation 2 | jq
Error: rpc error: code = Unknown desc = attestation not found: unknown request
Questions:
- How would I verify that attestation 2+ work? Do I need to run something else to get more attestations added to state
- Is that error expected for an attestation that doesn't exist yet?
At chain start, there is only 1 attestation. And after 400 blocks, you will see another one. For the tests, I will check if I can write something. The thing is it's just calling a query, it's not doing anything fancy. Thus, I thought that testing it would only be testing if cobra works well, then if the query works fine (and that's already tested in the keepers) Edit: added test |
Codecov Report
@@ Coverage Diff @@
## main #1892 +/- ##
==========================================
- Coverage 21.85% 21.22% -0.63%
==========================================
Files 117 120 +3
Lines 13304 13696 +392
==========================================
Hits 2907 2907
- Misses 10107 10499 +392
Partials 290 290
|
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 adding the test! LGTM. I'm not sure if you saw this:
Is that error expected for an attestation that doesn't exist yet?
Yes. I updated the error message to have a clearer message |
Overview
Checklist