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

fix(node): commit http errors in data request retrieval #2307

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

tmpolaczyk
Copy link
Contributor

Fix #2306

@aesedepece
Copy link
Member

This change sounds like a good candidate for being shipped with 1.6 — behind a TAPI flag if necessary. What do you think?

@tmpolaczyk
Copy link
Contributor Author

I believe this can be merged without TAPI, because the old behavior is to not commit anything at all. Therefore the data requests will be unsolved until a majority of miners update, and then it will just work as expected.

It is true that we won't have a guarantee that these data requests will be solved until the next TAPI update, which ensures that miners have updated, but I believe nothing will break if this change is merged without TAPI.

@aesedepece
Copy link
Member

It is true that we won't have a guarantee that these data requests will be solved until the next TAPI update, which ensures that miners have updated, but I believe nothing will break if this change is merged without TAPI.

I'm just fine with that approach. Thanks!

@guidiaz
Copy link
Contributor

guidiaz commented Jan 11, 2023 via email

@tmpolaczyk
Copy link
Contributor Author

I don't think it is a good idea to special-case HTTP/403 errors, because many APIs return 404 or 401 instead.

@guidiaz
Copy link
Contributor

guidiaz commented Jan 11, 2023

I don't think it is a good idea to special-case HTTP/403 errors, because many APIs return 404 or 401 instead.

Yeah right, either HTTP/401 or HTTP/403 errors in one single source would be sufficient condition for not commiting to a data request. Data requests relying on a majority of private API sources returning 404 error codes would just be resolved as HttpError { code: 404 }.

@aesedepece
Copy link
Member

@tmpolaczyk shall we rebase and merge this onto 1.6?

@tmpolaczyk tmpolaczyk changed the base branch from master to witnet-1.6 February 3, 2023 09:44
@tmpolaczyk
Copy link
Contributor Author

@tmpolaczyk shall we rebase and merge this onto 1.6?

Done

@tmpolaczyk tmpolaczyk merged commit c8e6f49 into witnet:witnet-1.6 Feb 3, 2023
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.

HTTP errors are not being commited since version 1.5.3
3 participants