-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
It looks like @kianenigma signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
1 similar comment
It looks like @kianenigma signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
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. |
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. |
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? |
In a first step I meant just its own crate in this repository. |
There was a problem hiding this 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
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.