feat: L3 support #437
feat: L3 support #437
Conversation
Trantorian1
added
the
research
| while !page_indicator { | ||
| let events = provider | ||
| .get_events( | ||
| EventFilter { | ||
| from_block: filter.from_block, | ||
| to_block: filter.to_block, | ||
| address: filter.address, | ||
| keys: filter.keys.clone(), | ||
| }, | ||
| continuation_token.clone(), | ||
| 1000, | ||
| ) | ||
| .await?; | ||
|
|
||
| event_vec.extend(events.events); | ||
| if let Some(token) = events.continuation_token { | ||
| continuation_token = Some(token); | ||
| } else { | ||
| page_indicator = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
this code exists in get_events in mod.rs as well, can we use that?
There was a problem hiding this comment.
That cannot be used unfortunately as there are some constraints in stream implementation.
So when I used the function it kills the stream when no event is returned. So there are no new calls to the stream.
I am checking more on this behaviour on why this is happening.
|
|
||
| // Initialize database service | ||
| let db = Arc::new( | ||
| DatabaseService::new(&base_path, backup_dir, false, chain_info.clone(), Default::default()) |
There was a problem hiding this comment.
open_for_testing can be used instead of new
crates/node/src/service/l1.rs
Outdated
| devnet: bool, | ||
| mempool: Arc<Mempool>, | ||
| ) -> anyhow::Result<Box<dyn Service>> { | ||
| match config.settlement_layer { |
There was a problem hiding this comment.
should we add the if !config.l1_sync_disabled && (config.l1_endpoint.is_some() || !devnet) else {} part that is common in eth and starknet here? we can have this match statement inside the if part and it will return the client
There was a problem hiding this comment.
as discussed this common logic should be in the create function
There was a problem hiding this comment.
SettlementClientError::Other seems to be used a lot. We either add more error variants or break this down into more error types, perhaps allow implementors of SettlementClientTrait to define their own error type as part of the trait (might conflict with dyn uses of the type, idk).
I'm also not sure that having SettlementClientError::Other wrap anyhow::Error is a good idea. It seems like we are missing out on a lot of constant errors this way and this should probably be moved to a wrap a String instead.
There also are a few uses of Arc<Box<dyn>> which seems strange. These should also probably be reduced to Arc<dyn>. The issue seems to come from the usage of Box::pin instead of Arc::pin but I have not checked out every instance where this is being used.
| #[derive(thiserror::Error, Debug)] | ||
| pub enum SettlementClientError { | ||
| #[error("Missing required field: {0}")] | ||
| MissingField(&'static str), | ||
|
|
||
| #[error("Value exceeds Felt max value for field: {0}")] | ||
| ValueExceedsFeltRange(&'static str), | ||
|
|
||
| #[error("Invalid log: {0}")] | ||
| InvalidLog(String), | ||
|
|
||
| #[error("Invalid contract: {0}")] | ||
| InvalidContract(String), | ||
|
|
||
| #[error("Conversion error: {0}")] | ||
| ConversionError(String), | ||
|
|
||
| #[error("Ethereum RPC error: {0}")] | ||
| EthereumRpcError(#[from] alloy::sol_types::Error), | ||
|
|
||
| #[error(transparent)] | ||
| Other(#[from] anyhow::Error), | ||
|
|
||
| #[error("Invalid event: {0}")] | ||
| InvalidEvent(String), | ||
|
|
||
| #[error("Invalid response: {0}")] | ||
| InvalidResponse(String), | ||
|
|
||
| #[error("Database error: {0}")] | ||
| DatabaseError(String), | ||
|
|
||
| #[error("Invalid data: {0}")] | ||
| InvalidData(String), | ||
| } |
| })?), | ||
| transaction_hash: Felt::from_bytes_be_slice( | ||
| log.transaction_hash | ||
| .ok_or_else(|| SettlementClientError::MissingField("transaction_hash"))? |
There was a problem hiding this comment.
This probably doesn't need to be behind a ok_or_else since this is essentially a compile-time constant. Same for the other uses below.
| #[fixture] | ||
| async fn setup_test_env() -> TestRunner { | ||
| // Start Anvil instance | ||
| let anvil = Anvil::new().block_time(1).chain_id(1337).try_spawn().expect("failed to spawn anvil instance"); |
There was a problem hiding this comment.
Makes sense, thanks for the info 👍
| /// 8. Assert that the event is successfully pushed to the db | ||
| /// 9. TODO : Assert that the tx was correctly executed | ||
| #[rstest] | ||
| #[traced_test] |
| @@ -26,48 +27,51 @@ pub struct CommonMessagingEventData { | |||
| } | |||
|
|
|||
| pub async fn sync<C, S>( | |||
| settlement_client: Arc<Box<dyn ClientTrait<Config = C, StreamType = S>>>, | |||
| settlement_client: Arc<Box<dyn SettlementClientTrait<Config = C, StreamType = S>>>, | |||
There was a problem hiding this comment.
Shouldn't Arc<dyn SettlementClientTrait<Config = C, StreamType = S>> be enough here?
| Ok(self.get_state_call().await?[0]) | ||
| } | ||
|
|
||
| async fn get_last_verified_block_hash(&self) -> anyhow::Result<Felt> { | ||
| // Block Hash index in call response : 2 | ||
| async fn get_last_verified_block_hash(&self) -> Result<Felt, SettlementClientError> { | ||
| Ok(self.get_state_call().await?[2]) |
There was a problem hiding this comment.
Same here, if this is safe add a comment.
| async fn fail_create_new_client_contract_does_not_exists( | ||
| #[future] test_context: anyhow::Result<TestContext>, | ||
| ) -> anyhow::Result<()> { | ||
| let _guard = get_state_update_lock().lock().await; |
There was a problem hiding this comment.
I'm wondering if we could have the guard as a fixture
|
|
||
| // These functions can now be marked as deprecated as they're replaced by the context system | ||
| #[deprecated(note = "Use init_test_context() instead")] | ||
| pub async fn prepare_starknet_client_test() -> anyhow::Result<(StarknetAccount, Felt, MadaraProcess)> { |
There was a problem hiding this comment.
Are we still using this somewhere in the codebase?
| &STATE_UPDATE_LOCK | ||
| } | ||
|
|
||
| pub async fn cleanup_test_context() { |
There was a problem hiding this comment.
It seems like this should be implemented through some king of drop guard so that this method is always called even if a test errors or panics.
| pub struct SyncWorkerConfig<C: 'static, S> { | ||
| pub backend: Arc<MadaraBackend>, | ||
| pub settlement_client: Arc<Box<dyn SettlementClientTrait<Config = C, StreamType = S>>>, | ||
| pub l1_gas_provider: GasPriceProvider, | ||
| pub gas_price_sync_disabled: bool, | ||
| pub gas_price_poll_ms: Duration, | ||
| pub mempool: Arc<Mempool>, | ||
| pub ctx: ServiceContext, | ||
| pub l1_block_metrics: Arc<L1BlockMetrics>, | ||
| } |
Added support for L3s. Starknet client is added in this PR and tests are added for several client functions. for now we are assuming that the l3 gas prices will be zero only.
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
What is the current behavior?
Currently we didn't support the L3 appchain spec.
What is the new behavior?
Included the starknet client and updated the cli arguments for running madara in appchain mode. For now for all the gas prices and metrics we are still using the l1 modules. We can cover this refactoring in another PR.
Key Changes
crates/madara/client/ethis renamed tocrates/madara/client/settlement_client:├── Cargo.toml ├── README.md └── src / ├── client.rs ├── error.rs ├── eth / │ ├── event.rs │ ├── mod.rs │ └── starknet_core.json ├── gas_price.rs ├── lib.rs ├── messaging.rs ├── starknet / │ ├── event.rs │ ├── mod.rs │ └── utils.rs ├── state_update.rs ├── sync.rs └── utils.rs.watch()function for listening to events we have added a custom stream implementation for starknet (Starknet Stream Implementation)eth_core_contract_addressandeth_gps_statement_verifierin chain config in order to be more accommodating to other types we might use in future and we are currently using.Important
For now as the PR is itself very long and cannot be broken into components because of inter component and e2e messaging tests. All the variables which are related to l1 are not renamed right now as it will be more time consuming and has no impact on functionality as of now.
Simple Diagram for messaging components
Does this introduce a breaking change?
No