This repository has been archived by the owner on Dec 20, 2023. It is now read-only.
Refactors fx wiring into self-contained modules #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change does a few things with the ultimate goal of having a cleaner organization of the fx dependency graph: (1) It completely strips out the functional "Options" layer that was being used on top of fx. This layer was obfuscating the real organization of the application dependencies, particularly through the abuse of the "Override" function. (2) Creates an fxmodules package where we moved almost all of the fx providers and invokes into self-contained modules that reflect the high-level organization of the code (e.g., libp2p, node, repo, consensus, etc.). (3) Moves some of the construction of the dependency graph that was outside of fx into the fx framework. Sadly, not all though, but it got the important pieces of the full node API and the RPC server construction. (4) Fixes some antipatterns in the code and the usage of fx. In particular: (a) no more overrides, now the application fails to launch if a dependency is provided twice; (b) in a few places (e.g., mir consensus), providers are now returning the specific struct type and not the interface. We used fx annotations to fix this; (c) Some providers are now private in order to keep modules as self-contained as possible. We didn't go to far with this as the API construction makes it difficult, but it's a start. Hopefully, this will serve as a starting point to make the lotus code cleaner and more modular. The fx module organization proposed here is just a starting point. There's a lot of room for further improvement.
dnkolegov
approved these changes
Jan 2, 2023
8 tasks
adlrocha
suggested changes
Jan 2, 2023
There was a problem hiding this comment.
Amazing job! Apart from a few cosmetic suggestions, can we include the new fx approach also in itests? https://github.com/consensus-shipyard/lotus/blob/spacenet/itests/kit/ensemble.go#L352-L458
If needed we can also choose to create an independent ensemble for eudico as we do here: https://github.com/filecoin-project/eudico/blob/eudico/itests/kit/ensemble_eudico.go (although I would rather avoid a lot of duplicate code).
Once this is done, you can check if it works with make spacenet-test and if this is the case we are ready to merge (and I'll start migrating tspow code here) :) Thanks! 🙏
…/lotus into hmoniz/fx_refactoring
adlrocha
approved these changes
Jan 5, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change does a few things with the ultimate goal of having a cleaner organization of the fx dependency graph:
(1) It completely strips out the functional "Options" layer that was being used on top of fx. This layer was obfuscating the real organization of the application dependencies, particularly through the abuse of the "Override" function.
(2) Creates an fxmodules package where we moved almost all of the fx providers and invokes into self-contained modules that reflect the high-level organization of the code (e.g., libp2p, node, repo, consensus, etc.).
(3) Moves some of the construction of the dependency graph that was outside of fx into the fx framework. Sadly, not all though, but it got the important pieces of the full node API and the RPC server construction.
(4) Fixes some antipatterns in the code and the usage of fx. In particular: (a) no more overrides, now the application fails to launch if a dependency is provided twice; (b) in a few places (e.g., mir consensus), providers are now returning the specific struct type and not the interface. We used fx annotations to fix this; (c) Some providers are now private in order to keep modules as self-contained as possible. We didn't go to far with this as the API construction makes it difficult, but it's a start.
Hopefully, this will serve as a starting point to make the lotus code cleaner and more modular. The fx module organization proposed here is just a starting point. There's a lot of room for further improvement.