[Core] WireEncode accepts a reference instead of an owned value #3214
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
muhamadazmy
approved these changes
May 2, 2025
There was a problem hiding this comment.
Thanks @AhmedSoliman for this PR. The changes looks good to me. It make sense to accept a ref.
The IntoDto trait is less important and possibly can be completely dropped after introducing the BilrostAs macro.
Although one small thing I am worried about is that the BilrostAs macro can only work on the inner fields but not root bilrost::Message structs. That's where the IntoDto trait can become handy.
This is necessary to allow `Box<dyn WireEncode>` to be used in the next PR. This also aligns with how most serialization libraries work, there is no need to send the owned value. I've temporarily disabled bilrost's IntoDto until we can get it to work with this design. Additionally. I didn't change the API of the send() API in network code to avoid making this as a sweeping change. If the need for passing the message by reference on that layer rises, we can change it. ``` // intentionally empty ```
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is necessary to allow
Box<dyn WireEncode>to be used in the next PR. This also aligns with how most serialization libraries work, there is no need to send the owned value. I've temporarily disabled bilrost's IntoDto until we can get it to work with this design. Additionally. I didn't change the API of the send() API in network code to avoid making this as a sweeping change. If the need for passing the message by reference on that layer rises, we can change it.Stack created with Sapling. Best reviewed with ReviewStack.