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 compound APR example that uses multi-call #95

Merged
merged 17 commits into from
Jun 13, 2024

Conversation

austinabell
Copy link
Contributor

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.

Base automatically changed from austin/multiviewcall to main May 14, 2024 11:19
Wollac added a commit that referenced this pull request May 14, 2024
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>
@nategraf
Copy link
Contributor

Superseded by #115

@austinabell
Copy link
Contributor Author

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)

@Wollac
Copy link
Contributor

Wollac commented Jun 4, 2024

This is a good example to show how multiple functions can be called with Steel. Let's use it.

@Wollac Wollac marked this pull request as ready for review June 4, 2024 21:16
@Wollac Wollac requested review from capossele and nategraf June 4, 2024 21:16
Copy link
Contributor

@nategraf nategraf left a 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

- name: test erc20-Counter
run: ./test-local-deployment.sh
working-directory: examples/erc20-counter

Comment on lines +14 to +15
cargo install cargo-binstall
cargo binstall cargo-risczero
Copy link
Contributor

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.

Copy link
Member

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.

@nategraf nategraf merged commit f9ecda0 into main Jun 13, 2024
9 checks passed
@nategraf nategraf deleted the austin/historical_example branch June 13, 2024 16:10
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"] }
Copy link
Member

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.

@flaub
Copy link
Member

flaub commented Jun 13, 2024

Generally we should try to avoid using branch = main when creating risc0 dependencies. Now that we have 1.0 out, is there any reason to use git dependencies?

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.

4 participants