-
Notifications
You must be signed in to change notification settings - Fork 496
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
Add support for Value::one()
and Value::zero()
and maybe, Value::<F::From>::from()
#698
Comments
Yes we have quite a few instances of These are all in src/circuit/note_commit.rs and src/circuit/commit_ivk.rs. |
I'll try to work on this and see if I can come up with something that reduces a bit the overhead. Thanks for the data @daira ! I was afraid we were doing something really wrong lol |
The tricky part with providing these is that there exist no standard library traits for |
This does not work: pub trait ValueExt {
const ZERO: Self;
const ONE: Self;
}
impl ValueExt for Value<u64> {
const ZERO: Value<u64> = Value::known(0);
const ONE: Value<u64> = Value::known(1);
}
impl<F: Field> ValueExt for Value<F> {
const ZERO: Value<F> = Value::known(F::ZERO);
const ONE: Value<F> = Value::known(F::ONE);
} for the reason I described above:
However, if we are content to have separate extension traits for primitive types and |
Extending the discussion in #699 about traitifying |
Bringing the general discussion back in here. @CPerezz said in #699 (comment):
For Associated Type Defaults to help here, we'd need a separate trait that is a bound on In any case, the reason why the concrete type pub trait SomeValueWrapper<V> {
type Inner = Value<T>;
} where (ignoring the ATD issue I outlined above) we have the core inner type being a "value in a circuit cell", and a generic ergonomic wrapper around it. But then that is what we are already trying to provide with the concrete
What are the problems you are running into using
The core issue here is that there is no common trait for "decompose into bytes" (partly because const generics aren't advanced enough, but also I suspect because this abstraction is too abstract). Instead you have either context-specific generic traits like For this particular example, I presume that what you are describing looks something like this (please give a more concrete example if this one is incorrect): let foo: Value<jubjub::ExtendedPoint> = ...; // from somewhere
let bar: Value<[u8; 32]> = foo.map(|p| p.to_bytes());
let baz: [Value<u8>; 32] = bar.transpose_array(); or more concisely (which is what you'd actually write; the above is just to show the types): let foo = ...; // A Value<jubjub::ExtendedPoint> from somewhere
let baz = foo.map(|p| p.to_bytes()).transpose_array(); This seems fine to me? It's about as concise as I can imagine this being for the logically-equivalent It feels to me like some of the current issues can be addressed with more standard library trait helpers. The above works because we have |
Thanks for the elaborated reply @str4d . The transposing example is brilliant. And I did not think about it. Considering that this solves byte-conversion, we lack the I'll try to submit a PR with the things that would be required for us. And see if we can avoid bigger refactors which do not clearly bring an improvement to the current API. |
It's just quite annoying the need to express
Value::known(F::one())
when it's implicit that if you're passingone
, the value is known. Same happens with 0.And this verbosity is extending significantly across the entire zkevm-circuits repo for example. Where we have +100 places where this happens for
1
or0
.Also, there's +150 places where
Value::known(F::from(x))
appears. This just feels the same for me, as if you pass a value to construct aValue
, it definitely has to beknown
and therefore, the call toknown
can be implicit.Is this something you do also experience on Orchad circuits? Have you ever been thought about this too/would consider to adopt it??
The text was updated successfully, but these errors were encountered: