Skip to content
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

Simplify ProtocolContext and ConsensusContext creation #7792

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Oct 21, 2024

PR description

Built on top of #7791, #7862 and #7863

There are no functional changes in this PR, since it is refactor only. The goal is to simplify the ProtocolContext creation,
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-required label 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 fab-10 marked this pull request as ready for review October 21, 2024 17:19
@daniellehrner
Copy link
Contributor

@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
Copy link
Contributor Author

fab-10 commented Oct 24, 2024

@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.

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

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good tradeoff to get rid of the cyclic dependency

@@ -724,6 +725,7 @@ public BesuController build() {
}

protocolContext.setSynchronizer(synchronizer);
protocolContext.seal();
Copy link
Contributor

@garyschulte garyschulte Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

another option could be to seal the context as part of setting the synchronizer. One less step to 'forget' for future implementers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @garyschulte for reviewing, but this part has been superseded by this new PR #7862 to remove the synchronizer from ProtocolContext

@fab-10 fab-10 force-pushed the protocol-context-builder branch 2 times, most recently from 94f1696 to 0534717 Compare November 8, 2024 16:00
@fab-10 fab-10 changed the title Improve ProtocolContext multistep creation Simplify ProtocolContext and ConsensusContext creation Nov 8, 2024
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10
Copy link
Contributor Author

fab-10 commented Nov 8, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants