Hello,
this is request for independent devs evaluation of the code of the Safe Network.
My request is to @happybeing, @mav and every other dev to give feedback in this topic on their personal impressions about the current Safe Network code.
This topic is mainly for impressions and testimonials - good or bad. Thank you!
Add @Scorch @bzee @anon57419684 (I know Iām missing one or two) to the list.
@tfa how could I forget! 
@Dimitar and @Nigel lists arenāt restricted and here are my quick feedbacks:
PROS:
Well commented code (and this is hard work to maintain)
Idiomatic Rust: Studying their code is a good experience for learning the language
Heavily tested code: They use Rust testing facilities and the test functions also help understand the code.
CON:
If the tests are so great, then why do I keep running into issues?
It happens on every new sn_node version.
This is because we are in a rush/sprint to stability. Nothing more. If you see our tests can be improved please PR or even suggest some test cases and we can look to get those incorporated. However rush/sprint is where itās at right now.
Code:
The master branch should always be in a compilable state with development taking place on a different branch. That wasnāt the case for the repo I worked on.
Contribution process:
I contributed working code that was ready to merge and instead it turned into bike shedding about something only tangentially related so I gave up and cut my losses.
What was that @anon57419684 ?
I fully concur with this. I forgot to mention this problem, but sometimes master branches are not compatible for months. To avoid this problem, Maidsafe should follow a workflow like this one:
Discussion: WIP: feat(cli): implements 'safe reset' subcommand - Core Libraries - Safe Dev Forum
Original topic: SAFE CLI subcommand suggestions: reset and uninstall - General - Safe Dev Forum
Sorry @anon57419684 but I do think folk were only trying to help. Perhaps we need to get better at this part?
I think we all do. In recent months some big changes have been across repoās and that has had a load of Engineers on slack trying to coordinate branches. So till launch we have been just merging to master and moving on quickly. Itās not the best but should calm right down now with also the 200 line limit on PRs. Itās all aimed now at reducing changes to testnet and beyond. Lot more CI checks going in as well now to catch incompatibilities so expect this part to really improve quickly now.
So good points guys, accurate and needing resolved.
I have no doubt everyoneās intentions were good however thatās irrelevant in my view.
I explicitly asked for guidance regarding the appropriate function for user confirmation. (SAFE CLI subcommand suggestions: reset and uninstall - #11 by anon78698497 - General - Safe Dev Forum) I didnāt receive a response so I created my own. Then the merge request was held up not for the feature I was adding, but to bike shed user confirmation. It wasnāt important enough to reply to originally, but it was important enough to hold up a contribution.
What should have happened is the merge accepted, user confirmation function bike shedding punted, and a forward issue created to track that.
Iāve yet to contribute to the safe network code base, but I did integrate with it in the past (years ago now, sadly). Iām not an expert in Rust, but Iāve been writing code in my day job for over 20 years, including Java (primarily), PHP and Javascript. Over the last 3.5 years, I have also been contracting, predominantly (but not exclusively) in Fintec. This has given me exposure to a lot of different applications, code bases, processes of varying quality and consistency.
Iāve recently started to look over the latest code base with renewed enthusiasm. Iāve read the Rust book and associated material and have started looking through the various Maidsafe repos. Iāve had a detailed look through sn_api (including sn_cli) and sn_client, tracing through authentication and sequences primarily. Iāve also started looking over sn_node and sn_routing.
My thoughts so far? In a nut shell, it is well organised and appears to be high quality code. There is good functional decomposition, well thought out abstractions (structs/objects) and as a result the code reads well. Given the degree of complexity and novelty, this is great to see.
On my travels I have seen far, far worse code. I have seen eye watering functions with hundreds of lines of code splatted within them. I have seen the absence of any structure beyond basic functional decomposition, with little in the way of types or abstractions. Some of this code was from institutions you would have thought (and hoped!) would have high standards. The Safe Network repos are a bastion of quality in comparison.
What would I improve? From what I have seen, unit testing isnāt a priority. There are unit tests, but they tend to be thinner on the ground than I would prefer. I understand that unit testing can slow down progress in the short term, but it has the opposite effect in the longer term. Personally, I tend to write more unit test code than application code and like a high level of coverage.
I understand that there are automated integration tests though (I havenāt seen these yet), which helps to bridge the unit testing gap though. As a newbie to Rust, I may also be overestimating how much unit testing needs to be done (relative to, say, Java). However, given the shape of the code looks āunit testableā, clearly the developers have a good understanding on how to separate concerns and use appropriate abstractions, etc.
Still, my journey into the safe code base continues. These are my honest opinions at this time though and I see much to be pleased about.
I donāt agree, I think it has been a priority for a long time. Each crate has many unit tests and this is what I meant by heavily tested codes. Unit tests are functions flagged with #[test] or #[tokio::test] attribute and for example sn_api has 235 of them and sn_node 25. Not bad IMO.
This is already true for some files. For example in sn_node, src/capacity/rate_limit.rs has 80 lines of application code and 200 lines of tests.
Itās only a matter of time to complete the set of tests functions if coverage is incomplete.
I havenāt reviewed everything. What I have reviewed of the latest versions I thought was well written and clean. The latest is much clearer than earlier pass throughs about 18 months ago.
The only recommendation I have at this point would be to add more comments. I know I am a bit biased on that point. Iām a sucker for comments pornā¦ meaning that I tend to prefer code that is loaded with comments and tells a story along side the functions about what they do and how to use them.
Thatās good to hear. I could find quite a few code files with no tests, but maybe that is my unfamiliarity with Rust. If there is good coverage and Iām just not seeing it, thatās great!
I just remembered something else that comfused me. The term chunk and blob appeared to be used interchangeably at times or in an inconsistent manner. I recall there was even something like a āchunkblobmapā or blobchunkstore. I would have needed more inline comments to guide me. I know @oetyng likes his blobs, but calling everything a chunk or making a much more clear distinction between the usage of chunk and blob would be helpful. Again, I havenāt read everything so you may have described this well somewhere that I missed. More inline comments will help to make sense of things when jumping in to the code at a random location.
We do have coverage results in the CI, so if you look at the actions you might see some or go to coveralls.io. I am not convinced we have as good coverage as we would like. In saying that rust is a wee bit different. I see it needs much less unit testing, but just as much integration testing as c++/c#/java. Reason for less unit testing IMO is the compiler forced correctness.
If you are from java/c++ then you generally write code, seems legit but wonāt compile, you tweak about and it works, then a unit test shows a really stupid bug. Rust has this weird thing where you write code, compiler forces strict rules on you but guides you through fixes and voila, it works and woks correctly, sounds weird but itās true.
Also rust has a lot of files for traits/errors etc. that are not easily unit tested (no logic).
For folk looking at comments (I agree, we need many more, especially in sn_node) if you type cargo doc --open then it will open a web page with the crate and all dependencies documentation. Thatās pretty cool, also docs have tests in them that CI runs, that way if the documented examples go stale (on code change) CI fails.
I think semver should/could help us with this soon as well, once we have all crates released as v1.x
Great topic, some very good comments. Will likely be in more later, just one thing about this
These are distinct in the design doc, but that design is not implemented yet.
The naming is for a design where a blob is a set of 1-n chunks. In the vein of that design, it could also be renamed to a set.
If these were to mean the same thing I would prefer āchunkā over āblobā 