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

Bitwise constraints submodule refactor for modularity #213

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Mar 8, 2023

This PR is a proposed restructuring of the bitwise constraints updated in #180 to address the discussion in #122. This PR is based on #180, since it hasn't been merged yet.

The purpose of this is to make sure we're aligned on syntax and submodules before we write the rest of the constraints. (although note that this is a very minor change, so updating other files would still be simple)

A couple open questions:

  • are we ok with passing periodic columns in to evaluators for now? This is probably not ideal in general, but it will simplify imports for v0.3
  • what do we think of using a different keyword for submodules than for the primary AIR definition? I used mod instead of def here as a proposal

@grjte grjte force-pushed the grjte-refactor-bitwise-constraints branch from dea6fa6 to 4659dc7 Compare March 8, 2023 13:14
@grjte grjte requested review from Fumuran, tohrnii and bobbinth March 8, 2023 13:16
@Fumuran
Copy link
Contributor

Fumuran commented Mar 8, 2023

Looks good to me!
I like the way we pass periodic columns to evaluators.
I like the idea of different keyword for the module.

Copy link
Contributor

@tohrnii tohrnii left a comment

Choose a reason for hiding this comment

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

Thank you @grjte, I left a few points of discussions/questions.

constraints/miden-vm/bitwise.air Outdated Show resolved Hide resolved
constraints/miden-vm/bitwise.air Outdated Show resolved Hide resolved
constraints/miden-vm/bitwise.air Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! This is not a full review - but I did leave a couple of comments inline.

constraints/miden-vm/bitwise.air Outdated Show resolved Hide resolved
constraints/miden-vm/bitwise.air Outdated Show resolved Hide resolved
# Enforces that column must be binary
### Helper functions/evaluators ###################################################################

#! Enforces that column must be binary.
ev check_binary(main: [a]):
enf a^2 - a = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment for the future: I wonder if we should add some "built-in" functions to help with things like this. For example, we could have is_binary() defined on the language level so that we don't have to redefine it in different modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bobbinth I was thinking we could have something like a standard lib for simple functions and evaluators like these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let's discuss this for the future. It would be a nice thing to add perhaps in v0.4

@grjte grjte force-pushed the grjte-refactor-bitwise-constraints branch from 4659dc7 to 4e67557 Compare March 9, 2023 09:01
Base automatically changed from andrew-bitwise-constraints to next March 9, 2023 09:01
@grjte grjte marked this pull request as ready for review March 9, 2023 09:02
@grjte
Copy link
Contributor Author

grjte commented Mar 9, 2023

Thank you all for the preliminary review & discussion. I believe we're aligned on syntax for submodules now as summarized in #122 , and I've updated the PR accordingly, so it's now ready for review

@grjte grjte requested review from tohrnii and bobbinth March 9, 2023 09:04
Copy link
Contributor

@tohrnii tohrnii left a comment

Choose a reason for hiding this comment

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

Thank you @grjte, looks good to me.

Copy link
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @grjte!

@grjte grjte merged commit e8a3334 into next Mar 23, 2023
@grjte grjte deleted the grjte-refactor-bitwise-constraints branch March 23, 2023 14:46
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.

4 participants