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

Integrate RunResources logic into HintProcessor trait #1274

Merged
merged 4 commits into from Jun 26, 2023

Conversation

fmoletta
Copy link
Member

Fixes CI warnings for #1272

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #1274 (6599155) into main (9cef244) will decrease coverage by 0.04%.
The diff coverage is 90.52%.

@@            Coverage Diff             @@
##             main    #1274      +/-   ##
==========================================
- Coverage   97.60%   97.57%   -0.04%     
==========================================
  Files          89       89              
  Lines       36335    36132     -203     
==========================================
- Hits        35466    35254     -212     
- Misses        869      878       +9     
Impacted Files Coverage Δ
...rc/hint_processor/builtin_hint_processor/bigint.rs 97.65% <ø> (ø)
..._processor/builtin_hint_processor/blake2s_utils.rs 99.78% <ø> (ø)
...uiltin_hint_processor/cairo_keccak/keccak_hints.rs 94.09% <ø> (ø)
...rocessor/builtin_hint_processor/dict_hint_utils.rs 99.20% <ø> (ø)
...int_processor/builtin_hint_processor/ec_recover.rs 100.00% <ø> (ø)
.../hint_processor/builtin_hint_processor/ec_utils.rs 98.53% <ø> (ø)
...ocessor/builtin_hint_processor/field_arithmetic.rs 98.95% <ø> (+0.01%) ⬆️
...cessor/builtin_hint_processor/find_element_hint.rs 96.43% <ø> (ø)
...rc/hint_processor/builtin_hint_processor/garaga.rs 100.00% <ø> (ø)
...t_processor/builtin_hint_processor/keccak_utils.rs 99.25% <ø> (ø)
... and 35 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fmoletta fmoletta marked this pull request as ready for review June 22, 2023 21:38
@@ -55,6 +51,9 @@ pub trait HintProcessor {
}
}

pub trait HintProcessor: HintProcessorLogic + ResourceTracker {}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about renaming HintProcessor -> HypervisorContext and HintProcessorLogic -> HintProcessor?
(Hypervisor: something "that creates and runs virtual machines". Seems fitting)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even just Hypervisor...?

Copy link
Member

Choose a reason for hiding this comment

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

I'm more in favor of something like plainly Context (since that's likely to be extended in the future), but I'd rather not do the bikeshedding while the urgency is still in need of fixing. We can fix those details during the week, after solving the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

"that creates and runs virtual machines", wouldn't that be the CairoRunner itself?
I agree with @Oppen Context is less confusing

@pefontana pefontana enabled auto-merge June 26, 2023 17:11
@pefontana pefontana added this pull request to the merge queue Jun 26, 2023
Merged via the queue into main with commit 18997e3 Jun 26, 2023
29 of 31 checks passed
@pefontana pefontana deleted the integrate-run-resources-into-hint-processor branch June 26, 2023 17:44
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 4, 2023
…s#1274)

* RunResources into context

Signed-off-by: Dori Medini <dori@starkware.co>

* Fix broken code under `cairo-1-hints` feature

* Add changelog entry

---------

Signed-off-by: Dori Medini <dori@starkware.co>
Co-authored-by: Dori Medini <dori@starkware.co>
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 4, 2023
…s#1274)

* RunResources into context

Signed-off-by: Dori Medini <dori@starkware.co>

* Fix broken code under `cairo-1-hints` feature

* Add changelog entry

---------

Signed-off-by: Dori Medini <dori@starkware.co>
Co-authored-by: Dori Medini <dori@starkware.co>
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 25, 2023
…s#1274)

* RunResources into context

Signed-off-by: Dori Medini <dori@starkware.co>

* Fix broken code under `cairo-1-hints` feature

* Add changelog entry

---------

Signed-off-by: Dori Medini <dori@starkware.co>
Co-authored-by: Dori Medini <dori@starkware.co>
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 25, 2023
…s#1274)

* RunResources into context

Signed-off-by: Dori Medini <dori@starkware.co>

* Fix broken code under `cairo-1-hints` feature

* Add changelog entry

---------

Signed-off-by: Dori Medini <dori@starkware.co>
Co-authored-by: Dori Medini <dori@starkware.co>
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.

None yet

4 participants