Add core API server in Rust #91

LukasGasior1 · 2022-08-18T19:00:30Z

Initial work towards core API server in Rust.

dhedey

Really nice work

Have a few misc comments - a few neatenings here or there, and slight questions about our need to us dyn, but basically this is almost ready to merge I think

dhedey · 2022-08-23T18:56:52Z

core/src/main/java/com/radixdlt/api/system/SystemApiModule.java

@@ -96,6 +96,9 @@ protected void configure() {
    binder.addBinding(HandlerRoute.get("/system/version")).to(VersionHandler.class);
    binder.addBinding(HandlerRoute.get("/system/peers")).to(PeersHandler.class);
    binder.addBinding(HandlerRoute.get("/system/addressbook")).to(AddressBookHandler.class);
+    binder
+        .addBinding(HandlerRoute.get("/system/network-sync-status"))


Moved from Core API? Seems sensible

dhedey · 2022-08-23T18:58:41Z

core/src/main/java/com/radixdlt/api/system/generated/models/NetworkSyncStatusResponse.java

@@ -73,37 +66,6 @@ public void setSyncStatus(SyncStatus syncStatus) {
  }


-  public NetworkSyncStatusResponse peers(List<Peer> peers) {


I guess peers is returned through some other call?

It's already in /system/peers

dhedey · 2022-08-23T19:00:04Z

core/src/main/java/com/radixdlt/api/system/prometheus/PrometheusService.java

@@ -141,8 +140,6 @@ public PrometheusService(
      InMemorySystemInfo inMemorySystemInfo,
      @Self BFTNode self,
      Addressing addressing) {
-    boolean enableTransactions = properties.get("api.transactions.enable", false);


DevOps currently have this on the dashboard - I imagine we'll still have something like this in the new Core API?

Do we need to remove this?

I guess it's something to discuss. Does it still make sense to put this API behind a flag in babylon?

dhedey · 2022-08-23T19:03:33Z

core/src/test/java/com/radixdlt/api/CoreApiServerTest.java

-    assertThat(bech32.getNodeHrp()).isEqualTo(addressing.forNodes().getHrp());
-    assertThat(bech32.getValidatorHrp()).isEqualTo(addressing.forValidators().getHrp());
-    assertThat(bech32.getResourceHrpSuffix()).isEqualTo(addressing.forResources().getHrp());
+  private static int mkNetworkConfigurationRequest(int port) throws Exception {


What does mk mean? Is it short for make?

yeah, not a particularly char-saving abbreviation :D but quite commonly used across Linux

Would you be up for making it make to avoid any potential confusion? :D

dhedey · 2022-08-23T19:05:59Z

core/src/test/java/com/radixdlt/api/CoreApiServerTest.java

+      // Act & Assert #1: start the server and verify it's up
+      server.start();
+      final var statusCode = mkNetworkConfigurationRequest(port);
+      assertEquals(200, statusCode);


If the assert fails, will our server be shut down properly, or do we leak unmanaged memory, and possibly threads?

Do you think it might be worth creating some easy-to-use with try-with-resources Core Api runner?

TBH I'm not a big fan of using auto-closeable for long living objects. Seems a bit off to me. I've added a regular try-finally block, if that's okay?

dhedey · 2022-08-23T19:22:43Z

state-manager/core-api-server/Cargo.toml

+
+jni = "0.19.0"
+
+# Deps for generated models and the Hyper server


So many dependencies and transient dependencies :( - are there any options on the generator to remove some of these?

unfortunately there aren't any config options for the generator; I've removed mime and percent-encoding deps which weren't used (all others are)

dhedey · 2022-08-23T19:32:00Z

state-manager/core-rust/src/lib.rs

@@ -0,0 +1,16 @@
+/// A workaround for including the symbols defined in state_manager / core_api_server
+/// in the output library file. See: https://github.com/rust-lang/rfcs/issues/2771
+/// I truly have no idea why this works, but it does.


🤣 love it

dhedey · 2022-08-23T19:36:56Z

state-manager/state-manager/src/jni/state_computer/mod.rs

    request_payload: jbyteArray,
 ) -> StateManagerResult<bool> {
-    let state_manager = JNIStateManager::get_state_manager(env, sm_instance);
+    let state_manager = JNIStateManager::get_state_manager(env, j_state_manager);


Side note - Ohh looks like I somehow missed changing these to use the new-fangled method for native functions 🤷

dhedey · 2022-08-23T19:37:37Z

state-manager/state-manager/src/jni/state_manager.rs

-        let jni_state_manager = JNIStateManager { state_manager };
+        let jni_state_manager = JNIStateManager {
+            state_manager,
+            // tokio_runtime,


I think this can be removed now :D

state-manager/state-manager/src/jni/state_manager.rs

dhedey · 2022-08-23T19:52:30Z

state-manager/core-api-server/src/core_api/server.rs

+        &self,
+        _context: &C,
+    ) -> Result<StatusNetworkConfigurationPostResponse, ApiError> {
+        let network = &self


Heads up - this will need changing after my network merge

talekhinezh · 2022-08-24T01:57:52Z

state-manager/state-manager/src/jni/state_manager.rs

 pub struct JNIStateManager {
-    pub state_manager: ActualStateManager,
+    pub state_manager: Arc<Mutex<dyn StateManager + 'static + Send + Sync>>,


Can we stick to the jni rw lock for now? Feels too early to start implementing multithreaded locks around state manager?

Main reason is that the server gets it's own copy of the StateManager arc and needs to be able to lock on it outside of the jni context.

talekhinezh · 2022-08-24T01:59:36Z

state-manager/state-manager/src/state_manager.rs

+    }
+}
+
+pub struct StateManagerImpl<M: Mempool, S> {


StateManagerImpl vs StateManager trait feels like a broken use of dyn and traits. Let's go over this sometime tomorrow.

sonarqubecloud · 2022-08-25T16:32:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

52.0% Coverage
0.0% Duplication

dhedey

Nice! I'd be happy to get this merged. I think Josh's objections have been dealt with, but if not, we can fix in a follow up PR? :)

dhedey · 2022-08-25T22:05:27Z

common/src/main/java/com/radixdlt/networks/Network.java

@@ -98,7 +99,8 @@ public enum Network {
  LOCALSIMULATOR(242 /* 0xF1 */, "simulator", "sim", GenesisSource.fromConfiguration);

  // For the Radix Shell to provide a default
-  public static final String DefaultHexGenesisTransaction = "01";
+  public static final String DefaultHexGenesisTransaction =


I'm a bit confused - why does this say Transaction if it's generating a public key?

I've changed the genesis from an integer (num of validators), to a list of validators pub keys a while back

dhedey · 2022-08-25T22:07:49Z

state-manager-bridge/src/main/java/com/radixdlt/api/CoreApiServer.java

-              .type(NetworkNotSupportedError.class.getSimpleName()));
-    }
+  /**
+   * Stores a pointer to the rust state manager across JNI calls. In the JNI model, this is


I think this comment still needs updating - it says "rust state manager" but the pointer says "Core API"

dhedey · 2022-08-25T22:10:31Z

state-manager/Cargo.lock

+]
+
+[[package]]
+name = "android_system_properties"


:'( yay open source. I just don't know how we somehow end up pulling in an android_system_properties library via transitive dependencies...

chrono (provides DateTime) -> iana-time-zone -> android_system_properties 🤷‍♂️

dhedey · 2022-08-25T22:21:14Z

state-manager/core-api-server/src/core_api/server.rs

+                        api_version: API_VERSION.to_string(),
+                    },
+                    network_identifier: NetworkIdentifier {
+                        network: format!("{:?}", network),


I think you want network.logical_name here

👍 will do in the follow up PR (#95)

dhedey · 2022-08-25T22:22:40Z

state-manager/core-api-server/src/core_api/server.rs

+                            .to_string(),
+                        validator_hrp: "TODO".to_string(),
+                        node_hrp: "TODO".to_string(),
+                        resource_hrp_suffix: hrp_set


This is all a bit outdated hmm. We should probably change this to account, resource, system, component. Or maybe just return the network_hrp_suffix.

👍 will do in the follow up PR (#95)

LukasGasior1 marked this pull request as draft August 18, 2022 19:04

Add core API server in Rust

c899cd3

LukasGasior1 force-pushed the feature/openapi-rust branch from 6756040 to c899cd3 Compare August 18, 2022 19:39

LukasGasior1 added 3 commits August 19, 2022 12:24

Remove frunk dependency

29d69fe

Remove core api from java

1e77bdc

Separate the core api server from state manager

125bc46

LukasGasior1 marked this pull request as ready for review August 23, 2022 11:20

LukasGasior1 added 4 commits August 23, 2022 13:48

Merge remote-tracking branch 'origin/main' into feature/openapi-rust

e9068d0

Bump openapi generator version

dd05081

Remove unused NetworkConfigurationRequest (with a hacky openapi fix)

077f2f5

Update lib name in build scripts

4e69ce8

dhedey approved these changes Aug 23, 2022

View reviewed changes

dhedey reviewed Aug 23, 2022

View reviewed changes

talekhinezh requested changes Aug 24, 2022

View reviewed changes

LukasGasior1 added 6 commits August 24, 2022 13:09

Remove comments and unused deps

2285712

Update comment; add try-finally in the CoreApiServerTest

c7051d2

Back to generics

e291af8

Use ActualStateManager in server

944d4c7

Bring back network configuration API

8446b01

Fix docker?

007a72d

LukasGasior1 force-pushed the feature/openapi-rust branch from 998934b to 007a72d Compare August 25, 2022 11:46

LukasGasior1 added 3 commits August 25, 2022 14:37

Fix node capabilities tests

82b8dae

Merge remote-tracking branch 'origin/main' into feature/openapi-rust

c653f2c

mk -> make; remove unused deps in cli-tools

5e3ac8a

dhedey approved these changes Aug 25, 2022

View reviewed changes

talekhinezh approved these changes Aug 26, 2022

View reviewed changes

Update comment

fa5bacf

LukasGasior1 merged commit a2291ea into main Aug 26, 2022

LukasGasior1 deleted the feature/openapi-rust branch August 26, 2022 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add core API server in Rust #91

Add core API server in Rust #91

LukasGasior1 commented Aug 18, 2022

dhedey left a comment •

edited

Loading

dhedey Aug 23, 2022

LukasGasior1 Aug 23, 2022

dhedey Aug 23, 2022

LukasGasior1 Aug 23, 2022

dhedey Aug 23, 2022

LukasGasior1 Aug 24, 2022

dhedey Aug 23, 2022

LukasGasior1 Aug 23, 2022

dhedey Aug 24, 2022

dhedey Aug 23, 2022

LukasGasior1 Aug 24, 2022

dhedey Aug 23, 2022

LukasGasior1 Aug 24, 2022

dhedey Aug 23, 2022

dhedey Aug 23, 2022

dhedey Aug 23, 2022

dhedey Aug 23, 2022

talekhinezh Aug 24, 2022

LukasGasior1 Aug 24, 2022

talekhinezh Aug 24, 2022 •

edited

Loading

sonarqubecloud bot commented Aug 25, 2022

dhedey left a comment

dhedey Aug 25, 2022

LukasGasior1 Aug 26, 2022

dhedey Aug 25, 2022

dhedey Aug 25, 2022

LukasGasior1 Aug 26, 2022

dhedey Aug 25, 2022

LukasGasior1 Aug 26, 2022

dhedey Aug 25, 2022

LukasGasior1 Aug 26, 2022

		@@ -73,37 +66,6 @@ public void setSyncStatus(SyncStatus syncStatus) {
		}


		public NetworkSyncStatusResponse peers(List<Peer> peers) {


		jni = "0.19.0"

		# Deps for generated models and the Hyper server

Add core API server in Rust #91

Add core API server in Rust #91

Conversation

LukasGasior1 commented Aug 18, 2022

dhedey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talekhinezh Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 25, 2022

dhedey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey left a comment •

edited

Loading

talekhinezh Aug 24, 2022 •

edited

Loading