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

refactor(api/gateway) return error from gateway if header is not synced yet #2108

Conversation

walldiss
Copy link
Member

Overview

If header was not in header store, gateway would hang until client context was canceled. PR allows gateway to return an error immediately in this case.

@walldiss walldiss added area:api Related to celestia-node API kind:refactor Attached to refactoring PRs labels Apr 20, 2023
@walldiss walldiss requested a review from renaynay as a code owner April 20, 2023 12:06
@walldiss walldiss self-assigned this Apr 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #2108 (6922a0e) into main (a98923b) will decrease coverage by 0.19%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2108      +/-   ##
==========================================
- Coverage   55.67%   55.49%   -0.19%     
==========================================
  Files         209      209              
  Lines       13267    13321      +54     
==========================================
+ Hits         7387     7393       +6     
- Misses       5135     5186      +51     
+ Partials      745      742       -3     
Impacted Files Coverage Δ
api/gateway/availability.go 0.00% <0.00%> (ø)
api/gateway/header.go 0.00% <0.00%> (ø)
api/gateway/share.go 8.91% <0.00%> (-0.77%) ⬇️

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Wondertan
Copy link
Member

Wondertan commented Apr 20, 2023

Perfect, let's add a TODO that we will change this to NetworkHead from LocalHead once the adjacency in the store is fixed. Issue to link: celestiaorg/go-header#32

Wondertan
Wondertan previously approved these changes Apr 20, 2023
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

One comment only

api/gateway/availability.go Outdated Show resolved Hide resolved
api/gateway/availability.go Outdated Show resolved Hide resolved
api/gateway/header.go Outdated Show resolved Hide resolved
api/gateway/share.go Outdated Show resolved Hide resolved
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

i would just say "local chain head"

@renaynay
Copy link
Member

Fixes #2118

@renaynay renaynay enabled auto-merge (squash) April 21, 2023 08:47
@renaynay renaynay merged commit 0d4e4bb into celestiaorg:main Apr 21, 2023
Wondertan pushed a commit that referenced this pull request Apr 27, 2023
…ed yet (#2108)

## Overview

If header was not in header store, gateway would hang until client
context was canceled. PR allows gateway to return an error immediately
in this case.

---------

Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Related to celestia-node API kind:refactor Attached to refactoring PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants