upd: using bincode and leveraging use of slices by muse254 · Pull Request #71 · globe-and-citizen/layer8-backbone

muse254 · 2025-11-16T15:39:05Z

Changes:

cargo-fmt and cargo-clippy
using bincode and leveraging use of slices

* rebase to clippy-fmt pr * rm unrelated

dtpthao · 2025-11-18T18:11:49Z

-                pub x509_certificate: String,
-                pub client_id: String,
+                body: Some(
+                    serde_json::to_vec(&response_body).expect("this struct is json serializable"),


why would you remove usage of .to_bytes() if you can add expect statement to the method?

Please don't hate me.

But the argument here is that this .to_bytes indirection is not necessary imo since we are only calling the single function serde_json::bytes. We are also introducing bincode so it removes any assumptions for whoever is reading the code.

It also gives more clarity to inline what we have and keep things moving instead of the reader navigating to the function that really doesn't do any other operations by itself. It's acting as an alias.

Indirection makes sense when there's reason for it e.g. testing(plugin custom logic), public APIs (traits come in handy) and where we need to isolate a chunk of repeatable code and such

Please don't hate me.

But the argument here is that this .to_bytes indirection is not necessary imo since we are only calling the single function serde_json::bytes. We are also introducing bincode so it removes any assumptions for whoever is reading the code.

It also gives more clarity to inline what we have and keep things moving instead of the reader navigating to the function that really doesn't do any other operations by itself. It's acting as an alias.

Indirection makes sense when there's reason for it e.g. testing(plugin custom logic), public APIs (traits come in handy) and where we need to isolate a chunk of repeatable code and such

you can change the name of the method if its current isn't clear enough. I agree it's just a simple operation, but if you need to update that operation, you'll need to update it repeatedly in every place you use it, so do I also need to review it repeatedly?

We only have a few usages of it at the moment, but we might need to update the codebase and will use more in the future. Also, when reading the main functions, I prefer to focus on the core logic rather than low-level implementation details.

dtpthao · 2025-11-18T18:28:48Z

+        .unwrap_or_default();
+
+        let res_from_rp =
+            match serde_json::from_slice::<InitTunnelResponseFromRP>(ctx.get_response_body()) {


Do you have a reason for removing the usage of utils here? I prefer not to import dependencies directly in the main functions. Instead, the main functions should call utils, which helps us control dependency usage and makes it easier to change dependencies later.

This argument makes sense, but we can have a more holistic conversation on the pros and cons of the api.
If we go this route for smaller/one-off calls the number of indirections will make the code spaggetti

This argument makes sense, but we can have a more holistic conversation on the pros and cons of the api. If we go this route for smaller/one-off calls the number of indirections will make the code spaggetti

I see your point, but in this context the pros still outweigh the cons for me. Including the details of a simple conversion operation inside a large function makes the code harder to read and review.

dtpthao · 2025-11-18T18:42:58Z

    }

-    fn get_handlers(&self, method: &Method, path: &str) -> Option<&Box<[APIHandler<T>]>> {
+    fn get_handlers(&self, method: &Method, path: &str) -> Option<&[APIHandler<T>]> {


nice refactor!

dtpthao · 2025-11-18T19:24:05Z

+
+        match encrypted_message {
            Ok(encrypted_message) => {
+                let data = bincode::encode_to_vec(&encrypted_message, bincode::config::standard())


please update to_bytes() methods, so you don't need to update here

The EncryptedMessage is from ntor crate

dtpthao · 2025-11-18T19:24:57Z

        ctx: &mut Layer8Context,
        header_key: &str,
-        jwt_secret: &Vec<u8>
+        jwt_secret: &[u8],


how [u8] is better than Vec<u8>?

&[u8] can accept both &Vec and &[u8] as arguments when calling a function
&Vec only accepts &Vec

dtpthao · 2025-11-18T19:27:50Z

-            Ok(res) => Ok(res),
+        ctx: &mut Layer8Context,
+    ) -> Result<EncryptedMessage, APIHandlerResponse> {
+        match bincode::decode_from_slice(&ctx.get_request_body(), bincode::config::standard()) {


please, update ProxyHandler::parse_request_body, it's reusable - so you don't need to update repeatedly

Most implementations still use the json implementation, the args are tightly-coupled with Generic args and would take some refactoring and unrelated code changes in diff files. Does creating a new method for ProxyHandler make sense instead?

dtpthao · 2025-11-18T19:41:08Z

                Err(APIHandlerResponse {
                    status: StatusCode::BAD_REQUEST,
-                    body,
+                    body: Some(


The parse_request_body and ErrorResponse were created to be reusable, so we don’t have to duplicate the same logic throughout the codebase or update it in multiple places later. Do you have a reason for not using them here?

Diff errors here since it's from bincode impl

dtpthao · 2025-11-18T19:44:30Z

-        let wrapped_request: L8RequestObject = bytes_to_json(decrypted_data)
-            .map_err(|err| {
+        let wrapped_request: L8RequestObject =
+            serde_json::from_slice(&decrypted_data).map_err(|err| {


please, update the utils function

dtpthao · 2025-11-18T19:46:07Z

        ntor_server.set_shared_secret(shared_secret);

-        let data = response_body.to_bytes();
+        let data = serde_json::to_vec(&response_body).expect("the struct is json serializable");


the work would be much easier if you just update the utils funciton/method?

muse254 · 2025-12-01T05:39:34Z

Will setup a diff clean PR

cargo-clippy&cargo-fmt

7e21bb4

muse254 marked this pull request as ready for review November 17, 2025 02:34

muse254 requested a review from dtpthao November 17, 2025 02:34

upd: using bincode and leveraging use of slices (#72)

5db3f5d

* rebase to clippy-fmt pr * rm unrelated

muse254 changed the title ~~chore: cargo-clippy&cargo-fmt~~ upd: using bincode and leveraging use of slices Nov 18, 2025

dtpthao reviewed Nov 18, 2025

View reviewed changes

upd: use existing utils

b973cfe

muse254 closed this Dec 1, 2025

Conversation

muse254 commented Nov 16, 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!

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!

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!

muse254 commented Dec 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

muse254 commented Nov 16, 2025 •

edited

Loading