Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Move client only primitives to another dir #9220

Merged
merged 8 commits into from
Jun 30, 2021
Merged

Move client only primitives to another dir #9220

merged 8 commits into from
Jun 30, 2021

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Jun 28, 2021 •
edited
Loading

Fixes #9218 Primitives only used by client moving to under client/primitives

@gilescope gilescope added A0-please_review Pull request needs code review. B5-clientnoteworthy C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 28, 2021
@gilescope gilescope requested a review from bkchr June 28, 2021 14:59
tomusdrw
tomusdrw reviewed Jun 28, 2021
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw 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 block-builder and offchain has been moved too eagerly cause they seem to be used in the runtime (most likely RuntimeAPI definitions).

bin/node-template/node/Cargo.toml Outdated Show resolved Hide resolved
bin/node/runtime/Cargo.toml Outdated Show resolved Hide resolved
bkchr
bkchr suggested changes Jun 28, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

sp-block-builder and sp-offchain are referenced by runtimes, this means they should stay in primitives.

sp-allocator could be renamed to sc-allocator and moved.
sp-chain-spec can just be merged with sc-chain-spec then.

bin/node/runtime/Cargo.toml Outdated Show resolved Hide resolved
bin/node/runtime/Cargo.toml Outdated Show resolved Hide resolved
@expenses
Copy link
Contributor

expenses commented Jun 29, 2021 •
edited
Loading

We probably want a readme.md in client/primitives that simply says something like 'hey, these are the client-only primitives'.

@expenses expenses self-requested a review June 29, 2021 08:28
expenses
expenses reviewed Jun 29, 2021
View reviewed changes
Copy link
Contributor

@expenses expenses left a comment

Choose a reason for hiding this comment

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

I'm happy to go with this when the other reviewers are satisfied

@gilescope gilescope force-pushed the giles-move-client-primatives branch from 0067b00 to 940b162 Compare June 29, 2021 13:27
@gilescope
Copy link
Contributor Author

(Might have squashed a couple of warnings of unneeded &mut while I was passing as they were annoying me)

bkchr
bkchr suggested changes Jun 29, 2021
View reviewed changes
client/executor/runtime-test/src/lib.rs Outdated Show resolved Hide resolved
client/primitives/chain-spec/Cargo.toml Outdated Show resolved Hide resolved
@gilescope gilescope marked this pull request as draft June 29, 2021 14:11
@gilescope gilescope marked this pull request as ready for review June 29, 2021 15:43
@gilescope
Copy link
Contributor Author

@bkchr any further issues or are we all good now?

@bkchr bkchr merged commit 631d4cd into paritytech:master Jun 30, 2021
@trevor-crypto
Copy link
Contributor

trevor-crypto commented Jul 2, 2021 •
edited
Loading

This introduces a breaking change for me because it introduces async-std somehow and I am on a non-async platform. The commit before this one works fine. Anyone know why this happened and if it is by design?

Edit: did more research and I guess it's because rpc-api is using sc-chain-spec instead of the primitive, thus bringing in async stuff

@gilescope
Copy link
Contributor Author

@trevor-crypto sorry I've caused you pain! I believe the substrate socket handling at the moment is done via asnyc-std with a tokio executor. (Rust's async story is as ever evolving)

When you say you're on a non-async platform can you elaborate on that? (Also always keen to hear a little context of what people are using substrate for - if you can say).

@trevor-crypto
Copy link
Contributor

@gilescope No problem. I was only using one type from sc-rpc-api -- ReadProof<Hash>. I simply copied it and made a note, so all is working again.

I am compiling for intel-sgx, but the target/toolchain does not currently support async/await (fortanix). Not doing anything special, just making modifications to substrate-api-client, the unofficial client lib which is also non-async, to use within a product

@github-actions github-actions bot mentioned this pull request Jul 8, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@expenses expenses expenses left review comments

@tomusdrw tomusdrw tomusdrw approved these changes

@bkchr bkchr bkchr approved these changes

@andresilva andresilva Awaiting requested review from andresilva

@sorpaas sorpaas Awaiting requested review from sorpaas

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Establish which crates are client only in the primatives dir
5 participants
@gilescope @expenses @trevor-crypto @tomusdrw @bkchr