Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added page_read_chip and page_controller #29

Merged
merged 10 commits into from
May 29, 2024

Conversation

OsamaAlkhodairy
Copy link
Contributor

@OsamaAlkhodairy OsamaAlkhodairy commented May 24, 2024

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

Mostly cosmetic changes requested. Can merge after addressing.

The main other change (for another PR) is that we need a slightly different version to handle indexed pages - these are where the key are several columns that are part of the cached trace, and we don't have an auto-generate index starting from 0.

I think there will be use cases for both, but not sure how to best refactor to make things as modular as possible. This is something to keep in mind when writing the read/write chip.

In other words, the current index column with the constraint that it auto-increments by 1, is not needed if the page itself has columns for keys. One option is to make that index column optional, and configurable in the chip constructor.

chips/src/page_read/air.rs Show resolved Hide resolved
chips/src/page_read/mod.rs Outdated Show resolved Hide resolved
chips/src/page_read/columns.rs Show resolved Hide resolved
chips/src/page_read/mod.rs Outdated Show resolved Hide resolved
chips/src/page_read/mod.rs Outdated Show resolved Hide resolved
chips/tests/integration_test.rs Outdated Show resolved Hide resolved
chips/src/page_controller/trace.rs Show resolved Hide resolved
chips/src/page_read/trace.rs Outdated Show resolved Hide resolved
chips/tests/integration_test.rs Outdated Show resolved Hide resolved
chips/tests/integration_test.rs Outdated Show resolved Hide resolved
@jonathanpwang
Copy link
Contributor

@OsamaAlkhodairy for future use, and to stabilize the Page interface, let's add the cached trace commitment of the page as a public value to the PageReadChip. However this requires being able to convert Com<SC> into a fixed length array. This may be a little annoying in that you'll need an additional trait bound on Com<SC>. Concretely for current testing utils, Com<SC> is this Hash type. DIGEST_WIDTH is the width here but we don't want to hard-code this since it will change if we change from poseidon2 to another hash. This means I think we need to have the commitment WIDTH in field elements as a const generic.

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

LGTM, change some debug prints to log

chips/src/page_controller/mod.rs Outdated Show resolved Hide resolved
@OsamaAlkhodairy OsamaAlkhodairy merged commit bed5ce2 into main May 29, 2024
4 checks passed
@OsamaAlkhodairy OsamaAlkhodairy deleted the feat/page_read_chip branch May 29, 2024 21:38
luffykai pushed a commit that referenced this pull request Dec 13, 2024
* feat: added page_read_chip and page_controller

* fix: rust lint

* fix: rust lint

* chore: small edits

* chore: updating to use the engine

* chore: fixing imports

* wip

* feat: storing commitment in page_controller and other fixes

* fix: adding tracing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants