wallet-core icon indicating copy to clipboard operation
wallet-core copied to clipboard

[Improvements]: Modularize / Reorganize source tree

Open Milerius opened this issue 3 years ago • 4 comments

Motivation

Currently, WalletCore has a single target that takes all the project sources and compiles them into a static library. The source tree contains folders for each blockchain but does not separate the different project modules (primitives, wallet, algorithm...)

Dependencies are used directly in implementations without proper isolation.

A WalletCore user may want to use some parts of the project without all compiled/linked, e.g.: (TW::eth).

Research

Some modular c++ projects:

  • https://pdimov.github.io/boostdep-report/master/module-levels.html (boost)
  • https://github.com/KomodoPlatform/antara-gaming-sdk/tree/master/modules (each module is a static library) module table
  • https://github.com/skypjack/entt/tree/master/src/entt (each folder is a header-only module and can be used separately)

Proposed Approach

  • [ ] Find the project parts with the lowest level - e.g: TW::eth require TW::primitives
  • [ ] Separate modules in their folder (one pr per module) - keep compiling all the sources the same way as now
  • [ ] Each module can now have its own CMakeSources and be compiled in a standalone library / contains their unit tests/management of dependencies.
  • [ ] Check the usage of C++ modules to reduce compilation time

Modules Research

Some modules ideas:

  • [ ] tw::algorithm (C++ algorithm/wrapper)
  • [ ] tw::primitives (crypto primitive)
  • [ ] tw::hex (hex helpers / hex manipulation)
  • [ ] tw::encoding (base32, base64, base58)
  • [ ] tw::base (base used almost everywhere e.g Data (vector of bytes) )
  • [ ] tw::bitcoin
  • [ ] tw::eth
  • [ ] tw::tezos
  • [ ] tw::wallet
  • [ ] tw::core (contains all the targets)

Milerius avatar Aug 05 '22 11:08 Milerius

Good ideas, we should do a cost/benefit analysis.

Pro:

  • Allows projects to link only a subset of TW -- this is not needed for Trust Wallet, likely other projects also not
  • Better separation of parts, better dependency control, more explicit cross-module dependencies
  • Overall improved build time, more efficient incremental build in case of small changes
  • Allows separating low-traffic blockchains in an 'optional' library

Cons:

  • Increased complexity in build process
  • Increased complexity in binary release handling
  • Increased complexity in projects linking with wallet-core (more libs)

Since linking to part of WC is not a priority right now, external-facing modularity is not required, so we should focus on internal modularity.

optout21 avatar Aug 12 '22 07:08 optout21

Since linking to part of WC is not a priority right now, external-facing modularity is not required, so we should focus on internal modularity.

Thanks for your feedback.

Yes, indeed, as mentioned in my comments - I do think that the first part should be:

Separate modules in their folder (one pr per module) - keep compiling all the sources the same way as now

Increased complexity in build process

Yes, internally, each of the modules could be a static library - however, this complexity is internal and once well arranged, we should not see the difference.

Increased complexity in binary release handling

Yes and no, the idea is to make this complexity invisible because the module that groups the others (tw::all or another name) will generate a unique static library that links all the others, so the consumption of this library will always be the same as today.

But yes if we want to release all the modules, we will need to have some extra script/work, in a first time I suggest just releasing the tw::all modules, and figure it out for the rest.

Increased complexity in projects linking with wallet-core (more libs)

I'm not sure linking to tw::all should automatically have the same effect as now in the future.

I will be sure to make a CMake option to be able to generate a module as a interface library (internal modularity) or a static library (external modularity)


I would like to do a proof of concept pr next week of a tw::base module that contains the class Data

Milerius avatar Aug 12 '22 09:08 Milerius

A comment: this would include source folder reorganization, to match modules, resulting is more structured source tree; a benefit in itself.

optout21 avatar Aug 18 '22 11:08 optout21

A small step: the tests folder has been reorganized (#2627). src folder should be reorganized in a similar manner.

optout21 avatar Oct 12 '22 09:10 optout21

With the upcoming rust effort I think it's time to close this one, will re-open if needed

Milerius avatar Feb 07 '23 06:02 Milerius