WIP: Status backend intergation #21517

ulisesmac · 2024-10-28T19:27:56Z

This PR is to show the current progress on the status-backend integration.

Setup

If you are willing to run this PR, these are the steps needed:

Run status-backend at
Run an Android emulator or physical device and reverse the :
adb reverse tcp:<PORT> tcp:<PORT>
This is needed to communicate the app with the back-end running on your computer
In Status mobile, go to the ns status-backend.config and define data-dir-path with your own.
Run the app as always.

If the app is stuck in a black screen, it means we have no connection with status-backend. Sometimes it crashes while creating a profile, sometimes after those steps. This still is a WIP:

Screencast.from.2024-10-28.13-04-38.mp4

Implementation details

Please refer to the ns status-backend.core for the main implementation and some REPL commands to play with.

Strategy to follow

In order to completely integrate status-backend, we need to port some native modules, see the discussion here.

So this PR is presenting an initial progress that consist of:

✅ Web socket based signals are 100% working
✅ RPC calls are 100% working
❗ A general mechanism to invoke status back-end methods async. This step needs to manually port some native modules since we expect a sync behavior, they will be addressed in following PRs

src/utils/network/core.cljs

ulisesmac · 2024-10-28T19:29:30Z

src/utils/network/core.cljs

+(defn fetch [url {:keys [body] :as params} callback]
+  (let [js-params (cond-> params
+                    (map? body) (update :body (comp js/JSON.stringify clj->js))
+                    :always     clj->js)]
+    (-> (js/fetch url js-params)
+        (.then (fn [response]
+                 (.text response)))
+        (.then callback)
+        (.catch #(log/error (str "Error while fetching: " url) %)))))


Added a general fetch function that is wrapping js/fetch to use it in a Clojure friendly way

I always find confusing to mix promises with callbacks. The fetch function is returning a promise, so we don't need a callback. I've only seen this style of code mixing promises with callbacks in status-mobile.

But I imagine you did it like this to avoid further changes in the repo? Which would be caused if you used an implementation like the one below.

(defn fetch [url {:keys [body] :as params}] (let [js-params (cond-> params (map? body) (update :body (comp js/JSON.stringify clj->js)) :always clj->js)] (-> (js/fetch url js-params) (.then (fn [response] (.text response))) (.catch #(log/error (str "Error while fetching: " url) %)))))

I think it's best to keep changes at a minimum, but I wanted to point out @ulisesmac. Sorry if you already know all this.

Thanks for paying attention to this @ilmotta

I even considered to return nil for this function, so it could be considered just as a function with side effects.

I know it's confusing, but I wanted to pass a callback as a way to easily deal with fetch, not having to worry about dealing with a promise, possibbly import promesa.core and propagate the promse (e.g. status-backend.core/fetch would also return a promise).

IMO we don't need all the flexibility/power of a promise for js/fetch, we just care about having a callback (or two: on-success and on-failure) to receive the result.

I'm open to listening to opinions on why this approach is worse. I wanted to keep things simple and familiar.

src/utils/re_frame.cljs

src/status_im/core.cljs

status-im-auto · 2024-10-28T19:31:13Z

Jenkins Builds

Click to see older builds (6)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	`29678bc`	#1	2024-10-28 19:31:12	~2 min	`tests`	📄`log`

❌	`e9b8118`	#2	2024-10-28 19:37:57	~2 min	`tests`	📄`log`
✔️	`e9b8118`	#2	2024-10-28 19:42:10	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`e9b8118`	#2	2024-10-28 19:43:46	~8 min	`android`	🤖`apk` 📲
✔️	`e9b8118`	#2	2024-10-28 19:45:27	~10 min	`ios`	📱`ipa` 📲

❌	`7c09b94`	#3	2024-10-30 03:20:36	~5 min	`tests`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`3fda24b`	#4	2024-10-30 03:28:17	~5 min	`tests`	📄`log`
✔️	`3fda24b`	#4	2024-10-30 03:30:17	~7 min	`android`	🤖`apk` 📲
✔️	`3fda24b`	#4	2024-10-30 03:32:44	~9 min	`ios`	📱`ipa` 📲
✔️	`3fda24b`	#4	2024-10-30 03:35:37	~12 min	`android-e2e`	🤖`apk` 📲

✔️	`276eea2`	#5	2024-11-01 14:55:21	~4 min	`tests`	📄`log`
✔️	`276eea2`	#5	2024-11-01 14:58:39	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`276eea2`	#5	2024-11-01 14:59:25	~8 min	`android`	🤖`apk` 📲
✔️	`276eea2`	#5	2024-11-01 15:00:53	~10 min	`ios`	📱`ipa` 📲

ulisesmac · 2024-10-28T19:33:11Z

src/status_backend/core.cljs

+(defn call-private-rpc [payload callback]
+  ;; NOTE: according to the status-backend readme, we call RPCs using the `CallRPC`
+  ;; endpoint, instead of using `CallPrivateRPC` as in mobile.
+  (fetch "CallRPC" payload callback))


According to status backend readme, we should invoke CallRPC for the services, but in mobile we use the effect :json-rpc/call which wraps CallPrivateRPC.

Why do we changed this? what is the difference between CallRPC and CallPrivateRPC? 🤔

cc: @ilmotta @qfrank

The current implementations in status-go are identical:

https://github.com/status-im/status-go/blob/132ea05fc8735b91922a440009efdbc7f1b8b8ce/mobile/status.go#L249-L273

And https://github.com/status-im/status-go/blob/132ea05fc8735b91922a440009efdbc7f1b8b8ce/api/geth_backend.go#L2233-L2249

@igor-sirotin and @qfrank can probably help clarify this point.

There's no difference after this PR , we can just use CallRPC, if you want to know details about the private things, maybe @Samyoul knows something

src/status_backend/core.cljs

ulisesmac · 2024-10-28T19:36:29Z

src/status_backend/config.cljs

@@ -0,0 +1,24 @@
+(ns status-backend.config)


This file should be simplified with our nix tooling. But currently you need to add your own paths

These settings could be configured using :clojure-defines in shadow-cljs.edn, with a sensible default being injected by a make target. Similar to how we run CLJS tests with the line :ns-regexp #shadow/env "SHADOW_NS_REGEXP".

The things that seem to require customization are the address, port, and data-dir-path, the rest is all derived. Or is there another setting you think we should parameterize?

Nope, this is everything. I'll use :clojure-defines. Thank you!

ulisesmac · 2024-10-28T19:37:13Z

src/native_module/core.cljs

+  (if false
+    (when (exists? (.-NativeModules react-native))
+      (.-AccountManager ^js (.-NativeModules react-native)))
+    status-backend/fetch-js-obj))


along this file I use if false as a temporary pattern to ignore the original module implementation and use the back-end fetch logic

ilmotta

Good progress @ulisesmac! I did check the branch locally and the app crashes at the same spot you described in the video.

Maybe another CC, such as @qfrank can help you spot the cause of the crash?

ilmotta · 2024-10-29T00:35:47Z

src/status_backend/config.cljs

@@ -0,0 +1,24 @@
+(ns status-backend.config)


These settings could be configured using :clojure-defines in shadow-cljs.edn, with a sensible default being injected by a make target. Similar to how we run CLJS tests with the line :ns-regexp #shadow/env "SHADOW_NS_REGEXP".

The things that seem to require customization are the address, port, and data-dir-path, the rest is all derived. Or is there another setting you think we should parameterize?

ilmotta · 2024-10-29T00:38:58Z

src/native_module/core.cljs


 (defn fleets
  []
  (.fleets ^js (status)))

 (defn keystore-dir
  []
-  (.keystoreDir ^js (utils)))
+  (if false


These checks could be replaced by something like when ^boolean js/goog.DEBUG to allow dead code elimination, but with a different definition from goog-define and shadow-cljs, as we do in status-im config.

Yeah, I was thinking that during the implementation, we don't want this code in release builds. I'll implement it, thank you

ilmotta · 2024-10-29T00:44:55Z

src/native_module/core.cljs


 (defn log-file-directory
  []
-  (.logFileDirectory ^js (log-manager)))
+  (if false


Instead of conditionals being repeated multiple times, we could have two implementations of a protocol, in two different namespaces to avoid any risk of mixing up things. It would be a bit better to have the status-backend as decoupled from production code as possible.

Edit: let's forget about my suggestion 😅 because the problem is that the native-module.core namespace is accessed directly by the rest of the code in the repo, which means we would need to change all places to access the correct instance implementing the protocol, and not the function vars directly. But this would require a ton of effort, not worth it, quite risky.

Maybe we could create to different implementations / protocols that are conditionally required from this namespace? For example,native-module.core would conditionally require native-module.mobile when in production (or when we want to test the bindings), and another namespace like native-module.backend could be required during development. Thoughts?

@seanstrom I remember now you found a way to conditionally require namespaces. That could work well 👍🏼 Just feels hacky, but might be the best.

Usually, in "good" backend Clojure repos, there's usually a controlled way to initialize system dependencies, and while supporting RDD. In frontend repos with CLJS, it's way less common practice. System initialization being different from dependency injection, but some Clojure libs help solve both problems.

@seanstrom

What you say sounds like creating a reader conditional to perform the expected require 🤔

@ilmotta
I don't like I had to use an if here, as I've been saying, I tried to impact as less as possible. I think this should work not so bad.

It's kind of bad IMO @ulisesmac that we have to sprinkle conditionals in all functions. This mix of dev-only and prod code repeated all the time is a bit error prone. Ideally we would use something like an instance of a record implementing a protocol and completely separate namespaces. Might be doable in the future as an interesting refactoring exercise.

@seanstrom's idea is based on this https://shadow-cljs.github.io/docs/UsersGuide.html#_conditional_reading, which is like a special feature only supported by shadow-cljs.

I know I started this discussion, but I think using the ^boolean FLAG checks in each function is fine for now 👍🏼 The simplest that works

src/status_backend/core.cljs

ilmotta · 2024-10-29T00:53:16Z

src/status_backend/core.cljs

+(defn call-private-rpc [payload callback]
+  ;; NOTE: according to the status-backend readme, we call RPCs using the `CallRPC`
+  ;; endpoint, instead of using `CallPrivateRPC` as in mobile.
+  (fetch "CallRPC" payload callback))


The current implementations in status-go are identical:

https://github.com/status-im/status-go/blob/132ea05fc8735b91922a440009efdbc7f1b8b8ce/mobile/status.go#L249-L273

And https://github.com/status-im/status-go/blob/132ea05fc8735b91922a440009efdbc7f1b8b8ce/api/geth_backend.go#L2233-L2249

@igor-sirotin and @qfrank can probably help clarify this point.

ilmotta · 2024-10-29T01:04:04Z

src/utils/network/core.cljs

+(defn fetch [url {:keys [body] :as params} callback]
+  (let [js-params (cond-> params
+                    (map? body) (update :body (comp js/JSON.stringify clj->js))
+                    :always     clj->js)]
+    (-> (js/fetch url js-params)
+        (.then (fn [response]
+                 (.text response)))
+        (.then callback)
+        (.catch #(log/error (str "Error while fetching: " url) %)))))


I always find confusing to mix promises with callbacks. The fetch function is returning a promise, so we don't need a callback. I've only seen this style of code mixing promises with callbacks in status-mobile.

But I imagine you did it like this to avoid further changes in the repo? Which would be caused if you used an implementation like the one below.

(defn fetch [url {:keys [body] :as params}] (let [js-params (cond-> params (map? body) (update :body (comp js/JSON.stringify clj->js)) :always clj->js)] (-> (js/fetch url js-params) (.then (fn [response] (.text response))) (.catch #(log/error (str "Error while fetching: " url) %)))))

I think it's best to keep changes at a minimum, but I wanted to point out @ulisesmac. Sorry if you already know all this.

qfrank

I saw there're many other methods like create-account-and-login / init-status-go-logging are not using status-backend server yet, but you will update it later right? Because this is just a solution demo draft?

qfrank · 2024-10-29T02:40:16Z

src/status_backend/core.cljs

+(defn call-private-rpc [payload callback]
+  ;; NOTE: according to the status-backend readme, we call RPCs using the `CallRPC`
+  ;; endpoint, instead of using `CallPrivateRPC` as in mobile.
+  (fetch "CallRPC" payload callback))


There's no difference after this PR , we can just use CallRPC, if you want to know details about the private things, maybe @Samyoul knows something

qfrank · 2024-10-29T03:17:01Z

src/status_backend/core.cljs

+  "
+  (js/Proxy. #js{}
+             #js{:get (fn [_target native-module-method-name]
+                        (let [endpoint (method-name->endpoint native-module-method-name)]


this won't work ... We cannot simply determine the endpoint name based on the native-module-method-name because there are many unsupported endpoints in the status-backend server. Therefore, I created this PR 5943 to solve this problem. For some unsupported endpoints, we need to use the corresponding V2 versions.
I also created another PR(not related to status backend integration) to use the V2 endpoint so that we can remove the old endpoint in the future

Based on Igor's this reply, I still need to supplement some missing V2 endpoints in PR#5943, such as AppStateChange. I plan to complete the supplementation today.

Based on Igor's this reply, I still need to supplement some missing V2 endpoints in PR#5943, such as AppStateChange. I plan to complete the supplementation today.

done

Hello @qfrank
Thank you for your review!

this won't work ... We cannot simply determine the endpoint name based on the native-module-method-name because there are many unsupported endpoints in the status-backend server

I know there are unsupported endpoints, I'm not expecting this draft PR fully works right now, but this could be a pattern to follow.

Regarding to:

For some unsupported endpoints, we need to use the corresponding V2 versions.
I also created another #21450 related to status backend integration) to use the V2 endpoint so that we can remove the old endpoint in the future

I don't fully agree with the approach taken. If we cannot infer the endpoint name based on the native module method name it implies:

We need to manually maintain a map (native-method -> endpoint) which will be error prone and will require an extra step from everyone while integrating a new native method. Since status-backend won't be used in production, we should reduce the divergence to the minimum.

Lack of standarization/consistency: Why do we want to give different names to functions that should only be wrappers?

As said in 2, IMO, we should keep native modules related to status-go as thin as possible, so I don't agree with the approach taken in:

chore_: use status.go v2 endpoint #21450

Since it exposes a native method signature that is different from the V2 endpoint called inside. By doing this, we add an extra layer of abstraction/opacity from a call in CLJS to an execution in Status-go. Again, IMO, that PR should update the native methods signature and fix the usages in the CLJS side (i.e. call these native methods with a single param being a JS object).

Another option might be to integrate the status-backend calls inside the native module methods, so it'd be implementing the integration in Java/Kotlin, but think that'd be harder to maintain.

CC: @ilmotta

@ulisesmac, I'll share my perspective, which can be incomplete.

We need to manually maintain a map (native-method -> endpoint) which will be error prone

I was imagining this manual mapping will only exist temporarily, until status-backend has no unsupported endpoints or very few (which would be easy to handle with a blocklist).

Lack of standarization/consistency: Why do we want to give different names to functions that should only be wrappers?

Isn't the case that unsupported endpoints always use the equivalent name, but with a V2 suffix? This is a temporary situation, but there is some consistency.

As said in 2, IMO, we should keep native modules related to status-go as thin as possible, so I don't agree with the approach taken in:

In the PR #21450, I understood that @qfrank opted to change the native implementation instead of the Clojure layer because all the changes to build up the JSON request and call the v2 functions would be isolated as internal implementation, and no Clojure code had to be changed in that PR. If, in that PR, we change the native function signatures to take a JSON payload, we will need to change all CLJS calls as well. Maybe it was just a case of different devs with different tech skills and what was most comfortable.

But I have to agree with you @ulisesmac, our agreement is that we only write native code if truly necessary (e.g. performance), otherwise we use Clojure, which is easier to manipulate for most CCs and works well with hot-reload and the REPL.

V2 endpoints bring following benefits as it use json data structure:

it will be easier to return an error

standarization/consistency for request/response between frontend/backend

ignore sensitive fields when log request/response

don't need to wait adaption from desktop side

I want this feature ASAP as I feel painful with build experience! Perhaps we can simply implement requesting the backend server in a native way that does not require much modification to ClojureScript. Only the configuration (server address/if enabled) needs to be passed to the native code. I can do it based on #21450 , WDYT? @ulisesmac @ilmotta

@qfrank

I agree on having a V2 version with a different API, for sure it is an improvement.

What I suggest is:

Update the native methods' signature to match the V2 endpoints.

Update the CLJS calls to these native methods (I can help with it, shouldn't be hard).

Integrate Status backend as suggested in this PR, by just adding a conditional call.

However, if we prefer to have a native implementation then it's fine for me. IMO implementing the request mechanism in Kotlin and another in ObjC is harder to test and maintain. Not sure, but according to the new arch docs, we could (when we adopt it) implement it in C++ and share it across platforms.

discard #21450 totally seems not good as we have to keep the old(V1) endpoints in status-go then 🤔

maybe adjust #21450 and update ReactMethod siganature, e.g.:

@ReactMethod fun inputConnectionStringForBootstrapping(connectionString: String, configJSON: String, callback: Callback) { val receiverClientConfig = JSONObject(configJSON) val params = JSONObject().apply { put("connectionString", connectionString) put("receiverClientConfig", receiverClientConfig) } val jsonString = params.toString() utils.executeRunnableStatusGoMethod( { Statusgo.inputConnectionStringForBootstrappingV2(jsonString) }, callback ) }

to

@ReactMethod fun inputConnectionStringForBootstrappingV2(jsonParam String, callback: Callback) { ... }

@ulisesmac

I want this feature ASAP as I feel painful with build experience! Perhaps we can simply implement requesting the backend server in a native way that does not require much modification to ClojureScript. Only the configuration (server address/if enabled) needs to be passed to the native code. I can do it based on #21450 , WDYT? @ulisesmac @ilmotta

@qfrank @ulisesmac This is all highly technical work, with no impact to users, therefore, we can first try to make things work, even if they are kind of dirty, if the code is safe to merge of course. On the other hand, unfortunately every time we change these parts of the code we risk introducing regressions, and the cost of asking a QA to review such PRs is considerable because they have to check a lot of stuff, so let's keep in mind the cost of follow-up PRs touching so many endpoints.

But whatever you guys align I'll probably agree because, in the end, I also just want this integration to work first, so we can all profit from it even during the next 2.32 release.

src/native_module/core.cljs

ulisesmac · 2024-10-29T19:40:34Z

@qfrank

I saw there're many other methods like create-account-and-login / init-status-go-logging are not using status-backend server yet, but you will update it later right? Because this is just a solution demo draft?

Oh, these methods are already using the status-backend 🤔 the substitution is probably unclear since I wanted to have a low impact in the codebase. We call the status backend when we execute a method from a native module.

Feel free to ping me on Discord if you have more questions.

ulisesmac · 2024-10-30T03:41:46Z

@ilmotta Pushed new code adding the shadow-cljs config stuff.

If the env var is marked (or the goog-define is set) as true the status backend runs. Ofc, it lacks of the proper migration for all the endpoints, we cannot complete the onboarding yet, but I think the code is good enough to be merged.

The only one point that I didn't addresss (since it still has an open discussion) is about the callback added in a promise flow.

nix/deps/gradle/generate.sh

qfrank

Pls use this PR instead :) @ulisesmac @ilmotta

ulisesmac · 2024-11-04T22:28:52Z

We changed the approach to a native implementation

ulisesmac requested review from ilmotta, seanstrom, qfrank and igor-sirotin October 28, 2024 19:27

ulisesmac self-assigned this Oct 28, 2024