feat(examples): finalize acl package #2987

0xtekgrinder · 2024-10-20T09:48:41Z

This PR adds missing functionalities from the acl package:

helpers to remove permissions.
helpers to adds multiple permissions at once -> {verbs: []string{"read","write"}}.
helpers to delete users from groups.
helpers to quickly reset states.

Everything can be tested by running gno test examples/gno.land/p/demo/acl

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests

notJoon

I left some comments. Please check and address them. thank you!

examples/gno.land/p/demo/acl/acl.gno

Gno2D2 · 2025-01-08T16:57:04Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)
The pull request description provides enough details

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
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:

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
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: 0xtekgrinder/gno)

Then

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

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🔴 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

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
The pull request description provides enough details
If
🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])
Can be checked by

team core-contributors

moul · 2025-02-24T09:27:25Z

examples/gno.land/p/demo/acl/acl.gno

 	bucket := "u:" + addr.String()
 	p := perm{
 		verbs:     []string{verb},
 		resources: []string{resource},
 	}
-	d.addPermToBucket(bucket, p)
+	d.addPermsToBucket(bucket, []perm{p})
+	return true


useless returned value

moul · 2025-02-24T09:28:03Z

examples/gno.land/p/demo/acl/acl.gno

 }

-func (d *Directory) AddGroupPerm(name string, verb, resource string) {
+func (d *Directory) AddUserPerms(addr std.Address, verbs []string, resource string) bool {


this helper should take multiple verbs AND resources, or multiple structs, not just multiple verbs.

moul · 2025-02-24T09:28:38Z

examples/gno.land/p/demo/acl/acl.gno

+func (d *Directory) ResetUserPerms(addr std.Address) bool {
+	bucket := "u:" + addr.String()
+	d.permBuckets.Remove(bucket)
+	return true


useless returned value

moul · 2025-02-24T09:29:38Z

examples/gno.land/p/demo/acl/acl.gno

+	return true
+}
+
+func (d *Directory) ResetUserGroups(addr std.Address) bool {


invalid function name, the code isn't resetting a user group,but removing a specific address from a group

moul · 2025-02-24T09:31:14Z

examples/gno.land/p/demo/acl/perms.gno

+	for _, pd := range newPerms {
+		if !p.Contains(pd) {


gas inefficient

moul · 2025-02-24T09:35:28Z

examples/gno.land/p/demo/acl/groups.gno

+package acl
+
+// Helper function to append without duplicates
+func appendGroupsWithoutDuplicates(slice []string, items []string) []string {


this is a generic strings helper, do not create a groups.gno file with this function containing group in the title as it's generic enough to keep the code lighter.

moul · 2025-02-24T09:36:01Z

examples/gno.land/p/demo/acl/acl.gno

+	return d.removePermsFromBucket(bucket, []perm{p})
+}
+
+func (d *Directory) RemoveGroupPerms(name string, verbs []string, resource string) bool {


same as https://github.com/gnolang/gno/pull/2987/files#r1967273393

moul · 2025-02-24T09:37:35Z

examples/gno.land/p/demo/acl/acl.gno

+	return foundGroups == len(groups)
+}
+
+func (d *Directory) removePermsFromBucket(bucket string, p []perm) bool {


we should switch to a map or a tree instead of a slice.
most of the code is hard to read and gas inefficient. let's just change how we store the data.

edit: let's use a tree directly.

moul · 2025-02-24T09:39:20Z

examples/gno.land/p/demo/acl/acl.gno

+	return true
+}
+
+func (d *Directory) RemoveUserFromGroups(user std.Address, groups []string) bool {


remove this helper, just call RemoveUserFromGroup several times.

moul

Improving this package is a worthwhile endeavor. However, the current implementation is difficult to read, gas-inefficient, not scalable, and the API does not seem optimized. New helpers appear to lack practical use cases, while other important helpers are missing.

If you want to enhance this package by adding more flexibility (thank you), please start by refactoring our data storage. I suggest using the recent libraries I added:

https://gno.land/p/moul/addreset for lists of addresses
https://gno.land/p/moul/ulist for sets

Alternatively, feel free to fork this ACL package under your username if you believe your API could better serve some users.

0xtekgrinder added 5 commits October 18, 2024 20:25

feat: new functions inside acl

b01d212

tests: add start of acl new tests

7aefac4

feat: optimize acl functions

3a803ee

feat: reset functions for acl

ba16572

tests: extra acl tests for new functions

5765f49

0xtekgrinder requested review from a team as code owners October 20, 2024 09:48

0xtekgrinder requested review from sw360cab and petar-dambovaliev and removed request for a team October 20, 2024 09:48

github-actions bot assigned 0xtekgrinder Oct 20, 2024

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 20, 2024

This was referenced Oct 20, 2024

(gnovm) Fmt function in testing environment crash the whole function #2988

Closed

0xtekgrinder Builder Journey gnolang/hackerspace#87

Open

notJoon added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 21, 2024

notJoon suggested changes Oct 21, 2024

View reviewed changes

examples/gno.land/p/demo/acl/acl.gno Outdated Show resolved Hide resolved

examples/gno.land/p/demo/acl/acl.gno Outdated Show resolved Hide resolved

0xtekgrinder added 7 commits October 23, 2024 11:39

feat: handle duplicates for perms

bec2056

feat: simplify handling of duplicates perms

f547dd0

feat: duplicate tests

a50c56b

feat: handle duplicates for users

efd0359

feat: remove entries in the tree whenever possible

8d52be7

feat: remove tree entries whenever possible

fa86ffc

style: format realms

8bae643

0xtekgrinder requested a review from notJoon October 23, 2024 10:37

omarsy reviewed Oct 27, 2024

View reviewed changes

examples/gno.land/p/demo/acl/acl.gno Outdated Show resolved Hide resolved

feat: return boolean on success of failure for all public functions

ad86820

0xtekgrinder requested a review from omarsy January 8, 2025 16:56

leohhhn self-requested a review January 15, 2025 12:09

moul reviewed Feb 24, 2025

View reviewed changes

Gno2D2 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Feb 24, 2025

moul reviewed Feb 24, 2025

View reviewed changes

examples/gno.land/p/demo/acl/perms.gno

Comment on lines +24 to +25

for _, pd := range newPerms {

if !p.Contains(pd) {

Copy link

Member

moul Feb 24, 2025

Choose a reason for hiding this comment

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

gas inefficient

moul reviewed Feb 24, 2025

View reviewed changes

moul requested changes Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(examples): finalize acl package #2987

feat(examples): finalize acl package #2987

0xtekgrinder commented Oct 20, 2024

notJoon left a comment

Gno2D2 commented Jan 8, 2025 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

If

Can be checked by

moul Feb 24, 2025

moul Feb 24, 2025

moul Feb 24, 2025

moul Feb 24, 2025

moul Feb 24, 2025

moul Feb 24, 2025

moul Feb 24, 2025

moul Feb 24, 2025 •

edited

Loading

moul Feb 24, 2025

moul left a comment

feat(examples): finalize acl package #2987

Are you sure you want to change the base?

feat(examples): finalize acl package #2987

Conversation

0xtekgrinder commented Oct 20, 2024

notJoon left a comment

Choose a reason for hiding this comment

Gno2D2 commented Jan 8, 2025 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

If

Can be checked by

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

moul Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moul left a comment

Choose a reason for hiding this comment

Gno2D2 commented Jan 8, 2025 •

edited

Loading

moul Feb 24, 2025 •

edited

Loading