search

risc0 / risc0-ethereum

Integration of the RISC Zero verifiable compute platform with Ethereum and EVM chains.
https://risczero.com
Apache License 2.0
60 stars 15 forks source link

Unintuitive `groth16::encode` API #176

Closed austinabell closed 3 weeks ago

austinabell commented 1 month ago

Currently, it's unintuitive that from a receipt you need to know about this method, as well as the fact that you have to use a git dependency of risc0-ethereum since not yet released.

Currently the usage looks like this: https://github.com/risc0/risc0-foundry-template/blob/f8671069cd5f7ea54743e1007f2c26acfd0c80ad/apps/src/bin/publisher.rs#L130

and the API is slightly improved by removing the need for a clone in #175 / #174 but the API is still based around an ambiguous [u8] which is unclear what that represents, as well as the return value also being Vec<u8>.

The functions also return a Result which I assume is for future proofing it, but confusing to me personally because there doesn't seem to be anything than can error in the future.

Possibly an opinionated point to have the param be more strictly typed or have the API based around the receipt type, but it seems the less opinionated action items might be:

nategraf commented 1 month ago

I agree this could be a lot better. On publishing, the blocker is the use of forge in the build.rs script. I do want to address this.

Have this method be released in some version, either this crate separately, or looped into the zkvm crate under some feature flag (to avoid adding deps).

Part of the assumption has been that this encoding is tailored to Ethereum, and so if you need it, you are already depending on risc0-ethereum. Are you saying this is a bad assumption, or something else?

austinabell commented 1 month ago

Part of the assumption has been that this encoding is tailored to Ethereum, and so if you need it, you are already depending on risc0-ethereum. Are you saying this is a bad assumption, or something else?

I think this is a correct assumption. I was just including that as an option for where it could live. Assumption for that is having it under some eth feature flag, but I agree the default is likely to be in some eth specific crate. I'm just trying to think of ways to make it more discoverable/usable, while also being more typesafe if possible.

nategraf commented 3 weeks ago

@austinabell, do you think the changes in #179 [1] address this issue? With this, the calling code looks like the following [2]. So there is still the need to know you should use risc0_ethereum_contracts::encode_seal(&receipt) rather than perhaps having EVM ABI encoding directly integrated into risc0-zkvm. It does handle both Groth16 and fake receipts now though, and doesn't require any manual destructuring of the receipt.

[1] https://github.com/risc0/risc0-ethereum/pull/179/files#diff-910e010e15db10bf19a54e60bce569d9c8a30def3b64dff2dac01b319b106f70R25-R52 [2] https://github.com/risc0/risc0-ethereum/blob/d2af8480aa06a36c59f67ee4ba74d865e0ade7ef/examples/erc20-counter/apps/src/bin/publisher.rs#L119-L154

austinabell commented 3 weeks ago

I definitely think it solves the issue! Probably good to close the issue, would be opinionated to make any other changes.

Only small (somewhat) unopinionated thing to improve here is just having this be released in some way, as discoverability is a bit unclear unless just going off of the template. Unrelated to this issue though so I will close the issue, and feel free to re-open if you'd like!