feat!: Switch to using jsonrpsee for foreign calls; refactor `run_t…

…est`; foreign call layering (#6849)

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>

Cargo.toml

@@ -148,7 +148,7 @@ num-traits = "0.2"
 similar-asserts = "1.5.0"
 tempfile = "3.6.0"
 test-case = "3.3.1"
-jsonrpc = { version = "0.16.0", features = ["minreq_http"] }
+jsonrpsee = { version = "0.24.7", features = ["client-core"] }
 flate2 = "1.0.24"
 color-eyre = "0.6.2"
 rand = "0.8.5"
@@ -159,7 +159,7 @@ sha2 = { version = "0.10.6", features = ["compress"] }
 sha3 = "0.10.6"
 strum = "0.24"
 strum_macros = "0.24"
-
+tokio = "1.42"
 im = { version = "15.1", features = ["serde"] }
 tracing = "0.1.40"
 tracing-web = "0.1.3"

compiler/wasm/Cargo.toml

@@ -17,9 +17,9 @@ workspace = true
 crate-type = ["cdylib"]
 
 [dependencies]
+
 acvm = { workspace = true, features = ["bn254"] }
 fm.workspace = true
-nargo.workspace = true
 noirc_driver.workspace = true
 noirc_frontend = { workspace = true, features = ["bn254"] }
 noirc_errors.workspace = true
@@ -33,6 +33,10 @@ gloo-utils.workspace = true
 tracing-subscriber.workspace = true
 tracing-web.workspace = true
 
+# Cannot use the `rpc` feature because the HTTP dependency wouldn't compile to Wasm.
+# We could use `path` if `rpc` was a default feature, but we made it opt-in so we don't get any problems when publishing the workspace.
+nargo.workspace = true
+
 # This is an unused dependency, we are adding it
 # so that we can enable the js feature in getrandom.
 getrandom = { workspace = true, features = ["js"] }

cspell.json

@@ -126,6 +126,7 @@
         "jmpifs",
         "jmps",
         "jsdoc",
+        "jsonrpsee",
         "Jubjub",
         "keccak",
         "keccakf",
@@ -166,6 +167,7 @@
         "nomicfoundation",
         "noncanonical",
         "nouner",
+        "oneshot",
         "overflowing",
         "pedersen",
         "peekable",

deny.toml

@@ -66,14 +66,19 @@ exceptions = [
     # so we prefer to not have dependencies using it
     # https://tldrlegal.com/license/creative-commons-cc0-1.0-universal
     { allow = ["CC0-1.0"], name = "more-asserts" },
-    { allow = ["CC0-1.0"], name = "jsonrpc" },
     { allow = ["CC0-1.0"], name = "notify" },
     { allow = ["CC0-1.0"], name = "tiny-keccak" },
     { allow = ["MPL-2.0"], name = "sized-chunks" },
     { allow = ["MPL-2.0"], name = "webpki-roots" },
     { allow = ["CDDL-1.0"], name = "inferno" },
+    { allow = ["OpenSSL"], name = "ring" },
 ]
 
+[[licenses.clarify]]
+crate = "ring"
+expression = "ISC"
+license-files = [{ path = "LICENSE", hash = 0xbd0eed23 }]
+
 # This section is considered when running `cargo deny check sources`.
 # More documentation about the 'sources' section can be found here:
 # https://embarkstudios.github.io/cargo-deny/checks/sources/cfg.html

tooling/acvm_cli/src/cli/execute_cmd.rs

@@ -9,7 +9,7 @@ use nargo::PrintOutput;
 
 use crate::cli::fs::inputs::{read_bytecode_from_file, read_inputs_from_file};
 use crate::errors::CliError;
-use nargo::{foreign_calls::DefaultForeignCallExecutor, ops::execute_program};
+use nargo::{foreign_calls::DefaultForeignCallBuilder, ops::execute_program};
 
 use super::fs::witness::{create_output_witness_string, save_witness_to_dir};
 
@@ -74,7 +74,8 @@ pub(crate) fn execute_program_from_witness(
         &program,
         inputs_map,
         &Bn254BlackBoxSolver,
-        &mut DefaultForeignCallExecutor::new(PrintOutput::Stdout, None, None, None),
+        &mut DefaultForeignCallBuilder { output: PrintOutput::Stdout, ..Default::default() }
+            .build(),
     )
     .map_err(CliError::CircuitExecutionError)
 }

tooling/debugger/src/foreign_calls.rs

@@ -4,7 +4,9 @@ use acvm::{
     AcirField, FieldElement,
 };
 use nargo::{
-    foreign_calls::{DefaultForeignCallExecutor, ForeignCallError, ForeignCallExecutor},
+    foreign_calls::{
+        layers::Layer, DefaultForeignCallBuilder, ForeignCallError, ForeignCallExecutor,
+    },
     PrintOutput,
 };
 use noirc_artifacts::debug::{DebugArtifact, DebugVars, StackFrame};
@@ -43,23 +45,31 @@ pub trait DebugForeignCallExecutor: ForeignCallExecutor<FieldElement> {
     fn current_stack_frame(&self) -> Option<StackFrame<FieldElement>>;
 }
 
-pub struct DefaultDebugForeignCallExecutor<'a> {
-    executor: DefaultForeignCallExecutor<'a, FieldElement>,
+#[derive(Default)]
+pub struct DefaultDebugForeignCallExecutor {
     pub debug_vars: DebugVars<FieldElement>,
 }
 
-impl<'a> DefaultDebugForeignCallExecutor<'a> {
-    pub fn new(output: PrintOutput<'a>) -> Self {
-        Self {
-            executor: DefaultForeignCallExecutor::new(output, None, None, None),
-            debug_vars: DebugVars::default(),
-        }
+impl DefaultDebugForeignCallExecutor {
+    fn make(
+        output: PrintOutput<'_>,
+        ex: DefaultDebugForeignCallExecutor,
+    ) -> impl DebugForeignCallExecutor + '_ {
+        DefaultForeignCallBuilder::default().with_output(output).build().add_layer(ex)
+    }
+
+    #[allow(clippy::new_ret_no_self, dead_code)]
+    pub fn new(output: PrintOutput<'_>) -> impl DebugForeignCallExecutor + '_ {
+        Self::make(output, Self::default())
     }
 
-    pub fn from_artifact(output: PrintOutput<'a>, artifact: &DebugArtifact) -> Self {
-        let mut ex = Self::new(output);
+    pub fn from_artifact<'a>(
+        output: PrintOutput<'a>,
+        artifact: &DebugArtifact,
+    ) -> impl DebugForeignCallExecutor + 'a {
+        let mut ex = Self::default();
         ex.load_artifact(artifact);
-        ex
+        Self::make(output, ex)
     }
 
     pub fn load_artifact(&mut self, artifact: &DebugArtifact) {
@@ -72,7 +82,7 @@ impl<'a> DefaultDebugForeignCallExecutor<'a> {
     }
 }
 
-impl DebugForeignCallExecutor for DefaultDebugForeignCallExecutor<'_> {
+impl DebugForeignCallExecutor for DefaultDebugForeignCallExecutor {
     fn get_variables(&self) -> Vec<StackFrame<FieldElement>> {
         self.debug_vars.get_variables()
     }
@@ -90,7 +100,7 @@ fn debug_fn_id(value: &FieldElement) -> DebugFnId {
     DebugFnId(value.to_u128() as u32)
 }
 
-impl ForeignCallExecutor<FieldElement> for DefaultDebugForeignCallExecutor<'_> {
+impl ForeignCallExecutor<FieldElement> for DefaultDebugForeignCallExecutor {
     fn execute(
         &mut self,
         foreign_call: &ForeignCallWaitInfo<FieldElement>,
@@ -165,7 +175,21 @@ impl ForeignCallExecutor<FieldElement> for DefaultDebugForeignCallExecutor<'_> {
                 self.debug_vars.pop_fn();
                 Ok(ForeignCallResult::default())
             }
-            None => self.executor.execute(foreign_call),
+            None => Err(ForeignCallError::NoHandler(foreign_call_name.to_string())),
         }
     }
 }
+
+impl<H, I> DebugForeignCallExecutor for Layer<H, I>
+where
+    H: DebugForeignCallExecutor,
+    I: ForeignCallExecutor<FieldElement>,
+{
+    fn get_variables(&self) -> Vec<StackFrame<FieldElement>> {
+        self.handler().get_variables()
+    }
+
+    fn current_stack_frame(&self) -> Option<StackFrame<FieldElement>> {
+        self.handler().current_stack_frame()
+    }
+}

tooling/lsp/src/requests/test_run.rs

@@ -3,6 +3,7 @@ use std::future::{self, Future};
 use crate::insert_all_files_for_workspace_into_file_manager;
 use async_lsp::{ErrorCode, ResponseError};
 use nargo::{
+    foreign_calls::DefaultForeignCallBuilder,
     ops::{run_test, TestStatus},
     PrintOutput,
 };
@@ -88,10 +89,16 @@ fn on_test_run_request_inner(
                 &mut context,
                 &test_function,
                 PrintOutput::Stdout,
-                None,
-                Some(workspace.root_dir.clone()),
-                Some(package.name.to_string()),
                 &CompileOptions::default(),
+                |output, base| {
+                    DefaultForeignCallBuilder {
+                        output,
+                        resolver_url: None, // NB without this the root and package don't do anything.
+                        root_path: Some(workspace.root_dir.clone()),
+                        package_name: Some(package.name.to_string()),
+                    }
+                    .build_with_base(base)
+                },
             );
             let result = match test_result {
                 TestStatus::Pass => NargoTestRunResult {

tooling/nargo/Cargo.toml

@@ -21,21 +21,27 @@ noirc_errors.workspace = true
 noirc_frontend.workspace = true
 noirc_printable_type.workspace = true
 iter-extended.workspace = true
+jsonrpsee.workspace = true
+rayon.workspace = true
 thiserror.workspace = true
 tracing.workspace = true
-rayon.workspace = true
-jsonrpc.workspace = true
-rand.workspace = true
 serde.workspace = true
 serde_json.workspace = true
 walkdir = "2.5.0"
 
+# Some dependencies are optional so we can compile to Wasm.
+tokio = { workspace = true, optional = true }
+rand = { workspace = true, optional = true }
+
 [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
-noir_fuzzer.workspace = true
-proptest.workspace = true
+noir_fuzzer = { workspace = true }
+proptest = { workspace = true }
 
 [dev-dependencies]
-jsonrpc-http-server = "18.0"
-jsonrpc-core-client = "18.0"
-jsonrpc-derive = "18.0"
-jsonrpc-core = "18.0"
+jsonrpsee = { workspace = true, features = ["server"] }
+
+[features]
+default = []
+
+# Execution currently uses HTTP based Oracle resolvers; does not compile to Wasm.
+rpc = ["jsonrpsee/http-client", "jsonrpsee/macros", "tokio/rt", "rand"]

tooling/nargo/src/foreign_calls/default.rs

@@ -0,0 +1,115 @@
+use acvm::AcirField;
+use serde::{Deserialize, Serialize};
+
+use crate::PrintOutput;
+
+use super::{
+    layers::{self, Layer, Layering},
+    mocker::MockForeignCallExecutor,
+    print::PrintForeignCallExecutor,
+    ForeignCallExecutor,
+};
+
+#[cfg(feature = "rpc")]
+use super::rpc::RPCForeignCallExecutor;
+
+/// A builder for [DefaultForeignCallLayers] where we can enable fields based on feature flags,
+/// which is easier than providing different overrides for a `new` method.
+#[derive(Default)]
+pub struct DefaultForeignCallBuilder<'a> {
+    pub output: PrintOutput<'a>,
+    #[cfg(feature = "rpc")]
+    pub resolver_url: Option<String>,
+    #[cfg(feature = "rpc")]
+    pub root_path: Option<std::path::PathBuf>,
+    #[cfg(feature = "rpc")]
+    pub package_name: Option<String>,
+}
+
+impl<'a> DefaultForeignCallBuilder<'a> {
+    /// Override the output.
+    pub fn with_output(mut self, output: PrintOutput<'a>) -> Self {
+        self.output = output;
+        self
+    }
+
+    /// Compose the executor layers with [layers::Empty] as the default handler.
+    pub fn build<F>(self) -> DefaultForeignCallLayers<'a, layers::Empty, F>
+    where
+        F: AcirField + Serialize + for<'de> Deserialize<'de> + 'a,
+    {
+        self.build_with_base(layers::Empty)
+    }
+
+    /// Compose the executor layers with `base` as the default handler.
+    pub fn build_with_base<B, F>(self, base: B) -> DefaultForeignCallLayers<'a, B, F>
+    where
+        F: AcirField + Serialize + for<'de> Deserialize<'de> + 'a,
+        B: ForeignCallExecutor<F> + 'a,
+    {
+        let executor = {
+            #[cfg(feature = "rpc")]
+            {
+                use rand::Rng;
+
+                base.add_layer(self.resolver_url.map(|resolver_url| {
+                    let id = rand::thread_rng().gen();
+                    RPCForeignCallExecutor::new(
+                        &resolver_url,
+                        id,
+                        self.root_path,
+                        self.package_name,
+                    )
+                }))
+            }
+            #[cfg(not(feature = "rpc"))]
+            {
+                base
+            }
+        };
+
+        executor
+            .add_layer(MockForeignCallExecutor::default())
+            .add_layer(PrintForeignCallExecutor::new(self.output))
+    }
+}
+
+/// Facilitate static typing of layers on a base layer, so inner layers can be accessed.
+#[cfg(feature = "rpc")]
+pub type DefaultForeignCallLayers<'a, B, F> = Layer<
+    PrintForeignCallExecutor<'a>,
+    Layer<MockForeignCallExecutor<F>, Layer<Option<RPCForeignCallExecutor>, B>>,
+>;
+#[cfg(not(feature = "rpc"))]
+pub type DefaultForeignCallLayers<'a, B, F> =
+    Layer<PrintForeignCallExecutor<'a>, Layer<MockForeignCallExecutor<F>, B>>;
+
+/// Convenience constructor for code that used to create the executor this way.
+#[cfg(feature = "rpc")]
+pub struct DefaultForeignCallExecutor;
+
+/// Convenience constructors for the RPC case. Non-RPC versions are not provided
+/// because once a crate opts into this within the workspace, everyone gets it
+/// even if they don't want to. For the non-RPC case we can nudge people to
+/// use `DefaultForeignCallBuilder` which is easier to keep flexible.
+#[cfg(feature = "rpc")]
+impl DefaultForeignCallExecutor {
+    #[allow(clippy::new_ret_no_self)]
+    pub fn new<'a, F>(
+        output: PrintOutput<'a>,
+        resolver_url: Option<&str>,
+        root_path: Option<std::path::PathBuf>,
+        package_name: Option<String>,
+    ) -> impl ForeignCallExecutor<F> + 'a
+    where
+        F: AcirField + Serialize + for<'de> Deserialize<'de> + 'a,
+    {
+        DefaultForeignCallBuilder {
+            output,
+            resolver_url: resolver_url.map(|s| s.to_string()),
+            root_path,
+            package_name,
+        }
+        .build()
+    }
+}