feat(rln-relay): abstract group management into its own api #1465

rymnc · 2022-12-14T13:20:00Z

Proposes a GroupManager, which will be a field on WakuRlnRelay, having a standard interface across all implementations. This aims to solve the issues related to readability of the code when it operates in different modes.

feat(rln-relay): group manager api

TODO (not exhaustive):

Implement event listener for onchain group manager
Move rln.nim into an ffi directory, with its wrapper procs (insertMember etc)
Move conversion utils into its own file
More tests for static group manager
clean up utils.nim

In follow up PR:

refactor mount to include group managers
move constants to appropriate locations

status-im-auto · 2022-12-14T23:00:57Z

Jenkins Builds

Click to see older builds (4)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`cfaf874`	#1	2022-12-14 23:00:56	~13 min	`macos`	📦`bin`

❌	`82fd516`	#2	2022-12-15 23:04:08	~16 min	`macos`	📄`log`

✔️	`134b664`	#3	2022-12-16 23:01:55	~14 min	`macos`	📦`bin`

✔️	`d864535`	#4	2023-01-03 23:02:10	~14 min	`macos`	📦`bin`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`4135b87`	#5	2023-01-12 23:04:08	~16 min	`macos`	📦`bin`

✔️	`776b616`	#6	2023-01-13 23:01:19	~13 min	`macos`	📦`bin`

rymnc · 2022-12-16T09:51:38Z

Note: imo we should create a separate PR to integrate these changes into the rln-relay itself. At the current moment, I have not removed any code from the utils.nim file, and left it as is for ease of review.
It's already 1500 loc :0

LNSD

Please, check my comments 🙂

LNSD · 2023-01-10T09:06:22Z

waku/v2/protocol/waku_rln_relay.nim

 export
-  rln,
+  ffi,
  constants,
  protocol_types,
  protocol_metrics,
+  conversion_utils,
  utils,
  contract


Are the members of all these modules used outside the waku_rln_relay module?

Typically you only expose/export the APIs used outside of the module, nothing else. This prevents the misusage of the module.

Yes, we do test even the ffi to ensure that breaking upstream changes are detected whenever we update submodules

I see. But... One thing is the module APIs, and another thing is testing.

A clean API without exposing internal symbols (types, methods, etc.) helps prevent the library's misusage that could break the program. You'll still be able to test the different submodules by importing them explicitly in the test file/module at the moment of testing.

Said that, do you still think that we should expose everything in this module?

I agree, we should just be functionally testing the logic. Tracking the fix - #1489

waku/v2/protocol/waku_rln_relay/conversion_utils.nim

LNSD · 2023-01-10T09:11:47Z

waku/v2/protocol/waku_rln_relay/conversion_utils.nim

+proc appendLength*(input: openArray[byte]): seq[byte] =
+  ## returns length prefixed version of the input
+  ## with the following format [len<8>|input<var>]
+  ## len: 8-byte value that represents the number of bytes in the `input`
+  ## len is serialized in little-endian
+  ## input: the supplied `input`
+  let
+    # the length should be serialized in little-endian
+    len = toBytes(uint64(input.len), Endianness.littleEndian)
+    output = concat(@len, @input)
+  return output


appendLength()

But:

... length prefixed version of the input ...

I would call this method something like encodeLengthPrefixed.

Will address when fixing #1487 :)

LNSD · 2023-01-10T09:15:08Z

waku/v2/protocol/waku_rln_relay/conversion_utils.nim

+    rootsBytes = concat(rootsBytes, @root)
+  return rootsBytes
+
+proc serializeIdCommitments*(idComms: seq[IDCommitment]): seq[byte] =


The two methods above are named as simple as serialize; this is serializeIdCommitments. I think that we need to improve the consistency in the procs naming and use only one naming schema :)

Will address when fixing #1487 :)

LNSD · 2023-01-10T09:21:44Z

waku/v2/protocol/waku_rln_relay/conversion_utils.nim

+proc toIdentityCredentials*(groupKeys: seq[(string, string)]): RlnRelayResult[seq[
+    IdentityCredential]] =
+  ## groupKeys is  sequence of membership key tuples in the form of (identity key, identity commitment) all in the hexadecimal format
+  ## the toIdentityCredentials proc populates a sequence of IdentityCredentials using the supplied groupKeys
+  ## Returns an error if the conversion fails
+
+  var groupIdCredentials = newSeq[IdentityCredential]()
+
+  for i in 0..groupKeys.len-1:
+    try:
+      let
+        idSecretHash = hexToUint[IdentitySecretHash.len*8](groupKeys[i][0]).toBytesLE()
+        idCommitment = hexToUint[IDCommitment.len*8](groupKeys[i][1]).toBytesLE()
+      groupIdCredentials.add(IdentityCredential(idSecretHash: idSecretHash,
+          idCommitment: idCommitment))
+    except ValueError as err:
+      warn "could not convert the group key to bytes", err = err.msg
+      return err("could not convert the group key to bytes: " & err.msg)
+  return ok(groupIdCredentials)


This proc could be collocated with the IdentityCredential type. In the "utils" modules, which I abhor and avoid, you should keep only things that are common to everything (e.g. toHex(), encodeLengthPrefixed(), toEpoch() or fromEpoch()).

Please review this module and see if it makes sense to collocate other methods next to the types based on the module imports graph. This is: if you are importing this utils module to use that proc, and the proc is not used elsewhere, then the proc is not in the right module.

Will address when fixing #1487 :)

LNSD · 2023-01-10T10:04:13Z

waku/v2/protocol/waku_rln_relay.nim

-  rln,
+  ffi,


Honestly, I prefer the rln name since it comes from librln. The ffi and wrapper names are too generic.

For example, I would name ffi a module that contains utilities to do ffi in an easier/opinionated way.

Would you prefer librln to be more explicit?

AFAIK in nim, the convention is to name the module after the library without the lib part (like in Rust, binding crates are appended with -sys). I prefer just rln, but I don't have a strong opinion as far as it is descriptive and not generic like ffi or wrapper :)

Addressed in 0f6a3a9 and 42231ae (some gitignore shenanigans)

LNSD · 2023-01-10T10:08:36Z

waku/v2/protocol/waku_rln_relay/group_manager/group_manager_base.nim

+type GroupManagerResult*[T] = Result[T, string]
+
+type
+  GroupManager*[Config] = ref object of RootObj


You declared the interface (the base object) here, but the associated procs are not using the method statement and the {.base.} pragma. Is there any reason for that?

oops! looks like this change was pushed by mistake, Addressed in 4135b87, thanks!

LNSD · 2023-01-12T14:50:53Z

One question/suggestion regarding the module's structure:

Would it be possible to move the rln (a.k.a. ffi in this PR) module to the waku/common folder? I see the RLN wrapper as a module that should be standalone, and the waku_rln_relay module should depend on this common module.

When doing the exercise to move the module to the common's folder, you have crossed dependencies (rln depending on waku_rln_relay and vice-versa), then there is something in the code layout that should be reviewed.

Disclaimer: I have not tried it. But from my experience, the likeliness is high when refactoring code and making it more modular.

rymnc · 2023-01-16T06:56:29Z

@LNSD can I merge this now?

LNSD

LGTM ✅

rymnc added this to the Release 0.14.0 milestone Dec 14, 2022

rymnc added track:maintenance track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications labels Dec 14, 2022

rymnc self-assigned this Dec 14, 2022

feat(rln-relay): group manager api

df21773

rymnc force-pushed the group-manager-api branch from cfca7a6 to df21773 Compare December 14, 2022 13:30

fix(rln-relay): static gm, method async works now

cfaf874

rymnc added 10 commits December 15, 2022 09:09

fix(rln-relay): cb naming, ensure merkle root changed

818b2d4

chore(rln-relay): static group manager fully tested

ae8ada6

chore(rln-relay): split ffi into its own module, split conversion_utils

d31ee5f

chore(rln-relay): refactor onchain group

8d2d125

fix(rln-relay): throw error if privatekey is malformed

82fd516

chore(rln-relay): convert methods to procs, remove warnings, formatting

dd736ea

chore(rln-relay): remove comment

3714ce0

style(rln-relay): fmt test

89883f6

feat(rln-relay): complete onchain group manager. pending tests

46ba7a2

fix(rln-relay): onchain implementation works now

8a2b0d7

rymnc added 2 commits December 16, 2022 15:27

fix(rln-relay): reg index

134b664

fix(rln): imports

d864535

rymnc marked this pull request as ready for review January 4, 2023 03:52

rymnc requested review from LNSD and s1fr0 January 9, 2023 08:07

LNSD reviewed Jan 10, 2023

View reviewed changes

This was referenced Jan 12, 2023

feat(rln-relay): 0.15.0 release #1486

Closed

chore(rln-relay): fix minor issues (fn naming etc) #1487

Closed

fix(rln-relay): revert method to proc conv

4135b87

rymnc modified the milestones: Release 0.14.0, Release 0.15.0 Jan 12, 2023

rymnc added 3 commits January 13, 2023 13:53

fix(rln-relay): s/ffi/rln

0f6a3a9

fix(rln-relay): remove rln/ from gitignore

42231ae

fix(rln-relay): s/ffi/rln

776b616

This was referenced Jan 16, 2023

chore: use npx to run ganache #1356

Closed

feat(rln-relay): Manage reconnections to the eth node #1306

Closed

feat(rln-relay): group management interface #1370

Closed

LNSD self-requested a review January 16, 2023 10:42

LNSD approved these changes Jan 16, 2023

View reviewed changes

rymnc merged commit 605cf1c into master Jan 16, 2023

rymnc deleted the group-manager-api branch January 16, 2023 12:56

rymnc mentioned this pull request Jan 24, 2023

feat(rln-relay): group manager integration #1496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rln-relay): abstract group management into its own api #1465

feat(rln-relay): abstract group management into its own api #1465

rymnc commented Dec 14, 2022 •

edited

status-im-auto commented Dec 14, 2022 •

edited

rymnc commented Dec 16, 2022

LNSD left a comment

LNSD Jan 10, 2023

rymnc Jan 12, 2023

LNSD Jan 12, 2023

rymnc Jan 13, 2023

LNSD Jan 10, 2023

rymnc Jan 12, 2023

LNSD Jan 10, 2023

rymnc Jan 12, 2023

LNSD Jan 10, 2023

rymnc Jan 12, 2023

LNSD Jan 10, 2023

rymnc Jan 12, 2023

LNSD Jan 12, 2023

rymnc Jan 13, 2023

LNSD Jan 10, 2023

rymnc Jan 12, 2023

LNSD commented Jan 12, 2023

rymnc commented Jan 16, 2023

LNSD left a comment

feat(rln-relay): abstract group management into its own api #1465

feat(rln-relay): abstract group management into its own api #1465

Conversation

rymnc commented Dec 14, 2022 • edited

status-im-auto commented Dec 14, 2022 • edited

Jenkins Builds

rymnc commented Dec 16, 2022

LNSD 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

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

LNSD commented Jan 12, 2023

rymnc commented Jan 16, 2023

LNSD left a comment

Choose a reason for hiding this comment

rymnc commented Dec 14, 2022 •

edited

status-im-auto commented Dec 14, 2022 •

edited