refactor_: add more default node config values for frontend when doing local pair sync #5098

qfrank · 2024-04-26T14:21:54Z

this is a continue work to PR
relate comment

status: ready.

status-im-auto · 2024-04-26T14:26:40Z

Jenkins Builds

Click to see older builds (8)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`9ab9fb9`	#1	2024-04-26 14:26:40	~4 min	`linux`	📦`zip`
✔️	`9ab9fb9`	#1	2024-04-26 14:27:20	~5 min	`ios`	📦`zip`
✔️	`9ab9fb9`	#1	2024-04-26 14:28:15	~6 min	`android`	📦`aar`
✔️	`9ab9fb9`	#1	2024-04-26 15:04:26	~42 min	`tests`	📄`log`

✖️	`c4f8840`	#2	2024-05-01 01:35:15	~1 min	`tests`	📄`log`
✔️	`c4f8840`	#2	2024-05-01 01:35:46	~1 min	`android`	📦`aar`
✔️	`c4f8840`	#2	2024-05-01 01:36:24	~2 min	`linux`	📦`zip`
✔️	`c4f8840`	#2	2024-05-01 01:37:35	~3 min	`ios`	📦`zip`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`0d394fb`	#3	2024-05-07 04:26:38	~3 min	`ios`	📦`zip`
✔️	`0d394fb`	#3	2024-05-07 04:26:58	~4 min	`linux`	📦`zip`
✔️	`0d394fb`	#3	2024-05-07 04:28:00	~5 min	`android`	📦`aar`
✖️	`0d394fb`	#3	2024-05-07 04:29:57	~7 min	`tests`	📄`log`

✔️	`2af2935`	#4	2024-05-07 09:34:37	~2 min	`android`	📦`aar`
✔️	`2af2935`	#4	2024-05-07 09:34:57	~2 min	`linux`	📦`zip`
✔️	`2af2935`	#4	2024-05-07 09:36:08	~3 min	`ios`	📦`zip`
✔️	`2af2935`	#4	2024-05-07 10:15:10	~42 min	`tests`	📄`log`

igor-sirotin · 2024-04-30T19:39:36Z

api/defaults.go

-	if request.LogLevel != nil {
-		nodeConfig.LogLevel = *request.LogLevel
-		nodeConfig.LogEnabled = true
-
-	} else {
-		nodeConfig.LogEnabled = false
-	}
-


Could you please elaborate why remove this?

it's duplicated... you can see them already in line 233

igor-sirotin · 2024-04-30T20:06:56Z

server/pairing/sync_device_test.go

+	}
+
+	// need specify cluster config here, otherwise TestPairingThreeDevices will fail due to no messages(CR) received
+	// TODO(frank) need to figure out why above happen
 	clusterConfig, err := params.LoadClusterConfigFromFleet("eth.prod")


Let's replace eth.prod with constant?

status-go/params/cluster.go

Line 6 in 34c693b

FleetProd = "eth.prod"

Or even better take one of supported fleets:

status-go/params/cluster.go

Lines 24 to 29 in 34c693b

// DefaultWakuNodes is a list of "supported" fleets. This list is populated to clients UI settings.

var supportedFleets = map[string][]string{

FleetWakuV2Prod: {"enrtree://ANEDLO25QVUGJOUTQFRYKWX6P4Z4GKVESBMHML7DZ6YK4LGS5FC5O@prod.wakuv2.nodes.status.im"},

FleetWakuV2Test: {"enrtree://AO47IDOLBKH72HIZZOXQP6NMRESAN7CHYWIBNXDXWRJRZWLODKII6@test.wakuv2.nodes.status.im"},

FleetShardsTest: {"enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.test.shards.nodes.status.im"},

}

I'll take the first since i only trust eth.prod ATM. 🙂

igor-sirotin · 2024-04-30T20:16:48Z

server/pairing/common.go

+	// following specifiedXXX variables are used to check if frontend has specified the value
+	// if not, the default value is set. NOTE: we also check 2 extra fields: WakuV2Config(LightClient|Nameserver)
+	// see api.SetFleet for more details
+	specifiedVerifyTransactionURL := c.ShhextConfig.VerifyTransactionURL
+	specifiedVerifyENSURL := c.ShhextConfig.VerifyENSURL
+	specifiedVerifyENSContractAddress := c.ShhextConfig.VerifyENSContractAddress
+	specifiedVerifyTransactionChainID := c.ShhextConfig.VerifyTransactionChainID
+	specifiedNetworkID := c.NetworkID
+	specifiedNetworks := c.Networks
+	specifiedUpstreamConfigURL := c.UpstreamConfig.URL
+	specifiedLogEnabled := c.LogEnabled
+	specifiedLogLevel := c.LogLevel
+	specifiedFleet := c.ClusterConfig.Fleet
+	specifiedInstallationID := c.ShhextConfig.InstallationID
+	specifiedTorrentConfigEnabled := c.TorrentConfig.Enabled
+	specifiedTorrentConfigPort := c.TorrentConfig.Port
+
+	if len(specifiedNetworks) == 0 {


Maybe I'm missing something, but this looks very confusing to me.
I know you have plans on re-implementing this with smth like login-with-syncing request concept. I think we need to go with without merging this PR. What's the point?

We really should not pass nodeConfig from client side, this is wrong. And will very soon become a pain, as we want to remove nodeConfig handling from desktop.

In general there's no need on passing NodeConfig from client.
In desktop we send a very default struct, no custom fields.

If we look at ReceiverConfig, we can see a few customizations there already, e.g. SettingCurrentNetwork:

status-go/server/pairing/config.go

Lines 25 to 42 in 3bacb84

type ReceiverConfig struct {

// nodeConfig is required, but we'll validate it separately

NodeConfig *params.NodeConfig `json:"nodeConfig"`

// ReceiverConfig.KeystorePath must not end with keyUID (because keyUID is not known yet)

KeystorePath string `json:"keystorePath" validate:"required,not_end_keyuid"`

// DeviceType SendPairInstallation need this information

DeviceType string `json:"deviceType" validate:"required"`

KDFIterations int `json:"kdfIterations" validate:"gte=0"`

// SettingCurrentNetwork corresponding to field current_network from table settings, so that we can override current network from sender

SettingCurrentNetwork string `json:"settingCurrentNetwork" validate:"required"`

DeviceName string `json:"deviceName"`

DB *multiaccounts.Database `json:"-"`

LoggedInKeyUID string `json:"-"`

}

Let's just follow this path and add required fields to this structure.
For sake of simplicity/readability/maintainability let's compose it into a single struct, maybe NodeConfigOptions (or other better name):

type NodeConfigOptions struct { // find better naming VerifyTransactionURL string ... }

@qfrank let me know what you think
We could merge this solution and improve it in a separate PR, but I don't understand why not do it properly straightaway.

The point is: This PR has sorted out which parameters should be configurable for the frontend, and which parameters can have default values provided by the backend. The frontend no longer needs to know so many configuration details of nodeconfig(there is a mobile PR to decrease such details). Refactoring is a step-by-step process.

We really should not pass nodeConfig from client side, this is wrong

agreed

Let's just follow this path and add required fields to this structure.

agreed

Let's merge this and improve it in a separate PR ❤️ @igor-sirotin

Samyoul

Thank you for this PR @qfrank . 🙏

For handling NodeConfig in local syncing, we only need a small amount of parameters if any at all.

Unless the parameter is related to getting keys or db data, shouldn't config data be the responsibility of the recipient / bootstrapped device? I may be mistaken about something, but I don't think we should share the nodeconfig settings of the sending / bootstrapping device.

Please forgive me if I'm missing something.

qfrank · 2024-05-06T03:58:55Z

Thank you for this PR @qfrank . 🙏

For handling NodeConfig in local syncing, we only need a small amount of parameters if any at all.

Unless the parameter is related to getting keys or db data, shouldn't config data be the responsibility of the recipient / bootstrapped device? I may be mistaken about something, but I don't think we should share the nodeconfig settings of the sending / bootstrapping device.

Please forgive me if I'm missing something.

yeah I agree, but currently there still are some params(e.g. OpenseaAPIKey) relate to node config need client to pass, we will remove NodeConfig and put the bare minimum needed from the client in some places as @igor-sirotin suggested before finally @Samyoul

…g local pair sync

Samyoul

Thank you @qfrank.

qfrank self-assigned this Apr 26, 2024

qfrank requested review from vitvly, cammellos, igor-sirotin and Samyoul April 26, 2024 14:22

qfrank changed the title ~~refactor_: add more default node config values for frontend when doin…~~ refactor_: add more default node config values for frontend when doing local pair sync Apr 26, 2024

qfrank marked this pull request as ready for review April 26, 2024 23:54

qfrank mentioned this pull request Apr 29, 2024

refactor_:remove part of node config for local pair sync status-im/status-mobile#19826

Merged

igor-sirotin reviewed Apr 30, 2024

View reviewed changes

Samyoul reviewed May 1, 2024

View reviewed changes

igor-sirotin approved these changes May 1, 2024

View reviewed changes

qfrank force-pushed the refactor/default_node_config_for_local_pair_sync branch from c4f8840 to 0d394fb Compare May 7, 2024 04:22

refactor_: add more default node config values for frontend when doin…

2af2935

…g local pair sync

qfrank force-pushed the refactor/default_node_config_for_local_pair_sync branch from 0d394fb to 2af2935 Compare May 7, 2024 09:32

Samyoul approved these changes May 7, 2024

View reviewed changes

qfrank merged commit 4f4b7a9 into develop May 7, 2024
7 checks passed

qfrank deleted the refactor/default_node_config_for_local_pair_sync branch May 7, 2024 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor_: add more default node config values for frontend when doing local pair sync #5098

refactor_: add more default node config values for frontend when doing local pair sync #5098

qfrank commented Apr 26, 2024 •

edited

status-im-auto commented Apr 26, 2024 •

edited

igor-sirotin Apr 30, 2024

qfrank May 1, 2024

igor-sirotin Apr 30, 2024

qfrank May 1, 2024

igor-sirotin Apr 30, 2024

qfrank May 1, 2024 •

edited

Samyoul left a comment •

edited

qfrank commented May 6, 2024

Samyoul left a comment

	// DefaultWakuNodes is a list of "supported" fleets. This list is populated to clients UI settings.
	var supportedFleets = map[string][]string{
	FleetWakuV2Prod: {"enrtree://ANEDLO25QVUGJOUTQFRYKWX6P4Z4GKVESBMHML7DZ6YK4LGS5FC5O@prod.wakuv2.nodes.status.im"},
	FleetWakuV2Test: {"enrtree://AO47IDOLBKH72HIZZOXQP6NMRESAN7CHYWIBNXDXWRJRZWLODKII6@test.wakuv2.nodes.status.im"},
	FleetShardsTest: {"enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.test.shards.nodes.status.im"},
	}

	type ReceiverConfig struct {
	// nodeConfig is required, but we'll validate it separately
	NodeConfig *params.NodeConfig `json:"nodeConfig"`

	// ReceiverConfig.KeystorePath must not end with keyUID (because keyUID is not known yet)
	KeystorePath string `json:"keystorePath" validate:"required,not_end_keyuid"`

	// DeviceType SendPairInstallation need this information
	DeviceType string `json:"deviceType" validate:"required"`
	KDFIterations int `json:"kdfIterations" validate:"gte=0"`

	// SettingCurrentNetwork corresponding to field current_network from table settings, so that we can override current network from sender
	SettingCurrentNetwork string `json:"settingCurrentNetwork" validate:"required"`

	DeviceName string `json:"deviceName"`
	DB *multiaccounts.Database `json:"-"`
	LoggedInKeyUID string `json:"-"`
	}

refactor_: add more default node config values for frontend when doing local pair sync #5098

refactor_: add more default node config values for frontend when doing local pair sync #5098

Conversation

qfrank commented Apr 26, 2024 • edited

status-im-auto commented Apr 26, 2024 • edited

Jenkins Builds

igor-sirotin Apr 30, 2024

Choose a reason for hiding this comment

qfrank May 1, 2024

Choose a reason for hiding this comment

igor-sirotin Apr 30, 2024

Choose a reason for hiding this comment

qfrank May 1, 2024

Choose a reason for hiding this comment

igor-sirotin Apr 30, 2024

Choose a reason for hiding this comment

qfrank May 1, 2024 • edited

Choose a reason for hiding this comment

Samyoul left a comment • edited

Choose a reason for hiding this comment

qfrank commented May 6, 2024

Samyoul left a comment

Choose a reason for hiding this comment

qfrank commented Apr 26, 2024 •

edited

status-im-auto commented Apr 26, 2024 •

edited

qfrank May 1, 2024 •

edited

Samyoul left a comment •

edited