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(rust): enable requests to be messages #8783

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

etorreborre
Copy link
Member

@etorreborre etorreborre commented Jan 31, 2025

This PR adjusts the serialization / deserialization of Ockam messages.

Motivation

The initial motivation was to be able to define Request<T> as a type of Message for workers processing messages.
Some additional objectives are to:

  • Decouple the general Encodable / Decodable interfaces from the Serializable / Deserializable implementations provided by the serde_bare library
  • Highlight the fact that Requests are currently doubly-encoded, once as a Vec<u8> with minicbor and then serialized with serde_bare. Fixing this issue will be a one-line (or 2) change after this PR but it is a breaking change, so I left this for later.

Changes

The main changes are:

  1. The removal of default instances for Encodable and Decodable.
  2. The addition of an Encodable and Decodable for a Request<T> provided that T: Message.
    • As a result, all the messages being sent via requests now have their own Message instance (that explains the number of touched files)
  3. The stricter typing of ask/tell/request/... methods for both requests and responses.
  4. A two-steps decoding process of Requests in workers like the NodeManager to:
    • First decode the request as Request<Vec<u8>> (now by the virtue of Request<T> being a Message) in order to read the RequestHeader.
    • And then decode the request payload using a Decodable<T> instance we know the expected type based on the header. Note that this is done without having to know that the request body was encoded using minicbor.

@etorreborre etorreborre self-assigned this Jan 31, 2025
@etorreborre etorreborre force-pushed the etorreborre/refactoring/request-messages branch 5 times, most recently from fe1cc3f to 5dd9725 Compare February 4, 2025 22:57
@etorreborre etorreborre force-pushed the etorreborre/refactoring/request-messages branch 6 times, most recently from 84da455 to 153a866 Compare February 10, 2025 16:24
@etorreborre etorreborre marked this pull request as ready for review February 10, 2025 16:39
@etorreborre etorreborre requested a review from a team as a code owner February 10, 2025 16:39
@davide-baldo davide-baldo self-requested a review February 10, 2025 17:11
Copy link
Member

@davide-baldo davide-baldo left a comment

Choose a reason for hiding this comment

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

There is no performance regression but maybe we can avoid manually implementing every message

@etorreborre etorreborre force-pushed the etorreborre/refactoring/request-messages branch 4 times, most recently from 4af8b7b to 8dde050 Compare February 11, 2025 15:35
@etorreborre etorreborre force-pushed the etorreborre/refactoring/request-messages branch 2 times, most recently from 3d16cdd to 53639d6 Compare February 11, 2025 16:10
@etorreborre etorreborre force-pushed the etorreborre/refactoring/request-messages branch from 53639d6 to f3072f0 Compare February 12, 2025 08:35
@etorreborre etorreborre added this pull request to the merge queue Feb 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 12, 2025
@etorreborre etorreborre added this pull request to the merge queue Feb 12, 2025
Merged via the queue into develop with commit 5ac3ff4 Feb 12, 2025
59 checks passed
@etorreborre etorreborre deleted the etorreborre/refactoring/request-messages branch February 12, 2025 18:32
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.

3 participants