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

Add Format Guidebook #987

Merged
merged 4 commits into from
Nov 29, 2024
Merged

Add Format Guidebook #987

merged 4 commits into from
Nov 29, 2024

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Nov 27, 2024

This small PR adds the Format Guidebook presented in the ABI PR to the miden-lib crate.

@Fumuran Fumuran requested a review from bobbinth November 27, 2024 13:06
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few small comments inline.

- The description of the list should have a colon at the end.
- Depending on what kind of sentences form a list, they should start with a capital letter and end\
with a dot, or start with a lowercase letter and without dot at the end.
- List should use a `-` symbol in case of unordered list or arabic numerals for ordered ones (for\
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, and in other similar places, I would use the word character instead of symbol.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks great!

Just a suggestion to make this guide as easy to use as possible with a comprehensive example as a quick reference.


### Outputs

Outputs should show the final state of each container, used in the inputs, except for the advice map: almost always the final state of the advice map is unimportant, since it is always the same as at the beginning of the procedure execution (see the example in the `Formats` section below).
Copy link
Contributor

Choose a reason for hiding this comment

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

almost always the final state of the advice map is unimportant, since it is always the same as at the beginning of the procedure execution

Nit: It's not entirely clear to me whether the advice map is always the same or almost always. Maybe this sentence could clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some instructions which put some data to the advice map, and I thought that we never used them in the miden-base, but I was wrong. I'll rewrite this paragraph and update the code which uses these instructions.

Comment on lines 19 to 34
Example:

```masm
#! Sets the code of the account the transaction is being executed against.
#!
#! Inputs: [CODE_COMMITMENT]
#! Outputs: []
#!
#! Where:
#! - CODE_COMMITMENT is the hash of the code to set.
#!
#! Panics if:
#! - this procedure is executed on the account which type differs from the `basic mutable`.
#!
#! Invocation: exec
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be great if this was a full example here that showcases most, or all possible cases, so it can serve as a great first reference to look at. And only if one can't figure out from that example what a comment should look like, we can use the reference level guide below.

So basically I would just extend the current example with

  • every possible value type (a felt, a word, a _ptr) in either Inputs or Outputs.
  • Inputs specifying Operand Stack, Advice Stack and Advice map.

@bobbinth bobbinth merged commit d0a56e7 into next Nov 29, 2024
9 checks passed
@bobbinth bobbinth deleted the andrew-add-guidebook branch November 29, 2024 07:57
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.

3 participants