Skip to content
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

refactor: Deduplicate Field implementations in ts #4189

Open
spalladino opened this issue Jan 23, 2024 · 2 comments
Open

refactor: Deduplicate Field implementations in ts #4189

spalladino opened this issue Jan 23, 2024 · 2 comments
Labels
T-refactor Type: this code needs refactoring

Comments

@spalladino
Copy link
Collaborator

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.

@github-project-automation github-project-automation bot moved this to Todo in A3 Jan 23, 2024
@spalladino spalladino added the T-refactor Type: this code needs refactoring label Jan 23, 2024
@benesjan
Copy link
Contributor

benesjan commented Jun 19, 2024

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
Copy link
Contributor

benesjan commented Jun 19, 2024

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

const buf = Buffer.isBuffer(bufferOrReader)
? bufferOrReader
: Buffer.from(bufferOrReader.buffer, bufferOrReader.byteOffset, bufferOrReader.byteLength);

@spalladino spalladino changed the title Deduplicate Field implementations in ts refactor: Deduplicate Field implementations in ts Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-refactor Type: this code needs refactoring
Projects
Status: Todo
Development

No branches or pull requests

2 participants