-
Notifications
You must be signed in to change notification settings - Fork 17
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 readme #421
base: main
Are you sure you want to change the base?
Add readme #421
Conversation
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.
Overall LGTM. Added comments though. As I am out next week, I'll "move the ownership" of my comments to @shaharsamocha7 so once he approves including handling my comments, just dismiss my review.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ArielElp and @maya-starkware)
README.md
line 22 at r1 (raw file):
🚧 The Stwo prover and its Cairo adaptation are still being built, therefore API breaking changes might happen often, so use it at your own risk. 🚧
I would remove, but it's only my opinion.
Code quote:
so use it at your own risk.
README.md
line 32 at r1 (raw file):
* memory.bin With the paths to the trace and memory files appearing in `air_private_inputs`. After building stwo-cairo, you can run the `adapted_stwo` binary and obtain a Stwo proof:
IIRC it didn't work with relative paths. Please check, and if that's true, may be good to add here that the paths should be absolute.
Code quote:
With the paths to the trace and memory files appearing in `air_private_inputs
README.md
line 35 at r1 (raw file):
./target/release/adapted_stwo \
There are no earlier instructions of how to build it. Don't you want to just instruct to run it with cargo (which also builds ti)?
Code quote:
./target/release/adapted_stwo \
README.md
line 84 at r1 (raw file):
Now we can move on to the code itself. An executable project must have exactly one function annotated with the
#[executable]
attribute. Consider the following simplelib.cairo
file of an executable project:
this is not true with cairo_execute (though if there are multiple, you need to specify which one to run). Is it because scarb doesn't support it?
Code quote:
An executable project must have **exactly one function** annot
README.md
line 101 at r1 (raw file):
Where `test_execute` is the name of the package with the executable target (as defined in our Scarb.toml manifest) The above command runs our executable function within the `test-execute` package and prints the program's output segment, which contains a success bit followed by the Cairo Serde of main’s output or the panic reason in case of a panic.
Suggestion:
ns a success bit (0 for success) followed by the Cairo
README.md
line 105 at r1 (raw file):
## Execution targets The `--target` flag allows specifying either a `standalone` target or `bootloader` target. Standalone means that the program will be executed as-is, and intended to be proven directly with Stwo. When we run with the bootloader target, the program’s execution is expected to be wrapped by the bootloader’s execution, which itself will be proven via Stwo.
any pointer/explanation about what the bootloader is?
Code quote:
e bootloader’
README.md
line 109 at r1 (raw file):
When executing with `--target standalone` (the default if not specified) we get the four files which consist as the input for the `adapted_stwo` binary (`air_private_input.json`, `air_public_input.json`, `trace.bin`, `memory.bin`), while when executing with `--target bootloader`, the output is given in the CairoPie format (Position Indenpendent Execution). In the meantime, `stwo-cairo` does not contain an API for executing the bootloader with the user's program as input, hence the only way to get a proof for a bootloader target is to take the generated CairoPie and send it to a third party like SHARP or [Atlantic](https://docs.herodotus.cloud/atlantic/introduction).
users can't really send to SHARP not via Atlantic...
Code quote:
arty like SHARP
README.md
line 125 at r1 (raw file):
See the [documentation](https://docs.starknet.io/architecture-and-concepts/smart-contracts/serialization-of-cairo-types/) for more information about Cairo’s Serde. Note that when using `--arguments-file`, the expected input is an array of felts represented as hex string. For example, `1,2,3` in the above table becomes `[0x1, 0x2, 0x3]`.
missing quotes surrounding the items
Code quote:
e table becomes `[0x1, 0x2, 0x3]`.
README.md
line 129 at r1 (raw file):
## Output format For standalone targets, the supported output is trace files (`air_public_input.json` & `air_private_input.json`)
should be the 4 files, no?
Code quote:
`air_public_input.json` & `air_private_input.json`)
README.md
line 131 at r1 (raw file):
For standalone targets, the supported output is trace files (`air_public_input.json` & `air_private_input.json`) For bootloader targets, the supported output is a Cairo pie
Suggestion:
For standalone targets, the output is trace files (`air_public_input.json` & `air_private_input.json`).
For bootloader targets, the output is a Cairo pie.
README.md
line 137 at r1 (raw file):
### Gas Executables must be compiled with the `enable-gas = false` config in the manifest file. Gas tracking introduces a computation overhead and makes less sense outside the context of Starnet smart contracts.
Suggestion:
the context of Starknet smart
README.md
line 141 at r1 (raw file):
### Syscalls Syscalls are not supported in an executable target. Using syscalls, directly or via corelib functions that uses syscall (such as sha256, keccak, secp256k/r1 operations) will cause compilation to fail.
Suggestion:
Syscalls are not supported in an executable target. Using syscalls, directly or via corelib functions that use syscalls (such as sha256, keccak, secp256k/r1 operations) will fail the compilation.
README.md
line 145 at r1 (raw file):
### Padding overhead At the time of writing, the execution (\# of steps and \# of builtin application per builtin) with a `standalone` target is still padded to powers of 2, w.r.t to the ratios in the [all_cairo](https://github.com/lambdaclass/cairo-vm/blob/15bf79470cdd8eff29f41fc0a87143dce5499c7e/vm/src/types/instance_definitions/builtins_instance_def.rs#L157) layout. This will be removed in the future as Stwo does not rely on padding. Bootloader target executions are not padded.
Suggestion:
. `bootloader` target ex
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.
Reviewable status: 0 of 1 files reviewed, 11 unresolved discussions (waiting on @maya-starkware and @yuvalsw)
README.md
line 22 at r1 (raw file):
Previously, yuvalsw wrote…
I would remove, but it's only my opinion.
I think we prefer stronger wording here, at least for now
README.md
line 32 at r1 (raw file):
Previously, yuvalsw wrote…
IIRC it didn't work with relative paths. Please check, and if that's true, may be good to add here that the paths should be absolute.
Done.
README.md
line 35 at r1 (raw file):
Previously, yuvalsw wrote…
There are no earlier instructions of how to build it. Don't you want to just instruct to run it with cargo (which also builds ti)?
Done.
README.md
line 84 at r1 (raw file):
Previously, yuvalsw wrote…
this is not true with cairo_execute (though if there are multiple, you need to specify which one to run). Is it because scarb doesn't support it?
Hhhmm it looks like:
- cairo-execute allows you to choose based on which entry point you want to create the executable (i.e. this executable will only support the one you chose)
- scarb didn't add support for this flag
leaving the readme as-is for now and will discuss further on the Scarb front
README.md
line 105 at r1 (raw file):
Previously, yuvalsw wrote…
any pointer/explanation about what the bootloader is?
Added a link to Moonsong's repo which has a brief explanation
README.md
line 109 at r1 (raw file):
Previously, yuvalsw wrote…
users can't really send to SHARP not via Atlantic...
Removed SHARP
README.md
line 125 at r1 (raw file):
Previously, yuvalsw wrote…
missing quotes surrounding the items
Done.
README.md
line 129 at r1 (raw file):
Previously, yuvalsw wrote…
should be the 4 files, no?
Done.
README.md
line 101 at r1 (raw file):
Where `test_execute` is the name of the package with the executable target (as defined in our Scarb.toml manifest) The above command runs our executable function within the `test-execute` package and prints the program's output segment, which contains a success bit followed by the Cairo Serde of main’s output or the panic reason in case of a panic.
Done.
README.md
line 131 at r1 (raw file):
For standalone targets, the supported output is trace files (`air_public_input.json` & `air_private_input.json`) For bootloader targets, the supported output is a Cairo pie
Done.
README.md
line 137 at r1 (raw file):
### Gas Executables must be compiled with the `enable-gas = false` config in the manifest file. Gas tracking introduces a computation overhead and makes less sense outside the context of Starnet smart contracts.
Done.
README.md
line 141 at r1 (raw file):
### Syscalls Syscalls are not supported in an executable target. Using syscalls, directly or via corelib functions that uses syscall (such as sha256, keccak, secp256k/r1 operations) will cause compilation to fail.
Done
README.md
line 145 at r1 (raw file):
### Padding overhead At the time of writing, the execution (\# of steps and \# of builtin application per builtin) with a `standalone` target is still padded to powers of 2, w.r.t to the ratios in the [all_cairo](https://github.com/lambdaclass/cairo-vm/blob/15bf79470cdd8eff29f41fc0a87143dce5499c7e/vm/src/types/instance_definitions/builtins_instance_def.rs#L157) layout. This will be removed in the future as Stwo does not rely on padding. Bootloader target executions are not padded.
Done.
Add readme with instructions on how to use the
adapted_stwo
binary and how to get the trace files from a Cairo package.This change is