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

feat: query earliest attestation nonce #2724

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Oct 19, 2023

Overview

So that the orchestartor is able to query the earliest attestation nonce and sign up to it.

This change is not breaking and is just adding a new API endpoint.

PS: I originally thought that this was already implemented. But sadly, we didn't.

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

@rach-id rach-id added the x/blobstream item is directly relevant to the blob module label Oct 19, 2023
@rach-id rach-id self-assigned this Oct 19, 2023
@celestia-bot celestia-bot requested a review from a team October 19, 2023 15:51
@rach-id rach-id added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Oct 19, 2023
proto/celestia/qgb/v1/query.proto Outdated Show resolved Hide resolved
// LatestAttestationNonce queries latest attestation nonce.
rpc EarliestAttestationNonce(QueryEarliestAttestationNonceRequest)
returns (QueryEarliestAttestationNonceResponse) {
option (google.api.http).get = "/qgb/v1/attestations/nonce/earliest";
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] did the rename PR not include a rename to the endpoint?

I would expect /blockstream/v1 and not `/qgb/v1/

Copy link
Member Author

Choose a reason for hiding this comment

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

that would be breaking, we wanted to keep that until v2

Copy link
Member

Choose a reason for hiding this comment

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

to clarify the smallest bit, we want to add the consensus breaking things, but we first need the infra to be able to test the upgrade/migrations

Also, if we don't already have an issue for that, can we get one?

Copy link
Member Author

Choose a reason for hiding this comment

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

@codecov-commenter
Copy link

Codecov Report

Merging #2724 (99f60dc) into main (c517bd2) will decrease coverage by 0.30%.
Report is 2 commits behind head on main.
The diff coverage is 2.91%.

@@            Coverage Diff             @@
##             main    #2724      +/-   ##
==========================================
- Coverage   19.89%   19.60%   -0.30%     
==========================================
  Files         139      139              
  Lines       16695    16994     +299     
==========================================
+ Hits         3322     3331       +9     
- Misses      13051    13341     +290     
  Partials      322      322              
Files Coverage Δ
app/app.go 4.36% <ø> (ø)
app/default_overrides.go 19.04% <100.00%> (+4.04%) ⬆️
x/blobstream/keeper/query_general.go 69.23% <0.00%> (-12.59%) ⬇️
x/blobstream/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/blobstream/types/query.pb.go 0.79% <0.80%> (+<0.01%) ⬆️

@celestia-bot celestia-bot requested a review from a team October 19, 2023 17:21
@rach-id rach-id requested a review from rootulp October 19, 2023 17:21
rootulp
rootulp previously approved these changes Oct 19, 2023
evan-forbes
evan-forbes previously approved these changes Oct 19, 2023
@rach-id rach-id enabled auto-merge (squash) October 19, 2023 18:46
@rach-id rach-id dismissed stale reviews from evan-forbes and rootulp via 0bc3d7e October 19, 2023 22:25
@celestia-bot celestia-bot requested a review from a team October 19, 2023 22:25
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

I think I saw these changes in the rename PR as well

But LGTM! 👍

@rach-id rach-id merged commit a3e1da0 into celestiaorg:main Oct 20, 2023
27 checks passed
mergify bot pushed a commit that referenced this pull request Oct 20, 2023
<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

So that the orchestartor is able to query the earliest attestation nonce
and sign up to it.

This change is not breaking and is just adding a new API endpoint.

PS: I originally thought that this was already implemented. But sadly,
we didn't.

## Checklist

<!--
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] 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]>
(cherry picked from commit a3e1da0)

# Conflicts:
#	x/qgb/types/query.pb.go
rach-id added a commit that referenced this pull request Oct 20, 2023
This is an automatic backport of pull request #2724 done by
[Mergify](https://mergify.com).
Cherry-pick of a3e1da0 has failed:
```
On branch mergify/bp/v1.x/pr-2724
Your branch is up to date with 'origin/v1.x'.

You are currently cherry-picking commit a3e1da0.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   app/test/qgb_rpc_test.go
	modified:   proto/celestia/qgb/v1/query.proto
	modified:   x/qgb/keeper/query_general.go
	modified:   x/qgb/types/query.pb.gw.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   x/qgb/types/query.pb.go

```


To fix up this pull request, you can check it out locally. See
documentation:
https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the
[documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on
`<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you
can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>

---------

Co-authored-by: CHAMI Rachid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging x/blobstream item is directly relevant to the blob module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants