Unintuitive groth16::encode API

[austinabell]

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:

• 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).
  • Latter might not be possible with circular deps, but quick scan does seem like it can be
• Document where the seal can be pulled from and examples in docs

  • risc0-ethereum/contracts/src/groth16.rs

    Line 24 in bfea3b8

    /// encoding of the seal with selector.

    I personally think would be a confusing explanation to someone unfamiliar?

[nategraf]

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]

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]

@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]

risc0-ethereum/examples/erc20-counter/apps/src/bin/publisher.rs

Lines 119 to 154 in d2af848

	let prove_info = task::spawn_blocking(move || {
	let env = ExecutorEnv::builder()
	.write(&view_call_input)?
	.write(&args.token)?
	.write(&args.account)?
	.build()
	.unwrap();
	default_prover().prove_with_ctx(
	env,
	&VerifierContext::default(),
	BALANCE_OF_ELF,
	&ProverOpts::groth16(),
	)
	})
	.await?
	.context("failed to create proof")?;
	let receipt = prove_info.receipt;
	let seal = encode_seal(&receipt)?;
	// Create an alloy instance of the Counter contract.
	let contract = ICounter::new(args.contract, provider);
	// Call the increment function of the contract and wait for confirmation.
	println!(
	"Sending Tx calling {} Function of {:#}...",
	ICounter::incrementCall::SIGNATURE,
	contract.address()
	);
	let call_builder = contract.increment(receipt.journal.bytes.into(), seal.into());
	let pending_tx = call_builder.send().await?;
	let receipt = pending_tx
	.with_timeout(Some(Duration::from_secs(60)))
	.get_receipt()
	.await?;
	ensure!(receipt.status(), "transaction failed");

[austinabell]

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!