-
Notifications
You must be signed in to change notification settings - Fork 48
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 compound APR example that uses multi-call #95
Conversation
Proposal to close #90 The least changing API would be to switch the `preflight` and `execute` methods to take in a mutable reference to the `env`, but this comes with the downside of having a breaking change from previous APIs (unless using different names) and an awkward API imo. This simple change mentioned above would look sth like: ```rust // Host let mut env = EthViewCallEnv::from_provider(provider, BLOCK)?; let _result = ViewCall::new(call, contract).preflight(&mut env)?; let _result2 = ViewCall::new(call2, contract).preflight(&mut env)?; let input = env.into_zkvm_input()? // Guest let env = input.into_env(); let result = ViewCall::new(call, contract).execute(&env); let result2 = ViewCall::new(call2, contract).execute(&env); ``` Where the proposal in this API flips the env and `ViewCall`, as it's more clear the call is operating on the `env`: ```rust // Host let env = EthViewCallEnv::from_provider(provider, BLOCK)?; let _result = env.preflight(ViewCall::new(call, contract))?; let _result2 = env.preflight(ViewCall::new(call2, contract))?; let input = env.into_zkvm_input()? // Guest let env = input.into_env(); let result = env.execute(ViewCall::new(call, contract)); let result2 = env.execute(ViewCall::new(call2, contract)); ``` ~~Opening as draft until approval, and I will add tests and an example with this.~~ basic test added and example could be #95, although that example probably isn't practical without historical queries supported better, in the example or in steel Another option is to create APIs around a type like `MultiViewCall`, and describe the multiple calls that way which returns a tuple of results with the zkvm input. I didn't love this option, as it would just be confusing to use, but figured I'd mention that also. Happy to elaborate on this if helpful, though! --------- Co-authored-by: Wolfgang Welz <welzwo@gmail.com>
Superseded by #115 |
I mean, it's pretty independent. This is just if we want an example that actually needs to call multiple functions. (will needed to be updated for new APIs though if that comes in) |
This is a good example to show how multiple functions can be called with Steel. Let's use it. |
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 is a well-factored example 👌
Can we go ahead and add this to the examples
CI job? We should at least make sure it builds. Better if we can run it too. Avoiding issues due to external API calls may mean using cached test data
risc0-ethereum/.github/workflows/main.yml
Lines 157 to 159 in da65e22
- name: test erc20-Counter | |
run: ./test-local-deployment.sh | |
working-directory: examples/erc20-counter |
cargo install cargo-binstall | ||
cargo binstall cargo-risczero |
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.
Some trickiness with this is that, on main, they will end up using two different versions of the zkVM. One is specified in the Cargo.toml here as the risc0-zkvm
dependency and will be main
, and the other is the latest release installed by cargo binstall
. It's unclear whether we are guaranteeing this will work or not, post 1.0.
At the very least, the result right now is
called `Result::unwrap()` on an `Err` value: incompatible client version: 1.1.0-alpha.1, server version: 1.0.1
This issue is not unique to this example, and we are trying to deal with this on risc0
as well.
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.
Might be better to just link to the install page on the docs so that if we make changes to the installation (like moving to rzup
), we don't need to update all the different descriptions of installation.
Co-authored-by: Victor Graf <victor@risczero.com>
alloy-sol-types = { version = "0.7" } | ||
core = { path = "../../core" } | ||
risc0-steel = { path = "../../../../steel" } | ||
risc0-zkvm = { git = "https://github.com/risc0/risc0", branch = "main", default-features = false, features = ["std"] } |
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.
It'd be better to pin to a specific commit rather than using the main branch. It's very easy for this to break in the future.
Generally we should try to avoid using |
Not done with this yet, I would like to potentially expand this to be able to prove an average APR over multiple points in time, but I might separate that into another PR if we want to keep this example simple.
Opening as draft because it depends on #91, and it's unclear we want that or this example here. Just opening for visibility.