Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

CALLDATALOAD: bus-mapping and refactoring#402

Merged
CPerezz merged 5 commits into
privacy-ethereum:mainfrom
scroll-tech:feat/busmapping-calldataload
Apr 12, 2022
Merged

CALLDATALOAD: bus-mapping and refactoring#402
CPerezz merged 5 commits into
privacy-ethereum:mainfrom
scroll-tech:feat/busmapping-calldataload

Conversation

@roynalnaruto
Copy link
Copy Markdown
Collaborator

@roynalnaruto roynalnaruto commented Mar 23, 2022 •
edited
Loading

Depends on handling internal calls:

This PR:

@github-actions github-actions Bot added the crate-bus-mapping Issues related to the bus-mapping workspace member label Mar 23, 2022
@lispc
Copy link
Copy Markdown
Collaborator

lispc commented Mar 23, 2022 •
edited
Loading

@roynalnaruto i think it is better to add some bus-mapping generated tests in circuit of calldataload

@github-actions github-actions Bot added the T-opcode Type: opcode-related and focused PR/Issue label Mar 23, 2022
@ed255 ed255 self-requested a review March 23, 2022 10:28
@CPerezz CPerezz self-requested a review March 30, 2022 07:30
@han0110 han0110 mentioned this pull request Apr 2, 2022
Merged
6 tasks
@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Apr 5, 2022

Until the PR is not up to date with main we will halt our reviews.

Feel free to update when you consider @lispc :)

@roynalnaruto roynalnaruto force-pushed the feat/busmapping-calldataload branch from 1ff0c82 to 666af54 Compare April 5, 2022 18:26
@roynalnaruto roynalnaruto requested a review from a team April 5, 2022 18:26
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

roynalnaruto commented Apr 5, 2022

Feel free to update

@CPerezz I've resolved the conflicts :)

icemelon
icemelon reviewed Apr 5, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

Since we have CALL in the busmapping, probably we can add CALLDATALOAD bus-mapping for internal call case as well

@roynalnaruto roynalnaruto force-pushed the feat/busmapping-calldataload branch from 5911330 to b8eb1f0 Compare April 7, 2022 06:22
CPerezz
CPerezz suggested changes Apr 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Good job with this!

I'm missing overall testcases for non-root calls as well as the implementation for the caller memory read.

Also added some nits and suggestions! 😄

Comment thread bus-mapping/src/evm/opcodes/calldataload.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/calldataload.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/calldataload.rs
Comment thread bus-mapping/src/evm/opcodes/calldataload.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/calldataload.rs Outdated
Comment thread bus-mapping/src/evm/opcodes/calldataload.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/calldataload.rs Outdated
@icemelon icemelon mentioned this pull request Apr 8, 2022
Closed
@lispc lispc mentioned this pull request Apr 11, 2022
Closed
@roynalnaruto roynalnaruto mentioned this pull request Apr 11, 2022
Merged
@roynalnaruto roynalnaruto force-pushed the feat/busmapping-calldataload branch from b8eb1f0 to fbf9112 Compare April 11, 2022 13:55
@CPerezz CPerezz removed the opcode label Apr 11, 2022
@github-actions github-actions Bot added crate-zkevm-circuits Issues related to the zkevm-circuits workspace member and removed T-opcode Type: opcode-related and focused PR/Issue labels Apr 12, 2022
@roynalnaruto roynalnaruto force-pushed the feat/busmapping-calldataload branch from acd8b1d to 0ed71bd Compare April 12, 2022 07:58
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

roynalnaruto commented Apr 12, 2022

@CPerezz @han0110 This PR is ready as well. The build will pass once #450 is merged and base branch is updated. Since #450 includes the fix to add Range64 and Range1024 to the fixed tables.

@roynalnaruto roynalnaruto changed the title bus-mapping for CALLDATALOAD opcode CALLDATALOAD: bus-mapping and refactoring Apr 12, 2022
CPerezz
CPerezz reviewed Apr 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

I just have some nits.

Other than that LGTM.

Comment thread bus-mapping/src/evm/opcodes/calldataload.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/calldataload.rs Outdated
@roynalnaruto roynalnaruto force-pushed the feat/busmapping-calldataload branch from eaa7387 to a69d474 Compare April 12, 2022 09:01
han0110
han0110 approved these changes Apr 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

@roynalnaruto roynalnaruto requested a review from CPerezz April 12, 2022 11:17
CPerezz
CPerezz approved these changes Apr 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!!

@CPerezz CPerezz merged commit 31ae5a1 into privacy-ethereum:main Apr 12, 2022
@roynalnaruto roynalnaruto deleted the feat/busmapping-calldataload branch April 12, 2022 12:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ed255 ed255 Awaiting requested review from ed255

@icemelon icemelon Awaiting requested review from icemelon

2 more reviewers

@han0110 han0110 han0110 approved these changes

@CPerezz CPerezz CPerezz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@roynalnaruto @lispc @CPerezz @icemelon @han0110