besu
besu copied to clipboard
Improve ProtocolContext multistep creation
PR description
Built on top of https://github.com/hyperledger/besu/pull/7791
There are no functional changes in this PR, since it is refactor only. The goal is to improve the ProtocolContext creation, making it clear that, due to cyclic deps it cannot be create in one shot, but it is first partially initialized and then other field are set, namely the synchronizer, and at the end it is sealed to prevent further modifications. Moreover the ConsensusContext is now created before the ProtocolContext, removing the need of the factory
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
- [ ] Checked out our contribution guidelines?
- [ ] Considered documentation and added the
doc-change-requiredlabel to this PR if updates are required. - [ ] Considered the changelog and included an update if required.
- [ ] For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests
Locally, you can run these tests to catch failures early:
- [ ] unit tests:
./gradlew build - [ ] acceptance tests:
./gradlew acceptanceTest - [ ] integration tests:
./gradlew integrationTest - [ ] reference tests:
./gradlew ethereum:referenceTests:referenceTests
@fab-10 Could we use a builder pattern instead? createPartiallyInitialized improves the readability, but I still don't know which parts are initialized and which are still missing.
The builder could hide the multi step creation from the user.
@fab-10 Could we use a builder pattern instead?
createPartiallyInitializedimproves the readability, but I still don't know which parts are initialized and which are still missing.The builder could hide the multi step creation from the user.
I thought about it, but in this case it is not possible since ProtocolContext is being used, before it is completely set, to create what it is missing, the synchronizer, so in order to use a builder we should make the objects that are needed to create the synchronizer use the ProtocolContext.Builder but it does not seem a good idea.
Maybe at the end the synchronizer should not be in the ProtocolContext at all, to break this circular dependency, will check what implies removing the synchronizer from the ProtocolContext
@daniellehrner @garyschulte please take another look, the PR has changed from the first version, since with the previous PRs there is more the need to create a partially initialized ProtocolContext and it can just be immutable.