feat!: construct Processor with Config and Builder #454

Ma233 · 2023-07-12T13:38:16Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes/features)
Docs have been added / updated (for bug fixes / features)

🔵 What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

In order to reduce the differences in Processor initialization between native and browser, this PR provides ProcessorBuilder and ProcessorConfig.

ProcessorConfig should be a yaml string from user.
And ProcessorBuilder::from_config will accept that string to initialize ProcessorBuilder.
Then the programming context may set storage/measure/message_callback by its respective methods.
Finally use ProcessorBuilder::build method to construct a Processor.

This PR also defined session_manager serialization method SessionManager::dump for saving it in ProcessorConfig.

This PR also fixed the unsafe signature of rpcserver endpoint by using SessionManager to sign each request. See also #447.

🟤 What is the current behavior? (You can also link to an open issue here)

User use variant ways to construct Swarm and Stabilization then make them into Processor in tests, rings bin and browser Client.

🟢 What is the new behavior (if this is a feature change)?

Use ProcessorConfig for configuration, use ProcessorBuilder for runtime setup.

☢️ Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Yes, it changed the initialization parameters of browser Client.

ℹ️ Other information

Closes #447

RyanKung · 2023-07-14T07:50:02Z

core/src/session.rs

@@ -118,6 +118,19 @@ impl TryFrom<(String, String)> for Authorizer {
    }
 }

+// A SessionManager can be converted to a string using JSON and then encoded with base58.
+// To load the SessionManager from a string, use `SessionManager::from_str`.
+impl FromStr for SessionManager {


Maybe we should rename SessionManager to a better name, Session Manager is not a Menager, it's actualy a wrapper of generated key and it's session.

DelegationToken ? DelegatedKey? and can be abbr. as DTOKEN / DKEY

How about SessionKeypair ? Better to do it in a separated PR.

It's wrap of key, not wrap of session. The field session is used as a proof of the sk.
So, SessionizeKey if you want to undeline it can be verified by session. (I dont think it's a good idea, but it's ok)
Or, Delegatedkey if you want to underline how and what it's desined for.

DelegatedKey sounds more understandable. 😆

RyanKung · 2023-07-14T07:53:11Z

node/src/processor.rs

@@ -132,31 +169,6 @@ impl Processor {
 }

 impl Processor {
-    /// Generate Signature for Authorization
-    pub fn generate_signature(secret_key: &SecretKey) -> String {


This change is related to issue #447.
The implementation incorrect, but we should not remove it for now, it may cause security issues.

maybe simply sign with DKEY/DTOKEN(SessionManager)

Agree that. Fixing now.

RyanKung · 2023-07-14T07:57:25Z

core/src/session.rs

+    /// Dump session_manager to string, allowing user to save it in a config file.
+    /// It can be restored using `SessionManager::from_str`.
+    pub fn dump(&self) -> Result<String> {
+        let s = serde_json::to_string(&self).map_err(|_| Error::SerializeError)?;


Why not bincode? It will be encoded into b58 finally, so readable is not necessary property.

Readable would be better for debug. You could simply decode b58 to see the content, because it's serialized json string after decoding.

Ok, I agree

RyanKung · 2023-07-14T08:01:41Z

node/bin/rings.rs


-    let key = get_value(args.ecdsa_key, c.ecdsa_key);


"Accept a secret key directly, generated delegatedKey, then drop it."
-- I don't think the origin impl is that bad. It's fine if the sk is not holding by Client.
-- Just use & drop.

We can support both way for Client initialization, by sk directly, and dumped DKEY(SessionManager)

The generation procedure is in init command.
Here is just a behavior that is easily misunderstood, it replace the key of config with the command line arguments, so that I removed this behavior.

RyanKung · 2023-07-14T08:05:58Z

node/src/browser/client.rs

@@ -83,11 +85,10 @@ pub struct Client {
 impl Client {
    /// Creat a new client instance.
    pub fn new_client(
-        session_manager: SessionManager,
-        stuns: String,
+        config: String,


what is this config.....
ProcessorConfig ?

Sure it's ProcessorConfig.
The original Config was moved to native module and should only be used in native mode.
The Client need indeed enhancement. Here I just temporarily make it work.

RyanKung · 2023-07-14T08:09:02Z

node/src/browser/client.rs

+        let measure = PeriodicMeasure::new(ms);
+
+        let processor = Arc::new(
+            ProcessorBuilder::from_config(config)?


How to generate the Config?

Ok, I got it from unittest

let config = serde_yaml::to_string(&ProcessorConfig { ice_servers: "stun://stun.l.google.com:19302".to_string(), external_address: None, session_manager: sm.dump().unwrap(), stabilize_timeout: 200, })

How to generate session manager (DKEY).

I think JSON is better than YAML here, for browser case. We can support both YAML and JSON.

Support serialized config is great, but I think It's also necessary to support Config Struct.

To generate the SessionManager, use the exported SessionManagerBuilder.

JSON is also available for YAML loader.

I'm trying to make the method as less as possible.

RyanKung · 2023-07-14T08:20:25Z

core/src/swarm.rs

-    pub fn measure(mut self, implement: MeasureImpl) -> Self {
-        self.measure = Some(implement);
+    pub fn measure(mut self, implement: Option<MeasureImpl>) -> Self {
+        self.measure = implement;


Avoid use Option as input param of function unless there is no other way to impl it.

BTW, you should add document here, we should add document for any new or modified code

Agree to avoid using Option as input param (especially in build pattern). Fixing now.

Ma233 · 2023-07-15T05:48:50Z

@RyanKung Two commits are appended. PR description updated.

RyanKung

Looks great

Ma233 changed the title ~~Construct Processor with Config and Builder~~ feature!: construct Processor with Config and Builder Jul 12, 2023

Ma233 force-pushed the config branch 3 times, most recently from 7f8f64a to 7eef535 Compare July 13, 2023 13:08

Ma233 changed the title ~~feature!: construct Processor with Config and Builder~~ feat!: construct Processor with Config and Builder Jul 13, 2023

Ma233 force-pushed the config branch 2 times, most recently from 99f8fd2 to 435edf1 Compare July 13, 2023 13:35

Ma233 requested a review from RyanKung July 13, 2023 13:35

Ma233 marked this pull request as ready for review July 13, 2023 13:39

RyanKung reviewed Jul 14, 2023

View reviewed changes

Ma233 added 4 commits July 14, 2023 21:19

Collect native modules

2ed1bbc

Construct Processor with Config and Builder

50770bc

Fix node tests

cee15e5

Fix errors after merge

5d56887

Ma233 force-pushed the config branch from 435edf1 to 5d56887 Compare July 14, 2023 13:45

Do not use Option type when set builder

336e36a

Sign rpc request by session_manager

e6da325

Ma233 force-pushed the config branch from 4015a01 to e6da325 Compare July 15, 2023 05:50

RyanKung approved these changes Jul 15, 2023

View reviewed changes

Ma233 merged commit 449adae into RingsNetwork:master Jul 15, 2023

Ma233 deleted the config branch July 15, 2023 09:46

RyanKung mentioned this pull request Jul 29, 2023

chore!: rename session manager to delegated sk #460

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: construct Processor with Config and Builder #454

feat!: construct Processor with Config and Builder #454

Ma233 commented Jul 12, 2023 •

edited

Loading

RyanKung Jul 14, 2023

Ma233 Jul 14, 2023 •

edited

Loading

RyanKung Jul 14, 2023 •

edited

Loading

Ma233 Jul 15, 2023

RyanKung Jul 14, 2023 •

edited by Ma233

Loading

RyanKung Jul 14, 2023

Ma233 Jul 14, 2023

RyanKung Jul 14, 2023

Ma233 Jul 14, 2023 •

edited

Loading

RyanKung Jul 14, 2023

RyanKung Jul 14, 2023

RyanKung Jul 14, 2023

Ma233 Jul 14, 2023 •

edited

Loading

RyanKung Jul 14, 2023

RyanKung Jul 14, 2023

Ma233 Jul 14, 2023

RyanKung Jul 14, 2023

RyanKung Jul 14, 2023

RyanKung Jul 14, 2023

RyanKung Jul 14, 2023

Ma233 Jul 14, 2023

RyanKung Jul 14, 2023

RyanKung Jul 14, 2023

Ma233 Jul 14, 2023

Ma233 commented Jul 15, 2023

RyanKung left a comment

feat!: construct Processor with Config and Builder #454

feat!: construct Processor with Config and Builder #454

Conversation

Ma233 commented Jul 12, 2023 • edited Loading

Please check if the PR fulfills these requirements

🔵 What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

🟤 What is the current behavior? (You can also link to an open issue here)

🟢 What is the new behavior (if this is a feature change)?

☢️ Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

ℹ️ Other information

Choose a reason for hiding this comment

Ma233 Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

RyanKung Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanKung Jul 14, 2023 • edited by Ma233 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

Ma233 Jul 14, 2023 • 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

Ma233 Jul 14, 2023 • 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

Ma233 commented Jul 15, 2023

RyanKung left a comment

Choose a reason for hiding this comment

Ma233 commented Jul 12, 2023 •

edited

Loading

Ma233 Jul 14, 2023 •

edited

Loading

RyanKung Jul 14, 2023 •

edited

Loading

RyanKung Jul 14, 2023 •

edited by Ma233

Loading

Ma233 Jul 14, 2023 •

edited

Loading

Ma233 Jul 14, 2023 •

edited

Loading