AztecProtocol / aztec-packages

Apache License 2.0
187 stars 189 forks source link

refactor: Deduplicate Field implementations in ts #4189

Open spalladino opened 8 months ago

spalladino commented 8 months ago

We currently have two Fr implementations, one in bb.js and one in Foundation. These are incompatible with each other, since one uses Uint8Arrays to store the field value, and the other bigint. The Fr and Fq in bb.js also differ in this.

Given that the Uint8Array implementation is more efficient, we should ensure both Fr and Fq in bb.js use that, and then remove the implementations in Foundation and just re-export the bb.js ones. Note that this may break avm field arithmetic, that depends on fields being bigints right now, so we'll need to re-convert fields into bigints before running these operations.

More context here.

benesjan commented 3 months ago

Tried tackling this one today and it's way trickier than I originally expected (how unexpected in software engineering).

The issue is that all the serialization in foundation package expects Buffer type and everything in BB expects Uint8Array. Given that we would want to stick to Uint8Array (since introducing Buffer to BB seems incorrect due to it not being natively supported in browsers) the only correct approach seems to be to also deduplicate all the serialization functionality and use the one from BB (e.g. BufferReader in BB works with Uint8Array, then there is a duplicate class in foudnation working with Buffer type) and consequently update everything in aztec-packages/yarn-project.

That's a lot of work (although probably quite straightforward).

benesjan commented 3 months ago

A solution proposed by Alex:

a stopgap solution would be to convert between Uint8Array and Buffer for serialisation purposes? Then

Node's Buffer extends Uint8Array so a buffer instance should be accepted by any function that expects a Uint8Array. Converting from Uint8Array to a Buffer can done quite cheaply by sharing the memory. BufferReader in foundation already handles both buffers and Uint8arrays

https://github.com/AztecProtocol/aztec-packages/blob/52ce25d1abcc5a8cff7ec360acf23806cb317b57/yarn-project/foundation/src/serialize/buffer_reader.ts#L39-L41