-
Notifications
You must be signed in to change notification settings - Fork 4
fix: ensure DA transaction throttling uses params that are set #8
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
base: main
Are you sure you want to change the base?
Conversation
6ec7f88
to
229acda
Compare
I thought that DAConfig is already contained in the base config |
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.
some small comments
229acda
to
f26dab5
Compare
f26dab5
to
dce9d4a
Compare
@@ -36,20 +37,26 @@ fn main() { | |||
let rollup_args = builder_args.rollup_args; | |||
|
|||
let op_node = OpNode::new(rollup_args.clone()); | |||
let builder_config = OpBuilderConfig::new(OpDAConfig::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.
Seems like i forgot to save 1 comment
Let's add this field to OpRbuilderConfig directly so we won't have extra staff in here
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 maybe misunderstanding, but I'm not sure we should do that. My understanding (please correct me if I'm wrong) is:
OpRbuilderConfig
is only used for integration testing (ref) (and is used to setup the builder binary)- We probably don't want to configure this parameter right now, as it's required in every case?
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.
Yes, you are correct!
I meant to say that we should use structure OpRbuilderArgs
We could add DAConfig field to it, and set it to default (unless configured).
As i see default DAConfig won't have any effect (because it's fields are zeroes)
Then in main
.with_components(op_node.components().payload(CustomOpPayloadBuilder::new(
builder_args.builder_signer,
std::time::Duration::from_secs(builder_args.extra_block_deadline_secs),
builder_args.enable_revert_protection,
builder_args.flashblocks_ws_url,
builder_args.chain_block_time,
builder_args.flashblock_block_time,
)))
We will pass daconfig from OpRbuilderArgs too, like this
.with_components(op_node.components().payload(CustomOpPayloadBuilder::new(
builder_args.builder_signer,
std::time::Duration::from_secs(builder_args.extra_block_deadline_secs),
builder_args.enable_revert_protection,
builder_args.flashblocks_ws_url,
builder_args.chain_block_time,
builder_args.flashblock_block_time,
builder_args.da_config,
)))
📝 Summary
Migrated from flashbots/rbuilder PR flashbots/rbuilder#616
OpDAConfig
that the RPC endpoint does.💡 Motivation and Context
Prior to this PR, I believe setting throttling via the RPC would not have any impact as an unrelated DAConfig object is created here. Part of the work to fix: flashbots/rbuilder#613
✅ I have completed the following steps:
make lint
make test