Skip to content

Require env_type transcoders to be Send + Sync#879

Merged
ascjones merged 3 commits intouse-ink:masterfrom
obrok:send-sync-env-types
Jan 5, 2023
Merged

Require env_type transcoders to be Send + Sync#879
ascjones merged 3 commits intouse-ink:masterfrom
obrok:send-sync-env-types

Conversation

@obrok
Copy link
Copy Markdown
Contributor

@obrok obrok commented Dec 21, 2022

They are already required to be 'static, so this seems like a reasonable requirement. In practice, it seems only AccountId and Hash transcoders are inserted into this collection. Adding these two traits allows a ContractMetadataTranscoder to be shared between threads via an Arc. This is very useful, especially given a ContractMetadataTranscoder is not even Clone, which makes it unwieldy to load the metadata once and use it around the program.

@cla-bot-2021
Copy link
Copy Markdown

cla-bot-2021 bot commented Dec 21, 2022

User @obrok, please sign the CLA here.

They are already required to be 'static, so this seems like a reasonable
requirement. In practice, it seems only `AccountId` and `Hash`
transcoders are inserted into this collection. Adding these two traits
allows a `ContractMetadataTranscoder` to be shared between threads via
an `Arc`.  This is very useful, especially given a
`ContractMetadataTranscoder` is not even `Clone`, which makes it
unwieldy to load the metadata once and use it around the program.
@obrok obrok force-pushed the send-sync-env-types branch from 06a7a1f to c0ade59 Compare December 21, 2022 16:59
pub struct EnvTypesTranscoder {
encoders: HashMap<u32, Box<dyn CustomTypeEncoder>>,
decoders: HashMap<u32, Box<dyn CustomTypeDecoder>>,
encoders: HashMap<u32, Box<dyn CustomTypeEncoder + Send + Sync>>,
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.

Would it also work if we added those constraints to the trait itself e.g. CustomTyoeEncoder: Send + Sync?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that works as well, updated the PR.

@ascjones ascjones merged commit 7ca8c36 into use-ink:master Jan 5, 2023
@ascjones ascjones mentioned this pull request Jan 9, 2023
@ascjones ascjones mentioned this pull request Feb 14, 2023
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.

2 participants