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

Add optional delay to AccessControl role grants #1317

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

immrsd
Copy link
Collaborator

@immrsd immrsd commented Jan 28, 2025

Fixes #1261
Continues the work started in #1262

This implementation is backward compatible. Contracts using the existing AccessControl component should be able to upgrade to this implementation without breaking the storage or the logic since they will have effective_from set as 0 by default for existing roles, and the AccountRoleInfo active member layout is compatible with the current boolean.

NOTE: This is a POC, so it lacks tests and documentation yet. MUST NOT be used as it is until is finished.

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

I left a small suggestion besides the missing tests that still need to be added.

packages/access/src/accesscontrol/accesscontrol.cairo Outdated Show resolved Hide resolved
Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

So far, so good @immrsd! I left a few minor comments

packages/access/src/accesscontrol/accesscontrol.cairo Outdated Show resolved Hide resolved
Comment on lines 18 to 21
#[starknet::interface]
pub trait IAccessControlWithDelay<TState> {
fn grant_role_with_delay(ref self: TState, role: felt252, account: ContractAddress, delay: u64);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also add this to the ABI, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I wonder if there's some value in having a new src5 id for this. I really don't think it's worth it, but I'm asking out loud just in case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Included this function and also new get_role_status function to the ABI
  2. I don't think it's worth it, especially considering difficulties with registering the additional interface ID (we gonna need two different initializers and also solve the problem of registering new interface ID for an upgraded contract)

packages/access/src/accesscontrol/account_role_info.cairo Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.59%. Comparing base (0676415) to head (656831a).
Report is 40 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1317      +/-   ##
==========================================
- Coverage   92.26%   89.59%   -2.68%     
==========================================
  Files          59       83      +24     
  Lines        1811     3557    +1746     
==========================================
+ Hits         1671     3187    +1516     
- Misses        140      370     +230     
Files with missing lines Coverage Δ
...kages/access/src/accesscontrol/accesscontrol.cairo 100.00% <100.00%> (+26.66%) ⬆️
...s/access/src/accesscontrol/account_role_info.cairo 100.00% <100.00%> (ø)
packages/access/src/accesscontrol/interface.cairo 100.00% <100.00%> (ø)

... and 80 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5301e7...656831a. Read the comment docs.

@immrsd
Copy link
Collaborator Author

immrsd commented Feb 13, 2025

Notable changes since the last update:

  1. Introduced RoleStatus enum with 3 invariants: NotGranted, Delayed(u64), Effective

  2. Added get_role_status function to AccessControlWithDelay

  3. Made IAccessControlWithDelay trait (get_role_status + grant_role_with_delay fns) a part of AccessControlMixin

  4. Modified the behaviour of grant_role and grant_role_with_delay fns. Previously, the functions did nothing if the role had already been granted. It could lead to unexpected behaviour. For example, if I execute grant_role successfully, I expect the role to be effective for the account immediately, but instead it will stay in the delayed state (if grant_role_with_delay was called earlier and delay hasn't passed yet).

Now, the functions work in such way that the state after the call is consistent with the last executed call:

  • If a role is delayed, but then grant_role is called, the role becomes effective immediately
  • If a role is already effective, grant_role does nothing, because the current state is equal to the one expected after the call
  • If a role is already effective and grant_role_with_delay is called, the component panics (the current state and the expected one conflict with one another, so it's admin's responsibility to resolve the conflict)
  • If a role is in Delayed state and another grant_role_with_delay call happens, the delay is overwritten

cc @ericnordelo @andrew-fleming

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

Amazing job @immrsd. Left some small comments, but the PR looks great. Let's not forget to add the corresponding entries to the doc.

packages/access/src/accesscontrol/accesscontrol.cairo Outdated Show resolved Hide resolved
packages/access/src/accesscontrol/accesscontrol.cairo Outdated Show resolved Hide resolved
packages/access/src/accesscontrol/accesscontrol.cairo Outdated Show resolved Hide resolved
packages/access/src/accesscontrol/interface.cairo Outdated Show resolved Hide resolved
packages/access/src/accesscontrol/accesscontrol.cairo Outdated Show resolved Hide resolved
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.

Add delay to role administration operations in AccessControl
3 participants