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: change json-rpc trait names, relax bounds #1921

Merged
merged 3 commits into from
Jan 18, 2025

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented Jan 16, 2025

Motivation

Make the RPC trait system more clear and flexible, provide non-owned options requirements baked into the traits

Solution

  • Rename RpcParam to RpcSend and RpcReturn to RpcReceive to make them clearer
  • Add lifetimed versions of RpcReturn and RpcObject to allow borrowing from deserializers
  • relax many bounds from RpcObject to either RpcBorrow or RpcSend, e.g. in ResponsePayload::serialize_payload we can relax to RpcSend

Note

The implementation of RpcRecv is still more strict than DeserializeOwned, as it requires + 'static rather than for<'de> Deserialize<'de>. This means that it is not iplemented on (e.g.) things that borrow from some non-deserializer context or environment during deserialization. Some parts of our future and streaming systems rely on this, and it seems like just marginal benefit to change

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

seems okay to me, I actually prefer send/recv over param/return

any objections @DaniPopes @klkvr ?

all of this is internal, so I expect the impact of this to be minimal

@mattsse mattsse merged commit 21e1dc4 into main Jan 18, 2025
26 checks passed
@mattsse mattsse deleted the prestwich/rpc-trait-changes branch January 18, 2025 15:37
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