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

Implement generic log #238

Merged
merged 5 commits into from
Dec 16, 2024
Merged

Implement generic log #238

merged 5 commits into from
Dec 16, 2024

Conversation

varunthakore
Copy link
Contributor

This PR implements logging of different types for R1CS backend. It is based on PR #231. Currently, the log function supports only single argument.

@mimoo
Copy link
Contributor

mimoo commented Nov 25, 2024

This is great! Can you screenshot an example to see that it's working? Also what is the error when you pass multiple arguments? I thought it would work :o

@varunthakore
Copy link
Contributor Author

This is great! Can you screenshot an example to see that it's working? Also what is the error when you pass multiple arguments? I thought it would work :o

These screenshots are generated by running tests from src/tests/example.rs.

Log Field and Bool.
Screenshot 2024-11-25 at 5 30 49 PM

Log Array.
Screenshot 2024-11-25 at 5 29 27 PM

Log Custom types.
Screenshot 2024-11-25 at 5 32 36 PM

When you pass multiple arguments to log() it throws error MismatchFunctionArguments because of the argument length check at the following places:

  1. https://github.com/varunthakore/noname/blob/c6f240a5ed803073f2d5c8809412cad9aa7a4fb6/src/mast/mod.rs#L1321-L1327
  2. https://github.com/varunthakore/noname/blob/c6f240a5ed803073f2d5c8809412cad9aa7a4fb6/src/type_checker/checker.rs#L873-L879

I think these checks cannot be skipped because it will be required for the generic assert_eq() function.

Copy link
Collaborator

@katat katat left a comment

Choose a reason for hiding this comment

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

This looks great!
I left a couple comments. It would be great to recursively print out the array or struct.

}

// Array
Some(TyKind::Array(_, _)) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also it could be recursive array.

For example

  1. [[1, 2],[3, 4]]
  2. [thing1, thing2] // thing* are structs

So maybe it would be better to recursively print out the elements based on the element's type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Log Array Array Field.
array_array_field

Log Array Array Bool.
array_array_bool

Log Array Custom.
array_custom

}

// Custom types
Some(TyKind::Custom { module: _, name: _ }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to the comment about recursive logging ^
I think it would be better to print out like

Thing1 {
  xx: ..
  another_thing: {...} recursively print out
  arr: [...] // recursively print out
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Log Custom Example 1
custom1

Log Custom Example 2
custom2

@varunthakore varunthakore requested a review from katat December 12, 2024 19:18
Copy link
Collaborator

@katat katat 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!
I add a couple minor comments. Also please resolve the code conflicts so it can be merged.

src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
src/backends/r1cs/mod.rs Outdated Show resolved Hide resolved
@varunthakore varunthakore requested a review from katat December 14, 2024 21:33
Copy link
Collaborator

@katat katat left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@katat katat merged commit 8188bf6 into zksecurity:main Dec 16, 2024
1 check passed
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.

3 participants