Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fellow members fetching #79
Fellow members fetching #79
Changes from 7 commits
abf4f78
4cbeed7
b37b39a
9a83f11
93fe9cd
b0db91e
7b7d42c
3e2b2e6
7fef023
d9615f9
7104f3a
2a53ed8
aae0bf2
7b25ca7
a52d4f7
1ce41b9
f1b7f9b
9f48f0a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, is it basically this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, andrank
is{rank: 1}
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this deserves a semaphore, so it could be queried in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm not understanding you. Could you please elaborate on what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why do we log an error and throw it again? Wouldn't this lead to double logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I didn't plan on adding it, but maybe it's no trouble at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually runs out of the box, so that's good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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