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

[Doc] core libfuncs - implement the page skeleton #72

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

remybar
Copy link
Collaborator

@remybar remybar commented Feb 26, 2024

Related to #67.

This PR contains 2 commits:

  • the first one adds a new libfuncs page which is accessible from the header menu. This page shows a table of Sierra libfuncs built from docs/libfuncs mdx files. The docs/HOWTO.md gives more details about these mdx files.
  • the second one adds a script to generate empty mdx files from a list of libfuncs. It has been executed with the audited list from here.

Now, the front-end displays the full list of libfuncs but all these functions has to be documented 🚀

@remybar remybar requested a review from a team as a code owner February 26, 2024 10:20
@mazurroman mazurroman self-requested a review February 26, 2024 14:10
@mazurroman mazurroman linked an issue Feb 26, 2024 that may be closed by this pull request
@mazurroman
Copy link
Contributor

mazurroman commented Feb 26, 2024

@remybar great start!

Few thoughts:

  • Can we add at least 1 libfunc description into this PR so that we agree on the libfunc's structure before merging
  • I suggest to remove keyboard functionality (ALT+K) for now. We can do keyboard shortcuts later as part of a separate PR. Would be good to add shortcuts to the whole app (especially the playground), not just the table.
  • Can we add permalink functionality into this PR (similar to evm.codes)? It's a nice addition that helps with sharing and also linking this tool from other sources like Cairo / Sierra documentation

@mazurroman
Copy link
Contributor

Some thoughts on your Telegram message:

If you have some comments/ideas to improve the table structure, don't hesitate to tell me 😉 I don't know if it would be a good idea to show brief information about inputs and outputs in 2 new columns (so name | inputs | outputs | short description). Maybe it is too much information and the table will be hard to read at first glance 🤔

In the table overview, what about using [0], [1], etc for inputs and outputs? Similarly to evm.codes, they use [a], [b]. This allows to keep the table clear while giving the reader some information about if there are any variables + how many. Each var description would go into the expand section (again — similar to evm.codes). WDYT @remybar ?

Secondly, from my earlier chat with @barabanovro :

Actually, there is no concept of inputs and outputs in Sierra. There are invoke refs, result refs, and output refs.
Invoke refs are variables inside round brackets after the libfunc name. Result refs are the same as invoke refs but only after return. Output refs are variables inside result branches (21, 6, 7, 8, 9 in previous examples).

We should update the table structure to reflect the actual Sierra terms: invoke refs, result refs, and output refs instead of inputs and outputs.

@remybar remybar force-pushed the feat-libfuncs-pagee branch from 5f30243 to 3a71ea6 Compare February 27, 2024 03:42
@remybar
Copy link
Collaborator Author

remybar commented Feb 27, 2024

Thanks @mazurroman for the feedbacks !

I've updated this PR:

  • Alt+K has been removed
  • permalink feature has been added. I improved a little bit the feature compared to evm.codes. When you click on the icon, it copies the permalink in the clipboard, with a short animation showing that the link has been copied.
  • Add invoke refs, result refs and output refs as new columns. Same behaviour than stack inputs/outputs on evm.codes. These refs are set in the front-matter of function mdx file like this:
----
shortDescription: blablabla...
invokeRefs: "[0] | [1]"
returnRefs:
outputRefs: "[2] = [0] + [1]"
----

We can write almost everything as refs, we just have to separate them with a |.

  • I've updateds all mdx files with the new refs.

Now I plan to document the array_append function as an example to discuss.

@remybar remybar force-pushed the feat-libfuncs-pagee branch from 3a71ea6 to 2e307d7 Compare February 27, 2024 08:55
@remybar
Copy link
Collaborator Author

remybar commented Feb 28, 2024

PR update:

  • returnRefs removed
  • fix table rendering issue in next-mdx-remote (It works on evm.codes because they use an old version for nextjs12 but our version for nextjs 13 needs a plugin and some configurations to render tables properly). For more information:
  • add documention for array_append.

Don't hesitate to give your opinion about the array_append documentation.
I will look after some different libfuncs to improve this first template.

@remybar remybar force-pushed the feat-libfuncs-pagee branch from 2e307d7 to 17a8e6b Compare February 28, 2024 07:38
@remybar
Copy link
Collaborator Author

remybar commented Feb 28, 2024

I tried to document array_get but I'm not sure how to:

  • handle the output refs with the fallthrough,
  • give a clean syntax of the function
  • give a clean example of how to use it

Some thoughts:

  • maybe we should give some insights before the libfuncs table about Sierra specific things (invoke/output refs, fallthrough, ..).
  • give some context for every examples. For example, for the array_get libfunc, we could write a full code snippet with creating an array with some values and (1) get a value at good index (2) get a value with an index out of bounds. In the code snippet, we could use a different highlighting for array_get calls and for what is related to context.

@mazurroman @barabanovro As you have far more knowledge than me about Sierra, maybe you can tell us how you would document array_get and it will give us a good example for others libfuncs :)

@mazurroman
Copy link
Contributor

@remybar This is a good idea — we will proceed with documenting the array_get libfunc and get back when ready

@barabanovro
Copy link
Contributor

Hey @remybar @mazurroman, I have updated array_get with result branches and a few other changes, let me know what you think

@remybar
Copy link
Collaborator Author

remybar commented Feb 29, 2024

Hey @remybar @mazurroman, I have updated array_get with result branches and a few other changes, let me know what you think

Thanks Roman for this update !

Some minor remarks:

  • I woul try to find a short way to document outputRefs to avoid having the text on several lines in the table. Maybe some accroyms like ft for fallthrough and si for StatementIdx ? Of course, in this case, we have to define them on top of the table (I would also define invoke refs, output refs, fallthrough, ... because it is really Sierra-specific).
  • In Syntax and Example, I think the -> is not needed.
  • To avoid having to many title levels, I would replace "Result branches" by "Output Refs" and remove the other "Output Refs" titles. So:

Output Refs

Success: Fallthrough Branch

... the table ...

Failure: Target Statement

... the table ...

One question: do we have to explain a little bit more RangeCheck or is it fine like that ?

@remybar
Copy link
Collaborator Author

remybar commented Mar 1, 2024

@mazurroman @barabanovro

If you have some time to have a look at this, we would be able to finalize this layout and start documenting all the functions in the next days :)

@mazurroman
Copy link
Contributor

Hey @remybar thank you for staying on top of this! Your feedback from yesterday was very good and we are in the process of incorporating it. Will get back when it's ready for another round of review.

@barabanovro
Copy link
Contributor

Hey @remybar, I've pushed the updates. Could you please wrap up the UI by arranging the Memory, Invoke Refs, and Fallthrough Branch Output Refs (Target Statement Branch Output Refs) tables side by side in a horizontal layout? Also, please share other ways to improve the UI here.

@remybar
Copy link
Collaborator Author

remybar commented Mar 1, 2024

Hey @remybar, I've pushed the updates. Could you please wrap up the UI by arranging the Memory, Invoke Refs, and Fallthrough Branch Output Refs (Target Statement Branch Output Refs) tables side by side in a horizontal layout? Also, please share other ways to improve the UI here.

Thanks Roman !

Ok I will try to improve examples readibility 👍

For the main table, would it be useful to group "fallthrough branch refs" and "statement branch refs" to "output refs ? I mean having a big "output refs" above "falthrough" and "statement".

@mazurroman
Copy link
Contributor

mazurroman commented Mar 1, 2024

@remybar @barabanovro the direction here is good, however there are still few things that need to be improved. We really want to get it to a point where everything is clear.

  • 1: I like we use "labels" in the main table rows for parameter names but we should probably also keep the indexes. So "0: rangeCheck 1: array 2: index" instead of "rangeCheck array index"
  • 2: The examples are super useful! Can we use evm.codes-like style for the table UI? So have both inputs and outputs part of the same table, as opposed to the current state
  • 3: In the Examples section, the headings for Example 1 and 2 could also mention which branch will it go to. I.e. Example 1: Fallthrough branch and Example 2: Statement branch. This way we make it even more explicit what the code does
  • 4: We have function syntax under Example 1. We should also add it into example 2. And we should highlight which branch will be called in each, i.e. make fallthrough part of the code bold in Example 1 and statement branch bold in Example 2.
  • 5: @barabanovro the memory indexing looks wrong. The memory table has indexes 140-148 but in Invoke refs : array you use 145, 149. Do you have a mistake here?
  • 6: The description should contain info about fallthrough not just statement branch, to make it complete
  • 7: We should create an About Sierra page to explain key concepts and add links from some keywords into that page. Otherwise hard to understand what's going on.
  • 8: We still don't know what range check is — need to figure it out and make it clear.
  • 9: In the example 2 section, we are completely missing logic around statementIndex. @barabanovro do you think we can add statementIdx into the Target Statement Branch Output Refs table after range check?
  • 10: In the example tables, it would be good to signal what's coming from memory vs what is a value. I.e. invoke refs in Example 1: array is coming from memory but index is a value. Maybe for references we can use [] and similar in the memory table? It could look like this:
+-------+-------+     +-------------------------+
|     Memory    |     | Invoke refs             |
+-------+-------+     +------------+------------+
|       | Value |     | rangeCheck | [140]      |
+-------+-------+     +------------+------------+
| [140] | 2     |     | array      | [142, 145] |
+-------+-------+     +------------+------------+
| [142] | 0     |     | index      | 0          |
+-------+-------+     +------------+------------+
| [145] | 5     |
+-------+-------+

@remybar, re your question: I think keeping the headers as-is is not too bad. Almost every libfunc will have fallthrough branch refs and statement branch refs. When you gasp this concept once it will be clear. We should also write an accompanying About Sierra page where the key concepts get explained.

@barabanovro
Copy link
Contributor

barabanovro commented Mar 1, 2024

@mazurroman
"1." A variable name can contain only one word, for example, [1] or [array]. So it can be disappointing to see a variable with this syntax [1: array].
"5." I assume that the array points to the first element and the next free memory cell, so it seems to be correct.
"9." StatementIdx is not an output reference, so it's incorrect to place it in the table.

@remybar
Copy link
Collaborator Author

remybar commented Mar 2, 2024

@remybar @barabanovro the direction here is good, however there are still few things that need to be improved. We really want to get it to a point where everything is clear.

* [ ]  I like we use "labels" in the main table rows for parameter names but we should probably also keep the indexes. So "`0: rangeCheck` `1: array` `2: index`" instead of "`rangeCheck` `array` `index`"

I agree with @barabanovro, I would use param names only and explain in the "about sierra" page that we chose to use names instead of numbers to give more meaning to parameters.

* [ ]  The examples are super useful! Can we use evm.codes-like style for the table UI? So have both inputs and outputs part of the same table, as opposed to the current state

Yes, I will try to improve tables layout for examples.

* [ ]  In the Examples section, the headings for Example 1 and 2 could also mention which branch will it go to. I.e. **Example 1: Fallthrough branch** and **Example 2: Statement branch**. This way we make it even more explicit what the code does

I agree.

* [ ]  We have function syntax under Example 1. We should also add it into example 2. And we should highlight which branch will be called in each, i.e. make fallthrough part of the code bold in Example 1 and statement branch bold in Example 2.

Good idea ! I think it's not directly possible in a code block in mdx but I will think how to this.

* [ ]  @barabanovro the memory indexing looks wrong. The memory table has indexes 140-148 but in Invoke refs : array you use 145, 149. Do you have a mistake here?

* [ ]  The description should contain info about fallthrough not just statement branch, to make it complete

I don't understand this point. To me, fallthrough branch is when there is no exception (the nominal case) and the statement branch is when there is an out-of-bound error. Both are mentioned in the description 🤔

* [ ]  We should create an About Sierra page to explain key concepts and add links from some keywords into that page. Otherwise hard to understand what's going on.

Would it be better to have a separated "about sierra" page or just a short section above the table to introduce this "libfuncs" page and give more insights about how it works ? I would prefer the second option.

* [ ]  We still don't know what range check is — need to figure it out and make it clear.

Yes, and after reading the code on starkware repo, I still don't know what is it and how it works 😅

* [ ]  In the example 2 section, we are completely missing logic around `statementIndex`. @barabanovro do you think we can add `statementIdx` into the `Target Statement Branch Output Refs` table after range check?

Maybe we should add a sentence at the beginning of the example to describe what we do in the example. For example, "accessing the element at index 4 of an array of 4 elements leads to an out-of-bound error (so a jump to the statement 59)".

* [ ]  In the example tables, it would be good to signal what's coming from memory vs what is a value. I.e. invoke refs in Example 1: array is coming from memory but index is a value. Maybe for references we can use `[]` and similar in the memory table? It could look like this:
+-------+-------+     +-------------------------+
|     Memory    |     | Invoke refs             |
+-------+-------+     +------------+------------+
|       | Value |     | rangeCheck | [140]      |
+-------+-------+     +------------+------------+
| [140] | 2     |     | array      | [142, 145] |
+-------+-------+     +------------+------------+
| [142] | 0     |     | index      | 0          |
+-------+-------+     +------------+------------+
| [145] | 5     |
+-------+-------+

Yes, I agree it may be confusing ... Yes, why not using [] for a ref and nothing for a value.

@remybar, re your question: I think keeping the headers as-is is not too bad. Almost every libfunc will have fallthrough branch refs and statement branch refs. When you gasp this concept once it will be clear. We should also write an accompanying About Sierra page where the key concepts get explained.

Ok 👍

@remybar
Copy link
Collaborator Author

remybar commented Mar 2, 2024

As we already explained what the function does and what the refs are, maybe we can just give a simple example like this. I think it would avoid confusion between refs and values as there are only refs in the code snippet and the memory mapping between refs and values is explicit.

Depending on the libfunc and if it's easy to do or not, as on evm.codes, we can provide a link to see the example in the playground tool. So, the user can play with the tool, observe refs values, ...
It would also give some context around the libfuncs. For example array_get is mostly used after array_new and array_append.

What do you think ?

Example 1 - Fallthrough branch

Reading the element at index 1 from an array of 4 elements.

array_get<u32>([140], [141], [142]) {
    fallthrough([144], [146])
    59([144])
}

Memory

Ref Value Comment
140 2 RangeCheck
141 145, 149 array
142 1 index
144 3 RangeCheck
145 4 array start
146 3
147 2
148 1 array end

Example 2 - Statement branch

Reading the element at index 4 from an array of 4 elements leads to an out-of-bound error, so a jump to the statement StatementIdx (59 in this example).

array_get<u32>([140], [141], [142]) {
    fallthrough([144], [150])
    59([144])
}

Memory

Ref Value Comment
140 2 RangeCheck
141 145, 149 array
142 4 index
144 3 RangeCheck
145 4 array start
146 3
147 2
148 1 array end

@mazurroman
Copy link
Contributor

As we already explained what the function does and what the refs are, maybe we can just give a simple example like this. I think it would avoid confusion between refs and values as there are only refs in the code snippet and the memory mapping between refs and values is explicit.

Depending on the libfunc and if it's easy to do or not, as on evm.codes, we can provide a link to see the example in the playground tool. So, the user can play with the tool, observe refs values, ... It would also give some context around the libfuncs. For example array_get is mostly used after array_new and array_append.

What do you think ?

Example 1 - Fallthrough branch

Reading the element at index 1 from an array of 4 elements.

array_get<u32>([140], [141], [142]) {
    fallthrough([144], [146])
    59([144])
}

Memory

Ref Value Comment
140 2 RangeCheck
141 145, 149 array
142 1 index
144 3 RangeCheck
145 4 array start
146 3
147 2
148 1 array end

Example 2 - Statement branch

Reading the element at index 4 from an array of 4 elements leads to an out-of-bound error, so a jump to the statement StatementIdx (59 in this example).

array_get<u32>([140], [141], [142]) {
    fallthrough([144], [150])
    59([144])
}

Memory

Ref Value Comment
140 2 RangeCheck
141 145, 149 array
142 4 index
144 3 RangeCheck
145 4 array start
146 3
147 2
148 1 array end

I like the syntax blocks and the descriptions! I am not a fan of the memory tables you are suggesting, for the following reasons:

  • They are called memory tables but what you present is not the representation of actual memory
  • "Ref" is inaccurate. It should probably be called "Address" as those are memory addresses
  • 145, 149 cannot both live on 141 address. Only one number for an address. Moreover, your table suggests that 145, 149 lives in the memory 141 address, which is not accurate. 145, 149 are instead values passed into the sierra libfunc call and are not part of the memory table at all. The numbers 145, 149 are instead references to addresses in memory
  • Comments are also not in the memory

@mazurroman
Copy link
Contributor

mazurroman commented Mar 4, 2024

Summarising what's left based on the comments above:

Left TODO:

  • The libfunc description is still saying "Gets an array element at a given index, handling out-of-range scenarios with a Statement branch." — we should also mention fall-through here to make it complete.
  • @remybar regarding full "About Sierra" page. I like your suggestion to just write some short copy and place it above the big table. Do you want to give it a shot?
  • For Invoke Refs and Output Refs, what about not using tables (similar to evm.codes). We have too many tables at this point and it's hard to visually navigate
  • Sierra function syntax in Example 2 is visualised differently than Example 1. We should make them consistent. I think I prefer the Example 2 version
  • Example 2 is currently missing a short description (we only added for Example 1)
  • Link to see example in the playground
  • Can we put memory, input refs and output refs next to each other, to save vertical space? Similar to this:
+-------+-------+     +-------------------------+
|     Memory    |     | Invoke refs             |
+-------+-------+     +------------+------------+
|       | Value |     | rangeCheck | [140]      |
+-------+-------+     +------------+------------+
| [140] | 2     |     | array      | [142, 145] |
+-------+-------+     +------------+------------+
| [142] | 0     |     | index      | 0          |
+-------+-------+     +------------+------------+
| [145] | 5     |
+-------+-------+

Unknowns but still left to do:

  • We need something for statementIdx
  • What is a range check

Rémy Baranx and others added 2 commits March 9, 2024 11:08
This new libfuncs page shows a table of Sierra libfuncs from markdown files
stored in `docs/libfuncs`.
@remybar remybar force-pushed the feat-libfuncs-pagee branch from b7a0ecb to fb3d854 Compare March 9, 2024 04:08
@remybar remybar changed the base branch from main to sierra-libfuncs March 11, 2024 08:27
@mazurroman mazurroman merged commit 34a073d into walnuthq:sierra-libfuncs Mar 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[doc] core libfuncs - implement the page skeleton
3 participants