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

Conversation

@cecton
Copy link
Contributor

@cecton cecton commented Apr 22, 2020

Related to #7

cecton and others added 30 commits April 7, 2020 16:44
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
Forked at: 461b971
Parent branch: origin/master
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.

Some small changes still need to be done, but overall looks good.

key: Arc<CollatorPair>,
polkadot_config: polkadot_collator::Configuration,
) -> sc_service::error::Result<impl AbstractService> {
parachain_config.announce_block = false;
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved into some function provided by Cumulus. As people will need to replicate this all the time and we may need to "tweak" the config even more in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I create a crate service for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok as discussed I placed it in the collator crate


use futures::FutureExt;

use futures::prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

I'm no huge fan of blindly including preludes. (But it is okay :P )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🦥 will fix it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
}

pub struct WaitToAnnounce<Block: BlockT> {
Copy link
Member

Choose a reason for hiding this comment

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

Some docs would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I was waiting for you to validate the implementation first to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cecton and others added 2 commits May 5, 2020 14:26
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
@cecton
Copy link
Contributor Author

cecton commented May 5, 2020

hmm 🤔 that's annoying, I need to create a fake NetworkService to fix the test

@cecton
Copy link
Contributor Author

cecton commented May 5, 2020

I replaced the NetworkService parameter with a closure that just does announce_block. It removed the dependencies on sc_network.

cecton added 5 commits May 5, 2020 15:49
Forked at: cd1eb37
Parent branch: origin/master
Parent branch: origin/master
Forked at: cd1eb37
Parent branch: origin/master
Forked at: cd1eb37
@cecton cecton force-pushed the cecton-keep-unpinned-para-blocks branch from fd5c870 to 19cab74 Compare May 6, 2020 10:07
@cecton cecton requested a review from bkchr May 6, 2020 10:40
@bkchr bkchr merged commit 032595d into master May 6, 2020
@bkchr bkchr deleted the cecton-keep-unpinned-para-blocks branch May 6, 2020 10:50
Maharacha pushed a commit to Maharacha/cumulus that referenced this pull request May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants