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

Makes async traits in the SDK be Send #1894

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented Sep 14, 2023

Describe your changes

Requires the Send bound on the async_traitss in the SDK. This allow the usage of the SDK in an environment which is both async and multithread (see https://rust-lang.github.io/async-book/07_workarounds/03_send_approximation.html).

Indicate on which release or other PRs this topic is based on

v0.22.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

grarco added a commit that referenced this pull request Sep 14, 2023
@grarco grarco mentioned this pull request Sep 14, 2023
@grarco grarco marked this pull request as ready for review September 14, 2023 16:14
mariari
mariari previously approved these changes Sep 14, 2023
Copy link
Member

@mariari mariari left a comment

Choose a reason for hiding this comment

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

Looks good, Thanks for the changelog.

Copy link
Collaborator

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

I think this will be a problem for the namada interface; see rustwasm/wasm-bindgen#2833

and also https://github.com/anoma/namada-interface/blob/2ccc40b1a5092373a1223c55a74d3dcaa2258d6c/packages/shared/lib/src/rpc_client.rs#L16C3-L20

We can gate the ability to send futures across threads with something like:

#[cfg_attr(not(feature = "send-sdk-client"), async_trait(?Send))]
#[cfg_attr(feature = "send-sdk-client", async_trait)]
trait Client { ... }

@Fraccaman
Copy link
Member

@sug0 yeah thats what we were also thinking! We asked @jurevans to test this branch, lets see how it goes

@grarco grarco force-pushed the grarco/sdk-async-traits-send branch from 12b2cf8 to d89c660 Compare September 19, 2023 15:21
grarco added a commit that referenced this pull request Sep 21, 2023
Fraccaman added a commit that referenced this pull request Sep 25, 2023
* origin/grarco/sdk-async-traits-send:
  changelog: add #1894
  Conditionally requires async traits `Send`ness
  Makes async traits in the SDK be `Send`
Fraccaman added a commit that referenced this pull request Sep 25, 2023
…rarco/sdk-async-traits-send: changelog: add #1894   Conditionally requires async traits `Send`ness   Makes async traits in the SDK be `Send`
Copy link

@jurevans jurevans left a comment

Choose a reason for hiding this comment

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

Tested, and works well for me!

@brentstone brentstone merged commit 2f30699 into main Sep 25, 2023
12 checks passed
@brentstone brentstone deleted the grarco/sdk-async-traits-send branch September 25, 2023 17:19
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.

6 participants