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

Dict hints #455

Merged
merged 9 commits into from
Jan 7, 2025
Merged

Dict hints #455

merged 9 commits into from
Jan 7, 2025

Conversation

ftheirs
Copy link
Collaborator

@ftheirs ftheirs commented Dec 27, 2024

Implement dictionary hints related to compression process.
Add constants.rs file to define constants that are needed across several hints

KZG tests still require extra hints.
Check orphan hints is failing because it detects two extra hints that will be covered in compress PR (COMPRESS and SET_DECOMPRESSED_DST)

Issue Number: N/A

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Breaking changes?

  • yes
  • no

@ftheirs ftheirs changed the base branch from main to os_v0_13_3 December 31, 2024 13:20
@ftheirs ftheirs marked this pull request as ready for review December 31, 2024 13:23
@ftheirs ftheirs requested a review from whichqua January 2, 2025 15:20

let bucket_index = get_maybe_relocatable_from_var_name(vars::ids::BUCKET_INDEX, vm, ids_data, ap_tracking)?;

let prev_offset = dict_tracker.get(&bucket_index).unwrap().clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handling this unwrap is the only thing I can thing of.

Copy link
Collaborator

@whichqua whichqua left a comment

Choose a reason for hiding this comment

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

Should be good!

@ftheirs ftheirs requested a review from GMKrieger January 6, 2025 12:21
Copy link
Collaborator

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but I would suggest one thing:

In cairo-lang this all comes from a file called compression.cairo, and we have tried to keep our file structure similar to theirs. So, my suggestions:

  • Rename dict.rs -> compression.rs
  • Drop constants.rs and include its contents in compression.rs

@ftheirs ftheirs merged commit 801812b into os_v0_13_3 Jan 7, 2025
3 of 5 checks passed
@ftheirs ftheirs deleted the gm/dict_hints branch January 7, 2025 13:00
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.

3 participants