Skip to content
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

Handle Permissions Events #60

Merged
merged 27 commits into from
Jun 5, 2023
Merged

Handle Permissions Events #60

merged 27 commits into from
Jun 5, 2023

Conversation

rdig
Copy link
Member

@rdig rdig commented Apr 24, 2023

This PR adds in logic to handle listening to permissions role events, create database entries for them, create historic entries, and finally create permissions actions.

To test this head on over to the CDapp and test the JoinColony/colonyCDapp#271

Some of the logic required gets pretty harry, and so I've left a bunch of comments all over the place. Please make sure to give them a read, to understand the flow of data through the ingestor.

@rdig rdig requested a review from a team April 24, 2023 05:03
@rdig rdig self-assigned this Apr 24, 2023
rdig added a commit to JoinColony/colonyCDapp that referenced this pull request Apr 24, 2023
@rdig rdig force-pushed the feature/handle-permissions-events branch from f412c1c to 6e27254 Compare April 24, 2023 05:06
rdig added a commit to JoinColony/colonyCDapp that referenced this pull request Apr 24, 2023
Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I can tell, it looks good 👍

src/handlers/actions/managePermissions.ts Outdated Show resolved Hide resolved
src/handlers/actions/managePermissions.ts Outdated Show resolved Hide resolved
src/utils/permissions.ts Outdated Show resolved Hide resolved
src/utils/permissions.ts Outdated Show resolved Hide resolved
src/utils/permissions.ts Outdated Show resolved Hide resolved
src/utils/permissions.ts Outdated Show resolved Hide resolved
src/utils/permissions.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@willm30 willm30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main question is what is the advantage of processing all RoleSet events in a given block at once, instead of processing them individually? I feel like the batch processing approach adds some complexity compared to making db mutatations on a per event basis (assuming we can link ColonyRoleSet events to their respective ColonyRole and ColonyAction db entries)

rdig added a commit to JoinColony/colonyCDapp that referenced this pull request May 2, 2023
@rdig rdig force-pushed the feature/handle-permissions-events branch from a11e110 to 27085f2 Compare May 18, 2023 16:00
jakubcolony pushed a commit to JoinColony/colonyCDapp that referenced this pull request May 23, 2023
rdig added a commit to JoinColony/colonyCDapp that referenced this pull request May 23, 2023
@rdig rdig requested a review from a team May 23, 2023 23:05
@rdig
Copy link
Member Author

rdig commented May 23, 2023

As with it's CDapp counterpart, this has been rebased, tested, and it's ready for re-review/merge.

As with that one, the more time consuming changes that were pointed out were extracted into a separate issue: #79

This was done in the interest of time, as to get this merged faster, and push forward towards production deployment.

Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple comments

src/handlers/actions/managePermissions.ts Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
src/trackExtensions.ts Show resolved Hide resolved
src/utils/colonyClient.ts Show resolved Hide resolved
@rdig rdig force-pushed the feature/handle-permissions-events branch from 20ea486 to da0b79a Compare May 31, 2023 02:37
@rdig rdig force-pushed the feature/handle-permissions-events branch from da0b79a to 66016f9 Compare May 31, 2023 02:44
jakubcolony pushed a commit to JoinColony/colonyCDapp that referenced this pull request May 31, 2023
rdig added a commit to JoinColony/colonyCDapp that referenced this pull request Jun 1, 2023
Copy link
Contributor

@willm30 willm30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Collaborator

@jakubcolony jakubcolony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Dockerfile Outdated Show resolved Hide resolved
@rdig rdig force-pushed the feature/handle-permissions-events branch 2 times, most recently from 2bead94 to 8c80a84 Compare June 5, 2023 13:23
@rdig rdig merged commit 57d2a0d into master Jun 5, 2023
1 check passed
@rdig rdig deleted the feature/handle-permissions-events branch June 5, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants