-
Notifications
You must be signed in to change notification settings - Fork 47
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
ABI for syscall
and call
instructions
#971
Conversation
During the refactoring I also updated the doc comments, making them consistent and uniform. I think this is a good opportunity to create some kind of format guidebook (or stylebook) to have some rules and templates of how doc comments should be formatted. It is still a matter of discussion, but here is what I used during this refactoring. Format guidebookA book defining the format for documentation comments and regular comments for GeneralEntire procedure doc comment should be created using the
Each block should be separated from the others with a blank line. Example:
Procedure descriptionContains the general information about the purpose of the procedure and the way it works. May contain any other valuable information. If some list is used for description, it should be formatted like so:
Some data could be formatted as a subparagraph, in that case a blank line should be used to separate them (note: not sure about that, wouldn't it be confusing since we are using a blank line to separate different blocks?) Example:
Inputs and outputsEach variable could represent a single value or a sequence of four values (a Word). Variable representing a single value should be written in lowercase, and a variable for the word should be written in uppercase. Example:
It is strongly not recommended to use the single-letter names for variables, with just an exception for the loop indexes (i.e. InputsInputs block could contain three containers: operand stack, advice stack and advice map. Operand stack and advice stack should be presented as an array containing some data.
// end of the option 2 To show that some internal value array could have dynamic length, additional brackets should be used (see the In case some inputs are presented on the stack only if some condition is satisfied, such inputs should be placed in the "optional" box: inside the parentheses with a question mark at the end. Opening and closing brackets should be placed on a new line with the same offset as the other inputs, and values inside the brackets should be offseted by two spaces. Example:
Advice map should be presented as a sequence of the key-value pairs in the curly brackets. Opening bracket should stay on the same line, and the closing bracket should be placed on the next line after the last key-value pair with the same offset as the OutputsOutputs 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). FormatsFull versionIn case the values are provided not only through the operand stack, but also through any other container, the full version if the inputs should be used. Example:
Short versionIn case the values are provided only through the operand stack, a short version of the inputs and outputs should be used. In that case only Input values array should be offseted by one space to be inline with the output values array (see the example). Example:
Description of the used valuesIf some value was used in the inputs and outputs block (and its meaning is not obvious) this value should be described. Values description block should start with Example:
Panic blockIf the describing procedure could potentially panic, a panic block should be specified. Panic block should start with Example:
Annotation hintA temporary comment showing how the procedure is used. It will help to implement the procedure annotations in future.
|
Regarding the procedures ABI the only thing left is to handle the
This looks quite strange to me, but that's how the call should be handled. cc @bobbinth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I really like the standardization and consistency that brings.
Some comments and thoughts.
Regarding the formatting book:
Description should have a colon at the end.
Did you mean "period" instead of "colon"?
Edit: probably we could use the second option for the input stacks, and the first one for the stacks state for the comments inside the procedure.
Sounds good! I would probably allow formatting according to how the elements are related to each other. What I mean is if we have a double word representing one logical value, like potentially non fungible assets in the future, or two elements that belong together as input, like a u64
split into hi
and lo
then it seems to me it would make senes to format them that way:
#! Advice stack: [
#! u64_value_hi, u64_value_lo,
#! ACCOUNT_VAULT_ROOT,
#! NON_FUNGIBLE_ASSET_HI, NON_FUNGIBLE_ASSET_LO
#! ]
(not sure if double words use "hi" and "lo", but I hope you get my point).
Some data could be formatted as a subparagraph, in that case a blank line should be used to separate them (note: not sure about that, wouldn't it be confusing since we are using a blank line to separate different blocks?)
Another option would be to include the Notes:
section we sometimes have in the guideline? It often states assumptions of procedures. This could contain everything that does not fit elsewhere like:
#! Requires that the account exposes:
#! - miden::contracts::wallets::basic::receive_asset procedure.
or
#! Note inputs are assumed to be as follows:
#! - target_account_id is the ID of the account for which the note is intended.
Having some structure may be beneficial for later automatic operations on the docs. I imagine we'll later have some Rust wrappers for these procedures and we'll want to document those. Having the docs be structured would be helpful so they can be automatically copied and converted. In the same vein I'm also wondering if we should perhaps try to use Markdown for these doc comments to align even more with Rust doc comments. But, as long as we have some standardization like these guidelines then we could probably write some conversion script fairly easily. In any case, consistency is king 😁️. But I guess one question is: Would having a Notes
section be beneficial over standalone paragraphs?
We might want to extend the guidelines with how to document executable procedures which don't have an ABI per se, but sometimes modify the stack beyond its actual inputs (see the finalize_transaction
inline comment for details).
You mostly replaced [...]
with pad
or nothing, and I think that's fine. There are still some occurrences of this string in the codebase, and we should probably also update those. If I understood correctly, what was previously [x, y, ...]
is now [x, y]
and as mentioned for the executable procedures which truncate the stack, those should use pad
. Which is all to ask that we no longer need ...
anywhere, right?
A meta comment about the way the guidelines are written. My assumption is that we'll add this guideline to some documentation somewhere so it can be easily found. So from time to time we might look it up to find out how to format a procedure (for less common cases). In that case, having the guidelines described as a set of fairly exhaustive examples would be great. Examples are easier to read and extrapolate from than a textual description like:
Definitions should be represented as an unordered list constructed with - symbols, without any space offset.
Each definition should start with the name of the variable followed by the is/are the phrase (note: not sure about that), after which the definition should be placed. At the end of each definition should be a dot.
My motivation here is to make the guidelines easy to parse at a glance and find the formatting rule I'm looking for. So I'm just saying: Having fairly exhaustive examples in the guidelines that cover many possible cases would be great, and you've already done that for many parts, and I would go further in that direction.
#! Inputs: [pad(16)] | ||
#! Outputs: [H, pad(12)] | ||
#! | ||
#! Where: | ||
#! - H is the initial account hash. | ||
export.get_initial_account_hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we perhaps have a rule for a minimum length of a value name or alternatively just ban single-letter names for non-trivial things*? I think we should avoid those because they increase cognitive load and HASH
is just more readable than H
and still short enough to write easily.
Below in get_current_account_hash
we're using an even longer name so INIT_HASH
might be even better.
*The common exception being loop indices with names like i
, and things like that, where it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree! My initial though was to move the update of the names to another PR (since this one is already huge, and I wanted to leave this one to be a format+ABI only (but I already failed to do so :) )), but probably it is not a common problem, so I can update the names here.
Edit: I also think that we should add this to the guidebook.
#! Stack: [value] | ||
#! Output: [0] | ||
#! Inputs: [value, pad(15)] | ||
#! Outputs: [pad(16)] | ||
#! | ||
#! Where: | ||
#! - value is the value to increment the nonce by. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think increment
would be a more precise name here than value
.
#! Output: [H] | ||
#! Inputs: [] | ||
#! Outputs: [H] | ||
#! | ||
#! Where: | ||
#! - H is the initial account hash. | ||
export.memory::get_init_acct_hash->get_initial_hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Perhaps we can also use INIT_HASH
here instead of H
.
#! - 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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#! - this procedure is executed on the account which type differs from the `basic mutable`. | |
#! - this procedure is executed on an account whose type differs from `regular mutable`. |
Feels like we're not using a consistent language for account types. Sometimes it's "regular updatable" sometimes "regular mutable". Personally, I would also go with "mutable" as the pendant to "immutable" rather than "updatable".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the notation we're using in the docs here, but I'm fine with other options, I think it is more important to make types consistent rather than use some specific term.
#! Stack: [] | ||
#! Output: [] | ||
#! - All storage offsets and sizes are in bounds. | ||
#! - All storage offsets adhere to account type specific rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this line actually. It's not very helpful for the reader because it doesn't state a precise condition and we're spelling out the precise conditions in the other bullet points.
The only account-type specific thing that is being checked is that slot 0 is not accessed on faucet accounts, which we already mention.
miden-lib/asm/miden/account.masm
Outdated
#! - acct_id is the account id. | ||
#! | ||
#! Annotation hint: is used only with `exec` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#! Annotation hint: is used only with `exec` | |
#! Annotations: exec |
Maybe a shorter version of this is sufficient for now? I would exhaustively describe what it can be used with and the unmentioned variants should not be used. Not sure about the backticks. Since we're not using markdown and this is temporary anyway I think we can omit them.
For example, the following can only be call
-ed or syscall
-ed but not used with exec
.
#! Annotations: call + syscall
miden-lib/asm/miden/account.masm
Outdated
#! Stack: [] | ||
#! Output: [H] | ||
#! Inputs: [] | ||
#! Outputs: [H] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#! Outputs: [H] | |
#! Outputs: [INIT_HASH] |
#! Stack: [index, V'] | ||
#! Output: [R', V] | ||
#! Inputs: [index, V'] | ||
#! Outputs: [R', V] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#! Outputs: [R', V] | |
#! Outputs: [ROOT', VALUE] |
Somewhat related, but doesn't have to be done in this PR or at all: Another thing for consistency is that we're sometimes using "root" and sometimes "commitment" for the vault's "root". There is AssetVault::commitment
, but we're calling it the vault_root
in many places. I think root is more prevalent right now, so renaming commitment
to root
might be best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree, we should use one thing. We should revisit it during the work on the #858 issue.
miden-lib/asm/miden/account.masm
Outdated
#! - index is the index of the map where the KEY VALUE should be set. | ||
#! - KEY is the key to set at VALUE. | ||
#! - VALUE is the value to set at KEY. | ||
#! - OLD_MAP_ROOT is the old map root. | ||
#! - OLD_MAP_VALUE is the old value at KEY. | ||
#! | ||
#! Panics if: | ||
#! - the index for the map is out of bounds, means >255. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#! - the index for the map is out of bounds, means >255. | |
#! - the index for the map is out of bounds, meaning > 255. |
(ERR_FAUCET_INVALID_STORAGE_OFFSET, "Storage offset is invalid for a faucet account (0 is prohibited as it is the reserved data slot for faucets)"), | ||
(ERR_FAUCET_INVALID_STORAGE_OFFSET, "for faucets)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, only the line directly above error messages is parsed as the error, so we have two options:
- Allow error messages to go beyond 100 characters,
- or implement multi-line comment parsing in the build script. I briefly tried when I implemented it but wasn't able to come up with a regex that worked, but it surely is possible to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad, I forgot that we parse just one line for the error message. For now I'll roll back these changes, but probably it makes sense to create a new issue for this.
@PhilippGackstatter Thank you for the detailed review!
Sorry, I wasn't specific enough, I phrased it in a confusing way. By the "colon at the end" I meant only the list description, not the overall procedure description. I'll update this line. Note section sounds good to me, but I'm not sure what is better in terms of standardization: if I remember correctly, we don't use a dedicated note section in
Right. Previously we used the
I agree, it's much easier for me to look at the example than to read the definition. We should definitely improve the current version, for now it is just a raw and relatively poorly structured set of my thoughts. |
Not a review yet, but a couple of comments:
As discussed offline, I would go for consistency here even if it ends up costing extra 32 cycles.
I would make a distinction here as follows:
So, for something like
because we expect that the depth of the stack is exactly 16 when the procedure is entered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Thank you! I left some comments inline - but most are pretty small.
There was a problem hiding this 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 more comments inline. Once these are addressed, we are good to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you!
cc @igamigo - changes in this PR may have small effects on the client (e.g., in how we call note scripts). Could you check if everything still works fine there? |
@Fumuran - could you extract the Format Guidebook from #971 (comment) and add it as a stand alone Markdown file in |
This PR updates the ABI of the assembly procedures, making their usage with
call
andsyscall
safe.Corresponding issue: #685