fix(core, ffi): should accept bigint inputs #24
Conversation
| // output = [public output c, public input a] | ||
| let expected_output = vec![Fr::from(c), Fr::from(a)]; | ||
| let serialized_outputs = SerializableInputs(expected_output); | ||
| let serialized_outputs = deserialize_inputs(vec![c.to_string(), a.to_string()]); |
There was a problem hiding this comment.
Why this change? Shouldn't it still be something Serializable?
There was a problem hiding this comment.
Just to simplify the serialization
or we will need
Fr::from(BigUint::from_str(a).unwrap()))to construct the element from string
| .serialize_uncompressed(&mut serialized_data) | ||
| .expect("Serialization failed"); | ||
| pub fn serialize_inputs(inputs: &SerializableInputs) -> Vec<String> { | ||
| let serialized_bigint_data: Vec<BigUint> = |
There was a problem hiding this comment.
I wonder if this function can be written more in same style as old one? Not sure diff has to be this big with this many operations (could be wrong though)
| pub fn deserialize_inputs(data: Vec<u8>) -> SerializableInputs { | ||
| SerializableInputs::deserialize_uncompressed(&mut &data[..]).expect("Deserialization failed") | ||
| pub fn deserialize_inputs(data: Vec<String>) -> SerializableInputs { | ||
| let deserialized_data = data |
There was a problem hiding this comment.
Ditto this - can we re-use existing serialization methods? don't see anything immediate that'd disqualify strings
Also think for both functions we can actually just add another function because signature is different, so don't need to "overwrite" existing ones in case we want it for other fields
| @@ -21,12 +21,12 @@ pub fn assert_paths_exists(wasm_path: &str, r1cs_path: &str) -> Result<(), Mopro | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub fn bytes_to_bits(bytes: &[u8]) -> Vec<bool> { | |||
mopro-ffi/src/lib.rs
Outdated
| let c = field.to_string(); | ||
| inputs.insert("a".to_string(), vec![a.clone()]); | ||
| inputs.insert("b".to_string(), vec![b.clone()]); | ||
| let serialized_outputs = vec![c, a]; |
There was a problem hiding this comment.
I'd expect this to call serialization method
mopro-ffi/src/mopro.udl
Outdated
| @@ -12,7 +12,7 @@ dictionary SetupResult { | |||
|
|
|||
| dictionary GenerateProofResult { | |||
| bytes proof; | |||
| bytes inputs; | |||
| sequence<string> inputs; | |||
There was a problem hiding this comment.
Do we need this change? If it is a byte string we can just deserialize it higher up?
It might make sense, just seems orthogonal to accepting bigint inputs
| var inputs = [String: [String]]() | ||
| let field = "21888242871839275222246405745257275088548364400416034343698204186575808495616" | ||
| inputs["a"] = [field] | ||
| inputs["b"] = ["1"] |
There was a problem hiding this comment.
In Swift, perhaps you'd still do normal math operations on numbers, and then only do the serialization to string at the very end before calling the FFI?
| @@ -6,51 +6,26 @@ let moproCircom = MoproCircom() | |||
| let wasmPath = "./../../../../mopro-core/examples/circom/multiplier2/target/multiplier2_js/multiplier2.wasm" | |||
| let r1csPath = "./../../../../mopro-core/examples/circom/multiplier2/target/multiplier2.r1cs" | |||
|
|
|||
| // TODO: should handle 254-bit input | |||
| func serializeOutputs(_ int32Array: [Int32]) -> [UInt8] { | |||
There was a problem hiding this comment.
I imagine we have functions like
func serializeInputs(_ int32Array: [Int32]) -> [String]
// TODO: Add proper BigNumber library
// For now can mock with struct BigNumber and then BigNumber("1532423423432")
func serializeInputs(_ BNArray: [BigNumber]) -> [UInt8]
Since Outputs and Inputs are symmetrical, and there's really no "output", perhaps we can just call this serializeInputs? If we think it is confusing to not have a serializeOutputs method we can just have it be identical / alias to serializeInputs.
Then similarly for deserialization
func deserializeInputs(_ strArray: [String]) -> [BigNumber]
That way the fact that we serialize to strings is an implementation detail (this could change in the future) and we can easily add serialization methods for various types
Use of BigNumber in Swift seems like a separate issue and more of an app concern for now, so just mocking this with a basic type wrapping string in app/tests seems OK for now
There was a problem hiding this comment.
I think we can compute over BigNumber before calling generateProof
(there should be a toString like function in such library)
but if we can handle serialize_inputs and deserialize_inputs in rust it could be more easier for swift developers?
I prefer not to have these two functions serializeInputs and deserializeInputs in swift
Fr->BigUInt->StringString->BigUInt->Fr21888242871839275222246405745257275088548364400416034343698204186575808495616