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: add inherit circuit tag #387

Merged
merged 4 commits into from
Nov 18, 2022
Merged

feat: add inherit circuit tag #387

merged 4 commits into from
Nov 18, 2022

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Oct 22, 2022

Resolves #385.

This PR adds new "inherit" tag to the circuit which allows inherit the visibility of the parent object. Useful for custom defined types.

Meanwhile, updated also some comments to take advantage of new go doc formatting rules (links and lists).

@ivokub ivokub added bug Something isn't working new feature labels Oct 22, 2022
@ivokub ivokub added this to the v0.8.0 milestone Oct 22, 2022
@ivokub ivokub requested a review from gbotrel October 22, 2022 12:22
@ivokub ivokub self-assigned this Oct 22, 2022
@ivokub ivokub marked this pull request as draft October 23, 2022 21:30
@ivokub
Copy link
Collaborator Author

ivokub commented Oct 23, 2022

There was before some inconsistency how the tags were parsed. If there was not tag, then we set the visibility from the parent (https://github.com/ConsenSys/gnark/blob/develop/frontend/schema/schema.go#L256 and https://github.com/ConsenSys/gnark/blob/develop/frontend/schema/schema.go#L281). If there was only a name tag gnark:"X", then we set the visibility to secret (https://github.com/ConsenSys/gnark/blob/develop/frontend/schema/schema.go#L264). The tests didn't have the edge case.

I'm trying to make the behaviour consistent and keep the existing promise. So, on top-level if there is no tag, then the visibility is left to Unset, but this resolves (even if the top-level element has sub-elements) in the variable parser to Secret. In every other case when there is not tag, we assume the visibility of the parent.

If there is only name tag, then for top-level variables we assume Secret visibility and lower levels inherit from the parent (this is the difference, previously we set the visibility to Secret).

Added also explicit "inherit" tag option which is like empty one, but which can not be set for the top-level element. I made this change before figuring out the name-only tag cases and now seems to be slightly redundant. But dunno, maybe is useful.

Question -- shouldn't we in the case when there is not tag and we are top-level, set the visibility to Secret eagerly? E.g. when we hit the case https://github.com/ConsenSys/gnark/blob/develop/frontend/schema/schema.go#L280. It should work now also nicely as it expected, but we take slightly different paths (compared to when we have name-only tag) and this could at some point create inconsistent behaviour.

@ivokub ivokub marked this pull request as ready for review October 23, 2022 22:48
@gbotrel gbotrel merged commit 03c9545 into develop Nov 18, 2022
@gbotrel gbotrel deleted the feat/inherit-vis branch November 18, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants