Add support ws RPC API and selectable http and ws api #195

winor30 · 2023-08-15T09:17:36Z

Changes

Introduced logic to allow for selectable HTTP and WebSocket endpoints in the json-rpc server.
Updated CLI arguments to handle --http and --ws flags for enabling HTTP and WebSocket respectively.
Modified JsonRpcServer instantiation to accept http and ws boolean flags.

example

> cargo run --release -- --eth-client-address http://127.0.0.1:8545 --mnemonic-file $HOME/.silius/0xabc.. --beneficiary 0xabc.. --entry-points 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789

...
2023-08-15T09:11:39.502408Z  INFO silius: Starting bundler JSON-RPC server...
2023-08-15T09:11:39.503029Z  INFO silius: Started bundler JSON-RPC server at http://127.0.0.1:8545 with http: true ws: true


> cargo run --release -- --eth-client-address http://127.0.0.1:8545 --mnemonic-file $HOME/.silius/0xabc.. --beneficiary 0xabc.. --entry-points 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789 --http true --ws false

...
2023-08-15T09:11:39.502408Z  INFO silius: Starting bundler JSON-RPC server...
2023-08-15T09:11:39.503029Z  INFO silius: Started bundler JSON-RPC server at http://127.0.0.1:8545 with http: true ws: false


> cargo run --release -- --eth-client-address http://127.0.0.1:8545 --mnemonic-file $HOME/.silius/0xabc.. --beneficiary 0xabc.. --entry-points 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789 --http false --ws

...
2023-08-15T09:11:39.502408Z  INFO silius: Starting bundler JSON-RPC server...
2023-08-15T09:11:39.503029Z  INFO silius: Started bundler JSON-RPC server at http://127.0.0.1:8545 with http: false ws: true

- Update the `cli.rs` file - Add `ArgAction::Set` to the `clap` import - Add `http` and `ws` fields to `RpcServiceOpts` - Add tests for `bundler_opts()` and `rpc_service_opts()`

- Add `tokio` as a dev-dependency with the `workspace` and `full` features to `crates/rpc/Cargo.toml`

- Modify the initialization of `JsonRpcServer` in `silius-rpc.rs` to include HTTP and WebSocket options - Add new fields to `EthApiServerImpl` and `DebugApiServerImpl` in `silius-rpc.rs` - Raise stack size for threads in `silius.rs` and log messages indicating the start of ERC-4337 AA Bundler - Implement various functionality for connecting to gRPC services and starting JSON-RPC server in `silius.rs` - Update `JsonRpcServer` struct in `rpc.rs` to include HTTP and WebSocket options and add new enum and functions for handling protocol types.

- Refactored code by extracting reusable functions and encapsulating related code into classes - Improved performance by optimizing algorithms and reducing time complexity - Added new features to enhance user experience and improve functionality - Fixed bugs and addressed issues reported in previous versions - Conducted thorough testing to ensure the changes are stable and do not introduce regressions

- Added `common.rs` file with implementation of `DummyApi` trait and `DummyApiServerImpl` struct - Added a `build_http_client` function for building `HttpClient` - Added an `build_ws_client` async function for building `WsClient` - Added unit tests for different combinations of HTTP and WS RPC servers in the `rpc.rs` file

- Add `use common::{ ... }` to `rpc.rs` file - Change `DummyApiClient` to `DummyEthApiClient` in all occurrences - Change `DummyApiServerImpl` to `DummyEthApiServerImpl` in all occurrences - Add test cases for `test_only_ws_rpc_server` and `test_http_and_ws_rpc_server` - Increment `PORT` variable using `fetch_add(1, Ordering::SeqCst)` to prevent multiple tests from using the same port - Rename `DummyApi` trait to `DummyEthApi` - Rename `DummyApiServerImpl` struct to `DummyEthApiServerImpl` - Update implementation of `chain_id` method in `DummyEthApiServerImpl`

- Add a test for the `RpcServiceOpts` when no `http` and `ws` flag is provided in `cli.rs` file.

- Add comments to explain the purpose of `http` and `ws` fields in `RpcServiceOpts` struct - Add tests for different combinations of `http` and `ws` flags in `RpcServiceOpts` - Verify that `RpcServiceOpts` can be parsed correctly from command line arguments

zsluedem

Looks good to me

zsluedem · 2023-08-16T17:38:05Z

bin/silius/src/cli.rs

+    ///
+    /// By default, this option is set to true.
+    /// - To enable: `--ws`, `--ws true`, or no `ws` flag.
+    /// - To disable: `--ws false`.


Actually, it is kind of wired for me to disable with --ws false

I think we should follow practices from other clients/projects.

https://geth.ethereum.org/docs/interacting-with-geth/rpc#websockets-server

thank you for your review.
also i fixed it aa0f011 (#195).
We can control true/false by just adding --http and --ws or not.

Vid201

Left two comments, but otherwise LGTM

Thanks for contributing!

Vid201 · 2023-08-16T21:44:49Z

crates/rpc/src/rpc.rs

+    /// Both HTTP and WS.
+    Both,
+    /// Only HTTP.
+    OnlyHttp,


I think only Http is fine.

thank you.
it is fixed 27e5d39 (#195)

Vid201 · 2023-08-16T21:48:21Z

bin/silius/src/cli.rs

+    ///
+    /// By default, this option is set to true.
+    /// - To enable: `--ws`, `--ws true`, or no `ws` flag.
+    /// - To disable: `--ws false`.


I think we should follow practices from other clients/projects.

https://geth.ethereum.org/docs/interacting-with-geth/rpc#websockets-server

- Renamed `JsonRpcProtocolType::OnlyHttp` to `JsonRpcProtocolType::Http` - Renamed `JsonRpcProtocolType::OnlyWs` to `JsonRpcProtocolType::Ws`

- Remove the actions `set` from the `http` and `ws` flags in the `RpcServiceOpts` struct - Change the default value of the `http` flag to false and the default value of the `ws` flag to true in the `RpcServiceOpts` struct - Remove multiple comments explaining flag usage, default values, and enabling/disabling options for the `http` and `ws` flags - Remove the `action = Set` from the `http` and `ws` flags in the `RpcServiceOpts` struct - Modify the comments related to flag usage with `--http` and `--ws` options

- Update test names in cli.rs file to better reflect the functionality being tested

- Added `--http` and `--ws` flags to the `run-silius` command in the Makefile - Added `--http` and `--ws` flags to the `run-silius-rpc` command in the Makefile - Added `--http` and `--ws` flags to the `run-silius-debug` command in the Makefile - Added `--http` and `--ws` flags to the `run-silius-debug-mode` command in the Makefile - Updated the command in the README to include the options `--http` and `--ws`

Vid201

Some last nitpicks

Also, the CI bundler spec tests are failing. Please check and fix, I think the JSON-RPC doesn't start correctly - maybe because (https://github.com/Vid201/silius/blob/main/bundler-spec-test/launcher.sh) this file doesn't contain --http flag?

Vid201 · 2023-08-19T07:46:10Z

bin/silius/src/cli.rs

+    /// By default, this option is set to true.
+    /// - To enable: `--http`.
+    /// - To disable: no `--http` flag.
+    #[clap(long)]


Let's make this default true and ws default false.

Vid201 · 2023-08-19T07:46:34Z

crates/rpc/tests/rpc.rs

+}
+
+#[tokio::test]
+async fn test_http_and_ws_rpc_server() {


remove prefix test_ in all tests

- Change the default value of `http` and `ws` in `bin/silius/src/cli.rs` from true to false.

- Renamed test functions in the `rpc.rs` file for better readability and consistency - Improved the naming of test functions by removing unnecessary words - Update the file to match the changes made in the test function names

Vid201 · 2023-08-19T10:38:53Z

Some last nitpicks

Also, the CI bundler spec tests are failing. Please check and fix, I think the JSON-RPC doesn't start correctly - maybe because (https://github.com/Vid201/silius/blob/main/bundler-spec-test/launcher.sh) this file doesn't contain --http flag?

@winor30 Let's also move info! log (https://github.com/Vid201/silius/blob/main/bin/silius/src/silius-rpc.rs#L40) in bin/silius.rs and bin/silius-rpc.rs inside if clause - only if either --http or --ws are present.

- Added options `--http` and `--ws` to the `silius` command in `launcher.sh` file - Added `--rpc-api` option with the values `eth,debug,web3` to the `silius` command in `launcher.sh` file

winor30 · 2023-08-19T12:05:14Z

@winor30 Let's also move info! log (main/bin/silius/src/silius-rpc.rs#L40) in bin/silius.rs and bin/silius-rpc.rs inside if clause - only if either --http or --ws are present.

@Vid201

I agree with this revised policy!
One thing I'm wondering, is no_rpc necessary?
If http and ws are false, it means no_rpc in effect, so I think it can be replaced.

// now
if !opt.no_rpc {

// my idea
if !(opt.rpc_opts.http || opt.rpc_opts.ws) {
// or
if !opt.rpc_opts.is_enabled() {

Vid201 · 2023-08-19T12:11:55Z

@winor30 Let's also move info! log (main/bin/silius/src/silius-rpc.rs#L40) in bin/silius.rs and bin/silius-rpc.rs inside if clause - only if either --http or --ws are present.

@Vid201

I agree with this revised policy! One thing I'm wondering, is no_rpc necessary? If http and ws are false, it means no_rpc in effect, so I think it can be replaced.
// now
if !opt.no_rpc {

// my idea
if !(opt.rpc_opts.http || opt.rpc_opts.ws) {
// or
if !opt.rpc_opts.is_enabled() {

That's a good point, there is no need for no_rpc . We can remove it

- Added `is_enabled` method to `RpcServiceOpts` struct in `cli.rs` - `is_enabled` method returns `true` if either HTTP or WebSocket RPC is enabled, otherwise `false` - Added tests for the `is_enabled` method to cover various combinations of HTTP and WebSocket flags - `is_enabled` method now returns `true` when only HTTP is enabled - `is_enabled` method now returns `true` when only WebSocket is enabled - `is_enabled` method now returns `true` when both HTTP and WebSocket are enabled - `is_enabled` method now returns `false` when both HTTP and WebSocket are disabled

- Added error handling for when no RPC protocol is enabled in silius-rpc.rs - Initialized the bundler JSON-RPC server in silius-rpc.rs - Added support for the "web3" API in silius-rpc.rs - Added support for the "eth" API in silius-rpc.rs - Added support for the "debug" API in silius-rpc.rs - Started the bundler JSON-RPC server at the specified address with http and ws options in silius-rpc.rs - Removed the unused `no_rpc` flag in silius.rs - Modified silius.rs to only start the bundler JSON-RPC server if RPC options are enabled

winor30 · 2023-08-19T12:51:09Z

That's a good point, there is no need for no_rpc . We can remove it

@Vid201 Thank you
I fixed it
75fb40a (#195)
72d7493 (#195)

Vid201 · 2023-08-19T13:14:08Z

@winor30 Thanks!!

winor30 added 8 commits August 15, 2023 14:45

feat: Update CLI module and add new fields to RPC service options

1838bff

- Update the `cli.rs` file - Add `ArgAction::Set` to the `clap` import - Add `http` and `ws` fields to `RpcServiceOpts` - Add tests for `bundler_opts()` and `rpc_service_opts()`

build: Update Cargo.toml dev-dependencies

55cb361

- Add `tokio` as a dev-dependency with the `workspace` and `full` features to `crates/rpc/Cargo.toml`

test: Add test for RpcServiceOpts without http and ws flags

0748001

- Add a test for the `RpcServiceOpts` when no `http` and `ws` flag is provided in `cli.rs` file.

zsluedem approved these changes Aug 16, 2023

View reviewed changes

Vid201 requested changes Aug 16, 2023

View reviewed changes

Vid201 linked an issue Aug 18, 2023 that may be closed by this pull request

Add support for ws RPC API #155

Closed

winor30 added 4 commits August 19, 2023 11:15

refactor: Rename JsonRpcProtocolType variants

27e5d39

- Renamed `JsonRpcProtocolType::OnlyHttp` to `JsonRpcProtocolType::Http` - Renamed `JsonRpcProtocolType::OnlyWs` to `JsonRpcProtocolType::Ws`

refactor: Update test names for rpc_service_opts.

c631c13

- Update test names in cli.rs file to better reflect the functionality being tested

winor30 requested a review from Vid201 August 19, 2023 02:47

Vid201 requested changes Aug 19, 2023

View reviewed changes

winor30 added 2 commits August 19, 2023 17:34

refactor: Update default values for HTTP and WebSocket in CLI

82d593e

- Change the default value of `http` and `ws` in `bin/silius/src/cli.rs` from true to false.

refactor: Refactor RPC test function names

cd373cb

- Renamed test functions in the `rpc.rs` file for better readability and consistency - Improved the naming of test functions by removing unnecessary words - Update the file to match the changes made in the test function names

feat: Update bundler-spec-test launcher with additional options

ae90235

- Added options `--http` and `--ws` to the `silius` command in `launcher.sh` file - Added `--rpc-api` option with the values `eth,debug,web3` to the `silius` command in `launcher.sh` file

winor30 added 2 commits August 19, 2023 21:39

Vid201 approved these changes Aug 19, 2023

View reviewed changes

Vid201 merged commit 184212b into silius-rs:main Aug 19, 2023
2 checks passed

winor30 deleted the feat/add-ws-rpc-option branch August 19, 2023 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support ws RPC API and selectable http and ws api #195

Add support ws RPC API and selectable http and ws api #195

winor30 commented Aug 15, 2023 •

edited

Loading

zsluedem left a comment

zsluedem Aug 16, 2023

Vid201 Aug 16, 2023

winor30 Aug 19, 2023 •

edited

Loading

Vid201 left a comment

Vid201 Aug 16, 2023

winor30 Aug 19, 2023

Vid201 Aug 16, 2023

Vid201 left a comment

Vid201 Aug 19, 2023

Vid201 Aug 19, 2023

Vid201 commented Aug 19, 2023

winor30 commented Aug 19, 2023 •

edited

Loading

Vid201 commented Aug 19, 2023

winor30 commented Aug 19, 2023

Vid201 commented Aug 19, 2023

Add support ws RPC API and selectable http and ws api #195

Add support ws RPC API and selectable http and ws api #195

Conversation

winor30 commented Aug 15, 2023 • edited Loading

Changes

Related

example

zsluedem left a comment

Choose a reason for hiding this comment

zsluedem Aug 16, 2023

Choose a reason for hiding this comment

Vid201 Aug 16, 2023

Choose a reason for hiding this comment

winor30 Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Vid201 left a comment

Choose a reason for hiding this comment

Vid201 Aug 16, 2023

Choose a reason for hiding this comment

winor30 Aug 19, 2023

Choose a reason for hiding this comment

Vid201 Aug 16, 2023

Choose a reason for hiding this comment

Vid201 left a comment

Choose a reason for hiding this comment

Vid201 Aug 19, 2023

Choose a reason for hiding this comment

Vid201 Aug 19, 2023

Choose a reason for hiding this comment

Vid201 commented Aug 19, 2023

winor30 commented Aug 19, 2023 • edited Loading

Vid201 commented Aug 19, 2023

winor30 commented Aug 19, 2023

Vid201 commented Aug 19, 2023

winor30 commented Aug 15, 2023 •

edited

Loading

winor30 Aug 19, 2023 •

edited

Loading

winor30 commented Aug 19, 2023 •

edited

Loading