feat: L3 support #437
feat: L3 support #437
Conversation
| @@ -68,3 +68,6 @@ tmp/ | |||
| # Running madara with make and docker compose | |||
| .secrets | |||
| image.tar.gz | |||
|
|
|||
| # madara test artifacts for testing l2 clients for appchains | |||
| test-artifacts/madara | |||
There was a problem hiding this comment.
should this be in the root folder?
There was a problem hiding this comment.
According to me ig test-artifacts folder should be there because we also have a devnet.yaml file in that.
There was a problem hiding this comment.
but why in root? it should be where the testcase is right
| starknet-accounts = "0.11.0" | ||
| starknet-contract = "0.11.0" | ||
| starknet-core = { workspace = true } | ||
| starknet-providers = { workspace = true } | ||
| starknet-signers = { workspace = true } |
There was a problem hiding this comment.
should we put these behind feature flags?
There was a problem hiding this comment.
So these are workspace dependencies they are already being built. Do you think it will be reducing any build time ?
There was a problem hiding this comment.
workspace dependencies should not be built unless being used in a crate afaik. this is also a good time to check if we should move starknet/eth into crates btw
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
can we test listen_for_update_state_events
There was a problem hiding this comment.
oh ok, I noticed we've a test for this in the common starknet/eth file. can we move it inside eth and starknet because they've different logics?
There was a problem hiding this comment.
But shouldn't the tests be with the module ?
| l1_gas_provider, | ||
| chain_id, | ||
| gas_price_sync_disabled: !gas_price_sync_enabled, | ||
| gas_price_poll, | ||
| mempool, | ||
| }) | ||
| } | ||
|
|
||
| // Factory method to create the appropriate service | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
should we make it a struct?
There was a problem hiding this comment.
I tried but lifetime specifier ka issue aa rha h. will try it in next commit.
| ) | ||
| .await?; | ||
| // Ensure correct read call : u256 (0, 0) | ||
| assert_eq!(call_res.len(), 2, "l1_to_l2_message_cancellations should return only 2 values"); |
| } | ||
| } | ||
|
|
||
| async fn listen_for_messaging_events( |
There was a problem hiding this comment.
This is the flow right now
I think this is how the flow should be like
You should look at this for implementing streams - https://docs.rs/futures/latest/futures/stream/trait.Stream.html
This is what I think alloy is also using fro event streams
| BlockId::Number(starting_block), | ||
| BlockId::Number(latest_block_number), |
There was a problem hiding this comment.
what happens if the gap is too big between start and end block? do RPCs throw an error? I think there's like a max 10k gap limit but we should confirm
| } | ||
| } | ||
|
|
||
| sleep(Duration::from_secs(5)).await; |
There was a problem hiding this comment.
we can reduce this time to ms I think
| } else if latest_block_number < 100 { | ||
| 0 | ||
| } else { | ||
| latest_block_number - 100 |
There was a problem hiding this comment.
instead of looping over last 100 blocks every time, I think we can keep state in memory here. the stream implementation i linked to allows for that
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.
Does this introduce a breaking change?
No