Skip to content

feat: Add Send + Sync supertraits and add Send to futures return by dyn-compatible traits#926

Merged
sergerad merged 4 commits into
nextfrom
tomyrd-send-sync-rpc-with-test
May 30, 2025
Merged

feat: Add Send + Sync supertraits and add Send to futures return by dyn-compatible traits#926
sergerad merged 4 commits into
nextfrom
tomyrd-send-sync-rpc-with-test

Conversation

@sergerad

@sergerad sergerad commented May 27, 2025

Copy link
Copy Markdown
Contributor

Context

Relates to #849. Following on from this we could make the overall Client struct Sync with Send futures too.

In the past it has been impossible to use TonicRpcClient in a tokio task across await points because the futures returned by its implementation of NodeRpcClient were not Send.

Also the miden-client contains instances of Box<dyn NodeRpcClient + Send>. Because these trait objects are not Sync, their futures cannot be Send.

Changes

  • Updated TonicRpcClient and Store traits to be subtraits of Send and Sync.
  • Updated TonicRpcClient and Store trait functions to return futures which are Send.

@sergerad

Copy link
Copy Markdown
Contributor Author

Can confirm that these changes fix the Send/Sync issue that blocked me from integrating the TonicRpcClient to this project
https://github.com/sergerad/blockrs
sergerad/blockrs#3

@tomyrd tomyrd force-pushed the tomyrd-send-sync-rpc-with-test branch 2 times, most recently from 2f2dde0 to 2d1fe58 Compare May 27, 2025 17:25
@tomyrd

tomyrd commented May 27, 2025

Copy link
Copy Markdown
Contributor

I'm sorry @sergerad. I didn't notice that you had added commits to this branch and just force pushed to it.

Could you please check again that the branch works with your blockrs project?

@tomyrd tomyrd added the no docs This PR does not require an update to documentation files label May 27, 2025
@sergerad

Copy link
Copy Markdown
Contributor Author

I'm sorry @sergerad. I didn't notice that you had added commits to this branch and just force pushed to it.

Could you please check again that the branch works with your blockrs project?

Thanks I have pushed up my changes now. Was just a bit of cleanup around the tests.

@sergerad sergerad changed the title Tomyrd send sync rpc with test Add Send + Sync supertraits and add Send to futures return by dyn-compatible traits May 28, 2025
@sergerad sergerad changed the title Add Send + Sync supertraits and add Send to futures return by dyn-compatible traits feat: Add Send + Sync supertraits and add Send to futures return by dyn-compatible traits May 28, 2025
@sergerad sergerad marked this pull request as ready for review May 28, 2025 01:03
@sergerad sergerad force-pushed the tomyrd-send-sync-rpc-with-test branch from c4ccfff to 87df3e2 Compare May 28, 2025 01:04
@sergerad sergerad requested review from bobbinth, igamigo and tomyrd and removed request for tomyrd May 28, 2025 01:05
@sergerad

Copy link
Copy Markdown
Contributor Author

@bobbinth @igamigo bumping for review 🙏

@igamigo igamigo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

I also agree we should do the same upstream to enable the Client to be Send and Sync as we discussed on the meeting

@sergerad sergerad merged commit efbbe4e into next May 30, 2025
21 checks passed
@sergerad sergerad deleted the tomyrd-send-sync-rpc-with-test branch May 30, 2025 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no docs This PR does not require an update to documentation files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants