Add helper traits for constructing Value::known constants
#699
Conversation
| Value::known(Assigned::Zero) | ||
| FieldValue::ZERO |
There was a problem hiding this comment.
In cases like this, the helper constant is a clear win.
| sign: Value::known(pallas::Base::one()), | ||
| sign: FieldValue::ONE, |
There was a problem hiding this comment.
Similarly here (though the length difference could be smaller as pallas::Base::ONE is now also valid).
| magnitude_error: Value::known(pallas::Base::from(1 << 1)), | ||
| }, | ||
| // -2^64 | ||
| MyCircuit { | ||
| magnitude: Value::known(pallas::Base::from_u128(1 << 64)), | ||
| sign: Value::known(-pallas::Base::one()), | ||
| sign: -Value::<pallas::Base>::ONE, |
There was a problem hiding this comment.
However, situations like this require type annotations (to know which impl Neg to use), so you save a bit of length overall but it's still roughly as complex (if not moreso).
There was a problem hiding this comment.
I think adding the need of the explicit type requirement makes it worse TBH. Although -1 is also one of the "magic numbers" usually. But I kinda agree that should not have it's own constant..
There was a problem hiding this comment.
Why not (have its own constant)? IIRC, -1 was the most common constant in the Orchard circuit.
There was a problem hiding this comment.
Yes I was remembering correctly: #698 (comment)
There was a problem hiding this comment.
Why not (have its own constant)?
For -1 specifically it would help:
let a = -Value::<pallas::Base>::ONE;
let a = FieldValue::MINUS_ONE;But the point still applies generally.
| @@ -979,7 +978,7 @@ mod tests { | |||
| config.q.enable(&mut region, 1)?; | |||
|
|
|||
| // Assign a = 0. | |||
| region.assign_advice(|| "a", config.a, 0, || Value::known(Fp::ZERO))?; | |||
| region.assign_advice(|| "a", config.a, 0, || Value::<Fp>::ZERO)?; | |||
There was a problem hiding this comment.
Similarly here we cannot avoid the type annotation because of how flexibly the Region::assign_advice API is (intentionally) set up, so we only save a few characters.
| let mut digest = [BlockWord(Value::known(0)); DIGEST_SIZE]; | ||
| let mut digest = [BlockWord(IntegerValue::ZERO); DIGEST_SIZE]; |
There was a problem hiding this comment.
Here the result is actually longer, as for primitive integers the type can often be elided.
|
Will continue the discussion of #698 here. Thanks @str4d for taking initiative on this. The issues are clear. And they obviously prevent us to go on the direction this PR is IMO.. For some reason I was thinking that maybe GATs would allow us to do something like: pub trait Value<T> {
type Inner: Option<T>;
}EDIT: It's Associated Type Defaults what would solve the issue of constraining more the inner type. Then we can define the same exact implementations for Value generically and add a few extra if needed. There are times where we need to decompose a Value into bytes. And it's simply a nightmare. When instead we could have a Traitify the I think anyway, that right now, Value is really tied to WDYT? Does the trait approach sound like a possibility to you? |
|
We should keep discussion in this PR around the API that is proposed here. Proposals for new alternatives should be in the main issue, otherwise keeping track of the discussion will get unwieldy and slow down (as people may only be tracking that issue, not the PRs). |
Part of #698.