nim-waku refactor 'n upstream week 12 - 16 July 2021

[jm-clius]

Background

This is a proposal for a refactoring/backlog week, where we divide amongst ourselves and revisit some of the most pressing quality and maintainability issues in the nim-waku codebase to make some strides towards production-quality code.

This does not necessarily have to be a "week", of course. The idea is simply that we set aside some dedicated time in between our general dev efforts to focus on getting as much technical debt squashed as possible.

The idea was born out of a recent attempt to update submodules and realising that some "nice-to-haves" in terms of best practices easily become "must-haves" after a while. :)

The dates 12 - 16 July is chosen somewhat randomly to give us enough time to prepare, yet not be too far ahead in the future.

Many of these are born out of the Nim style guide and we could likely identify a few more issues from this.

Priority issues

This is a laundry list of some of the issues I think we could tackle, roughly in order of priority:

• Refactor: Enforce explicit exception handling #289, Refactor: Waku v2: Better error handling #146: Enforce explicit error handling. Specifically, we should start by adding a top-level {.push raises: [Defect]} to every module and then add explicit {.raises } annotation for every public function. Add -d:chronosStrictException compiler directive for all MAKE targets. See https://status-im.github.io/nim-style-guide/errors.exceptions.html for more.
• Better handling of empty/failed returns: Use Option wherever a return type may be empty; Result where multiple failure paths exist. Replace checks for empty initialisations (e.g. here) with either Option or Result. Remove references
• Ensure logscope added for all modules.
• Improvement: wakunode2 setup #581: Improve wakunode2 setup
• Clean up APIs (Nim and JSON-RPC): e.g. users of each API should be forced to set a contentTopic and be unable to modify the WakuMessage timestamp when publishing (may also cover WakuMessage timestamp is not set #582).
• Simplify wakunode2 subscriptions model #574: Simplify subscriptions model
• Review WakuRelay parameter/field initialisation #562: Clean up parameter initialisation, especially for WakuRelay
• Whisper/Waku types in a different repo #654: Whisper/Waku v1 types
• Update submodules, again
• Refactor: Clean up import paths #288: Clean up imports.
• Clean up open nim-waku issues
• Add readme file to explain nim-waku structure
• Unittest style guide (also refactoring of existing tests?)
• Refactor: Avoid result return, prefer expression #287: Avoid result return.
• CI improvements, e.g. Speed up CI #391. Also see if CI logging can be improved.
• Simplifying the pagination logic #484: Simplify pagination logic
• Improve docs re external tools (jq, multitail) #341: External dependency clean-up/documentation
• WakuFilter: Reconsider implementation of requestId in FilterRPC #257: Use of requestId confusing. Can probably be improved.

@staheri14 @oskarth @EbubeUd WDYT? Will this be a worthwhile effort? Anything to add/alter to the priorities?
Also pinging @D4nte, as he may have some ideas as to priorities.

[EbubeUd]

This is nice.
Worth adding is the use of discard in async function calls.

[jm-clius]

Worth adding is the use of discard in async function calls.

Thanks! Should be covered in #146

[staheri14]

Nice proposal! @jm-clius
whatever I am thinking of adding falls into the category of "Clean up open nim-waku issues", so I have no comments at the moment, will write here if more comments come to my mind.

The followings are certainly "nice to have"s but not "must-have"s!

• A readme file to explain the structure of the nim-waku codebase (I think we already had a PR/issue for this and we can just mirror its content into a file in the repo and make it accessible to the contributors/developers)
• A style guide on the test development and unit tests namings e.g., some of the test names are capital and some small

[D4nte]

Better CI log format? Window CI failed and I simply cannot find any error that causes the failure.

[oskarth]

Good idea, and great list of issues! Let's do it

[jm-clius]

Better CI log format?

Thanks, @D4nte. Agree the current logs can be confusing. Added general CI improvements to list to have some visibility. Not sure how feasible it will be to change CI logs, but we can certainly review.

[jm-clius]

• A readme file to explain the structure of the nim-waku codebase (I think we already had a PR/issue for this and we can just mirror its content into a file in the repo and make it accessible to the contributors/developers)
• A style guide on the test development and unit tests namings e.g., some of the test names are capital and some small

Thanks, @staheri14! Added to list.

[oskarth]

Another candidate #654

[jm-clius]

Another candidate #654

Added!

[jm-clius]

I've divided the issues in the original post into three tracks to make it slightly more readable: https://hackmd.io/beMbu_fdRymYvaXDANukIw

Most issues have been merged into a single checklist that all modules should adhere to: https://hackmd.io/1imOGULZRsed2HpgmzGleA

[oskarth]

Moar issues:

vacp2p/rfc#422
vacp2p/rfc#346
vacp2p/rfc#368

[jm-clius]

Closing as refactor week was completed. A laundry list of all outstanding refactor tasks is tracked in a separate issue: #688