Fixes RequestManager handling and adding of the 'data' listener

[nivida]

Fixes #1648, #3042

Descriptions

Fixes the MaxEventListener issue with adding of the setRequestManager function to all packages. The provider will now only get resolved once and the data listener is no longer added on each initiation of a Module, Contract, or on executing of the setProvider method on each package.

Old Behaviour

1. call setProvider.
2. create RequestManager and register data listener.
3. call setProvider on all children's.
4. create RequestManager and register data listener for each child package.

New Behaviour

1. call setProvider.
2. create RequestManager and register data listener.
3. call setRequestManager on all children's.

[coveralls]

Coverage decreased (-0.008%) to 84.338% when pulling d624492 on issue/1648 into 1c0ad97 on 1.x.

[cgewecke]

Fixes the warning which is awesome. 💯 💯

• without this PR
• with this PR

Is there anyway to get the diff into a state where only the meaningful logic changes are represented before going over for a quick second pass?

[nivida]

@cgewecke Because those code style improvements are made within commits with actually fixes is it really hard to remove those changes. I will not do such changes to newly proposed PRs. Thanks for the hint!

[cgewecke]

This will take some time to review, it touches some of the most sensitive logic at Web3. The original design seems a little unusual. I think we need to go through the git blame and make sure we understand why it's currently like this.

@nivida Did you ever discuss the RequestManager logic with Frozeman? Do you remember what his views about it were?

[cgewecke]

@frozeman Just pinging you here in case you might remember running into any specific difficulties or design questions around how the RequestManager and setProvider are hooked together.

Do you have any ideas for things to test / double-check?

[frozeman]

Without reading all the comments. One reason why I made it this way, was so that certain modules can have a storage privet from the global one.

Eg. Swarm or whisper can (did) run on separate binaries, and Providers.

[nivida]

This will take some time to review, it touches some of the most sensitive logic at Web3. The original design seems a little unusual. I think we need to go through the git blame and make sure we understand why it's currently like this.

I've went through anything and wrote the logic shortly down on paper to be sure I'm not breaking anything during fixing it.

@nivida Did you ever discuss the RequestManager logic with Frozeman? Do you remember what his views about it were?

I've discussed this logic around a year ago and the rule was that child modules do inherit the providers from their parents. This means that providers and default options a module can have do overwrite the values of the child modules when a new value/provider got set but not vice versa.

@frozeman

Without reading all the comments. One reason why I made it this way, was so that certain modules can have a storage privet from the global one.
Eg. Swarm or whisper can (did) run on separate binaries, and Providers.

Yep, this is the implemented logic. 👌

@cgewecke
The big difference is that it does only resolve the provider once now and does set it downstream on all child modules without resolving the provider again and without creating a new RequestManager instance for each module and Contract. Because it was resolving the provider and creating the RequestManager object all the time again was it register the data listener N times.

[cgewecke]

@nivida

Because it was resolving the provider and creating the RequestManager object all the time again was it register the data listener N times.

Yes! I understand this - it's good. I would still like to take a block of time to look through this a bit to think about it, sorry...

[nivida]

Moved this fix to #3190