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

feat: add page for describing cairo builtins #1202

Closed
wants to merge 8 commits into from

Conversation

xiaolou86
Copy link
Contributor

@xiaolou86 xiaolou86 commented Mar 28, 2024

Description of the Changes

Closes #1126
Add pages for describing cairo builtins

PR Preview URL

After you push a commit to this PR, a preview is built and a URL to the root of the preview appears in the comment feed.

Paste here the specific URL(s) of the content that this PR addresses.

Check List

  • Changes have been done against main branch, and PR does not conflict
  • PR title follows the convention: <docs/feat/fix/chore>(optional scope): <description>, e.g: fix: minor typos in code

This change is Reviewable

@xiaolou86 xiaolou86 force-pushed the builtins branch 2 times, most recently from b473943 to bd3e467 Compare March 29, 2024 10:22
@xiaolou86 xiaolou86 changed the title [WIP]feat: add pages for describing cairo builtins feat: add pages for describing cairo builtins Mar 29, 2024
@xiaolou86
Copy link
Contributor Author

@stoobie I finished the doc, please help me to review. Thank you very much.

@xiaolou86 xiaolou86 changed the title feat: add pages for describing cairo builtins feat: add page for describing cairo builtins Mar 29, 2024
@stoobie
Copy link
Collaborator

stoobie commented Mar 31, 2024

@xiaolou86 Thank you! I've forwarded this on for a technical review. Once that is done, I'll go over it and edit or make suggestions.

@stoobie
Copy link
Collaborator

stoobie commented Apr 2, 2024

@xiaolou86 Here's the tech review feedback I received:


  • the original Caior0 docs describes builtins better.
    (From me (stoobie): That doesn't mean you should just copy. I don't like the word "vanilla", the grammar is not great, and the page doesn't follow the Starknet docs writing guidelines and our style guide.)
  • builtins are not a “basic language construct”, they have a concrete definition
  • arithmetic operations aren’t builtins (only builtins are the ones listed on the docs list in the fee page)
  • some of the code is referring to Cairo0 (e.g. hash2, no such function in Cairo 1)
  • no mention of builtins being used under the hood in Cairo 1 (you can’t use them directly, there are libfuncs and corelib functions that make use of them)

It would be good to provide a code example of a builtin vs accomplishing the same thing in standard Cairo.

@xiaolou86
Copy link
Contributor Author

ok, I will update the docs.

@xiaolou86
Copy link
Contributor Author

@stoobie please help to review again.

@stoobie
Copy link
Collaborator

stoobie commented Apr 9, 2024

Please mention that you don't need to do anything to use a builtin. The library you use should automatically scan your code and use a builtin where appropriate. I'm not 100% sure if I got this right, so please wait for a confirmation from @ArielElp.

Also, the only builtins that you should have on this page are the ones [isted in on the docs list in the fee page in Table 1. Amount of gas used per Cairo step or per each time a Cairo builtin is applied)

Copy link
Collaborator

@stoobie stoobie left a comment

Choose a reason for hiding this comment

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

@xiaolou86 I made a couple more comments/suggestions. I asked someone more knowledgeable than me to comment as well, so please be aware that he needs to respond. You can make the minor changes I requested in the meantime.

@xiaolou86
Copy link
Contributor Author

Please mention that you don't need to do anything to use a builtin. The library you use should automatically scan your code and use a builtin where appropriate. I'm not 100% sure if I got this right, so please wait for a confirmation from @ArielElp.

All right.

Also, the only builtins that you should have on this page are the ones [isted in on the docs list in the fee page in Table 1. Amount of gas used per Cairo step or per each time a Cairo builtin is applied)

I think I have the only ones on this page, but I am not much sure.

Thank you very much.

…art_Contracts/builtins.adoc

Co-authored-by: Steve Goodman <[email protected]>
@stoobie
Copy link
Collaborator

stoobie commented May 2, 2024

@xiaolou86 After reviewing this with our expert, I found that we were on the wrong track. I created a new PR (#1243) with the necessary info.

Feel free to review it and make suggestions or ask questions if anything is unclear. After the new PR is merged, I'll settle with you on OnlyDust.

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

4 similar comments
@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

2 similar comments
@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1202/documentation/ .

@stoobie
Copy link
Collaborator

stoobie commented May 19, 2024

#1243 Closes this.

@stoobie stoobie closed this May 19, 2024
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.

Request for Contribution: Describe Cairo builtins
3 participants