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.

Sign up for GitHub

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

Jump to bottom

Dynamic arrays in o1js #1848

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft

Dynamic arrays in o1js #1848

wants to merge 30 commits into from
+971 −0

Conversation

querolita
Copy link
Member

@querolita querolita commented Oct 2, 2024 •
edited
Loading

Addressing #1086, based on #633

Making this public as WIP to receive early feedback

@querolita querolita changed the title DynamicArrays in o1js ArrayLists in o1js Oct 7, 2024
@mitschabaude
Copy link
Contributor

mitschabaude commented Oct 11, 2024 •
edited
Loading

@querolita @Trivo25 maybe of interest: I created quite polished general-purpose types for both static and dynamic arrays in this PR: zksecurity/mina-attestations#25

They made it pretty easy to implement dynamic-length sha256.

I think they could serve as a starting point for arrays in o1js that are (even) better designed than the D. Gretzke repo!

@querolita querolita changed the title ArrayLists in o1js Dynamic arrays in o1js Oct 17, 2024
@querolita querolita marked this pull request as ready for review October 17, 2024 17:51
@querolita
Merge branch 'main' into feature/dynamic-arrays
4fe6f43
@querolita
revert subproject commit of bindings and mina
da4990c
@querolita
revert function name changes after merge collision toBigInt/toBigint …
e813b7c 
…and null/synthesize
@querolita
remove runtime tables from this branch, and revert package.json modif…
77a80db 
…ications
@querolita
revert changes to package-lock.json
354bb0e
@querolita
remove changes to esbuild
2cf25fa
@querolita
revert more changes to package-lock.json caused by esbuild
c8e21f1
@querolita
update changelog
fcd6ebf
@querolita
use zkprogram to be able to test prover functions in dynamic arrays
57dd9cb
@querolita
comment parts of the unit test
4fb72f0
@querolita
enable mapping test
677ee2c
@querolita
include set and get test
5fb42f8
@querolita
fix popping multiple elements
aceb442
@querolita
increment and decrement length
49138bf
@querolita
move functions around
fd6007e
@querolita
simplify add in map test
3af547b
@querolita
remove unused functions
01cb38b
@querolita
create assert none in Option
a40cb0a
@querolita
shift left and test
37a72b2
@querolita
allow shifting left length positions
8536114
@querolita
copy, slice, and fix shift left
45ea600
@querolita
added shiftRight function
a06a607
@querolita
added concat functionality
0b54106
@querolita querolita changed the base branch from v1 to main November 7, 2024 08:41
@querolita querolita requested review from a team as code owners November 7, 2024 08:41
@querolita
Copy link
Member Author

querolita commented Nov 12, 2024 •
edited
Loading

TODO

  • Include complexity of each function
  • Improve efficiency making further use of cached masked positions
  • Randomized tests

@Shigoto-dev19
Copy link
Contributor

Is it possible to use DynamicArray as a method input? I believe this is feasible for zkApps but not yet for ZkProgram, as Yoni is addressing it in #1828.

Additionally, it would be great to get some insights into the constraint optimization for dynamic array indexing or updates. For example, how does its efficiency compare to classic techniques for an array of 100 elements versus using dynamic arrays?

@mitschabaude
Copy link
Contributor

mitschabaude commented Nov 21, 2024 •
edited
Loading

Is it possible to use DynamicArray as a method input? I believe this is feasible for zkApps but not yet for ZkProgram, as Yoni is addressing it in #1828.

zkApps and ZkProgram are the same actually.
In both of them, you can use non-pure provable types for private inputs.
However, you can't use them for public inputs. In zkApps, the public input type is fixed / not configurable, so that's not a noticeable limitation for users.

This limitation shouldn't be relevant to dynamic arrays since it's pure whenever the element type is pure. However, this might not be currently reflected in the type signature!

Trivo25
Trivo25 reviewed Nov 23, 2024
View reviewed changes
Copy link
Member

@Trivo25 Trivo25 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Let's wait for the runtime tables before we finish and merge this PR, otherwise we would have to break backwards compatilbility later which is a bit meh

src/lib/provable/test/dynamic-array.unit-test.ts
methods: {
pushAndPop: {
privateInputs: [],
async method() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall the tested cases look good to me, but we should restructure the tests a bit differently - lets talk about that next time!

Geometer1729
Geometer1729 approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@Geometer1729 Geometer1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This was referenced Nov 25, 2024
Merged
Merged
Merged
querolita
querolita commented Nov 25, 2024
View reviewed changes
Shigoto-dev19
Shigoto-dev19 reviewed Nov 25, 2024
View reviewed changes
Copy link
Contributor

@Shigoto-dev19 Shigoto-dev19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@querolita querolita marked this pull request as draft December 3, 2024 12:48
@Trivo25
Copy link
Member

Trivo25 commented Mar 13, 2025 •
edited
Loading

soft blocked on #2079

This was referenced Mar 27, 2025
Open
Open
@querolita querolita changed the base branch from main to feature/lookup-runtime-tables March 31, 2025 16:58
Base automatically changed from feature/lookup-runtime-tables to main April 1, 2025 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trivo25 Trivo25 Trivo25 left review comments

@Shigoto-dev19 Shigoto-dev19 Shigoto-dev19 left review comments

@Geometer1729 Geometer1729 Geometer1729 approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@querolita @mitschabaude @Shigoto-dev19 @Trivo25 @Geometer1729