-
Notifications
You must be signed in to change notification settings - Fork 151
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
[Access] Add latest finalized block ID and height #1325
[Access] Add latest finalized block ID and height #1325
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.
Thanks for the PR @Guitarheroua!
This looks pretty good. 2 general comments:
- I think we should reframe the entity as
Metadata
so it can be extended in the future. - Please also include the metadata in the following response objects:
SendTransactionResponse
ExecuteScriptResponse
TransactionResultResponse
TransactionResultsResponse
(we'll want to omit the metadata within eachTransactionResultResponse
when sending this)
PingResponse
and GetNetworkParametersResponse
were also skipped, but I agree that it's not needed for those.
option go_package = "github.com/onflow/flow/protobuf/go/flow/entities"; | ||
option java_package = "org.onflow.protobuf.entities"; | ||
|
||
message LastFinalizedBlock { |
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.
Can we call this Metadata
? That way it can be extended to include additional information in the future. Please also rename the file and arguments. maybe something like latest_finalized_block_id
and latest_finalized_height
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.
* Renamed `LastFinalizedBlock` to `Metadata`. Renamed fields. * Added `metadata` to more queries responces.
|
||
message Metadata { | ||
bytes latest_finalized_block_id = 1; | ||
uint64 latest_finalized_height = 2; |
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 should have asked in my previous request. Can you also add bytes node_id
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.
Sorry, I missed it in a ticket. Added with a7105d4
4191: [Access] Add latest block Height and ID to api responses r=peterargue a=Guitarheroua #2906 ### Context This pull request extends the necessary responses for the Access Node with additional fields containing information about the finalized block ID and height. To achieve this, the `synchronization.FinalizedHeaderCache` was propagated to the Access Node handler, and finalized block ID and height were added to query responses As part of this PR: * The protobuf structures were extended([PR 1325](onflow/flow#1325)) * The tests in [access_test.go](https://github.com/onflow/flow-go/blob/master/engine/access/access_test.go) were updated ### Note: * These changes include only GRPC modifications and not REST. REST changes will be made in a separate pull request after getting a reply on [the issue comment](#2906 (comment)). Co-authored-by: Andriy Slisarchuk <andriyslisarchuk@gmail.com> Co-authored-by: Andrii Slisarchuk <Guitarheroua@users.noreply.github.com> Co-authored-by: Andrii Slisarchuk <andriyslisarchuk@gmail.com>
onflow/flow-go#2906
Description
Extended necessary responses for the Access Node with additional fields containing information about the finalized block ID and height.