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

Docs: Add builtins documentation for cairo vm #379

Merged
merged 8 commits into from
May 24, 2024

Conversation

jimenezz22
Copy link
Contributor

This PR is for the issue Builtins Documentation #326. Adding a complete documentation to understand how builtins works on cairo virtual machine and how bulitins perform complex operations at the cost of extra constraints when proving.

@cicr99 cicr99 linked an issue May 3, 2024 that may be closed by this pull request
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

To be honest, I liked the beginning but after that it all started feeling more and more as something generated by an IA and no honest effort put into it.

Some general feedback:

  1. Is ok to write just what you understood of builtins.
  2. If you wanted to write more, is ok for you to ask us questions, we will gladly answer or point towards some documentation.
  3. Is ok to make mistakes

On the other hand is not Ok to fill the PR with content just for the sake of filling it with content. Usually the content doesn't adds anything meaningful, at instances, is slightly correct but it will cause more confusion than anything. Just by trying to run some Cairo Code (or reading the docs) you would realize that there are things written here that don't make sense (which begs the question, how much effort was put into this PR).

Some specific feedback:

  1. The introduction can stay as is
  2. What is builtins in Cairo and Builtins and Cairo memory should be explained as what they are without any analogies.
  3. General types of builtins in Cairo VM: The concept is good, but it has builtins I don't know about. Also, sepcific builtins in Cairo Vm could be merged with this section.
  4. Finally, How builtins perform ... should explain the trade-off that brings builtins to the table which has nothing to do with what was written in that section.

docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
@jimenezz22
Copy link
Contributor Author

Hi! @rodrigo-pino
I'm working on the changes and will make a new commit as soon as possible. Sorry for the use of analogies, I used them because it is easier for me to understand these types of concepts through the use of analogies and I thought it would be good to include them in the documentation. I will make documentation cleaner, shorter and with more technical language.

@jimenezz22
Copy link
Contributor Author

@rodrigo-pino I already made the commit with the requested changes

Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Much better @jimenezz22 ! Thank you. Just some comments

docs/docs/vm-fundamentals/builtins.md Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Show resolved Hide resolved
@jimenezz22
Copy link
Contributor Author

Hi! @rodrigo-pino
I'll make a new commit as soon as possible

@rodrigo-pino
Copy link
Contributor

@jimenezz22 do you have any updates on this?

@jimenezz22
Copy link
Contributor Author

@rodrigo-pino I was waiting for you to answer the questions I had about the comments. In the next few hours you will have the issue ready

@jimenezz22
Copy link
Contributor Author

@rodrigo-pino new commit ready

Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Cool, just some nitpicks. Will approve once resolved

docs/docs/vm-fundamentals/builtins.md Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
@jimenezz22
Copy link
Contributor Author

@rodrigo-pino Changes made! Waiting for approval.

Copy link
Contributor

@TAdev0 TAdev0 left a comment

Choose a reason for hiding this comment

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

left minor comments. Looks good overall, a few improvments should be done but this can be done later in another PR

docs/docs/vm-fundamentals/builtins.md Show resolved Hide resolved
docs/docs/vm-fundamentals/builtins.md Outdated Show resolved Hide resolved
@rodrigo-pino rodrigo-pino merged commit d062196 into NethermindEth:main May 24, 2024
4 checks passed
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.

Builtins Documentation
3 participants