Fellow members fetching #79

Bullrich · 2023-09-13T18:25:31Z

Created the method to fetch the fellows from the parachain using Polkadot-JS. This will be updated to the new library once it becomes available.

The system fetches all the available fellows with a GitHub handle and caches them. When requesting a fellow, it automatically filters the one it needs and returns an array of handles.

This resolves #61

Co-authored-by: Yuri Volkov mutantcornholio@users.noreply.github.com
Co-authored-by: Maksym Hlukhovtsov mordamax@users.noreply.github.com

mordamax · 2023-09-13T20:06:48Z

src/polkadot/fellows.ts

+    const requiredRank = Number(ranking);
+    this.logger.info(`Fetching members of rank '${requiredRank}' or higher`);
+
+    if (this.fellowsCache.size < 1) {


this will use data from cache in case if there are more than 1 rule bucket with rank: x?
i don't see any other use case then... right?

It is a map. The idea for this is to fetch all the available fellows once and then we filter per request, instead of fetching them multiple times per required rank.

So if you request in one rule a rank 4, it will already fetch them all, so, if you request later rank 2, all the ranks will be available already.

README.md

mordamax · 2023-09-13T20:11:14Z

src/test/fellows.test.ts

+    mockClear(logger);
+    const members2 = await fellows.getTeamMembers("2");
+    expect(members2.length).toBeGreaterThan(0);
+    expect(logger.debug).not.toHaveBeenCalledWith("Connecting to collective parachain");


this is nice test. do you plan to add the actual rule with rank:2 where you see it allows / or disallows if approver is rank < 2 ?

UPD: ah I see you keep adding stuff :D

That's a good question. I didn't plan on adding it, but maybe it's no trouble at all.

It actually runs out of the box, so that's good!

It actually runs out of the box, so that's good!

where? I haven't seen in this diff changes where the rank: 2 approvals are being tested, could you please point my eyes?

FYI: if there's no test, not a blocker for this PR, just needs to be done now or later

I added a test to src/test/runner/runner.test.ts where it verifies that the user is of a given rank and skips the test.

hmm.. so if author is fellow - it means he doesn't need review? or I don't get
I thought they need to check whether someone who has approved - is a fellow with dan >= X, so then it counts. otherwise the review-bot blocks merge. So I was expecting this kind of test

src/polkadot/fellows.ts

rzadp

Looks good, just one thing: do you have plans for invalidating the cache of fellows?

mutantcornholio · 2023-09-14T07:43:01Z

src/polkadot/fellows.ts

+        const [address]: [string] = key.toHuman();
+        fellows.push({ address, ...(rank.toHuman() as object) } as FellowData);


Wait, is it basically this?

fellows.push({address: key.toHuman()[0], rank: rank.toHuman()[0]});

Haha yes.

I just used destructuring so that I doesn't look like I'm accessing random array elements which makes it a bit harder to read, but yes.

key is a string with the address, and rank is {rank: 1}.

src/polkadot/fellows.ts

mutantcornholio · 2023-09-14T07:50:03Z

src/polkadot/fellows.ts

+      await api.disconnect();
+      return users;
+    } catch (error) {
+      console.error(error);


But why do we log an error and throw it again? Wouldn't this lead to double logging?

As you have seen in a previous comment, the idea was to log the error, disconnect the API and then throw the error, and, if for some reason, there was a problem during the disconnection of the api, this new error would not overshadow the other error.

mutantcornholio · 2023-09-14T07:51:30Z

src/polkadot/fellows.ts

+      const users: Map<string, number> = new Map<string, number>();
+      for (const fellow of fellows) {
+        this.logger.debug(`Fetching identity of '${fellow.address}', rank: ${fellow.rank}`);
+        const fellowData = (await api.query.identity.identityOf(fellow.address)).toHuman() as


I feel like this deserves a semaphore, so it could be queried in parallel.

Do you know if there is a limit to stop abuse? That was my main concern.

The current execution takes about 2s, which given the fact that this is an action, is not really a problem (the whole action is executed in about 5s).

Do you know if there is a limit to stop abuse? That was my main concern.

Could you elaborate?

Same problem that we have with GitHub, which has a rate limit.

I assume that there is some kind of rate limit when using Polkadot-JS (else what stop people from DOSing it?), and, because I'm fetching 60 accounts, I found it better to do it in order.

Maybe I'm not understanding you. Could you please elaborate on what you had in mind?

I think we're speaking of the same thing here. I suggest adding a semaphore, so requests would be processed in parallel, but with a limit on how much parallel requests to make.

In gitspiegel, I've used semaphore-promise library for that.
Check out the usage in core.ts

Co-authored-by: Przemek Rzad <roopert7@gmail.com>

Bullrich · 2023-09-14T13:15:52Z

Looks good, just one thing: do you have plans for invalidating the cache of fellows?

No, as the cache is only generated during the instance and not stored long term. The same as with the GitHub reviews, the cache is only used if we need to fetch the same information during the same execution (that lasts ~5 seconds).

So if rule 1 request the teams, we fetch them and cache them, and if rule 2 requests it, we already have them. But once the execution is over the cache disappears with it.

README.md

mordamax · 2023-09-15T15:01:24Z

src/test/fellows.test.ts

+    mockClear(logger);
+    const members2 = await fellows.getTeamMembers("2");
+    expect(members2.length).toBeGreaterThan(0);
+    expect(logger.debug).not.toHaveBeenCalledWith("Connecting to collective parachain");


It actually runs out of the box, so that's good!

where? I haven't seen in this diff changes where the rank: 2 approvals are being tested, could you please point my eyes?

FYI: if there's no test, not a blocker for this PR, just needs to be done now or later

Discussed about the Semaphore and decided to not go with it for this particular case

Bullrich added 6 commits September 12, 2023 13:26

created base class to fetch team members

abf4f78

added team fetching object

4cbeed7

created code to get users from fellowship

b37b39a

finished testing of polkadot teams feature

9a83f11

updated fellow name

93fe9cd

updated readme

b0db91e

Bullrich self-assigned this Sep 13, 2023

Bullrich mentioned this pull request Sep 13, 2023

Merge/Approval rights for Fellows polkadot-fellows/runtimes#7

Closed

added missing dependency

7b7d42c

Bullrich requested review from mutantcornholio and mordamax September 13, 2023 19:18

Bullrich marked this pull request as ready for review September 13, 2023 19:18

Bullrich requested a review from a team as a code owner September 13, 2023 19:18

mordamax reviewed Sep 13, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

mordamax reviewed Sep 13, 2023

View reviewed changes

Bullrich added 5 commits September 13, 2023 17:12

added validation tests

3e2b2e6

converted log to debug

7fef023

fixed location of "rank" field in readme

d9615f9

added test to skip check if author belongs to rank

7104f3a

fixed linting issue

2a53ed8

Bullrich requested a review from mordamax September 13, 2023 20:41

rzadp reviewed Sep 14, 2023

View reviewed changes

src/polkadot/fellows.ts Outdated Show resolved Hide resolved

rzadp reviewed Sep 14, 2023

View reviewed changes

src/polkadot/fellows.ts Outdated Show resolved Hide resolved

rzadp approved these changes Sep 14, 2023

View reviewed changes

mutantcornholio previously requested changes Sep 14, 2023

View reviewed changes

Bullrich and others added 2 commits September 14, 2023 10:11

Fixed log

aae0bf2

Co-authored-by: Przemek Rzad <roopert7@gmail.com>

Used join in string array

7b25ca7

Co-authored-by: Przemek Rzad <roopert7@gmail.com>

moved the disconnect to the finally block

a52d4f7

Bullrich requested a review from mutantcornholio September 14, 2023 13:30

fixed linting problems

1ce41b9

mordamax reviewed Sep 15, 2023

View reviewed changes

renamed rank to minFellowsRank

f1b7f9b

Bullrich requested a review from mordamax September 15, 2023 16:27

added test to verify the fellows rank

9f48f0a

mordamax approved these changes Sep 15, 2023

View reviewed changes

Bullrich mentioned this pull request Sep 15, 2023

Add continuos testing with the new rank feature #81

Closed

Bullrich merged commit 29b16b5 into main Sep 15, 2023
13 checks passed

Bullrich deleted the fellow-team-fetching branch September 15, 2023 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fellow members fetching #79

Fellow members fetching #79

Bullrich commented Sep 13, 2023 •

edited

Loading

mordamax Sep 13, 2023

Bullrich Sep 13, 2023

mordamax Sep 13, 2023 •

edited

Loading

Bullrich Sep 13, 2023

Bullrich Sep 13, 2023

mordamax Sep 15, 2023 •

edited

Loading

Bullrich Sep 15, 2023

mordamax Sep 15, 2023 •

edited

Loading

rzadp left a comment

mutantcornholio Sep 14, 2023

Bullrich Sep 14, 2023

mutantcornholio Sep 14, 2023

Bullrich Sep 14, 2023 •

edited

Loading

mutantcornholio Sep 14, 2023

Bullrich Sep 14, 2023

mutantcornholio Sep 15, 2023

Bullrich Sep 15, 2023

Bullrich Sep 15, 2023

mutantcornholio Sep 15, 2023

Bullrich commented Sep 14, 2023 •

edited

Loading

mordamax Sep 15, 2023 •

edited

Loading

		const [address]: [string] = key.toHuman();
		fellows.push({ address, ...(rank.toHuman() as object) } as FellowData);

Fellow members fetching #79

Fellow members fetching #79

Conversation

Bullrich commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mordamax Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mordamax Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mordamax Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

rzadp 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

Bullrich Sep 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

Bullrich commented Sep 14, 2023 • edited Loading

mordamax Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Bullrich commented Sep 13, 2023 •

edited

Loading

mordamax Sep 13, 2023 •

edited

Loading

mordamax Sep 15, 2023 •

edited

Loading

mordamax Sep 15, 2023 •

edited

Loading

Bullrich Sep 14, 2023 •

edited

Loading

Bullrich commented Sep 14, 2023 •

edited

Loading

mordamax Sep 15, 2023 •

edited

Loading