Skip to content
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

Merged
merged 37 commits into from
Oct 3, 2024

Conversation

jbcaron
Copy link
Member

@jbcaron jbcaron commented Sep 9, 2024

Pull Request type

  • Feature

What is the current behavior?

Resolves: #186

What is the new behavior?

Does this introduce a breaking change?

No

Other information

@jbcaron jbcaron added sequencer The label for sequencing related issues full node Full Node only related issues labels Sep 9, 2024
@jbcaron jbcaron self-assigned this Sep 9, 2024
@jbcaron jbcaron force-pushed the gateway branch 2 times, most recently from ba05390 to 4220b60 Compare September 9, 2024 14:54
Copy link

github-actions bot commented Sep 9, 2024

Coverage report

The coverage rate is 69.10867865519937%

47% of new lines are covered.

Diff Coverage details (click to unfold)

crates/primitives/gateway/src/receipt.rs

80.0% of new lines are covered

crates/client/gateway/src/client/builder.rs

36.0% of new lines are covered

crates/primitives/receipt/src/into_starknet_core.rs

100.0% of new lines are covered

crates/primitives/receipt/src/lib.rs

83.33333333333334% of new lines are covered

crates/primitives/gateway/src/state_update.rs

100.0% of new lines are covered

crates/client/gateway/src/client/request_builder.rs

63.88888888888889% of new lines are covered

crates/primitives/receipt/src/from_blockifier.rs

0.0% of new lines are covered

crates/node/src/cli/gateway.rs

0.0% of new lines are covered

crates/client/gateway/src/server/worker.rs

0.0% of new lines are covered

crates/client/gateway/src/client/methods.rs

97.95918367346938% of new lines are covered

crates/primitives/gateway/src/transaction.rs

50.0% of new lines are covered

crates/client/gateway/src/server/handler.rs

0.0% of new lines are covered

crates/client/gateway/src/error.rs

0.0% of new lines are covered

crates/primitives/transactions/src/lib.rs

88.23529411764706% of new lines are covered

crates/node/src/main.rs

100.0% of new lines are covered

crates/primitives/gateway/src/block.rs

47.36842105263158% of new lines are covered

crates/primitives/receipt/src/from_starknet_provider.rs

100.0% of new lines are covered

crates/primitives/utils/src/lib.rs

100.0% of new lines are covered

crates/client/gateway/src/server/helpers.rs

0.0% of new lines are covered

crates/primitives/convert/src/hex_serde.rs

100.0% of new lines are covered

crates/client/gateway/src/server/router.rs

0.0% of new lines are covered

crates/node/src/service/gateway.rs

59.25925925925926% of new lines are covered

@jbcaron jbcaron force-pushed the gateway branch 4 times, most recently from b9b65a8 to 38256b5 Compare September 10, 2024 11:32
Copy link
Member

@cchudant cchudant left a 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 {
Copy link
Member

@cchudant cchudant Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why block provider name

Copy link
Member

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

crates/primitives/gateway/src/block.rs Show resolved Hide resolved
crates/tests/src/lib.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/server/handler.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/server/router.rs Show resolved Hide resolved

/// The gateway port to listen at.
#[arg(long, value_name = "GATEWAY PORT", default_value = "8080")]
pub gateway_port: u16,
Copy link
Member

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

Copy link
Member

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),
Copy link
Member

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());
Copy link
Member

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())
        );

@antiyro antiyro marked this pull request as ready for review September 12, 2024 12:08
@jbcaron jbcaron force-pushed the gateway branch 4 times, most recently from bbbf378 to 4eb62dc Compare September 19, 2024 12:20
Copy link
Collaborator

@Trantorian1 Trantorian1 left a 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 👍

crates/client/gateway/src/client/methods.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/methods.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/server/error.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/server/helpers.rs Outdated Show resolved Hide resolved
Copy link
Member

@cchudant cchudant left a 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

crates/client/gateway/src/server/error.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/server/handler.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/request_builder.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/server/handler.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/server/handler.rs Outdated Show resolved Hide resolved
crates/primitives/gateway/src/block.rs Show resolved Hide resolved
crates/primitives/gateway/src/test.rs Outdated Show resolved Hide resolved
crates/primitives/gateway/src/test.rs Outdated Show resolved Hide resolved
crates/primitives/utils/src/lib.rs Show resolved Hide resolved
crates/client/gateway/src/server/error.rs Outdated Show resolved Hide resolved
@antiyro antiyro changed the title feat: gateway feat(fgw): addition of a gateway + removal of starknet-provider Sep 25, 2024
crates/client/gateway/src/client/mod.rs Outdated Show resolved Hide resolved
crates/client/sync/src/fetch/fetchers_real_fgw_test.rs Outdated Show resolved Hide resolved
crates/client/sync/src/l2.rs Show resolved Hide resolved
crates/node/src/cli/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@cchudant cchudant left a 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

crates/client/gateway/src/server/handler.rs Outdated Show resolved Hide resolved
crates/primitives/gateway/src/receipt.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/builder.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/builder.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/methods.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/methods.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/methods.rs Outdated Show resolved Hide resolved
crates/client/gateway/src/client/methods.rs Outdated Show resolved Hide resolved
@antiyro antiyro merged commit 0c821ae into main Oct 3, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full node Full Node only related issues sequencer The label for sequencing related issues
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(provider): Replace SequencerGatewayProvider with native provider
5 participants