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

Decouple Phragmen from Staking. #3498

Merged
merged 13 commits into from
Aug 29, 2019
Merged

Decouple Phragmen from Staking. #3498

merged 13 commits into from
Aug 29, 2019

Conversation

kianenigma
Copy link
Contributor

For the sake of it being used in two modules now (Staking and new council) I want to both clean and optimize the phragmen code to some extend.

So what happened is that:

At this point, I don't want to take #3356 any further because I want to rebase everything into a phragmen implementation which is 100% decoupled from staking module. I assumed #3364 would get merged easily but I don't see that happening anytime soon because @gavofyork is offline this week. Hence, this PR is to override this. Also renames some terms and types to make it independent of Staking.

As soon as it is done, both of the mentioned PRs above should be rebased with these changes. From that point onward #3364 would stay unchanged and just needs a merge and #3456 can proceed without the problem of a merge-hell later on

An enhancement would be to move the benchmarks and tests of phragmen out of staking but I will not do that in this PR to minimize the changes.

@kianenigma kianenigma added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 27, 2019
@parity-cla-bot
Copy link

It looks like @kianenigma signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

1 similar comment
@parity-cla-bot
Copy link

It looks like @kianenigma signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@bkchr
Copy link
Member

bkchr commented Aug 28, 2019

Why not move it into an own crate? This is an algorithm and sounds like something that can happily live in its own space. In the future, I would even expect that this crate live in its own repository like Grandpa.

@kianenigma kianenigma added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 28, 2019
@kianenigma
Copy link
Contributor Author

I am totally in for that but once the tests + benchmarked + reference impl with f64 is also provided. If you agree with my roadmap above and why I really want this decoupling to push my other two PRs forward, for now, we can start reviewing this (it is almost insubstantial in terms of logic change) and migrate to a crate soonish.

@kianenigma
Copy link
Contributor Author

Well, it is possible but moving to its own crate needs a bit more work since we are using a few traits/type from substrate primitives. Not sure how valuable will the migration be if we still depend on substrate?

@bkchr
Copy link
Member

bkchr commented Aug 28, 2019

In a first step I meant just its own crate in this repository.

@kianenigma kianenigma changed the title Move Phragmen into sr-primitives Decouple Phragmen from Staking. Aug 28, 2019
srml/staking/src/lib.rs Outdated Show resolved Hide resolved
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 nitpicks. I trust you to just have moved most of the code :D

core/phragmen/Cargo.toml Outdated Show resolved Hide resolved
srml/staking/src/benches.rs Outdated Show resolved Hide resolved
core/phragmen/src/lib.rs Outdated Show resolved Hide resolved
core/phragmen/src/lib.rs Outdated Show resolved Hide resolved
core/phragmen/src/lib.rs Outdated Show resolved Hide resolved
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.

3 participants