Improve api compare ergonomics #4018

elmattic · 2024-03-01T12:35:23Z

Summary of changes

Changes introduced in this pull request:

Add debug trace with the request's response
Derive communication protocols from ApiInfos and check that:
- Forest and Lotus protocol should match
- Protocol should either be /http or /ws
Remove forest-tool api compare --ws flag

Usage examples:

$ RUST_LOG=forest_filecoin::rpc_client=debug ./forest-tool api compare forest_snapshot_calibnet_2024-03-18_height_1448006.forest.car.zst --filter EthGasPrice 
2024-03-18T18:03:19.494692Z DEBUG forest_filecoin::rpc_client: Using JSON-RPC v2 HTTP URL: http://127.0.0.1:2345/rpc/v1
2024-03-18T18:03:21.694154Z DEBUG forest_filecoin::rpc_client: Response: Ok(String("0x0"))
2024-03-18T18:03:21.694480Z DEBUG forest_filecoin::rpc_client: Using JSON-RPC v2 HTTP URL: http://127.0.0.1:1234/rpc/v1
2024-03-18T18:03:21.697877Z DEBUG forest_filecoin::rpc_client: Response: Ok(String("0x4e99f2f5"))
| RPC Method           | Forest | Lotus |
|----------------------|--------|-------|
| Filecoin.EthGasPrice | Valid  | Valid |

$ RUST_LOG=forest_filecoin::rpc_client=debug ./forest-tool api compare --lotus /ip4/127.0.0.1/tcp/1234/ws --forest /ip4/127.0.0.1/tcp/2345/http forest_snapshot_calibnet_2024-03-18_height_1448006.forest.car.zst --filter EthGasPrice
Error: communication protocols mismatch: Some("http") (Forest) is different from Some("ws") (Lotus)

$ RUST_LOG=forest_filecoin::rpc_client=debug ./forest-tool api compare --lotus /ip4/127.0.0.1/tcp/1234/ws --forest /ip4/127.0.0.1/tcp/2345/ws forest_snapshot_calibnet_2024-03-18_height_1448006.forest.car.zst --filter EthGasPrice
2024-03-18T18:15:42.306023Z DEBUG forest_filecoin::rpc_client: Using JSON-RPC v2 WS URL: ws://127.0.0.1:2345/rpc/v1
2024-03-18T18:15:43.929513Z DEBUG forest_filecoin::rpc_client: Response: Ok(String("0x0"))
2024-03-18T18:15:43.929617Z DEBUG forest_filecoin::rpc_client: Using JSON-RPC v2 WS URL: ws://127.0.0.1:1234/rpc/v1
2024-03-18T18:15:43.933110Z DEBUG forest_filecoin::rpc_client: Response: Ok(String("0x30c04"))

Reference issue to close (if applicable)

Closes

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

src/rpc_client/mod.rs

src/tool/subcommands/api_cmd.rs

src/rpc_client/mod.rs

src/tool/subcommands/api_cmd.rs

src/rpc_client/mod.rs

LesnyRumcajs · 2024-03-19T15:59:08Z

src/tool/subcommands/api_cmd.rs

+    fn test_derive_protocol() {
+        let forest = ApiInfo::from_str("/ip4/127.0.0.1/tcp/2345/http").expect("infallible");
+        let lotus = ApiInfo::from_str("/ip4/127.0.0.1/tcp/1234/http").expect("infallible");
+        assert!(matches!(
+            derive_protocol(&forest, &lotus),
+            Ok(CommunicationProtocol::Http)
+        ));
+
+        let forest = ApiInfo::from_str("/ip4/127.0.0.1/tcp/2345/ws").expect("infallible");
+        let lotus = ApiInfo::from_str("/ip4/127.0.0.1/tcp/1234/ws").expect("infallible");
+        assert!(matches!(
+            derive_protocol(&forest, &lotus),
+            Ok(CommunicationProtocol::Ws)
+        ));
+
+        let forest = ApiInfo::from_str("/ip4/127.0.0.1/tcp/2345/http").expect("infallible");
+        let lotus = ApiInfo::from_str("/ip4/127.0.0.1/tcp/1234/ws").expect("infallible");
+        assert!(derive_protocol(&forest, &lotus).is_err());
+
+        let forest = ApiInfo::from_str("/ip4/127.0.0.1/tcp/2345/wss").expect("infallible");
+        let lotus = ApiInfo::from_str("/ip4/127.0.0.1/tcp/1234/wss").expect("infallible");
+        assert!(derive_protocol(&forest, &lotus).is_err());


nit: I'd unwrap on those, infallible doesn't bring any more context, and these are tests anyway

Improve api compare ergonomics

94240c1

hanabi1224 reviewed Mar 1, 2024

View reviewed changes

src/rpc_client/mod.rs Outdated Show resolved Hide resolved

elmattic added 2 commits March 1, 2024 13:55

Use enum instead

8a4063a

Check that we don't specify http in case --ws is used

3fd787f

elmattic mentioned this pull request Mar 1, 2024

fix(deps): remove indirect tokio-tungstenite dependencies #4014

Closed

4 tasks

elmattic marked this pull request as ready for review March 1, 2024 13:23

elmattic requested a review from a team as a code owner March 1, 2024 13:23

elmattic requested review from hanabi1224 and LesnyRumcajs and removed request for a team March 1, 2024 13:23

hanabi1224 reviewed Mar 1, 2024

View reviewed changes

src/rpc_client/mod.rs Outdated Show resolved Hide resolved

hanabi1224 approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into elmattic/api-compare-checks

eeb108c

LesnyRumcajs reviewed Mar 4, 2024

View reviewed changes

elmattic added 12 commits March 18, 2024 10:42

Merge branch 'main' into elmattic/api-compare-checks

a1128da

Add back trace

b14f02f

Rename enum to CommunicationProtocol

efb734d

Add better error message

87eb6cd

Derive CommunicationProtocol from ApiInfo

c5d159d

Merge branch 'main' into elmattic/api-compare-checks

fb89fb8

Remove usage of websocket flag

00d96c3

Add back http in default values

152f4e5

Remove --ws flag

dab1395

Merge branch 'main' into elmattic/api-compare-checks

5f221d8

Add back debug trace in WS mode

343dd62

Updage CHANGELOG file

94be09d

elmattic requested a review from LesnyRumcajs March 19, 2024 08:20

LesnyRumcajs reviewed Mar 19, 2024

View reviewed changes

src/rpc_client/mod.rs Outdated Show resolved Hide resolved

src/rpc_client/mod.rs Outdated Show resolved Hide resolved

elmattic added 2 commits March 19, 2024 11:25

Merge branch 'main' into elmattic/api-compare-checks

7dc9983

Remove the matches

4affe0c

elmattic added 3 commits March 19, 2024 15:28

Merge branch 'main' into elmattic/api-compare-checks

a89e206

Remove useless const bindings

22caf55

Merge branch 'main' into elmattic/api-compare-checks

b74c3bc

LesnyRumcajs approved these changes Mar 19, 2024

View reviewed changes

elmattic added this pull request to the merge queue Mar 19, 2024

Merged via the queue into main with commit d44ae18 Mar 19, 2024
28 checks passed

elmattic deleted the elmattic/api-compare-checks branch March 19, 2024 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve api compare ergonomics #4018

Improve api compare ergonomics #4018

elmattic commented Mar 1, 2024 •

edited

LesnyRumcajs Mar 19, 2024

Improve api compare ergonomics #4018

Improve api compare ergonomics #4018

Conversation

elmattic commented Mar 1, 2024 • edited

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

LesnyRumcajs Mar 19, 2024

Choose a reason for hiding this comment

elmattic commented Mar 1, 2024 •

edited