-
Notifications
You must be signed in to change notification settings - Fork 26
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
chore(gateway,batcher): use VersionedConstantsOverrides in both gw and batcher #1946
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @yair-starkware and the rest of your teammates on Graphite |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1946 +/- ##
==========================================
+ Coverage 40.10% 44.87% +4.76%
==========================================
Files 26 208 +182
Lines 1895 24368 +22473
Branches 1895 24368 +22473
==========================================
+ Hits 760 10934 +10174
- Misses 1100 12923 +11823
- Partials 35 511 +476 ☔ View full report in Codecov by Sentry. |
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.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)
crates/gateway/src/stateful_transaction_validator.rs
line 98 at r1 (raw file):
let state_reader = state_reader_factory.get_state_reader(latest_block_info.block_number); let state = CachedState::new(state_reader); let versioned_constants = VersionedConstants::latest_constants_with_overrides(
Also delete this function?
Code quote:
latest_constants_with_overrides
crates/batcher/src/block_builder.rs
line 267 at r1 (raw file):
use_kzg_da: block_builder_config.use_kzg_da, }; let versioned_constants = VersionedConstants::latest_with_overrides(
Delete this function?
Code quote:
latest_with_overrides
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5, @Yael-Starkware, and @yair-starkware)
crates/gateway/src/stateful_transaction_validator.rs
line 98 at r1 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Also delete this function?
Alon is right.
crates/batcher/src/block_builder.rs
line 202 at r1 (raw file):
use_kzg_da: true, tx_chunk_size: 100, versioned_constants_overrides: VersionedConstantsOverrides::default(),
Blocking untill the default of VersionedConstantsOverrides
is fixed as discussed.
Code quote:
VersionedConstantsOverrides::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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5, @Yael-Starkware, and @yair-starkware)
crates/batcher/src/block_builder.rs
line 202 at r1 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Blocking untill the default of
VersionedConstantsOverrides
is fixed as discussed.
(using VersionedConstants::Default()
)...
Previously, ArniStarkware (Arnon Hod) wrote…
See: #1962 |
Previously, ArniStarkware (Arnon Hod) wrote…
Why is this blocking? |
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @Yael-Starkware)
crates/batcher/src/block_builder.rs
line 267 at r1 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Delete this function?
Done.
crates/gateway/src/stateful_transaction_validator.rs
line 98 at r1 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Alon is right.
Done.
9a55c5c
to
606a6e0
Compare
Artifacts upload triggered. View details 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.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)
crates/batcher/src/block_builder.rs
line 202 at r1 (raw file):
Previously, yair-starkware (Yair) wrote…
Why is this blocking?
- Merged, so no longer blocking.
- Before the fix, the value used here is really bad. It had no meaning. Now, the value is correct.
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.
Reviewed 2 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yair-starkware)
crates/batcher/src/block_builder.rs
line 223 at r2 (raw file):
use_kzg_da: true, tx_chunk_size: 100, versioned_constants_overrides: VersionedConstantsOverrides::default(),
The default values should be taken from VersionedConstants::latest() and not hard coded.
Code quote:
VersionedConstantsOverrides::default()
Previously, Yael-Starkware (YaelD) wrote…
This is what |
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)
crates/batcher/src/block_builder.rs
line 223 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
This is what
VersionedConstantsOverrides::default()
does now :) See #1962.
You could argue it is confusing.
The right way to do it is to not have VersionedConstantsOverrides
in the first place.
I think for now using the default is ok.
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)
No description provided.