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

wip: implements r/sys/teams #3568

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Jan 20, 2025

WIP

Address #2195

This PR introduces a potential implementation of the r/sys/teams package for managing team memberships and permissions through an access-controlled lifecycle management system. The package uses encapsulated tasks to handle state updates, ensuring both permission and flexibility in team operations.

Currently, the implementation needs more fine-tuning, but the core idea is present.

TODO:

  • Validate implementation
  • Improve default behaviours and enhance API usability
  • Add unit test
  • Bring more usage example
  • Render method
  • Members Iteration and list
  • ...

Signed-off-by: gfanton [email protected]

Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
@gfanton gfanton self-assigned this Jan 20, 2025
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jan 20, 2025
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Jan 20, 2025

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: gfanton/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

Signed-off-by: gfanton <[email protected]>

// Check if caller is not already registered as a team
if teams.Has(caller.String()) {
panic("team already registered: " + caller.String())
Copy link
Member

Choose a reason for hiding this comment

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

it's okay to have this condition, but then we need a Replace(newTeam ITeam) companion func.

// ITeam combines the AccessController and Lifecycle interfaces to define a complete
// team interface. It ensures that a team has both access control and lifecycle
// management capabilities.
type ITeam interface {
Copy link
Member

Choose a reason for hiding this comment

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

need another name, because of the type Team struct which isn't related.

Copy link
Member

Choose a reason for hiding this comment

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

why 3 interfaces instead of 1?


// reHomeRealm validate thehome realm path format `xxx.xx/r/<myteam>/home`.
// XXX: Use somthing simpler
var reHomeRealm = regexp.MustCompile(`^([a-zA-Z0-9-]+\.)*[a-zA-Z0-9-]+\.[a-zA-Z]{2,}/r/[a-z0-9_]+/home$`)
Copy link
Member

Choose a reason for hiding this comment

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

FYI, I'm not opposed to reimplementing the logic in this area, but take a look at this library: https://github.com/gnolang/gno/blob/master/examples/gno.land/p/moul/realmpath/realmpath.gno.

I believe that ensuring full security also requires verifying that the domain is from the same chain due to IBC.

Comment on lines +100 to +101
Lifecycle: iteam,
AccessController: iteam,
Copy link
Member

Choose a reason for hiding this comment

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

really looks like a single interface would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I wanted some modularity and a straightforward way to update the access controller without touching the lifecycle part. However, upon further reflection, I think you right, enforcing the whole ITeam update is simpler for both implementation and the client. I will merge this into a single interface.

AccessController: iteam,
}
// Assert that team implementation correctly uses fallback
// XXX: do we want this?
Copy link
Member

Choose a reason for hiding this comment

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

No, we don't want this. However, it's acceptable to have it if creating a "Noop" command is straightforward. Consider that you need to define an interface that accommodates most of your workflows. Additionally, some teams may prefer to remain static, with no option for updates.

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

I need to read it again later today if possible. However, I think the direction looks good, but it mixes independent topics that should exist separately:

  1. The r/sys/teams registration system, which is primarily a smart map of interfaces.
  2. The {p,r}/guigui/generic_team, an opinionated framework for managing teams that is compatible with r/sys/teams.

I also believe that due to the upcoming inter-realm ownership constraints and our desire for the /home to be an upgradeable contract (not importable and unable to store local items on other realms), we will need a standalone registry.

In summary, I anticipate two main usages:

  1. func init() { teams.ConfigureWithPrimitiveTypes(std.Address, string, bool) } registers or overrides using only primitive types that can be stored by r/sys/teams. This approach should provide very simple team support without the ability to "add" members, as the only way to do so is by calling "Configure" repeatedly.
  2. team := teamregistry.New(guiguiteam.New()); teams.RegisterInstance(team)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

3 participants