-
Notifications
You must be signed in to change notification settings - Fork 30
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(fgw): addition of a gateway + removal of starknet-provider #252
Conversation
ba05390
to
4220b60
Compare
Coverage reportThe coverage rate is
Diff Coverage details (click to unfold)crates/primitives/gateway/src/receipt.rs
crates/client/gateway/src/client/builder.rs
crates/primitives/receipt/src/into_starknet_core.rs
crates/primitives/receipt/src/lib.rs
crates/primitives/gateway/src/state_update.rs
crates/client/gateway/src/client/request_builder.rs
crates/primitives/receipt/src/from_blockifier.rs
crates/node/src/cli/gateway.rs
crates/client/gateway/src/server/worker.rs
crates/client/gateway/src/client/methods.rs
crates/primitives/gateway/src/transaction.rs
crates/client/gateway/src/server/handler.rs
crates/client/gateway/src/error.rs
crates/primitives/transactions/src/lib.rs
crates/node/src/main.rs
crates/primitives/gateway/src/block.rs
crates/primitives/receipt/src/from_starknet_provider.rs
crates/primitives/utils/src/lib.rs
crates/client/gateway/src/server/helpers.rs
crates/primitives/convert/src/hex_serde.rs
crates/client/gateway/src/server/router.rs
crates/node/src/service/gateway.rs
|
b9b65a8
to
38256b5
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.
cool!
I think we should move the client to another crate
|
||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
#[serde(deny_unknown_fields)] | ||
pub struct BlockProvider { |
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.
why block provider name
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.
rename it to GatewayBlock or something imo
|
||
/// The gateway port to listen at. | ||
#[arg(long, value_name = "GATEWAY PORT", default_value = "8080")] | ||
pub gateway_port: u16, |
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.
maybe we want CORS at least here, i don't really know
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.
this comment can be ignored for now but i think we want CORS settings like the rpcs @Trantorian1
@@ -262,15 +262,15 @@ fn block_hash( | |||
parent_block_hash, | |||
block_number, | |||
global_state_root, | |||
sequencer_address, | |||
sequencer_address: Some(sequencer_address), |
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 don't have an opinion on whether we should have that as an option (same for the other 3 fields)
} | ||
}); | ||
|
||
log::info!("🌐 Gateway endpoint started at {}", listener.local_addr()); |
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.
if we add cors, match the message from rpc
log::info!(
"📱 Running JSON-RPC server at {} (allowed origins={})",
local_addr.map_or_else(|| "unknown".to_string(), |a| a.to_string()),
format_cors(cors.as_ref())
);
bbbf378
to
4eb62dc
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.
A few nitpicks, otherwise all seems good 👍
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.
error handling isnt that clean yet
67787f9
to
f54b361
Compare
> Note: This overrides the fgw url set by the current chain
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.
perfect! very minor things that i noted, mostly just unwraps
Pull Request type
What is the current behavior?
Resolves: #186
What is the new behavior?
Does this introduce a breaking change?
No
Other information