Skip to content

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Sep 10, 2025

This makes it easier to configure the builder slot calculator through setting the chain name, instead of setting each slot calculator configuration variable.

Closes ENG-1372

@rswanson this will lets us load the slot calculator from the CHAIN_NAME env var.

This also updates signet dependencies because it's needed to use the latest bin-base. No behavior change is expected.

…f verbose values

This makes it easier to configure the builder through setting the chain name, instead of setting each calculator configuration variable.

Closes ENG-1372
Copy link
Member Author

Evalir commented Sep 10, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

This makes the builder unrunnable for any other chain. is that intentional?

@Evalir
Copy link
Member Author

Evalir commented Sep 10, 2025

It's runnable for any chain that the slot calculator supports through KnownChains? Which I think we're ok with?

@Evalir Evalir requested a review from prestwich September 10, 2025 16:34
@prestwich
Copy link
Member

this is a significant change to builder functionality that makes it impossible to run on custom nets. is that intentional?

@Evalir
Copy link
Member Author

Evalir commented Sep 10, 2025

Yes this is intentional. We want to make the builder easier to configure. Restricting the builders to chains we have the values in constants for is one way to do so.

@Evalir Evalir requested a review from rswanson September 10, 2025 16:42
@Evalir
Copy link
Member Author

Evalir commented Sep 10, 2025

cc @rswanson i'd like your ACK/NACK on this before we merge

@Evalir
Copy link
Member Author

Evalir commented Sep 10, 2025

Alright, with the new bin-base version now having the constants is still an option, if the CHAIN_NAME env var is not present. So this doesn't require an immediate infra change, making this safe to merge as-is.

@Evalir Evalir merged commit 64e3377 into main Sep 10, 2025
6 checks passed
@Evalir Evalir deleted the evalir/calc-chain-name branch September 10, 2025 20:25
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.

2 participants