-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(kona-exex): basic exex setup & routing #12
feat(kona-exex): basic exex setup & routing #12
Conversation
bin/kona-exex/src/main.rs
Outdated
Cli::<KonaArgsExt>::parse().run(|builder, args| async move { | ||
let node = EthereumNode::default(); |
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.
qq: should we try to init the ROLLUP_CONFIG here or is it better to panic! inside KonaExEx::new?
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.
probably better to do rollup config init here if it's fallible, unless you expect new
to also be fallible, since run returns a Result
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.
for now we don't expect new() to be fallible, ty!
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.
I think @Rjected meant the rollup config initialization is fallible - which it should be in your draft.
let Some(cfg) = ROLLUP_CONFIGS.get(&args.l2_chain_id).cloned().map(Arc::new) else {
bail!("Rollup configuration not found for L2 chain id: {}", args.l2_chain_id);
};
Since ROLLUP_CONFIGS
is a hashmap in the superchain-registry
, the kv pair may not exist so this becomes fallible right
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.
@refcell not sure I follow - if the config doesn't exist for the specified chain id we should exit the exex, correct?
0518bb1
to
bb75457
Compare
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.
LGTM
|
||
fn main() -> Result<()> { | ||
Cli::<KonaArgsExt>::parse().run(|builder, args| async move { | ||
let Some(cfg) = ROLLUP_CONFIGS.get(&args.l2_chain_id).cloned() else { |
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.
in the future would it be worth allowing rollup configs outside of the registry using a cli param or file (maybe for ephemeral devnets or something)? if so would make an issue for it
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.
I think so, for stuff like kurtosis specifying a path to a TOML file should be a good idea.
This should live inside the superchain-primitives crate imo, if it makes sense i'll open an issue, cc @refcell
closes #3