Cleanup plan and thoughts about the future

[dexX7]

I intend to start a broader cleanup operation, but would like to get some input on this, before I end up with several PRs that won't get merged. The motivation is based on the need for unit tests and an easier understanding and maintenance of code. The reason I started with those silly one-purpose files was mainly due to similar reasons, but in particular based on the heavy dependencies and the difficulty to include and access single functions.

Here is roughly the plan:

• [ ] one class per file (negative example: mastercore_dex.h)
• [ ] if definition is longer than one line, it should live in the cpp, not in the header
• [ ] headers and files should be self-contained
• [ ] no use of "using namespace ..." in most cases
• [ ] dead and commented code can be removed (mastercore.cpp#L730-L844)
• [ ] everything should live in the mastercore namespace (?)
• [ ] introduction of more namespaces such mastercore::persistence (?)
• [ ] replace old C style (static_cast(value) over implicit cast, std::vector vs. fixed length array, ...)
• [ ] consistent naming (we have: write_msc_balances(), getMPbalance(), rounduint64(), StrToInt64())
• [ ] fixed length ints whenever Mastercoin fields are used (e.g. uint64_t vs. long int)
• [ ] const T value instead of #define value

Once done, I suggest to start using comments to tag similarities and then extract and group similar content, such as persistence and serialization for example. Furthermore it would be helpful to break down large functions such as parseTransaction, interpretPacket, with the goal of modules such as "extractPackages", "deobfuscatePackages", "determineDestination", "encodeAsMultisig" (don't nail me regarding the names, just some examples). In the best case a transition to OP_RETURN should be possible by changing just a few lines.

Another idea that I had was to use interfaces to seperate and access Bitcoin Core stuff. The reason is that it should not require major refactorings, when Master Core switches to the Bitcoin Core 0.10 codebase or so, but in the best case there is just one place that needs to be updated.

For easier review and do avoid breaking pending PRs etc., I would start file-by-file instead of one-PR-that-changes-everything.

Input on the general idea and specifics is highly welcomed, especially from @m21.

[m21]

Great ideas for major re-factoring effort. Probably not the best time for right now. How about we revisit this in a few months with most big features out of the way and full cross-OS compatibility?

[zathras-crypto]

Agreed some great ideas here. Also agreed with Michael that since we're being asked for a 0.0.9 ASAP to live STO, and MetaDEx will be wanted soon after that I'd see we get agreement for a bit of a breather after that and look at things like this refactor and outstanding technical debt :)

[dexX7]

Hehe, I consider this as "sounds ok, but only if it's done in mini steps and without getting in the way of achieving other goals with higher priority". Would that be accurate? ;)

Just a few notes:

• There is never a great time.
• Unit tests of almost every function are basically impossible due to the wild dependencies. This is meant literally and not in the sense of "it's just not nice to do it this way", but "it doesn't work, if you try to include mastercore.h in a test".
• The goal is to make things easier and to save time, not to make things more difficult.

Let's recap what happend with "cancel-everything", and I'm not referring to whether it's limited to an ecosystem or not:

1. There was a proposal for [version] [type] [action] [ecosystem]
2. One for all actions was chosen: [version] [type] [action] [property] [number] [property] [number]
3. The reasons were "it's coded this and that way" and "let's do it later"
4. Turned out the RPC layer wasn't able to deal with "zero" values
5. Turned out the isTransactionTypeAllowed check wasn't able to deal with "zero" values
6. None of this was covered by the meta DEx test plan and there were no unit tests
7. It was discovered by additional regtests
8. Resulted in adjusting trade_MP
9. Resulted in adjusting isTransactionTypeAllowed

You may say "but those adjustments were not necessary, we could have enforced some constraints", but likewise, this wouldn't be "for free", because it then would require adjusting the spec and obviously introduce limitations that are not very intuitive.

Let me be clear: I don't want to shift your priorities or todos and I'm very willing to contribute here and start this in the smallest steps possible. I'm looking for some consensus regarding the topic in general and when it comes to specifics, for feedback, if required, where a one liner such as "not this way, because ..." would be more than enough. :)

[dexX7]

@m21: I'm half way of fixing and ordering all header includes, so they become self sustaining and can be included from other files, such as Boost tests. I won't touch anything else and the review would require:

1. Confirm only includes and using directives were modified
2. Build and confirm that there are no build errors

Fortunally any mistake would show up during compilation. But just to make sure: you would merge such a PR, right? It's probably finished later.

Edit: example: https://github.com/dexX7/bitcoin/commit/baa01ff0821709abcf3706d4a6a9517bafd8dc28

[dexX7]

I have this plan still in mind, and I'm going to try to integrate it step by step for https://github.com/OmniLayer/omnicore.