Code Review, volunteers needed
@jacogr was reviewing the code in the beginning and provided useful input, but since he's not working on the codebase and it grew bigger, at one point it was becoming useless for him to review all PRs, esp. architecture-wise.
So I'm looking for volunteers to help me review the code before we ship Fether.
Guideline
- I put a list of files/folders in the table below, with some info about each item such as severity (security-wise) or difficulty of review (how easy would it be to review the item).
- You choose one or several items from the table, and for each item, complete the Review Template (inspired from Yelp).
- Create a reply in this github thread for each item you review, see Example. If you think the review doesn't pass and the code needs change, then open a separate Github issue.
Review Template
click to expand
Item: name of the item in the table
-
Understand the code. Make sure you completely understand the code -
Check code quality. Check for well-organized and efficient core logic. Is the code as general as it needs to be, without being more general that it has to be? Make sure the code is maintainable. -
Verify that the code is tested well. Confirm adequate test coverage. Check tests having the right dependencies and are testing the right things. -
Check for any security holes and loop. -
Answer: If this code breaks at 3am and you’re called to diagnose and fix this issue, will you be able to?
Comments: comments if any
Example
click to expand (this should be one reply in this thread)
Item: myFile.js
-
Understand the code. -
Check code quality. -
Verify that the code is tested well. -
Check for any security holes and loop. -
Answer: If this code breaks at 3am and you’re called to diagnose and fix this issue, will you be able to?
Comments: Missing some tests, and I think they should be required here. Will open a new github issue for this.
Review Items
Name | Description | Severity | Difficuty | Comments |
---|---|---|---|---|
MobX sendStore.js | Logic for sending tx | High | Easy | Make sure that we're sending the correct tx. |
MobX parityStore.js | Require a secure token from the node | Medium | Medium | Make sure that this token is securely stored. |
@parity/light.js | High-level Reactive JS lib (oo7-parity with RxJS) | Medium | Difficult | Can be skipped now, as it's a big codebase. Will create a similar issue as this one on that repo. It might make sense now to have a look at the Readme though. |
@parity/light.js-react | Using parity/light.js with React (equivalent of oo7-react) |
Medium | Medium | Uses JS Proxy. |
MobX createAccountStore.js | Logic to create an account | Medium | Easy | Holds the user's secret phrase at some point. |
fether-electron | Electron wrapper | Medium | Medium | Make sure Electron's guide is followed |
@parity/electron | Interaction between Parity and Electron | Low | Medium | |
Other Mobx Stores | The rest of the MobX stores (other than the 3 above) | Low | Easy | Non-critical logic |
React app | React app | Low | Easy | Contains minimal logic, only UI and UI state changes (all logic is in MobX stores) |
fether-ui | UI Buildling Blocks Components | Low | Easy |
It would be recommendable to get High- and Medium-severity items reviewed before shipping.