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

Configure permissionless builder in native and docker demo #1324

Merged
merged 11 commits into from
Apr 17, 2024

Conversation

sveitser
Copy link
Collaborator

@sveitser sveitser commented Apr 11, 2024

A bunch of fixes to make the builder work.

@sveitser sveitser requested review from move47 and tbro April 11, 2024 23:20
@sveitser sveitser changed the title Configure permissionless builder in native demo Configure permissionless builder in native and docker demo Apr 12, 2024
@sveitser sveitser marked this pull request as ready for review April 12, 2024 03:51
Copy link
Contributor

@nyospe nyospe left a comment

Choose a reason for hiding this comment

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

No programatic issues. A naming quibble and a stale comment.

long,
env = "ESPRESSO_SEQUENCER_DA_SERVER_URL",
env = "ESPRESSO_BUILDER_SEQUENCER_URL",
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this name a bit confusing. Is this the URL of an espresso sequencer node that is a DA committee member and provides the hotshot_event_service? If so, I'd suggest something like ESPRESSO_HS_EVENTS_SEQUENCER_URL, or even (referencing the naming used for the port, ESPRESSO_SEQUENCER_HOTSHOT_EVENT_STREAMING_API_URL, since this URL will be specific to that API, rather than the general URL of the node.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this the URL of an espresso sequencer node that is a DA committee member and provides the hotshot_event_service?

Yes, think so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The explicit ESPRESSO_SEQUENCER_HOTSHOT_EVENT_STREAMING_API_URL is probably best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -25,12 +24,11 @@ pub struct NonPermissionedBuilderOptions {
/// URL of the HotShot DA web server.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should also be updated. @move47

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sveitser sveitser merged commit 9407d51 into main Apr 17, 2024
15 checks passed
@sveitser sveitser deleted the ma/builder-demo branch April 17, 2024 02:03
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