Skip to content

Commit

Permalink
Add Milestone 1 evaluation notes for spartan poc consensus module (#168)
Browse files Browse the repository at this point in the history
  • Loading branch information
Jan Azzati authored May 10, 2021
1 parent 1458404 commit d1f0de8
Showing 1 changed file with 40 additions and 0 deletions.
40 changes: 40 additions & 0 deletions evaluations/spartan-poc-consensus-module_1_protyze.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Evaluation

- **Status:** In Progress
- **PR Link:** Spartan POC Consensus Module: [Accepted W3F Grant App PR](https://github.com/w3f/Open-Grants-Program/pull/357).
- **Milestone:** 1
- **Kusama Identity:** t.b.a.
- **Previously successfully merged evaluation:** None

| Number | Deliverable | Accepted | Link | Evaluation Notes |
| ------ | ---------------------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------- |
| 0a. | License | <ul><li>[X]</li></ul> | - | Placed in each module: Apache 2.0, Unlicense and GPL-3.0-or-later WITH Classpath-exception-2.0 |
| 0b. | Documentation | <ul><li>[ ]</li></ul> | [Node Template Spartan Readme](https://github.com/subspace/substrate/blob/a88f2703ee153cdb5ae67e6e047bc86076370b60/bin/node-template-spartan/README.md) | Adjustments requested |
| 0c. | Testing Guide | <ul><li>[ ]</li></ul> | [From Readme](https://github.com/subspace/substrate/blob/a88f2703ee153cdb5ae67e6e047bc86076370b60/bin/node-template-spartan/README.md#run-tests) | Spartan, PoC, and Farmer all have minimal tests, one set of tests still needs to be delivered |
| 0d. | Article | <ul><li>[X]</li></ul> | [Article](https://medium.com/@jeremiahwagstaff/bringing-poc-consensus-to-substrate-d49d49a912bd) | |
| 1. | Design Document | <ul><li>[X]</li></ul> | [From `pallet_spartan`](https://github.com/subspace/substrate/blob/a88f2703ee153cdb5ae67e6e047bc86076370b60/frame/spartan/design.md) | |
| 2. | `sp_consensus_PoC` | <ul><li>[ ]</li></ul> | [Module Link](https://github.com/subspace/substrate/tree/bfe9476526780ba4182d518764bc11e320222cc5/primitives/consensus/poc) | Copyright/Author adjustments |
| 3. | `sc_consensus_PoC` | <ul><li>[ ]</li></ul> | [Module Link](https://github.com/subspace/substrate/tree/a48694d8a31afb369bdaa57d34a80e52f8c8445b/client/consensus/poc) | Tests missing |
| 4. | `sp_consensus_spartan` | <ul><li>[ ]</li></ul> | [Module Link](https://github.com/subspace/substrate/tree/385ba07f119f7b5163cb4876771a5854a24028e9/primitives/consensus/spartan) | Copyright/Author adjustments |
| 5. | `sc_consensus_spartan` | <ul><li>[X]</li></ul> | None | This is essentially the functionality provided by `spartan-farmer` and proved unessecary. |
| 6. | `pallet_spartan` | <ul><li>[X]</li></ul> | [Module Link](https://github.com/subspace/substrate/tree/c0e058ff8da7a8ba25dde936adc9eecb4d22beb0/frame/spartan) | |
| 7. | `spartan_farmer` | <ul><li>[ ]</li></ul> | [Module Link](https://github.com/subspace/spartan-farmer/tree/e6d5f866e09eecc1629f433c67c841fcefcbdd47) | Adjustments requested |
| 8. | `spartan_client` | <ul><li>[ ]</li></ul> | [Module Link](https://github.com/subspace/substrate/tree/a88f2703ee153cdb5ae67e6e047bc86076370b60/bin/node-template-spartan) | Adjustments requested |
| 9. | Docker | <ul><li>[ ]</li></ul> | [From Readme](https://github.com/subspace/substrate/blob/a88f2703ee153cdb5ae67e6e047bc86076370b60/bin/node-template-spartan/README.md#run-with-docker) | Docker image for node-template-spartan didn't work during my test (OSX) |

Ideally all links inside the above table should include the commit hash,
which was used for testing the delivery. It should also be checked if the software is published under the correct open-source license.

## General Notes

IMHO the team delivered on their promises and shipped an overall solid M1 which can probably be approved once some of the remarks as [discussed here](https://github.com/w3f/Grant-Milestone-Delivery/pull/165#issuecomment-835794353) have been resolved.

Besides some minor caveats the documentation was very comprehensive, well-structured and understandable so that the process to get the deliveries up and running was straight forward. Besides the instructions on how to build and run things, both the design paper and the blog post are clear, well written and helped to understand their solution, vision and roadmap better.

Although this is just the first milestone and it's of course early days, the spartan-farmer could be improved a bit in terms of ease-of-use to make it more resilient outside of the happy case. For example the spartan-farmer wasn't very verbose at telling the user to erase previously created plots when attempting to create another plot later on or the sensitivity to connection interruptions when farming, which could be a bit daunting during development (when you restart/rebuild your node couple of times) and of course in production.

For the primitives, crates and pallets within substrate (deliveries 2-6 & 8), the team was able to start off based on existing implementations (BABE, VRF, ...) and adapt them to their needs - therefore it's clear that they were able to follow good practices quite easily and its great to see that they consider/extend existing solutions rather than re-inventing the wheel – however with the spartan-farmer they had to write and structure the code mostly on their own. This might have led to less inline documentation, minimal testing and may have left some potential to better structure the code – but this is just a first and high-level impression as I'm not experienced with Rust and a second opinion on this would be helpful. Again, overall the farmer does what it should and delivers as expected.

Regarding licensing, I have to mention that besides `Apache-2.0` and `Unlicense` they are also using the `GPL-3.0-or-later WITH Classpath-exception-2.0` license within [sc_consensus_poc](https://github.com/subspace/substrate/blob/poc/client/consensus/poc/README.md). That hasn't been mention in the [Guidelines](https://github.com/w3f/General-Grants-Program/blob/master/grants/milestone-deliverables-guidelines.md) specifically.

Automated testing could always be a bit more, as it seems minimal for the farmer and they've limited tests to the adaptation of pre-existing ones form the projects they started on with substrate. At one occasion they didn't update previous tests, which was pointed out in the [discussion](https://github.com/w3f/Grant-Milestone-Delivery/pull/165#issuecomment-835794353). But there was at least a minimum set of tests in each delivery, which should be fine for now.

0 comments on commit d1f0de8

Please sign in to comment.