Skip to content

Conversation

@einat-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@einat-starkware einat-starkware marked this pull request as ready for review July 3, 2025 12:31
Copy link
Contributor Author

einat-starkware commented Jul 3, 2025

@github-actions
Copy link

github-actions bot commented Jul 3, 2025

Benchmark movements: No major performance changes detected.

@einat-starkware einat-starkware changed the base branch from einat/chain_info_l3_compatibility to graphite-base/7763 July 3, 2025 12:47
@einat-starkware einat-starkware changed the base branch from graphite-base/7763 to einat/chain_info_l3_compatibility July 3, 2025 13:11
@einat-starkware einat-starkware force-pushed the einat/add_l1_address branch 3 times, most recently from 35d402a to 3b0b16e Compare July 6, 2025 06:40
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 11 files at r1.
Reviewable status: 5 of 11 files reviewed, 6 unresolved discussions


crates/apollo_batcher/src/cende_client_types.rs line 258 at r1 (raw file):

            call.execution.l2_to_l1_messages.iter().map(|l2_to_l1_message| L2ToL1Message {
                from_address: call.call.caller_address,
                to_address: EthAddress::try_from(l2_to_l1_message.message.to_address).unwrap(),

Add informative message (we don't use plain unwrap in production code)

Suggestion:

.except("..."),

crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 423 at r1 (raw file):

impl SyscallRequest for SendMessageToL1Request {
    // The Cairo struct contains: `to_address`, `payload_size`, `payload`.
    fn read(

What about the deprecated syscall?


crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 427 at r1 (raw file):

        ptr: &mut Relocatable,
    ) -> DeprecatedSyscallExecutorBaseResult<SendMessageToL1Request> {
        let felt_to_address = felt_from_ptr(vm, ptr)?;

"felt_to_address" sounds like a conversion from felt to address

Suggestion:

to_address_felt

crates/blockifier/src/execution/syscalls/vm_syscall_utils.rs line 407 at r1 (raw file):

        ptr: &mut Relocatable,
    ) -> SyscallBaseResult<SendMessageToL1Request> {
        let felt_to_address = felt_from_ptr(vm, ptr)?;

Suggestion:

 to_address_felt

crates/apollo_rpc_execution/src/objects.rs line 467 at r1 (raw file):

            message: MessageToL1 {
                from_address,
                to_address: EthAddress::try_from(blockifier_message.message.to_address).unwrap(),

Same


crates/blockifier/src/execution/native/syscall_handler.rs line 571 at r1 (raw file):

        )?;

        if !self.base.context.tx_context.block_context.chain_info.is_layer_3 {

To share this code between the VM and Native, please move this to self.base.send_message_to_l1

Code quote:

if !self.base.context.tx_context.block_context.chain_info.is_layer_3 {

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r1.
Reviewable status: 6 of 11 files reviewed, 6 unresolved discussions (waiting on @einat-starkware)

@einat-starkware einat-starkware changed the base branch from einat/chain_info_l3_compatibility to graphite-base/7763 July 6, 2025 10:42
@einat-starkware einat-starkware changed the base branch from graphite-base/7763 to einat/chain_info_l3_compatibility July 6, 2025 12:10
Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 12 files reviewed, 4 unresolved discussions (waiting on @Yoni-Starkware)


crates/apollo_batcher/src/cende_client_types.rs line 258 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Add informative message (we don't use plain unwrap in production code)

Done.


crates/apollo_rpc_execution/src/objects.rs line 467 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Same

Done.


crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 423 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

What about the deprecated syscall?

How exactly do I handle the deprecated syscall? I'm not sure how to access the ChainInfo here


crates/blockifier/src/execution/native/syscall_handler.rs line 571 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

To share this code between the VM and Native, please move this to self.base.send_message_to_l1

Done.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r2, all commit messages.
Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @einat-starkware)


crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 423 at r1 (raw file):

Previously, einat-starkware wrote…

How exactly do I handle the deprecated syscall? I'm not sure how to access the ChainInfo here

Here, using the syscall_hanlder

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @einat-starkware)


crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 423 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Here, using the syscall_hanlder

(Similar to what you did in the non-deprecated handler)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r2.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @einat-starkware)

Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 13 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)


crates/blockifier/src/execution/deprecated_syscalls/mod.rs line 423 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

(Similar to what you did in the non-deprecated handler)

Done.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @einat-starkware)

@einat-starkware einat-starkware changed the base branch from einat/chain_info_l3_compatibility to graphite-base/7763 July 9, 2025 07:07
@einat-starkware einat-starkware changed the base branch from graphite-base/7763 to main July 9, 2025 07:07
Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 6 of 13 files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

Copy link
Contributor Author

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 7 files at r2, 2 of 2 files at r3, 7 of 7 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @einat-starkware)

@einat-starkware einat-starkware enabled auto-merge July 9, 2025 07:34
@einat-starkware einat-starkware added this pull request to the merge queue Jul 9, 2025
Merged via the queue into main with commit 7eb420d Jul 9, 2025
35 of 56 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2025
@einat-starkware einat-starkware deleted the einat/add_l1_address branch July 15, 2025 08:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants