-
Notifications
You must be signed in to change notification settings - Fork 115
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
Specify feature flags for Pickles.sideLoaded
#1688
Conversation
src/lib/proof-system/zkprogram.ts
Outdated
* You can also toggle specific feature flags manually by specifying them here. | ||
* Alternatively, you can use {@FeatureFlags.fromZkProgram} to compute the set of feature flags that are compatible with a given program. | ||
*/ | ||
static featureFlags: FeatureFlags = FeatureFlags.allMaybe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't changing the default from None
to Maybe
a breaking change?
I do agree that maybe makes sense as the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it does, sadly.. I'll set it to none
for now to maintain backwards compatibility :/ The good thing is developers will notice that they need to change something when proofs with different feature flags wont verify
src/lib/proof-system/zkprogram.ts
Outdated
* You can also toggle specific feature flags manually by specifying them here. | ||
* Alternatively, you can use {@FeatureFlags.fromZkProgram} to compute the set of feature flags that are compatible with a given program. | ||
*/ | ||
static featureFlags: FeatureFlags = FeatureFlags.allMaybe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it does, sadly.. I'll set it to none
for now to maintain backwards compatibility :/ The good thing is developers will notice that they need to change something when proofs with different feature flags wont verify
return flags; | ||
} | ||
|
||
function featureFlagsToMlOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the old binary true | false
feature flag data structure and replaced it entirely with the new option
one, in picklesRuleFromFunction
as well so we dont have to maintain multiple of these conversion helpers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final comment, otherwise lgtm!
src/lib/proof-system/zkprogram.ts
Outdated
@@ -40,6 +37,7 @@ import { | |||
import { ProvablePure } from '../provable/types/provable-intf.js'; | |||
import { prefixToField } from '../../bindings/lib/binable.js'; | |||
import { prefixes } from '../../bindings/crypto/constants.js'; | |||
import { rangeCheck0 } from '../provable/gates.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this import is accidental and doesn't belong here
Allows of custom specification of feature flags under the
DynamicProof
classbindings o1-labs/o1js-bindings#277
closes #1686