feat: Implement a generic request canister interface by ninegua · Pull Request #18 · dfinity/sol-rpc-canister

ninegua · 2025-02-25T12:56:46Z

Implement a generic request interface to query Solana API.

…ter-request

ninegua · 2025-02-28T13:35:28Z

libs/types/src/lib.rs

 mod rpc_client;

-pub use lifecycle::InstallArgs;
+pub use evm_rpc_types::{


Here it directly re-exports a few definitions from evm_rpc_types, which are generic enough and require no modification.

As commented above, I would personally avoid having direct dependencies on EVM RPC if possible. If anything I would maybe move some of the types to a new crate that can be shared between the two repositories.

ninegua · 2025-02-28T13:36:28Z

Cargo.toml

 async-trait = "0.1.86"
 candid = "0.10.13"
 candid_parser = "0.1.4"
+canhttp = { git = "https://github.com/dfinity/evm-rpc-canister", rev = "bd02c54bba5d4df424909cbf809f5a1a07739467" }


It is not the latest, but maybe we can wait a bit until canhttp stabilizes?

sure, this is totally fine. Just FRY I think there are still 2 things missing from canhttp to be fully functional:

JSON-RPC layer

Retry layer, double max response size if request failed because response was too big.

gregorydemay

Thanks a lot @ninegua for this PR! A preliminary review

gregorydemay · 2025-03-03T08:43:46Z

Cargo.toml

 candid_parser = "0.1.4"
+canhttp = { git = "https://github.com/dfinity/evm-rpc-canister", rev = "bd02c54bba5d4df424909cbf809f5a1a07739467" }
 ciborium = "0.2.2"
+evm_rpc_types = "1.3.0"


understanding question: is-it mean to be temporary (e.g. until canhttp stabilizes)? I think a dependency there on an EVM-related crate is unexpected.

There are a few types that are not EVM specific and have been used here with no modification:

HttpOutcallError, JsonRpcError, MultiRpcResult, ProviderError, RpcError, RpcResult, ValidationError

I think it is best to avoid duplication. But at the same time I don't know if they warrant a separate crate. What's your thought on this?

I think I would start by duplicating the types and removing everything that we currently don't need:

The error types in the EVM RPC canister grew historically and where initially implemented when providers could be added dynamically which is no longer the case and not something that we want to support with SOL RPC. That means that some error variants are useless (e.g. ProviderError::NoPermission, ValidationError::InvalidHex) and I wouldn't want to start a new project with some tech debt of another project.

Generally I think it will probably make sense to have a generic RPC canister crate to put those common types as well as common logic regarding reduction of multiple results but I have the impression that's something that can wait.

Does it make sense?

I would also avoid direct dependencies on EVM RPC unless they are meant to be temporary, but then I would for sure add a TODO so we don't forget to remove the dependencies. Otherwise ideally I would just copy the necessary error types as suggested by Grégory.

gregorydemay · 2025-03-03T08:44:58Z

Cargo.toml

 async-trait = "0.1.86"
 candid = "0.10.13"
 candid_parser = "0.1.4"
+canhttp = { git = "https://github.com/dfinity/evm-rpc-canister", rev = "bd02c54bba5d4df424909cbf809f5a1a07739467" }


sure, this is totally fine. Just FRY I think there are still 2 things missing from canhttp to be fully functional:

JSON-RPC layer

Retry layer, double max response size if request failed because response was too big.

gregorydemay · 2025-03-03T08:47:19Z

canister/sol_rpc_canister.did

  getProviders : () -> (vec Provider) query;
  updateApiKeys : (vec record { ProviderId; opt text }) -> ();
-};
+  request : (RpcService, json : text, maxResponseBytes : nat64) -> (RequestResult);


nit: I think we should start also documenting the Candid API, like we did here for example.

Added some docs.

canister/src/main.rs

canister/src/http.rs

gregorydemay · 2025-03-03T08:58:15Z

integration_tests/src/lib.rs

+    async fn request(
+        &self,
+        service: RpcService,
+        json_rpc_payload: &str,
+        max_response_bytes: u64,
+    ) -> CallFlow<'_, RpcResult<String>> {
+        CallFlow::from_update(
+            self.runtime.clone(),
+            self.sol_rpc_canister,
+            "request",
+            Encode!(&service, &json_rpc_payload, &max_response_bytes).unwrap(),
+        )
+        .await
+    }


I don't think we should have a test implementation here. We should use the client from sol_rpc_client (and add the corresponding request method)

I did some refactoring and now the request method is part of the rpc client, but we need to setup mocks in the test client before making the call.

Thanks! Cool idea setting up the mock as part of the PocketIcRuntime 💡

gregorydemay

Thanks a lot @ninegua for this PR! I left some comments but otherwise the code LGTM!

gregorydemay · 2025-03-07T07:46:00Z

canister/sol_rpc_canister.did

  getProviders : () -> (vec Provider) query;
  updateApiKeys : (vec record { ProviderId; opt text }) -> ();
-};
+  request : (RpcService, json : text, maxResponseBytes : nat64) -> (RequestResult);


gregorydemay · 2025-03-07T07:56:44Z

Cargo.toml

 candid_parser = "0.1.4"
+canhttp = { git = "https://github.com/dfinity/evm-rpc-canister", rev = "bd02c54bba5d4df424909cbf809f5a1a07739467" }
 ciborium = "0.2.2"
+evm_rpc_types = "1.3.0"


I think I would start by duplicating the types and removing everything that we currently don't need:

The error types in the EVM RPC canister grew historically and where initially implemented when providers could be added dynamically which is no longer the case and not something that we want to support with SOL RPC. That means that some error variants are useless (e.g. ProviderError::NoPermission, ValidationError::InvalidHex) and I wouldn't want to start a new project with some tech debt of another project.

Generally I think it will probably make sense to have a generic RPC canister crate to put those common types as well as common logic regarding reduction of multiple results but I have the impression that's something that can wait.

Does it make sense?

gregorydemay · 2025-03-07T08:16:27Z

integration_tests/src/lib.rs

+    async fn request(
+        &self,
+        service: RpcService,
+        json_rpc_payload: &str,
+        max_response_bytes: u64,
+    ) -> CallFlow<'_, RpcResult<String>> {
+        CallFlow::from_update(
+            self.runtime.clone(),
+            self.sol_rpc_canister,
+            "request",
+            Encode!(&service, &json_rpc_payload, &max_response_bytes).unwrap(),
+        )
+        .await
+    }


Thanks! Cool idea setting up the mock as part of the PocketIcRuntime 💡

gregorydemay · 2025-03-07T08:20:47Z

integration_tests/src/lib.rs

    async fn retrieve_logs(&self, priority: &str) -> Vec<LogEntry<Priority>>;
    fn with_caller<T: Into<Principal>>(self, id: T) -> Self;
+    fn mock_http(self, mock: impl Into<MockOutcall>) -> Self;
+    fn mock_http_once(self, mock: impl Into<MockOutcall>) -> Self;


I know this comes a bit from the EVM RPC canister, but do you see a need for mock_http/mock_http_n_times? My impression is that we should be very explicit as to when an HTTPs outcall is made and for this only mock_http_once seems necessary. WDYT?

gregorydemay · 2025-03-07T08:23:17Z

integration_tests/src/lib.rs

+            .submit_call(id, self.caller, method, PocketIcRuntime::encode_args(args))
+            .await
+            .expect("failed to submit call");
+        self.execute_mock().await;


gregorydemay · 2025-03-07T08:27:51Z

integration_tests/src/mock.rs

+};
+use std::collections::BTreeSet;
+
+pub struct MockOutcallBody(pub Vec<u8>);


nit: it might be also useful to construct a MockOutcallBody from a serde_json::Value so that we can use the json! macro in test. E.g.

let mock = MockOutcallBuilder::new(200, json!({"foo": "bar"}));

gregorydemay · 2025-03-07T08:31:45Z

integration_tests/tests/tests.rs

+                    0,
+                )
+                .await,
+            Ok(_)


nit: the response doesn't seem to be ever modified, shouldn't we assert that response is MOCK_REQUEST_RESPONSE to strengthen the test?.

It's not yet clear to me what the tests in mock_request_tests bring in addition to the ones in generic_request_tests? Are they just some smoke tests?

…ter-request

lpahlavi

Thanks a lot for this PR @ninegua, a few small comments but overall it LGTM!

It seems there is a bit of overlap with my PR here, although it's not tragic IMO. Some of the biggest differences are due to my PR being a little bit more recent and hence making use of the new canhttp layers. Let us discuss offline how best to resolve the conflicts.

lpahlavi · 2025-03-17T16:18:48Z

Cargo.toml

 candid_parser = "0.1.4"
+canhttp = { git = "https://github.com/dfinity/evm-rpc-canister", rev = "bd02c54bba5d4df424909cbf809f5a1a07739467" }
 ciborium = "0.2.2"
+evm_rpc_types = "1.3.0"


I would also avoid direct dependencies on EVM RPC unless they are meant to be temporary, but then I would for sure add a TODO so we don't forget to remove the dependencies. Otherwise ideally I would just copy the necessary error types as suggested by Grégory.

lpahlavi · 2025-03-17T16:21:16Z

canister/src/http.rs

+};
+use tower::{BoxError, Service, ServiceBuilder};
+
+pub fn json_rpc_request_arg(


Since the JSON layer was added to canhttp it would be nice to use that here, but it can also wait for a follow-up PR.

lpahlavi · 2025-03-17T16:23:33Z

libs/types/src/lifecycle/mod.rs

+
+/// Mode of operation
+#[derive(Debug, Copy, Clone, Default, PartialEq, Eq, CandidType, Deserialize, Serialize)]
+pub enum Mode {


Good idea! That's much nicer than the boolean value used in the EVM RPC canister.

lpahlavi · 2025-03-17T16:24:43Z

libs/types/src/lib.rs

 mod rpc_client;

-pub use lifecycle::InstallArgs;
+pub use evm_rpc_types::{


As commented above, I would personally avoid having direct dependencies on EVM RPC if possible. If anything I would maybe move some of the types to a new crate that can be shared between the two repositories.

ninegua · 2025-03-18T07:53:55Z

As discussed offline with @lpahlavi , I will close this one for now and make a new PR after rebasing onto #33

feat: implement a generic request canister interface

9713a60

ninegua force-pushed the paulliu/generic-canister-request branch from b34110b to 9713a60 Compare February 25, 2025 13:11

ninegua added 6 commits February 25, 2025 05:45

Update

11c98d4

Update did file

ddab5db

Minor

091a619

Add mock test

c47079e

Tests for generic request

6bf2cbf

Merge remote-tracking branch 'origin/main' into paulliu/generic-canis…

9f8aa9b

…ter-request

ninegua commented Feb 28, 2025

View reviewed changes

Fix test

38139d9

ninegua requested review from gregorydemay and lpahlavi February 28, 2025 13:39

ninegua marked this pull request as ready for review February 28, 2025 13:42

ninegua requested a review from a team as a code owner February 28, 2025 13:42

gregorydemay reviewed Mar 3, 2025

View reviewed changes

ninegua added 2 commits March 5, 2025 22:10

Move request to client, and change how mock is used

543fa0b

Address reviewer comments

487ceb5

ninegua requested a review from gregorydemay March 6, 2025 08:48

Fix

712cd7c

gregorydemay approved these changes Mar 7, 2025

View reviewed changes

ninegua added 3 commits March 17, 2025 08:30

Merge remote-tracking branch 'origin/main' into paulliu/generic-canis…

7bef7e4

…ter-request

Merge remote-tracking branch 'origin/main' into paulliu/generic-canis…

d6ea54b

…ter-request

Fix

c77e21b

lpahlavi reviewed Mar 17, 2025

View reviewed changes

ninegua closed this Mar 18, 2025

Conversation

ninegua commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninegua Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregorydemay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregorydemay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpahlavi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninegua commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ninegua commented Feb 25, 2025 •

edited

Loading

ninegua Feb 28, 2025 •

edited

Loading