Neo: Should we use dependency injection in Neo 3?

[lock9]

Hi @neo-project/core,

I think it would be good for this project if we can update it to use dependency injection. This will make the code more modular and more testable. We already did this in neo-sharp and it worked really well.
I did some brief research and it seems that dependency injection fits with Akka:
https://doc.akka.io/docs/akka/current/actors.html#dependency-injection

What do you think? Shoud we use our own dependency injection or use an existing framework? We could copy from neo-sharp.

[shargon]

It has a lot of benefits for unit testing because this could help with isolation

[igormcoelho]

Definitely. This first thing @rodoufu told me too, when he first saw neo :joy: good to dettach and test components. I suppose this solves other concerns by @jsolman , on component load orders.

[rodoufu]

That would make the project a lot easier to test and to mock.
We don't need to inject everything from the beginning, we may start with some objects and go moving then to DI as we can.

I had started a branch about that but it's too old, like 8 months old, I think:
https://github.com/rodoufu/neo/tree/IoC

[rodoufu]

We may also improve some life scope implementations like the one related to Blockchain, so we don't need to handle the scope ourselves, leaving that to the DI container.
https://github.com/neo-project/neo/blob/master/neo/Ledger/Blockchain.cs#L74

[igormcoelho]

I fully agree @rodoufu , this is the most critical part.

[rodoufu]

I think that a good approach for that would be using AutoFac, it already has a project for DI using Akka framework "Akka.DI.AutoFac".
So we can register a dependency like Builder.RegisterType<Blockchain>().AsSelf();

After that we need to tell Akka actor system to use the DI container:
ActorSystem.UseAutofac(_neoContainer.Container)

Then an actor with a mailbox would be something like:
system.ActorSystem.DI().Props<Blockchain>().WithMailbox("blockchain-mailbox");

At December 2018, when I first started the fork Autofac was the only one able to integrate with Akka but now we may also use CastleWindsor and Ninject
https://getakka.net/articles/actors/dependency-injection.html

[lock9]

@neo-project/core
Hi, if anyone wants to block this issue, now is the time to say.

[theofanis]

As long as DI is build on top of Microsoft.Extensions.DependencyInjection. Any container of choice should only be used in the top level and not pollute in-depth parts with its namespaces. Make this friendly for other containers.

[rodoufu]

I'm creating an implementation for the DI using Autofac but I'm dealing with some coupling issues.
As you can see below, I've mapped the dependencies in the library.

The yellow nodes are the explicit singleton on the code, and an arrow from A to B means that A depends on B to be created, the "Actor" nodes (e.g. BlockchainActor) are the Akka actor for the classes (Blockchain in the example).
The arrow with the label "uses" indicates that the Object needs the dependency but does not have it as an attribute.
We have a lot of cycles and the DI containers cannot handle it so I need to rewrite some of the code in order to make it possible.

[rodoufu]

I've created another dependency graph but for removing the cycles involving the NeoSystem it's necessary to include the dependencies directly on the objects and it makes the graph a little more complex.

I've added a new class NeoContainer which registers and then resolves the dependencies.
It's going to transfer the singleton implementations from the code to the DI container.

[devhawk]

It seems odd to be using DI and Actor model. I mean, I can see using DI inside of an actor, but if we are managing dependencies between subsytems as actors, why do we need a system level DI model like this?

[rodoufu]

It would make much easier to test and add new stuff.
We have a lot of circular dependencies what makes the code very coupled.
We have some objects that depends on NeoSystem but they don't really needs it, they only use it to access the Blockchain instance and some of the actors.
But I think the best thing would be not needing to handle the life scope of some components ourselves, we have some singleton objects and we need to make sure it has only one instance and at the same time control the concurrency on it.

[devhawk]

I'm not arguing against DI. I'm suggesting that we choose a single mechanism to eliminate circular dependencies and isolate subsystems from each other. I don't think it's a good idea to add DI to the system without also removing actor based abstractions.

[igormcoelho]

My opinion is that DI should first solve this line: https://github.com/neo-project/neo/blob/b7be2c76dc5b67c66e0a7854904054bb745f60a3/neo/Ledger/Blockchain.cs#L79

By the way, very nice diagram Rodolfo :joy:

[igormcoelho]

We could start re-routing stuff through NeoSystem (specially Blockchain dependencies), having it as our "centralizer" object. This may help unveiling some of the dependencies.

[shargon]

Agree with @devhawk , we should first remove the Blockchain.Singleton and other static variables.

[devhawk]

@rodoufu can we build a similar diagram at a higher level of abstraction? Subsystem level instead of classes?

[rodoufu]

@rodoufu can we build a similar diagram at a higher level of abstraction? Subsystem level instead of classes?

Yes @devhawk, I think some diagrams may help us getting the best solution for this.
But what do you mean by subsystem level?