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

Fuel/Request_Response v0.0.2: More meaningful error messages #2377

Merged

Conversation

acerone85
Copy link
Contributor

@acerone85 acerone85 commented Oct 21, 2024

Linked Issues/PRs

Closes #1311

Description

This PR follows #2362, which introduces a new version of the protocol /fuel/req_response/0.0.2 which allows to return Errors in the response. This PR makes the following changes:

  • the different errors that can be returned in a response are implemented.
  • The type of ResponseSender used by a peer to propagate received responses upstream is changed to allow the rrors to be propagated
  • Error Responses received by a peer following a request are displayed with as log messages with warning level.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@acerone85 acerone85 changed the base branch from release/v0.40.0 to 1311-return-better-error-messages-from-p2p-requests October 21, 2024 15:40
@acerone85 acerone85 force-pushed the 1311-return-better-error-messages-from-p2p-requests-pt-2 branch from 11000a8 to 7b2bf3b Compare October 21, 2024 18:27
@acerone85 acerone85 force-pushed the 1311-return-better-error-messages-from-p2p-requests-pt-2 branch from 7b2bf3b to 1bc4253 Compare October 21, 2024 18:33
Base automatically changed from 1311-return-better-error-messages-from-p2p-requests to master October 22, 2024 12:46
@acerone85 acerone85 changed the title (WIP) Fuel/Request_Response v0.0.2: More meaningful error messages Fuel/Request_Response v0.0.2: More meaningful error messages Oct 22, 2024
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

LGTM. I think you can run a search over : "#1311" in codebase and remvoe all todo related and make sure they are all addressed

crates/services/p2p/src/p2p_service.rs Outdated Show resolved Hide resolved
crates/services/p2p/src/service.rs Outdated Show resolved Hide resolved
#[error("Timeout while processing request")]
Timeout = 2,
#[error("Sync processor is out of capacity")]
SyncProcessorOutOfCapacity = 3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, maybe we need to add Unknown(u32) for backward compatibility in the case if we can't deserialize the error.

Copy link
Contributor Author

@acerone85 acerone85 Oct 31, 2024

Choose a reason for hiding this comment

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

Tried something in 5ea9b30. There is an unknown variant, but the error code is erased away. The reason for this is that deserializing unknown values in serde has been recently implemented via the attribute serde(untagged), but this is not supported by postcard (fails with error WontImplement). See serde-rs/serde#912 (comment) for the discussion on serde(untagged). There are other alternatives proposed, such as nested enums or custom deserialization functions for a variant, but they won't either work with postcard or are too complicated imho.

Adding an Unknown variant which abstracts away the error code does at least ensure backwards compatibility.

@acerone85 acerone85 self-assigned this Oct 31, 2024
xgreenx
xgreenx previously approved these changes Nov 13, 2024
AurelienFT
AurelienFT previously approved these changes Nov 14, 2024
@acerone85 acerone85 dismissed stale reviews from AurelienFT and xgreenx via 8ca1dba November 14, 2024 11:33
@acerone85 acerone85 merged commit 552ba75 into master Nov 14, 2024
36 of 38 checks passed
@acerone85 acerone85 deleted the 1311-return-better-error-messages-from-p2p-requests-pt-2 branch November 14, 2024 13:00
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.

Return better error messages from P2P requests
3 participants