-
Notifications
You must be signed in to change notification settings - Fork 381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove coupling from merge plugin and aura #7044
base: master
Are you sure you want to change the base?
Remove coupling from merge plugin and aura #7044
Conversation
@@ -9,6 +9,7 @@ | |||
<ProjectReference Include="..\Nethermind.Blockchain\Nethermind.Blockchain.csproj" /> | |||
<ProjectReference Include="..\Nethermind.Core\Nethermind.Core.csproj" /> | |||
<ProjectReference Include="..\Nethermind.Consensus\Nethermind.Consensus.csproj" /> | |||
<ProjectReference Include="..\Nethermind.Init\Nethermind.Init.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it was implicitly referenced by Merge.Plugin -> Aura -> Init.
So when i broke the coupling, it became needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we test it with gnosis node and mainnet node?
@@ -73,6 +73,11 @@ protected override PostMergeBlockProducerFactory CreateBlockProducerFactory() | |||
_blocksConfig, | |||
_api.LogManager); | |||
|
|||
protected override IBlockFinalizationManager InitializeMergeFinilizationManager() | |||
{ | |||
return new MergeFinalizationManager(_blockFinalizationManager, _api.FinalizationManager, _poSSwitcher); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be AuraMergeFinalizationManager here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
public AuRaMergeFinalizationManager(IManualBlockFinalizationManager manualBlockFinalizationManager, IBlockFinalizationManager? blockFinalizationManager, IPoSSwitcher poSSwitcher) : base(manualBlockFinalizationManager, blockFinalizationManager, poSSwitcher) | ||
{ | ||
_auRaBlockFinalizationManager = blockFinalizationManager as IAuRaBlockFinalizationManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make to instantiate this and put blockFinalizationManager
as anything else than IAuRaBlockFinalizationManager
? If not why not make it explicit in constructor? And why use as
instead of proper casting and throwing exceptions?
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs
. Remove if not applicable.Requires explanation in Release Notes