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

Conversation

@rphmeier
Copy link
Contributor

tiny patch for paritytech/substrate#4630

@rphmeier rphmeier added A0-please_review Pull request needs code review. and removed A1-onice labels Jan 16, 2020
impl_name: create_runtime_str!("parity-kusama"),
authoring_version: 2,
spec_version: 1040,
spec_version: 1041,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was part of #769 which I merged in, so @expenses may have more background.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might not be needed, CI have me an error before though.

// Rust bug: https://github.com/rust-lang/rust/issues/24159
sp_api::StateBackendFor<P, Block>: sp_api::StateBackend<HasherFor<Block>> + Send,
{
type CreateProposer = Pin<Box<
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary for this to be Pin? In case the future is Unpin I would suggest pinning on polling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess not - I'll avoid pinning in the follow-up if possible.

Copy link
Contributor

@montekki montekki left a comment

Choose a reason for hiding this comment

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

LGTM

// Rust bug: https://github.com/rust-lang/rust/issues/24159
sp_api::StateBackendFor<P, Block>: sp_api::StateBackend<HasherFor<Block>> + Send,
{
type CreateProposer = Pin<Box<
Copy link
Contributor

Choose a reason for hiding this comment

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

The following compiles for me:

diff --git a/validation/src/lib.rs b/validation/src/lib.rs
index 860c1a6f..51f09700 100644
--- a/validation/src/lib.rs
+++ b/validation/src/lib.rs
@@ -569,9 +569,7 @@ impl<C, N, P, SC, TxPool, B> consensus::Environment<Block> for ProposerFactory<C
        // Rust bug: https://github.com/rust-lang/rust/issues/24159
        sp_api::StateBackendFor<P, Block>: sp_api::StateBackend<HasherFor<Block>> + Send,
 {
-       type CreateProposer = Pin<Box<
-               dyn Future<Output = Result<Self::Proposer, Self::Error>> + Send + Unpin + 'static
-       >>;
+       type CreateProposer = futures::future::Ready<Result<Self::Proposer, Self::Error>>;
        type Proposer = Proposer<P, TxPool, B>;
        type Error = Error;
 
@@ -597,7 +595,7 @@ impl<C, N, P, SC, TxPool, B> consensus::Environment<Block> for ProposerFactory<C
                        backend: self.backend.clone(),
                });
 
-               Box::pin(future::ready(maybe_proposer))
+               future::ready(maybe_proposer)
        }
 }

@mxinden
Copy link
Contributor

mxinden commented Jan 16, 2020

A follow up pull request in regards to my suggestion is fine.

@rphmeier rphmeier merged commit 989db4b into master Jan 16, 2020
@rphmeier rphmeier deleted the rh-environment-async branch January 16, 2020 15:09
andresilva added a commit that referenced this pull request Jan 17, 2020
andresilva added a commit that referenced this pull request Jan 17, 2020
* update latest substrate polkadot-master

* fix test compilation

* bump version to 0.7.18

* bump impl_version

* update substrate

* Revert "Instantiate environment with asynchronous API (#768)"

This reverts commit 989db4b.

* update substrate

* remove unused parameter type

* bump trie-db version for tests

* fix collator test

* update substrate

* remove unnecessary service changes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants