feat: L3 support #437

ocdbytes · 2024-12-20T16:30:19Z

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:

Feature
Testing

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

apoorvsadana · 2024-12-21T13:22:50Z

.gitignore

@@ -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


should this be in the root folder?

According to me ig test-artifacts folder should be there because we also have a devnet.yaml file in that.

but why in root? it should be where the testcase is right

apoorvsadana · 2024-12-21T13:24:28Z

crates/client/settlement_client/Cargo.toml

+starknet-accounts = "0.11.0"
+starknet-contract = "0.11.0"
+starknet-core = { workspace = true }
+starknet-providers = { workspace = true }
+starknet-signers = { workspace = true }


should we put these behind feature flags?

So these are workspace dependencies they are already being built. Do you think it will be reducing any build time ?

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

crates/client/settlement_client/src/client.rs

crates/client/settlement_client/src/eth/mod.rs

crates/client/settlement_client/src/starknet/utils.rs

crates/client/settlement_client/src/starknet/mod.rs

apoorvsadana · 2024-12-23T11:04:14Z

crates/client/settlement_client/src/starknet/mod.rs

+
+        Ok(())
+    }
+}


can we test listen_for_update_state_events

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?

But shouldn't the tests be with the module ?

should listen_for_update_state_events now be removed in favor of our event stream?

apoorvsadana · 2024-12-23T11:20:02Z

crates/node/src/service/l1.rs

            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)]


should we make it a struct?

I tried but lifetime specifier ka issue aa rha h. will try it in next commit.

…ts 1

apoorvsadana · 2024-12-28T11:57:46Z

crates/client/settlement_client/src/starknet/mod.rs

+            )
+            .await?;
+        // Ensure correct read call : u256 (0, 0)
+        assert_eq!(call_res.len(), 2, "l1_to_l2_message_cancellations should return only 2 values");


same as before

apoorvsadana · 2024-12-28T12:58:46Z

crates/client/settlement_client/src/starknet/mod.rs

+        }
+    }
+
+    async fn listen_for_messaging_events(


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

apoorvsadana · 2024-12-28T12:59:53Z

crates/client/settlement_client/src/starknet/mod.rs

+                BlockId::Number(starting_block),
+                BlockId::Number(latest_block_number),


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

apoorvsadana · 2024-12-28T13:02:01Z

crates/client/settlement_client/src/starknet/mod.rs

+                }
+            }
+
+            sleep(Duration::from_secs(5)).await;


we can reduce this time to ms I think

apoorvsadana · 2024-12-28T13:06:02Z

crates/client/settlement_client/src/starknet/mod.rs

+            } else if latest_block_number < 100 {
+                0
+            } else {
+                latest_block_number - 100


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

apoorvsadana · 2025-01-07T19:28:23Z

crates/client/settlement_client/src/messaging/mod.rs

why do a folder for messaging?

Initially made for the message processor approach will remove.

apoorvsadana · 2025-01-07T19:30:41Z

crates/client/settlement_client/src/starknet/mod.rs

+
+        Ok(())
+    }
+}


should listen_for_update_state_events now be removed in favor of our event stream?

apoorvsadana · 2025-01-07T19:36:33Z

crates/client/settlement_client/src/messaging/sync.rs

+// L2 (Starknet) <-> L3 messaging format
+// GitHub Ref : https://github.com/cartridge-gg/piltover/blob/saya/src/messaging/component.cairo#L85
+#[derive(Clone)]
+pub struct MessageSent {


i don't think this is being used anywhere

removed sorry missed it.

apoorvsadana · 2025-01-07T19:40:17Z

crates/client/settlement_client/src/client.rs

+    // ============================================================
+    // Stream Implementations :
+    // ============================================================
+    type StreamType: Stream<Item = Option<anyhow::Result<CommonMessagingEventData>>>;
+    async fn get_event_stream(&self, last_synced_event_block: LastSyncedEventBlock)


comment can be better

Adding a proper description.

// Stream implementation for settlement client. // StreamType represents the data returned by the stream and the implemented version should always return `CommonMessagingEventData` type StreamType: Stream<Item = Option<anyhow::Result<CommonMessagingEventData>>>; // This function should return the event stream implementation that is adhering to the `StreamType` return type. async fn get_event_stream(&self, last_synced_event_block: LastSyncedEventBlock) -> anyhow::Result<Self::StreamType>;

This description works ? Or should I do like more elaborate description about the stream implementation design ?

apoorvsadana · 2025-01-07T20:27:10Z

crates/client/settlement_client/src/messaging/sync.rs

+    let res = mempool.accept_l1_handler_tx(transaction.into(), fees.unwrap_or(0))?;
+
+    // TODO: remove unwraps
+    // Ques: shall it panic if no block number of event_index?


why are block_num and event_index optional

event index is optional as initially also it was optional. Now have made the block_number as non option.

apoorvsadana · 2025-01-07T20:28:13Z

crates/client/settlement_client/src/messaging/sync.rs

+    // TODO: remove unwraps
+    // Ques: shall it panic if no block number of event_index?
+    let block_sent = LastSyncedEventBlock::new(settlement_layer_block_number.unwrap(), event_index.unwrap_or(0));
+    backend.messaging_update_last_synced_l1_block_with_event(block_sent)?;


isn't this also happening in the caller?

removed from process_message function.

apoorvsadana · 2025-01-07T20:29:25Z

crates/client/settlement_client/src/messaging/sync.rs

+    Some(u128::from_be_bytes(padded.try_into().unwrap()))
+}
+
+fn to_hex_string(bytes: &[u8]) -> String {


hex::encode should work here (already imported)

hex::encode(event_data.transaction_hash.clone().unwrap_or(vec![0]).as_slice()), hex::encode(event_data.from.as_slice()),

apoorvsadana · 2025-01-07T20:43:56Z

crates/client/settlement_client/src/messaging/sync.rs

+    pub from: Vec<u8>,
+    pub to: Vec<u8>,
+    pub selector: Vec<u8>,
+    pub nonce: Vec<u8>,
+    pub payload: Vec<Vec<u8>>,
+    pub fee: Option<Vec<u8>>,
+    pub transaction_hash: Option<Vec<u8>>,
+    pub message_hash: Option<Vec<u8>>,
+    pub block_number: Option<u64>,
+    pub event_index: Option<u64>,


does everything need to be optional + raw u8 type? i think we should be able to reuse some types like ContractAddress etc. right?

I have changed the block_number as non optional.

Current implementation :

#[derive(Clone)] pub struct CommonMessagingEventData { pub from: Vec<u8>, pub to: Vec<u8>, pub selector: Vec<u8>, pub nonce: Vec<u8>, pub payload: Vec<Vec<u8>>, pub fee: Option<Vec<u8>>, // not returned in L2 event pub transaction_hash: Vec<u8>, pub message_hash: Option<Vec<u8>>, // not returned in L1 event pub block_number: u64, pub event_index: Option<u64>, // not returned in L2 event }

I chose Vec as it can be converted into any type for both ethereum and starknet and type varies across the different clients.

apoorvsadana · 2025-01-07T20:47:18Z

crates/client/settlement_client/src/starknet/event.rs

+                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;
+                    }
+                }


this code exists in get_events in mod.rs as well, can we use that?

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.

apoorvsadana · 2025-01-07T20:48:11Z

crates/client/settlement_client/src/starknet/event.rs

+
+type FutureType = Pin<Box<dyn Future<Output = anyhow::Result<(Option<EmittedEvent>, EventFilter)>> + Send>>;
+
+pub struct StarknetEventStream {


we also need tests for this and the eth stream right?

Adding tests (In progress ⌛.....)

Mohiiit

I need some more context in order to understand the event structure a bit better, by structure I mean, how we are fetching for both cases and how is cancelling of messages is working.

crates/client/settlement_client/Cargo.toml

crates/client/settlement_client/src/client.rs

Mohiiit · 2025-01-09T11:16:01Z

crates/client/settlement_client/src/eth/mod.rs

        let block_number = self.provider.get_block_number().await?.as_u64();
        Ok(block_number)
    }

    /// Get the block number of the last occurrence of a given event.
-    pub async fn get_last_event_block_number<T: SolEvent>(&self) -> anyhow::Result<u64> {


why have we removed this? ideally this function should be able to filter out on any event right? if not let's update it's name and comment accordingly?

This is not removed get_last_event_block_number function still exists although it is not used anywhere, but it is still there in the client trait implementation.

my bad for wrong comment, I meant why we removed <T: SolEvent>?

Mohiiit · 2025-01-09T11:21:20Z

crates/client/settlement_client/src/eth/mod.rs

+    // Stream Implementations :
+    // ============================================================
+    type StreamType = EthereumEventStream;
+    async fn get_event_stream(


we should change it's name or we can it more generic?

Can you give an example ? Ik the current stream implementation is not the best approach as there are many things I am also learning when I am writing tests for streams.

by generic I mean, instead of using hardcoded event, what if we make it take which would be solEvent and then pass on that to the function below? in future we would be able to use this function to create event stream of any event? wdyt?

crates/client/settlement_client/src/messaging/sync.rs

Mohiiit · 2025-01-09T12:09:24Z

crates/client/settlement_client/src/state_update.rs

+
+        // Initialize database service
+        let db = Arc::new(
+            DatabaseService::new(&base_path, backup_dir, false, chain_info.clone(), Default::default())


open_for_testing can be used instead of new

crates/client/settlement_client/src/state_update.rs

crates/node/src/cli/chain_config_overrides.rs

crates/node/src/main.rs

Mohiiit · 2025-01-09T12:22:56Z

test-artifacts/devnet.yaml

we can fetch it from the configs/presets/devnet.yaml?

Makes sense but I changed the time in this devnet.yaml. Should I write a shell script to change that particular param ?

you can use --chain-config-override cli param to update the block_time and other stuff too if you need

example: --chain-config-override=block_time=30s,pending_block_update_time=2s

ocdbytes added 7 commits December 18, 2024 23:54

feat : l3 support init

8949443

code : refactor

abc808e

feat : added tests for starknet client

a49e209

changelog update

0d6168e

Merge branch 'main' into l3-support

76bc944

fix : lint

75229a4

feat: added update state worker test for starknet client

661cd33

apoorvsadana marked this pull request as draft December 23, 2024 03:40

apoorvsadana reviewed Dec 23, 2024

View reviewed changes

ocdbytes added 4 commits December 26, 2024 19:13

feat : added messaging implementation for l2 client & resolved commen…

62fb337

…ts 1

feat : updated l2 messaging implementation

d4aa988

feat : updated l2 messaging implementation

a4314be

feat : updated messaging logic

9dea223

apoorvsadana reviewed Dec 28, 2024

View reviewed changes

Trantorian1 assigned ocdbytes Dec 31, 2024

Trantorian1 added the research Research and exploration required before implementation label Dec 31, 2024

ocdbytes added 3 commits January 6, 2025 17:25

clients refactoring done

fafd2c3

refactor : tests

aa7834e

fix : tests

6c1ff95

apoorvsadana reviewed Jan 7, 2025

View reviewed changes

Mohiiit reviewed Jan 9, 2025

View reviewed changes

		BlockId::Number(starting_block),
		BlockId::Number(latest_block_number),


		type FutureType = Pin<Box<dyn Future<Output = anyhow::Result<(Option<EmittedEvent>, EventFilter)>> + Send>>;

		pub struct StarknetEventStream {

feat: L3 support #437

Are you sure you want to change the base?

feat: L3 support #437

Conversation

ocdbytes commented Dec 20, 2024

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mohiiit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mohiiit Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mohiiit Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Mohiiit Jan 10, 2025 •

edited

Loading

Mohiiit Jan 10, 2025 •

edited

Loading