Disclaimer: These reviews are done as is from what is on display in the master branch of the repo’s made available. This review is not a comment on the overall project, scope, or success thereof. This was done as an educational review by me and any comments in the article are simply my opinion. It should not be used as any comment or advice on the project as a whole.
Review Date: 18/03/2018
Elastos has a lot of repo’s and they are all active. Not a lot of activity, but there is activity. I’m not actually sure where to begin. So will go through all the different repo’s and see which one we are interested in the most;
Just a link to their gitlab (interesting choice), but I have no idea what this is and the documentation is non existent, so let’s move on
Moving on, I am not understanding their repo structure at all, the next one just points to another one.
Ok, Elastos.ELA, from the readme this isn’t the right code, this is suppose to be the Elacoin implementation in the Elastos eco system, but this looks like the standard blockchain structure to me, so let’s start digging;
Tabs set to 4 spaces, what cruel monster develops with tabs at 4 spaces! This is just personal preference though, so can ignore it :)
RPC, Rest, and websocket server, normal PoW consensus, basic P2P startup, all the basics are checked here.
Finally someone doing SSL nodes, even if selfsigned this is still better, I’m glad to finally see it, for the crypto world we live in there is way to much plain text broadcasting going on.
Sending parameters as a map, I understand why people might want to do this since it means you don’t have to worry about function definitions, but this is bad practice because of something called type safety. Compilers interpret and optimize functions based on their type declarations, if it knows it is expecting a string and an int (kind of number in coding), it can optimize both and ensure type safety, if everything is passed as a map of strings then you lose that power and protection, this also means for each function you need internal knowledge of what the calling functions are sending. This is not good practice and from a compilation level will slow down code processing. I don’t like seeing this.
Now you have to constantly do conversions such as the above, this is wasting processor cycles and will decrease overall speed
These should not all be in one function, it makes the function difficult to read and understand, from a pure coding layout perspective these should have been extracted into sub functions. This is just personal preference again though.
I get what they were trying to do at least, they are trying to have everything interact as if it was sending JSON objects from one function to the next, from a microservices perspective this is good design, but then have middleware that handles this for you so you can keep your coding clean.
So let’s talk about ports quickly, a server has limited ports from 0 to 65535, from 1 to 1080 are generally reserved, you don’t really use them since they already are dedicated to specific services, 20 for example to FTP, so what happens if a blockchain implementation decides to run on 20? It won’t work, because most servers already have 20 dedicated to FTP, so mostly, you will see blockchains running on their own domain groupings, normally in the 3000–5000 range. This for example means you might not be able to run 2 blockchains at the same time, because they might have port conflicts. So what is running on 443? 443 is dedicated for HTTPS, this is predominantly used by services like Apache, Nginx, etc, these are web hosting solutions. So Elastos is running on 443, this means I can’t have it running on an existing webserver (which is where a lot of people would run their blockchains since they have the existing resources available), so this will cause port conflicts, and in this case, Apache, Nginx, other web services will win this fight. Why would they want to use 443? So that their API is secure, however, instead, they can still run on another port with a cert implementation. They took the easy way out here and this will cause problems in the future.
Ok, so remember earlier when we spoke about type optimization? That’s another reason why you declare something called CONSTANTS, you normally give them a descriptive name like; const SENDBLOCK = 1, this means that when you do a case statement you refer to SENDBLOCK, but the actual value is 1, because numeric lookups are significantly faster than string lookups. Above again shows a lack of optimization experience, and this will hurt them in the long run.
This is not the way to set up restful routes, again this is bad optimization and string comparisons will cause them to lose a lot of processing power.
Optimization again, you know what the output will be, don’t waste CPU cycles figuring it out. Just a comment about the calculation.
If you have defaults that work in 2 out of 3 cases, set them right at the start, don’t duplicate them, I don’t like seeing this kind of duplication.
Why don’t I see functional coding being used nowadays? Most blockchain projects seem to avoid using maps and filters, functional coding is faster than iterative coding, that’s not even an opinion.
No threading being used in PoW and again functional coding missing.
Conclusion: It is code, I’m not a fan of it, it’s difficult to read and the layout isn’t to my personal liking. That however is personal preference and doesn’t inherently mean it’s bad. What I don’t like is the lack of optimization, strings are mostly used for cross function calls and that will cost them very heavy in the long run.
Other than that, this is just normal blockchain code, what makes them different? I don’t see how they are going to compete with the already established guys when they aren’t bringing anything new to the table, this is just standard blockchain 101 and I don’t recommend trying to dissect the code.
