-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier_reexecution: move rpc_state_reader from apollo_gateway into #11628
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?
blockifier_reexecution: move rpc_state_reader from apollo_gateway into #11628
Conversation
ArniStarkware
left a comment
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.
@ArniStarkware reviewed all commit messages.
Reviewable status: 0 of 18 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @noaov1).
ArniStarkware
left a comment
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.
@ArniStarkware partially reviewed 3 files and made 1 comment.
Reviewable status: 3 of 18 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @lev-starkware, and @noaov1).
a discussion (no related file):
Should the gateway's Cargo.toml file be affected?
| apollo_rpc.workspace = true |
Please remove all unnecessary dependancies (if any).
ArniStarkware
left a comment
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.
@ArniStarkware reviewed 2 files.
Reviewable status: 5 of 18 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @lev-starkware, and @noaov1).
ArniStarkware
left a comment
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.
@ArniStarkware made 3 comments.
Reviewable status: 5 of 18 files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware, @lev-starkware, and @noaov1).
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 43 at r1 (raw file):
use crate::rpc_state_reader::config::RpcStateReaderConfig; use crate::rpc_state_reader::rpc_objects::{BlockHeader, BlockId, GetBlockWithTxHashesParams}; use crate::rpc_state_reader::rpc_state_reader::RpcStateReader as GatewayRpcStateReader;
This name no longer makes sense.
Code quote:
use crate::rpc_state_reader::rpc_state_reader::RpcStateReader as GatewayRpcStateReader;crates/blockifier_reexecution/Cargo.toml line 18 at r1 (raw file):
apollo_gateway.workspace = true apollo_gateway_config.workspace = true apollo_gateway_types.workspace = true
Why is this here? (Saddly, blockifier reexecution was already dependent on apollo_gateway, so this is not a big deal.)
Code quote:
apollo_gateway_types.workspace = truecrates/blockifier_reexecution/src/state_reader/rpc_state_reader_test.rs line 34 at r1 (raw file):
use crate::rpc_state_reader::config::RpcStateReaderConfig; use crate::rpc_state_reader::rpc_objects::BlockId; use crate::rpc_state_reader::rpc_state_reader::RpcStateReader as GatewayRpcStateReader;
This name no longer makes sense.
Code quote:
use crate::rpc_state_reader::rpc_state_reader::RpcStateReader as GatewayRpcStateReader;f5c3778 to
f4600ff
Compare
lev-starkware
left a comment
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.
@lev-starkware made 3 comments and resolved 3 discussions.
Reviewable status: 5 of 20 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @AvivYossef-starkware, and @noaov1).
a discussion (no related file):
Previously, ArniStarkware (Arnon Hod) wrote…
Should the gateway's
Cargo.tomlfile be affected?
apollo_rpc.workspace = true Please remove all unnecessary dependancies (if any).
Done. Removed al unnecessary dependences.
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 43 at r1 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
This name no longer makes sense.
I leave this to Noa's team
crates/blockifier_reexecution/src/state_reader/rpc_state_reader_test.rs line 34 at r1 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
This name no longer makes sense.
I leave this to Noa's team
ArniStarkware
left a comment
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.
@ArniStarkware resolved 1 discussion.
Reviewable status: 5 of 20 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @noaov1).
ArniStarkware
left a comment
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.
@ArniStarkware reviewed 1 file.
Reviewable status: 6 of 20 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @noaov1).
ArniStarkware
left a comment
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.
@ArniStarkware reviewed 1 file.
Reviewable status: 7 of 20 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @noaov1).
eccb269 to
e90eb75
Compare
e90eb75 to
3cf1a72
Compare
615b58b to
97cf2fd
Compare
AvivYossef-starkware
left a comment
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.
We need to merge both RpcStateReader into one struct.
you can just merge it, and I'll refactor it later ( you can add TODO with my name )
@AvivYossef-starkware partially reviewed 6 files and all commit messages, and made 1 comment.
Reviewable status: 7 of 20 files reviewed, all discussions resolved (waiting on @ArniStarkware and @noaov1).
97cf2fd to
22f7d5c
Compare
3cf1a72 to
c1a6429
Compare
c1a6429 to
97c8376
Compare
97c8376 to
30596cb
Compare
30596cb to
014c72f
Compare
|
If we are cleaning things up. Suggestion: // Converts a serde error to the error type of the state reader.
fn serde_err_to_state_err(err: SerdeError) -> StateError {
StateError::StateReadError(format!("Failed to parse rpc result {:?}", err.to_string()))
} |
ArniStarkware
left a comment
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.
@ArniStarkware reviewed 2 files.
Reviewable status: 4 of 20 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @lev-starkware, and @noaov1).
ArniStarkware
left a comment
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.
@ArniStarkware reviewed 1 file.
Reviewable status: 5 of 20 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @lev-starkware, and @noaov1).
ArniStarkware
left a comment
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.
@ArniStarkware partially reviewed 7 files and made 2 comments.
Reviewable status: 12 of 20 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @lev-starkware, and @noaov1).
crates/blockifier_reexecution/src/rpc_state_reader/rpc_state_reader_test.rs line 20 at r7 (raw file):
RpcSuccessResponse, }; use crate::rpc_state_reader::rpc_state_reader::RpcStateReader;
I am a bit sad that we have: rpc_state_reader::rpc_state_reader. Will this be cleaned up soon?
Code quote:
use crate::rpc_state_reader::rpc_state_reader::RpcStateReader;crates/blockifier_reexecution/src/rpc_state_reader/mod.rs line 4 at r7 (raw file):
pub mod rpc_objects; #[allow(clippy::module_inception)] pub mod rpc_state_reader;
Please explain this error.
@AvivYossef-starkware are you fine with that? This is a bit ugly in my opinion (and causes lines like:
use crate::rpc_state_reader::rpc_state_reader::RpcStateReader;
which has the word rpc_state_reader appear three times).
Code quote:
#[allow(clippy::module_inception)]
pub mod rpc_state_reader;
No description provided.