Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat(acvm)!: Introduce Error type for fallible Backend traits #248

Merged
merged 2 commits into from
Apr 28, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented Apr 28, 2023

Related issue(s)

Resolves #235

Description

Summary of changes

This adds a required Fallible trait to ACVM backends. It is just used to create an Associated Type called Error that implements std::error::Error because traits aren't allowed to use impl std::error::Error and we don't want to require Backend implementors to have to Box all of their errors.

This uses the Error type to make other trait functions return Result types, which will allow Nargo to handle errors returned by a backend instead of the backend needing to panic. For example, the barretenberg wasm backend and https://github.com/TomAFrench/acvm-backend-groth16 both needed to unwrap/expect a lot of things but those errors should be bubbled up to nargo.

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

Planning to implement this throughout aztec_backend and nargo.

@TomAFrench
Copy link
Member

I'm wondering whether we want to allow for multiple error types here? e.g. ProofSystemCompiler should return different errors to SmartContract?

We could also go more granular.

@phated
Copy link
Contributor Author

phated commented Apr 28, 2023

I'm wondering whether we want to allow for multiple error types here? e.g. ProofSystemCompiler should return different errors to SmartContract?

I think it's fine to group them all under one error type. My prototype used a thiserror defined by the backend so it can provide whatever errors it needs.

@phated phated changed the title feat(acvm)!: Introduce Fallible trait for Backends & make functions fallible feat(acvm)!: Introduce Error type for fallible Backend traits Apr 28, 2023
@phated
Copy link
Contributor Author

phated commented Apr 28, 2023

As per @TomAFrench and @kevaundray, I've switched this to having a separate type Error on each trait that has fallible functions.

@kevaundray kevaundray added this pull request to the merge queue Apr 28, 2023
Merged via the queue into master with commit 45c45f7 Apr 28, 2023
@github-actions github-actions bot mentioned this pull request Apr 28, 2023
@TomAFrench TomAFrench deleted the phated/fallible branch April 28, 2023 21:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACVM Backend functions should be fallible
4 participants