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

feat: p/zteams implementation #2830

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

irreverentsimplicity
Copy link
Contributor

@irreverentsimplicity irreverentsimplicity commented Sep 22, 2024

An alternative team package implementation. Primarily inspired by building the ZenTasktic framework.

@irreverentsimplicity irreverentsimplicity requested review from ajnavarro and zivkovicmilos and removed request for a team September 22, 2024 06:18
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Sep 22, 2024
Copy link

codecov bot commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.11%. Comparing base (a73cb22) to head (50a8062).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2830   +/-   ##
=======================================
  Coverage   61.10%   61.11%           
=======================================
  Files         564      564           
  Lines       75355    75355           
=======================================
+ Hits        46045    46050    +5     
+ Misses      25945    25943    -2     
+ Partials     3365     3362    -3     
Flag Coverage Δ
contribs/gnodev 61.46% <ø> (ø)
contribs/gnofaucet 14.46% <ø> (-0.86%) ⬇️
gno.land 67.92% <ø> (ø)
gnovm 66.17% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (-0.36%) ⬇️
tm2 62.11% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul
Copy link
Member

moul commented Sep 22, 2024

Thank you, Dragos.

Please review issue #2195. The primary goal is to create the sys/teams realm, which will allow admins to configure their teams either statically or dynamically, based on their preferences.

The p/teams implementation cannot be singular. Instead, we should anticipate multiple opinionated "team management frameworks," including yours. However, it is currently too early to focus on this aspect.

Feel free to continue working on this version if you wish, but we cannot review it until we complete at least the interfaces for r/sys/teams. If you choose to keep working on this, please rename it to reflect that it is "one opinionated team management framework that will implement the official interface." This way, users can choose between your framework, others, creating their own, or managing members statically.

I’m converting your PR to draft for now. However, feel free to mark it as ready for review if you believe it would be beneficial for the project to be reviewed again.

@moul moul marked this pull request as draft September 22, 2024 07:46
@irreverentsimplicity
Copy link
Contributor Author

irreverentsimplicity commented Sep 22, 2024

Thank you, Manfred

I reviewed the #2195 issue and I understand now the goal of creating sys/teams and I fully agree with the approach.

However, I think going forward with my implementation can have some benefits. The main reason behind making such a package in the first place was my work on ZenTasktic, specifically the zentasktic_project version: placing teams logic outside the ZenTasktic package may create a more flexible realm, and also provide users with an alternative, opinionated teams implementation.

I'm thinking about renaming this as p/zteams to make it clear that:

  • this is not the official Gno teams package
  • this comes from ZenTasktic project

I'm also thinking about getting read of the CanDeploy permission, as this is not needed in ZenTasktic (I added it because it was part of the discussions we had in the dev call, but now I see this is easily doable by implementing #2195).

Instead, I'll add a CanDisburse permission, which may allow a team member to disburse funds (or task rewards, in ZenTasktic).

I'll mark it as ready for review when this is done. Hope this is ok.

@irreverentsimplicity irreverentsimplicity changed the title p/teams implementation feat: p/zteams implementation Sep 22, 2024
@irreverentsimplicity
Copy link
Contributor Author

I'm asking for a re-review of this because I'm working on a zentasktic_project implementation which uses this package for team & user management. I already finished it on my local dev env, and I would like to move on with deploying on the public testnet.

Thank you!

@irreverentsimplicity irreverentsimplicity marked this pull request as ready for review October 1, 2024 05:12
@leohhhn leohhhn requested review from moul and leohhhn and removed request for a team October 2, 2024 10:41
@Kouteki Kouteki added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 3, 2024
Copy link
Member

@notJoon notJoon left a comment

Choose a reason for hiding this comment

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

Most of it looks good, but I've left a few suggestions for improvement. Please review and address them. Thank you!

examples/gno.land/p/demo/zteams/errors.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/zteams/gno.mod Show resolved Hide resolved
examples/gno.land/p/demo/zteams/teams.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/zteams/teams.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/zteams/teams_test.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/zteams/teams.gno Outdated Show resolved Hide resolved
@notJoon
Copy link
Member

notJoon commented Oct 8, 2024

Everything looks good to me. For the finishing, could you run fmt again? The action is showing as failed. thank you

@irreverentsimplicity
Copy link
Contributor Author

Everything looks good to me. For the finishing, could you run fmt again? The action is showing as failed. thank you

Done, looks like tests are passing.

@jefft0 jefft0 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 17, 2024
@jefft0
Copy link
Contributor

jefft0 commented Oct 17, 2024

Removed the "review/triage-pending" label because notJoon approved and this is ready for core dev review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants