Direct settings updates (API) #5237

friofry · 2024-05-27T13:35:27Z

Adds 2 API endpoints for direct settings modification see status-im/status-desktop#12918

SaveNewWakuNode
SetMaxLogBackups

Closes status-im/status-desktop#12918

Further changes:

(NodeConfig): Added Waku nodes are lost after a restart status-desktop#14929

status-im-auto · 2024-05-27T13:40:10Z

Jenkins Builds

Click to see older builds (32)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`3fcf2ec`	#1	2024-05-27 13:40:09	~4 min	`linux`	📦`zip`
✔️	`3fcf2ec`	#1	2024-05-27 13:41:47	~6 min	`android`	📦`aar`
✔️	`3fcf2ec`	#1	2024-05-27 13:45:21	~9 min	`ios`	📦`zip`

✔️	`3fcf2ec`	#1	2024-05-27 14:21:11	~45 min	`tests`	📄`log`

✔️	`5598aa1`	#2	2024-05-29 18:47:52	~2 min	`linux`	📦`zip`
✔️	`5598aa1`	#2	2024-05-29 18:48:13	~3 min	`android`	📦`aar`
✔️	`5598aa1`	#2	2024-05-29 18:49:17	~4 min	`ios`	📦`zip`

✔️	`5598aa1`	#2	2024-05-29 19:28:23	~43 min	`tests`	📄`log`

✔️	`230f96d`	#3	2024-05-29 20:26:12	~1 min	`android`	📦`aar`
✔️	`230f96d`	#3	2024-05-29 20:26:36	~2 min	`linux`	📦`zip`
✔️	`230f96d`	#3	2024-05-29 20:27:44	~3 min	`ios`	📦`zip`

✔️	`230f96d`	#3	2024-05-29 21:07:35	~43 min	`tests`	📄`log`

✖️	`cdb936d`	#4	2024-05-30 11:15:59	~2 min	`tests`	📄`log`

✔️	`cdb936d`	#4	2024-05-30 11:17:28	~4 min	`linux`	📦`zip`
✔️	`cdb936d`	#4	2024-05-30 11:18:53	~5 min	`ios`	📦`zip`
✔️	`cdb936d`	#4	2024-05-30 11:18:56	~5 min	`android`	📦`aar`

✖️	`4a442ab`	#5	2024-06-04 14:09:29	~2 min	`tests`	📄`log`

✔️	`4a442ab`	#5	2024-06-04 14:10:44	~3 min	`ios`	📦`zip`
✔️	`4a442ab`	#5	2024-06-04 14:10:58	~4 min	`linux`	📦`zip`
✔️	`4a442ab`	#5	2024-06-04 14:12:41	~5 min	`android`	📦`aar`

✖️	`31365a0`	#6	2024-06-04 14:10:46	~1 min	`tests`	📄`log`

✔️	`31365a0`	#6	2024-06-04 14:13:38	~2 min	`linux`	📦`zip`
✔️	`31365a0`	#6	2024-06-04 14:14:32	~3 min	`ios`	📦`zip`
✔️	`31365a0`	#6	2024-06-04 14:18:01	~5 min	`android`	📦`aar`

✔️	`054e5b3`	#7	2024-06-04 14:22:29	~2 min	`android`	📦`aar`
✔️	`054e5b3`	#7	2024-06-04 14:23:48	~3 min	`ios`	📦`zip`
✔️	`054e5b3`	#7	2024-06-04 14:24:19	~4 min	`linux`	📦`zip`

✖️	`054e5b3`	#7	2024-06-04 14:22:45	~2 min	`tests`	📄`log`

✖️	`721896d`	#8	2024-06-04 14:30:42	~1 min	`tests`	📄`log`

✔️	`721896d`	#8	2024-06-04 14:31:18	~2 min	`android`	📦`aar`
✔️	`721896d`	#8	2024-06-04 14:32:58	~3 min	`ios`	📦`zip`
✔️	`721896d`	#8	2024-06-04 14:33:30	~4 min	`linux`	📦`zip`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`d2d463b`	#9	2024-06-04 14:51:43	~2 min	`linux`	📦`zip`
✔️	`d2d463b`	#9	2024-06-04 14:52:49	~3 min	`ios`	📦`zip`
✔️	`d2d463b`	#9	2024-06-04 14:55:08	~5 min	`android`	📦`aar`

✔️	`d2d463b`	#9	2024-06-04 15:29:42	~40 min	`tests`	📄`log`

jrainville

Looks good. I'd just add some simple validation, since it comes from user inputs

jrainville · 2024-05-27T17:37:31Z

protocol/messenger_settings.go

@@ -20,10 +20,18 @@ func (m *Messenger) SetLogLevel(request *requests.SetLogLevel) error {
 	return nodecfg.SetLogLevel(m.database, request.LogLevel)
 }

+func (m *Messenger) SetMaxLogBackups(request *requests.SetMaxLogBackups) error {
+	return nodecfg.SetMaxLogBackups(m.database, request.MaxLogBackups)


I wonder if we should do some validations on the request first? Like make sure MaxLogBackups is not empty and not smaller than 0?

We have a couple Validate() functions in the code to check the content of the requests. You can take inspiration (copy paste 😄 )

we should make it an uint, that's going to be granted that is 0 or greater

jrainville · 2024-05-27T17:37:37Z

protocol/messenger_settings.go

 func (m *Messenger) SetCustomNodes(request *requests.SetCustomNodes) error {
 	return nodecfg.SetWakuV2CustomNodes(m.database, request.CustomNodes)
 }

+func (m *Messenger) SaveNewWakuNode(request *requests.SaveNewWakuNode) error {
+	return nodecfg.SaveNewWakuNode(m.database, request.NodeAddress)


Same thing here

Added a fix.

I think it's a general check and we shouldn't be testing these here:

peer.AddrInfoFromP2pAddr addr.ValueForProtocol

igor-sirotin · 2024-05-29T12:49:46Z

@friofry, can you please add a test for the new functions?

This doesn't look good 🙂

cammellos · 2024-05-29T12:53:07Z

protocol/requests/save_new_waku_node.go

+package requests
+
+type SaveNewWakuNode struct {
+	NodeAddress string `json:"nodeAddress"`


you want to validate this most likely

jrainville · 2024-05-29T18:44:35Z

@friofry I think you forgot to push. There were no new changes

friofry · 2024-05-29T18:46:46Z

@friofry, can you please add a test for the new functions?

This doesn't look good 🙂

Hey @igor-sirotin
It's hard to have 50% tests here because most of the functions either call other functions or insert something into the db.
I'll add a test for the Validate() function.

friofry · 2024-05-29T18:55:12Z

@friofry I think you forgot to push. There were no new changes

Yes, my mistake, it should be pushed now

protocol/requests/save_new_waku_node.go

friofry · 2024-05-29T20:25:05Z

@friofry, can you please add a test for the new functions?

This doesn't look good 🙂

Added TestMessengerValidateRequestSuite

igor-sirotin · 2024-05-29T20:33:15Z

It's hard to have 50% tests

I don't mean that it should be 50. I mean it shouldn't be 0 😄
The 50% check is not required yet 🙂

We discussed your PR today with the Snow Blowers crew, as a good example of when 50% threshold might not be reached because most of the added code is some wrapper functions. We're aware of this and We will exclude such API files from calculation for test coverage.

Nevertheless, it's not expected to cover the 50% threshold now, my comment is actually about the ... insert something into db. This must be covered by tests.

We have many persistence_test.go files in our codebase, you can take take ActivityCenterPersistenceTestSuite as an example.

friofry · 2024-05-30T06:05:19Z

It's hard to have 50% tests

I don't mean that it should be 50. I mean it shouldn't be 0 😄 The 50% check is not required yet 🙂

We discussed your PR today with the Snow Blowers crew, as a good example of when 50% threshold might not be reached because most of the added code is some wrapper functions. We're aware of this and We will exclude such API files from calculation for test coverage.

Nevertheless, it's not expected to cover the 50% threshold now, my comment is actually about the ... insert something into db. This must be covered by tests.

We have many persistence_test.go files in our codebase, you can take take ActivityCenterPersistenceTestSuite as an example.

Now I got it, thanks for the example! I'll add a test for all these "direct" settings updaters

friofry · 2024-05-30T11:26:59Z

We have many persistence_test.go files in our codebase, you can take take ActivityCenterPersistenceTestSuite as an example.

Done 👍 Added node_config_persistence_test.go
Writing small tests is rewarding. Found out that we shouldn't call these methods until the database is initialised for the first time

igor-sirotin

Thanks for the test, looks good! 👍
Left a few minor suggestions

igor-sirotin · 2024-05-31T08:06:51Z

protocol/messenger_validate_requests_test.go

+}
+
+func (s *MessengerValidateRequestSuite) TestSaveNewWakuNodeRequestValidate_Enrtree() {
+	alice := s.newMessenger()


You can reuse m from MessengerBaseTestSuite

IF that one is not enough and you need to create another messenger instance, then use newTestMessenger

protocol/messenger_validate_requests_test.go

igor-sirotin · 2024-05-31T08:14:54Z

protocol/node_config_persistence_test.go

+var testWakuNodeEnrtree = "enrtree://AL65EKLJAUXKKPG43HVTML5EFFWEZ7L4LOKTLZCLJASG4DSESQZEC@prod.status.nodes.status.im"
+var testWakuNodeMultiaddr = "/ip4/127.0.0.1/tcp/34012"


Suggested change

var testWakuNodeEnrtree = "enrtree://AL65EKLJAUXKKPG43HVTML5EFFWEZ7L4LOKTLZCLJASG4DSESQZEC@prod.status.nodes.status.im"

var testWakuNodeMultiaddr = "/ip4/127.0.0.1/tcp/34012"

const (

testWakuNodeEnrtree = "enrtree://AL65EKLJAUXKKPG43HVTML5EFFWEZ7L4LOKTLZCLJASG4DSESQZEC@prod.status.nodes.status.im"

testWakuNodeMultiaddr = "/ip4/127.0.0.1/tcp/34012"

)

igor-sirotin · 2024-05-31T08:17:15Z

protocol/node_config_persistence_test.go

+	config, err := nodecfg.GetNodeConfigFromDB(s.db)
+	s.Require().NoError(err)
+	s.config = config


Suggested change

config, err := nodecfg.GetNodeConfigFromDB(s.db)

s.Require().NoError(err)

s.config = config

s.config, err = nodecfg.GetNodeConfigFromDB(s.db)

s.Require().NoError(err)

* add waku nodeaddr and max log backups validation * Allow to set new waku node * Allow to set custom max backups * add tests

friofry requested review from cammellos, jrainville and mprakhov May 27, 2024 13:35

friofry changed the title ~~Ab/issue 12918 setting update with diffs~~ API to direct settings updates May 27, 2024

friofry changed the title ~~API to direct settings updates~~ Direct settings updates (API) May 27, 2024

friofry mentioned this pull request May 27, 2024

Save only the settings that are changed status-im/status-desktop#14930

Merged

jrainville reviewed May 27, 2024

View reviewed changes

igor-sirotin mentioned this pull request May 29, 2024

Make codeclimate diff coverage test reports Required #5254

Open

cammellos reviewed May 29, 2024

View reviewed changes

friofry requested review from cammellos and jrainville May 29, 2024 18:38

friofry force-pushed the ab/issue-12918-setting-update-with-diffs branch from 3fcf2ec to 5598aa1 Compare May 29, 2024 18:44

jrainville reviewed May 29, 2024

View reviewed changes

protocol/requests/save_new_waku_node.go Show resolved Hide resolved

friofry requested a review from jrainville May 30, 2024 05:55

friofry force-pushed the ab/issue-12918-setting-update-with-diffs branch from 230f96d to cdb936d Compare May 30, 2024 11:12

igor-sirotin approved these changes May 31, 2024

View reviewed changes

friofry force-pushed the ab/issue-12918-setting-update-with-diffs branch 4 times, most recently from 054e5b3 to 721896d Compare June 4, 2024 14:28

fix(config)_: add API calls to set new waku node and max backups

d2d463b

* add waku nodeaddr and max log backups validation * Allow to set new waku node * Allow to set custom max backups * add tests

friofry force-pushed the ab/issue-12918-setting-update-with-diffs branch from 721896d to d2d463b Compare June 4, 2024 14:49

friofry merged commit fe25d97 into develop Jun 4, 2024
10 checks passed

friofry deleted the ab/issue-12918-setting-update-with-diffs branch June 4, 2024 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Direct settings updates (API) #5237

Direct settings updates (API) #5237

friofry commented May 27, 2024

status-im-auto commented May 27, 2024 •

edited

Loading

jrainville left a comment

jrainville May 27, 2024

cammellos May 29, 2024

friofry May 29, 2024

jrainville May 27, 2024

friofry May 29, 2024

igor-sirotin commented May 29, 2024 •

edited

Loading

cammellos May 29, 2024

jrainville commented May 29, 2024

friofry commented May 29, 2024

friofry commented May 29, 2024

friofry commented May 29, 2024

igor-sirotin commented May 29, 2024 •

edited

Loading

friofry commented May 30, 2024

friofry commented May 30, 2024

igor-sirotin left a comment

igor-sirotin May 31, 2024

igor-sirotin May 31, 2024

igor-sirotin May 31, 2024

		var testWakuNodeEnrtree = "enrtree://AL65EKLJAUXKKPG43HVTML5EFFWEZ7L4LOKTLZCLJASG4DSESQZEC@prod.status.nodes.status.im"
		var testWakuNodeMultiaddr = "/ip4/127.0.0.1/tcp/34012"

Direct settings updates (API) #5237

Direct settings updates (API) #5237

Conversation

friofry commented May 27, 2024

status-im-auto commented May 27, 2024 • edited Loading

Jenkins Builds

jrainville 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

igor-sirotin commented May 29, 2024 • edited Loading

Choose a reason for hiding this comment

jrainville commented May 29, 2024

friofry commented May 29, 2024

friofry commented May 29, 2024

friofry commented May 29, 2024

igor-sirotin commented May 29, 2024 • edited Loading

friofry commented May 30, 2024

friofry commented May 30, 2024

igor-sirotin 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

status-im-auto commented May 27, 2024 •

edited

Loading

igor-sirotin commented May 29, 2024 •

edited

Loading

igor-sirotin commented May 29, 2024 •

edited

Loading