Skip to content

client: submitpfb partial response support#59

Open
walldiss wants to merge 5 commits intocelestiaorg:mainfrom
walldiss:submitpfb_partial_resonse_support
Open

client: submitpfb partial response support#59
walldiss wants to merge 5 commits intocelestiaorg:mainfrom
walldiss:submitpfb_partial_resonse_support

Conversation

@walldiss
Copy link
Copy Markdown
Member

@walldiss walldiss commented Jun 6, 2023

Overview

Resolves #58

@walldiss walldiss self-assigned this Jun 6, 2023
@walldiss walldiss requested review from Wondertan and tzdybal June 6, 2023 17:04
@walldiss walldiss force-pushed the submitpfb_partial_resonse_support branch from 30197bc to 1913919 Compare June 6, 2023 17:06
Copy link
Copy Markdown
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.

Thank you!

@walldiss walldiss changed the title Submitpfb partial resonse support Submitpfb partial response support Jun 6, 2023
@walldiss walldiss changed the title Submitpfb partial response support client: submitpfb partial response support Jun 6, 2023
@omritoptix
Copy link
Copy Markdown

as far as I can tell if an error occures the txresp is still not returned as show here and here

@walldiss walldiss force-pushed the submitpfb_partial_resonse_support branch 3 times, most recently from 9532396 to b87ae04 Compare June 13, 2023 11:38
@walldiss walldiss force-pushed the submitpfb_partial_resonse_support branch from b87ae04 to 64c58de Compare June 13, 2023 11:45
@walldiss walldiss force-pushed the submitpfb_partial_resonse_support branch from 447a228 to 3248676 Compare June 13, 2023 11:50
@walldiss
Copy link
Copy Markdown
Member Author

walldiss commented Jun 13, 2023

@omritoptix This error handling is a bit tricky. First error here is networking client error, and if it happens, there will be no response value returned. The second error here is constructed inside client, when http code is >500. For partial responses in node we use code 206, so if code 500 is returned, there were no txresp to return.

I did a simplification of this PR, so hopefully it will get merged soon.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 13, 2023

Codecov Report

Merging #59 (3248676) into main (ec221ef) will decrease coverage by 1.66%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
- Coverage   37.14%   35.48%   -1.66%     
==========================================
  Files           3        4       +1     
  Lines         175      186      +11     
==========================================
+ Hits           65       66       +1     
- Misses         97      106       +9     
- Partials       13       14       +1     
Impacted Files Coverage Δ
types.go 0.00% <0.00%> (ø)
client.go 51.13% <40.00%> (-1.25%) ⬇️

@walldiss walldiss requested review from Wondertan and nashqueue June 13, 2023 11:55
@@ -1,6 +1,6 @@
module github.com/celestiaorg/go-cnc

go 1.19
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we want to update the version, we should be doing it in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

resp is not returned from submitPFB in case an error returns

5 participants