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: use context-based role assignments to implement the permission model #19

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

hiddentao
Copy link
Contributor

@hiddentao hiddentao commented Feb 23, 2025

Notes:

  • All new ACL that allows for context-based role setting and other fun features. Fully tested!
  • The transfer lock is complex to do using roles because it would require going into the ERC1155 too to handle approvals etc. So I opted for a flag instead for that one.

@hiddentao hiddentao marked this pull request as draft February 23, 2025 08:09
@coveralls
Copy link

coveralls commented Feb 23, 2025

Pull Request Test Coverage Report for Build 13712517344

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 94 of 180 (52.22%) changed or added relevant lines in 8 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-3.2%) to 54.758%

Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/src/registry/RootRegistry.sol 2 3 66.67%
contracts/src/registry/PermissionedRegistry.sol 2 4 50.0%
contracts/src/registry/NameWrapperRegistry.sol 0 83 0.0%
Files with Coverage Reduction New Missed Lines %
contracts/src/registry/RootRegistry.sol 1 75.0%
contracts/src/registry/PermissionedRegistry.sol 3 73.68%
Totals Coverage Status
Change from base Build 13351832110: -3.2%
Covered Lines: 278
Relevant Lines: 495

💛 - Coveralls

@hiddentao hiddentao requested a review from Arachnid February 23, 2025 09:24
*
* See v1 fuse: CAN_EXTEND_EXPIRY
*/
function allowOwnerToRenew(uint256 tokenId) public onlyRole(_tokenIdContext(tokenId), EMANCIPATOR_ROLE) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that if you emancipate a name, it's no longer possible to grant the owner permission to renew it?

Copy link
Contributor Author

@hiddentao hiddentao Feb 25, 2025

Choose a reason for hiding this comment

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

Yes, because in the name wrapper, once you emancipate you can no longer burn any other fuses on it. That was my interpretation.

Copy link
Member

Choose a reason for hiding this comment

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

The parent owner cannot - but if I'm reading this correctly, the emancipated owner cannot renew either!

@hiddentao hiddentao requested a review from Arachnid February 26, 2025 11:19
feat: add _transferRole method
@hiddentao
Copy link
Contributor Author

hiddentao commented Feb 28, 2025

@Arachnid Given that we only need to transfer two roles to a new registrar (registrar and renew) I felt that it was easier to do that manually whilst keeping the current storage approach in EnhancedAccessControl, i.e. not using a bitmap. Even if we use a Bitmap then keeping the "revoke all assignments of this role within this resource" O(1) feature would still require versioning of assignments, meaning that we'd still be doing a lot of complex storage work in access control.

Plus, role transfers only work across a particular context. There's no way to transfer all of a user's roles in all contexts. I suppose if we implement hierarchical contexts of some sort we could perhaps do this. But again, this complicates the access control storage.

Happy to discuss this further over calls.

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.

3 participants