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

Parameterize asset balance number #29

Merged
merged 9 commits into from
Jun 7, 2023

Conversation

alxkzmn
Copy link
Contributor

@alxkzmn alxkzmn commented Jun 1, 2023

Resolves #18 by parameterizing the number of assets for the MST node and the Poseidon hasher dimensions. Hasher configs and the circuit generic parameters can be generated using the Python script that takes the number of assets as an argument.

@alxkzmn alxkzmn requested review from enricobottazzi and sifnoc June 1, 2023 16:15
@alxkzmn alxkzmn self-assigned this Jun 1, 2023
src/merkle_sum_tree/mod.rs Outdated Show resolved Hide resolved
src/merkle_sum_tree/mod.rs Show resolved Hide resolved
src/merkle_sum_tree/mst.rs Show resolved Hide resolved
src/merkle_sum_tree/utils/csv_parser.rs Show resolved Hide resolved
src/merkle_sum_tree/params.rs Outdated Show resolved Hide resolved
src/chips/merkle_sum_tree.rs Outdated Show resolved Hide resolved
src/chips/poseidon/hash.rs Show resolved Hide resolved
circuit_parameters_gen/calc_round_numbers.py Show resolved Hide resolved
README.md Show resolved Hide resolved
src/merkle_sum_tree/utils/hash.rs Show resolved Hide resolved
@alxkzmn alxkzmn requested a review from enricobottazzi June 4, 2023 11:03
@enricobottazzi
Copy link
Member

The PR looks good to me @alxkzmn. One thing that I noticed is that moving from vectors to arrays have downgraded the performance in building the merkle sum tree, contrary to what I thought. In particular, building a mst with 24 levels takes 592 s using array, while it took 12s less before. Not a huge deal, but I suggest reverting these changes.

Sorry for the wrong heads up 🥲

Copy link
Member

@enricobottazzi enricobottazzi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sifnoc sifnoc left a comment

Choose a reason for hiding this comment

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

Everything looks great!

Just a small note - we have to revise the csv_parser if we are going to process CSV files with multiple asset data in the future. But as of now, we're good to go!

@enricobottazzi
Copy link
Member

Merginn

@enricobottazzi enricobottazzi merged commit cf77cf0 into summa-dev:master Jun 7, 2023
sifnoc added a commit that referenced this pull request Jun 8, 2023
@alxkzmn alxkzmn deleted the multiasset-mst branch November 9, 2023 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add multi-asset support
3 participants