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

Check if we should implement Pages & Attributes for OutputBuiltinRunner #1380

Closed
fmoletta opened this issue Aug 15, 2023 · 3 comments
Closed
Labels
enhancement New feature or request question Further information is requested

Comments

@fmoletta
Copy link
Contributor

fmoletta commented Aug 15, 2023

The cairo-lang implementation of OutputBuiltinRunner contains the pages and attributes fields, which are created via the add_page & add_attribute methods. These methods are not used during normal execution and are only used by os hints. While running os hints is outside the scope of the vm these methods would be needed for an OsSyscallHintProcessor to implement those hints. The data on these fields is also used when creating a cairo pie.

Adding this behaviour would entail:

  • Creating the pages and attributes fields in the OutputBuiltinRunner
  • Implementing the methods add_page & add_attribute for OutputBuiltinRunner
  • Implementing OutputBuiltinRunner.finalize_segments (This would add the logic of pages and attributes on top of the common builtin segment finalization)
@fmoletta fmoletta added enhancement New feature or request question Further information is requested labels Aug 15, 2023
@Oppen
Copy link
Contributor

Oppen commented Aug 16, 2023

I would suggest converting the BuiltinRunner back into a trait. It was my mistake to think converting to enum was the only way to make it Sync + Send at the time due to lack of knowledge at the time.
If we make it a trait, I think we can simply handle it as a different builtin in SiR with its own extra methods/fields that the OsSyscallHintProcessor will know about and use.
Does this make sense to you?

@fmoletta
Copy link
Contributor Author

fmoletta commented Aug 16, 2023

I don't think adding an alternative output builtin is a good solution. These fields are part of the original `OutputBuiltinRunner', we just didn't implement them because they weren't used. This would be a small extension of the builtin's functionality, not a breaking change in the builtin's behaviour, so it doesn't really merit possibly breaking changes in how the runner interprets builtins (as this is currently a closed behaviour, not something extensible/customizable).

@igaray igaray added this to Starknet Jan 15, 2024
@igaray igaray moved this to Backlog in Starknet Jan 15, 2024
@fmoletta
Copy link
Contributor Author

Implemented by #1580

@github-project-automation github-project-automation bot moved this from Backlog to Done in Starknet Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
Status: Done
Development

No branches or pull requests

2 participants