[Cairo] Support AccessControlDefaultAdminRules #698
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors the access control system from string-based values to an enum-based model with support for Default Admin Rules (DAR). Core changes include introducing Changes
Sequence Diagram(s)sequenceDiagram
participant UI as AccessControlSection
participant Core as set-access-control
participant Builder as ContractBuilder
UI->>UI: User selects accessType
activate UI
Note over UI: accessType: 'ownable' | 'roles' | 'roles-dar' | false
UI->>UI: If roles-dar: show darInitialDelay, darDefaultDelayIncrease
deactivate UI
Core->>Core: setAccessControl(contract, accessObj)
activate Core
alt accessType === 'roles-dar'
Core->>Builder: Add AccessControlDefaultAdminRulesComponent
Core->>Builder: Configure INITIAL_DELAY
Core->>Builder: Configure DEFAULT_ADMIN_DELAY_INCREASE_WAIT
Core->>Builder: Add DAR-based constructor params
else accessType === 'roles'
Core->>Builder: Add AccessControlComponent
Core->>Builder: Grant roles (PAUSER, MINTER, UPGRADER)
else accessType === 'ownable'
Core->>Builder: Add OwnableComponent
else accessType === false
Core->>Builder: No access control
end
deactivate Core
Builder->>Builder: Generate Cairo contract
Note over Builder: With appropriate component<br/>initialization and role grants
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: The refactoring spans multiple domains (core logic, tests, UI) with heterogeneous changes—from fundamental type system updates in Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
packages/core/cairo_alpha/CHANGELOG.md (1)
5-6: Remove extra blank line for consistency.The changelog entry is properly formatted, but there's an extra blank line after it (line 6) that's inconsistent with other entries in the Unreleased section. Based on learnings.
Apply this diff:
- Add AccessControlDefaultAdminRules. ([#698](https://github.com/OpenZeppelin/contracts-wizard/pull/698)) - - **Breaking changes**:packages/core/cairo_alpha/src/erc20.test.ts.md (1)
452-453: Consider addressing hard tabs in snapshot file.The markdownlint static analysis tool reports hard tabs at several locations in this snapshot file. While this is a test snapshot file and may be auto-generated, consistent use of spaces would align with standard formatting practices.
Also applies to: 1236-1237, 1242-1242, 1345-1345, 2262-2263
packages/core/cairo_alpha/src/multisig.ts (1)
49-50: Explicitly handletype: falseinsetAccessControlto future‑proof multisig.Passing
AccessControl.None(i.e.,type: false) relies onsetAccessControldoing nothing for that case. Today the switch has nofalsebranch; consider adding one to make intent clear and resilient if a default branch is added later.For example in
packages/core/cairo_alpha/src/set-access-control.ts:switch (access.type) { case 'ownable': { /* ... */ break; } case 'roles': { /* ... */ break; } case 'roles-dar': { /* ... */ break; } case false: { // No access-control component for multisig-governed contracts. break; } // Optionally: add a `default` with `never` check to keep exhaustiveness tight. }Please confirm this is the intended behavior for multisig (no ACL components).
packages/core/cairo_alpha/src/erc20.test.ts (1)
6-6: Nit: unify naming —darCustomOpsvsdarDefaultOpts.Consider renaming
darCustomOpstodarCustomOptsfor consistency with “Opts”.packages/core/cairo_alpha/src/custom.test.ts.md (1)
395-396: markdownlint MD010: hard tabs in snapshot.Snapshots include tabs (tooling output). Prefer excluding snapshots from MD lint, or disable MD010 locally around the code block to avoid churn.
Example options:
- Add this file to
.markdownlintignore, or- Surround the block with
<!-- markdownlint-disable MD010 -->/<!-- markdownlint-enable MD010 -->.packages/ui/src/cairo_alpha/ERC1155Controls.svelte (1)
80-86: Verify two‑way binding reactivity for nestedopts.access.*.
requireAccessControldepends onopts; ensure changes fromAccessControlSectioninvalidateoptsso the reactive statement recalculates.You can quickly check in the UI: toggle access type/DAR delays and confirm the “required” state updates immediately. If not, reassign
opts = { ...opts }in the binding handlers or have the child emit an updatedaccessobject to assign toopts.access.packages/core/cairo_alpha/src/set-royalty-info.ts (1)
56-66: Access handling: copy and defaulting — sensible.
- Copying
accessObjavoids mutating caller state.- Defaulting
type: falsetoDEFAULT_ACCESS_CONTROL(“ownable”) ensures royalty admin paths are protected.Comment nit: replace “avoid triggering UI updates” with “avoid mutating caller-supplied object” to keep UI concerns out of core.
packages/core/cairo_alpha/src/custom.test.ts (1)
7-7: Rename for consistency:darCustomOps→darCustomOpts.
darDefaultOptsuses “Opts”; consider aligning the custom variant too. Add a back-compat alias if public.packages/ui/src/cairo_alpha/AccessControlSection.svelte (1)
67-83: Optional: add UX hints for duration inputs.Consider placeholders like “e.g., 1 day” / “e.g., 1 week”, or a
titlewith acceptable formats to reduce validation round-trips.packages/core/cairo_alpha/src/set-access-control.ts (2)
26-29: Align naming:darCustomOps→darCustomOpts(+ temporary alias).Keep API consistent with
darDefaultOpts. Provide a back-compat alias to avoid breaking tests/consumers.-export const darCustomOps: RolesDefaultAdminRulesOptions = { +export const darCustomOpts: RolesDefaultAdminRulesOptions = { darInitialDelay: '2 days', darDefaultDelayIncrease: '1 week', }; +// Back-compat alias (deprecate next major) +export const darCustomOps = darCustomOpts; - AccessControl.RolesDefaultAdminRules(darCustomOps), + AccessControl.RolesDefaultAdminRules(darCustomOpts),Also applies to: 46-47
49-49: Prefer BigInt literals and strict equality.Use BigInt literals and
===for clarity and type safety.-const DEFAULT_ADMIN_DELAY_INCREASE_WAIT = BigInt(5 * 24 * 60 * 60); // 5 days +const DEFAULT_ADMIN_DELAY_INCREASE_WAIT = 5n * 24n * 60n * 60n; // 5 days -if (defaultAdminDelayIncreaseWait == DEFAULT_ADMIN_DELAY_INCREASE_WAIT) { +if (defaultAdminDelayIncreaseWait === DEFAULT_ADMIN_DELAY_INCREASE_WAIT) {Also applies to: 136-136
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
packages/core/cairo_alpha/src/custom.test.ts.snapis excluded by!**/*.snappackages/core/cairo_alpha/src/erc1155.test.ts.snapis excluded by!**/*.snappackages/core/cairo_alpha/src/erc20.test.ts.snapis excluded by!**/*.snappackages/core/cairo_alpha/src/erc721.test.ts.snapis excluded by!**/*.snappackages/core/cairo_alpha/src/governor.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (24)
packages/core/cairo_alpha/CHANGELOG.md(1 hunks)packages/core/cairo_alpha/src/common-options.ts(2 hunks)packages/core/cairo_alpha/src/custom.test.ts(3 hunks)packages/core/cairo_alpha/src/custom.test.ts.md(1 hunks)packages/core/cairo_alpha/src/erc1155.test.ts(5 hunks)packages/core/cairo_alpha/src/erc20.test.ts(7 hunks)packages/core/cairo_alpha/src/erc20.test.ts.md(31 hunks)packages/core/cairo_alpha/src/erc721.test.ts(3 hunks)packages/core/cairo_alpha/src/erc721.test.ts.md(22 hunks)packages/core/cairo_alpha/src/governor.test.ts.md(10 hunks)packages/core/cairo_alpha/src/governor.ts(4 hunks)packages/core/cairo_alpha/src/index.ts(1 hunks)packages/core/cairo_alpha/src/multisig.ts(2 hunks)packages/core/cairo_alpha/src/print.ts(4 hunks)packages/core/cairo_alpha/src/set-access-control.ts(4 hunks)packages/core/cairo_alpha/src/set-royalty-info.ts(2 hunks)packages/core/cairo_alpha/src/test.ts(1 hunks)packages/core/cairo_alpha/src/utils/duration.ts(1 hunks)packages/core/cairo_alpha/src/vesting.ts(4 hunks)packages/ui/src/cairo_alpha/AccessControlSection.svelte(1 hunks)packages/ui/src/cairo_alpha/CustomControls.svelte(2 hunks)packages/ui/src/cairo_alpha/ERC1155Controls.svelte(2 hunks)packages/ui/src/cairo_alpha/ERC20Controls.svelte(2 hunks)packages/ui/src/cairo_alpha/ERC721Controls.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-19T15:31:24.984Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#0
File: :0-0
Timestamp: 2025-08-19T15:31:24.984Z
Learning: Changes within packages/core/cairo_alpha should have a corresponding changelog entry in packages/core/cairo_alpha/CHANGELOG.md under the Unreleased section to track these changes. When cairo_alpha is eventually promoted to cairo (stable), these entries will be moved into a changeset for cairo (stable).
Applied to files:
packages/core/cairo_alpha/CHANGELOG.md
📚 Learning: 2025-09-12T15:07:08.673Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#663
File: packages/core/cairo_alpha/src/custom.test.ts.md:12-12
Timestamp: 2025-09-12T15:07:08.673Z
Learning: In the OpenZeppelin contracts-wizard cairo_alpha package changelog (packages/core/cairo_alpha/CHANGELOG.md), each alpha version gets its own separate entry under the "Unreleased" section rather than updating a single entry. This allows tracking of changes across different alpha releases (e.g., v3.0.0-alpha.0, v3.0.0-alpha.1, v3.0.0-alpha.2 all have separate entries).
Applied to files:
packages/core/cairo_alpha/CHANGELOG.md
🧬 Code graph analysis (11)
packages/core/cairo_alpha/src/common-options.ts (1)
packages/core/cairo_alpha/src/set-access-control.ts (1)
AccessControl(31-38)
packages/core/cairo_alpha/src/erc721.test.ts (1)
packages/core/cairo_alpha/src/set-access-control.ts (3)
AccessControl(31-38)darDefaultOpts(21-24)darCustomOps(26-29)
packages/core/cairo_alpha/src/multisig.ts (1)
packages/core/cairo_alpha/src/set-access-control.ts (1)
AccessControl(31-38)
packages/core/cairo_alpha/src/set-royalty-info.ts (2)
packages/core/cairo_alpha/src/contract.ts (2)
ContractBuilder(101-308)components(124-126)packages/core/cairo_alpha/src/set-access-control.ts (3)
Access(19-19)DEFAULT_ACCESS_CONTROL(40-40)setAccessControl(52-162)
packages/core/cairo_alpha/src/erc1155.test.ts (2)
packages/core/cairo_alpha/src/set-access-control.ts (3)
AccessControl(31-38)darDefaultOpts(21-24)darCustomOps(26-29)packages/core/cairo_alpha/src/set-royalty-info.ts (1)
royaltyInfoOptions(18-30)
packages/core/cairo_alpha/src/print.ts (2)
packages/core/cairo_alpha/src/contract.ts (2)
constants(136-138)Variable(88-94)packages/core/cairo_alpha/src/utils/format-lines.ts (1)
Lines(1-1)
packages/core/cairo_alpha/src/erc20.test.ts (1)
packages/core/cairo_alpha/src/set-access-control.ts (3)
AccessControl(31-38)darDefaultOpts(21-24)darCustomOps(26-29)
packages/core/cairo_alpha/src/vesting.ts (2)
packages/core/cairo_alpha/src/set-access-control.ts (1)
AccessControl(31-38)packages/core/cairo_alpha/src/utils/duration.ts (1)
durationToSeconds(14-26)
packages/core/cairo_alpha/src/custom.test.ts (1)
packages/core/cairo_alpha/src/set-access-control.ts (3)
AccessControl(31-38)darDefaultOpts(21-24)darCustomOps(26-29)
packages/core/cairo_alpha/src/governor.ts (1)
packages/core/cairo_alpha/src/utils/duration.ts (1)
durationToSeconds(14-26)
packages/core/cairo_alpha/src/set-access-control.ts (5)
packages/core/cairo_alpha/src/contract.ts (3)
ContractBuilder(101-308)components(124-126)BaseImplementedTrait(57-68)packages/core/cairo_alpha/src/common-components.ts (1)
addSRC5Component(59-70)packages/core/cairo_alpha/src/utils/convert-strings.ts (1)
toUint(87-102)packages/core/cairo_alpha/src/utils/duration.ts (1)
durationToSeconds(14-26)packages/core/cairo_alpha/src/error.ts (1)
OptionsError(3-7)
🪛 markdownlint-cli2 (0.18.1)
packages/core/cairo_alpha/src/custom.test.ts.md
395-395: Hard tabs
Column: 9
(MD010, no-hard-tabs)
396-396: Hard tabs
Column: 9
(MD010, no-hard-tabs)
packages/core/cairo_alpha/src/erc20.test.ts.md
452-452: Hard tabs
Column: 9
(MD010, no-hard-tabs)
453-453: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1236-1236: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1237-1237: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1242-1242: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1345-1345: Hard tabs
Column: 9
(MD010, no-hard-tabs)
2262-2262: Hard tabs
Column: 9
(MD010, no-hard-tabs)
2263-2263: Hard tabs
Column: 9
(MD010, no-hard-tabs)
packages/core/cairo_alpha/src/erc721.test.ts.md
866-866: Hard tabs
Column: 9
(MD010, no-hard-tabs)
867-867: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1194-1194: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1293-1293: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1400-1400: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1401-1401: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1406-1406: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1519-1519: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1844-1844: Hard tabs
Column: 9
(MD010, no-hard-tabs)
1845-1845: Hard tabs
Column: 9
(MD010, no-hard-tabs)
2084-2084: Hard tabs
Column: 9
(MD010, no-hard-tabs)
2611-2611: Hard tabs
Column: 9
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate-cairo-alpha
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (36)
packages/core/cairo_alpha/src/governor.ts (3)
10-10: LGTM!The rename from
durationToTimestamptodurationToSecondsis more accurate since the function returns seconds, not a timestamp value.
272-272: LGTM!The alias
GovernorDefaultConfigprovides clearer naming to distinguish governor's default configuration from other component defaults, aligning with the broader AccessControl refactor.
319-319: LGTM!Function calls correctly updated to use the renamed
durationToSecondsfunction, maintaining consistency with the duration utility refactor.Also applies to: 337-337
packages/core/cairo_alpha/src/governor.test.ts.md (1)
16-18: LGTM!The test snapshots correctly reflect the import alias change from
governor.ts, ensuring consistency between implementation and test expectations.packages/core/cairo_alpha/src/utils/duration.ts (1)
14-14: LGTM!The function rename from
durationToTimestamptodurationToSecondsis more accurate and improves clarity, since the function returns the duration in seconds rather than a timestamp value.packages/core/cairo_alpha/src/vesting.ts (2)
4-4: LGTM!The migration from string-based
'ownable'to the enum-likeAccessControl.Ownableimproves type safety and aligns with the broader AccessControl refactor across the codebase.Also applies to: 58-58
11-11: LGTM!Function calls correctly updated to use the renamed
durationToSecondsfunction, maintaining consistency with the duration utility refactor.Also applies to: 162-162, 176-176
packages/core/cairo_alpha/src/print.ts (2)
10-10: LGTM!The refactor of
printConstantsto acceptVariable[]instead ofContractimproves function cohesion and reusability. The function now focuses solely on printing constants without needing the entire contract object.Also applies to: 139-156
38-38: LGTM!All call sites correctly updated to pass the constants array directly, ensuring consistency with the refactored function signature.
Also applies to: 288-288
packages/core/cairo_alpha/src/erc20.test.ts.md (1)
439-568: LGTM!The new test snapshots for AccessControlDefaultAdminRules (DAR) variants provide excellent coverage for both default and custom configuration options, validating the pausable, mintable, and upgradeable scenarios with the new DAR-based access control.
Also applies to: 570-700, 1223-1434, 2248-2627
packages/core/cairo_alpha/src/common-options.ts (2)
3-3: LGTM - Import necessary for the new access control model.The import of
AccessControlsupports the refactored access control defaults on line 15.
15-15: No breaking changes detected. The change is safe and consistent with cairo_alpha's type design.The verification shows that
cairo_alphauses an object-basedAccesstype (with atypeproperty and DAR options), not a boolean. The change fromaccess: falsetoaccess: { ...AccessControl.None }is consistent with this design. The test at line 115 already expects the object structure by checkingaccess?.type, and no code in the codebase treats cairo_alpha's access field as a boolean. UI types do not constrain cairo_alpha's access field to a boolean, so there are no type conflicts across the codebase.packages/core/cairo_alpha/src/index.ts (1)
9-9: LGTM - AccessType export supports the expanded access control model.Adding
AccessTypeto the public API allows consumers to work with the discriminated union of access control types (None, Ownable, Roles, RolesDefaultAdminRules).packages/ui/src/cairo_alpha/CustomControls.svelte (2)
16-16: LGTM - Nested access object initialization prevents mutation.Creating a new object from
custom.defaults.accessensures the default values aren't mutated whenoptsis modified. This pattern is consistent with howinfois initialized on line 15.
51-57: LGTM - Granular bindings enable DAR configuration.The refactor from a single
bind:accessto separate bindings foraccessType,darInitialDelay, anddarDefaultDelayIncreaseallows the UI to independently control each aspect of the access control configuration, supporting the new Default Admin Rules feature.packages/ui/src/cairo_alpha/ERC721Controls.svelte (2)
19-19: LGTM - Consistent nested object initialization.Creating a new object from
erc721.defaults.accessprevents mutation and follows the same pattern used in CustomControls and for theinfoobject on line 18.
116-122: LGTM - Granular access control bindings.The migration to separate bindings for
accessType,darInitialDelay, anddarDefaultDelayIncreaseis consistent with the pattern in CustomControls.svelte and enables fine-grained control over Default Admin Rules configuration.packages/core/cairo_alpha/src/erc1155.test.ts (4)
8-8: LGTM - Necessary imports for the new access control model.Importing
AccessControl,darDefaultOpts, anddarCustomOpsenables the tests to use the new enum-based access control system and test DAR configurations.
58-58: LGTM - Comprehensive migration to AccessControl enum.The replacement of string literal
'roles'withAccessControl.Rolesand the addition of DAR test cases with both default and custom options provide thorough test coverage for the new access control variants.Also applies to: 79-90
98-129: LGTM - Royalty info tests updated for all access control variants.The royalty info tests now cover all access control types (Ownable, Roles, and Roles-DAR with both default and custom options), ensuring comprehensive test coverage for the feature combinations.
131-165: LGTM - Full feature tests cover DAR variants.The full feature test suite now includes separate test cases for:
- Roles (non-upgradeable and upgradeable)
- Roles-DAR with default options (non-upgradeable and upgradeable)
- Roles-DAR with custom options (non-upgradeable and upgradeable)
This provides excellent coverage of the new DAR functionality.
packages/core/cairo_alpha/src/erc721.test.ts (3)
7-7: LGTM - Required imports for AccessControl refactor.The import of
AccessControl,darDefaultOpts, anddarCustomOpsaligns with the broader refactoring to support the enum-based access control model and DAR configurations.
94-105: LGTM - Consistent migration to AccessControl enum with DAR coverage.Replacing the string literal
'roles'withAccessControl.Rolesand adding test cases forRolesDefaultAdminRuleswith both default and custom options ensures comprehensive testing of the new access control model.
113-149: LGTM - Thorough royalty and access control test coverage.The test suite now covers all combinations of:
- Royalty info (disabled, enabled with default config, enabled with custom config)
- Access control types (Ownable, Roles, Roles-DAR with default/custom options)
This provides excellent coverage for the refactored access control system.
packages/core/cairo_alpha/src/test.ts (1)
115-115: No issues found; conditional logic correctly implements the new access control model.The condition
if (!contract.options.access?.type)is correct for cairo_alpha's restructured access control design. Unlike cairo which uses a primitive type (false | 'ownable' | 'roles'), cairo_alpha'sAccessis now an object ({ type: AccessType, ...darOptions }). The change from!contract.options.accessto!contract.options.access?.typeproperly accounts for this architectural shift.Verification confirms:
AccessControl.Nonewithtype: false→ condition evaluates to true ✓AccessControl.Ownablewithtype: 'ownable'→ condition evaluates to false ✓AccessControl.Roleswithtype: 'roles'→ condition evaluates to false ✓Test coverage remains equivalent since both conditions check whether access control is disabled; the test logic for validating ownable patterns is unchanged.
packages/core/cairo_alpha/src/multisig.ts (1)
5-5: Enum import migration looks good.Switching to
AccessControlmatches the new API surface.packages/core/cairo_alpha/src/erc20.test.ts (1)
52-53: Tests updated toAccessControlAPI and DAR variants — LGTM.
- Enum usage (
Ownable,Roles) and newroles-darscenarios are consistent and broaden coverage.- Good mix across pausable/mintable/full/upgradeable.
Please run snapshots locally and confirm no unintended diffs beyond ACL scaffolding.
Also applies to: 57-59, 60-64, 65-68, 100-101, 105-107, 108-112, 113-116, 159-162, 173-174, 185-193, 195-206, 207-210, 228-229
packages/core/cairo_alpha/src/custom.test.ts.md (1)
383-462: New DAR snapshots align with the component model.The default/custom variants reflect
AccessControlDefaultAdminRules*storage, events, and upgrade gating viaUPGRADER_ROLE. The alias import forDefaultConfigmatches the default-delay path.Also applies to: 464-544
packages/ui/src/cairo_alpha/ERC1155Controls.svelte (1)
17-18: Good: clone nestedaccessdefaults.Avoids mutating shared defaults when toggling DAR options.
packages/core/cairo_alpha/src/set-royalty-info.ts (2)
75-111: DAR integration for ERC2981 — LGTM; add coverage forNone → ownablefallback.
- Correct impl selection per access type, with
_grant_roletargetingaccesscontrolvsaccesscontrol_dar.- Exhaustive
defaultwithneveris good.Please add/confirm a test where royalties are enabled with
AccessControl.None, asserting the output falls back toownableand wiresERC2981AdminOwnableImpl.
113-114: Alias import for default fee denominator — good practice.Using
ERC2981DefaultConfigavoids name collisions acrossuseclauses.packages/ui/src/cairo_alpha/ERC20Controls.svelte (2)
18-18: Good defensive copy for nested access defaults.Spreading
erc20.defaults.accessavoids shared references and unintended mutations.
118-124: Bindings align with new AccessType/DAR model.Granular binds and
{errors}plumbing look correct and consistent withAccessControlSection.packages/core/cairo_alpha/src/custom.test.ts (2)
50-57: LGTM on AccessControl migration and coverage.Enum-based access, “disabled” case, and full-upgradeable path are updated correctly.
Also applies to: 71-74, 80-86
63-69: Snapshot verification cannot proceed: snapshots directory does not exist.The code logic is correct—darDefaultOpts (5 days) will trigger DefaultConfig, while darCustomOps (7 days) will emit ImmutableConfig with DEFAULT_ADMIN_DELAY_INCREASE_WAIT. However, the verification request assumes snapshots already exist. AVA snapshots are created on test execution; they must be generated first.
To fulfill this review's intent, run the tests to generate snapshots, then commit them for inspection.
packages/ui/src/cairo_alpha/AccessControlSection.svelte (1)
44-66: Radio group wiring looks correct.Bindings and labels for 'ownable' | 'roles' | 'roles-dar' are consistent with AccessType.
| export type AccessType = | ||
| | false | ||
| | 'ownable' | ||
| | 'roles' // AccessControl | ||
| | 'roles-dar'; // AccessControlDefaultAdminRules |
There was a problem hiding this comment.
Yes, it's used in this file to declare the type of Access type property and also in AccountControls.svelte
| export const contractDefaults: Required<CommonContractOptions> = { | ||
| ...defaults, | ||
| access: false, | ||
| access: { ...AccessControl.None }, |
There was a problem hiding this comment.
Why does this need an object? Why not just AccessControl.None?
There was a problem hiding this comment.
I used this pattern to make a copy of the object to safeguard against its possible mutations. However, I've pushed changes that make AccessControl.None a function returning a brand-new object each time rather than a standalone object. That prevents this very issue in a better way so that you don't have to worry about object references at the call site
| $: hasErrors = errors[tab] !== undefined; | ||
| const language = 'cairo'; | ||
| const language = 'cairo-alpha'; |
There was a problem hiding this comment.
Should the language include alpha?
There was a problem hiding this comment.
I agree this should include alpha, as it allows differentiating from the stable version's options. It was previously missed.
There was a problem hiding this comment.
Yes, just like @ericglau said. It also has to deviate from Cairo-stable now since there's a difference in contracts options between Cairo-stable and Cairo-alpha versions (because of the introduced 'roles-dar' option)
There was a problem hiding this comment.
Just one additional suggestion in addition to @ericnordelo's questions.
| $: hasErrors = errors[tab] !== undefined; | ||
| const language = 'cairo'; | ||
| const language = 'cairo-alpha'; |
There was a problem hiding this comment.
I agree this should include alpha, as it allows differentiating from the stable version's options. It was previously missed.
… subset for ERC20, ERC721, ERC1155, and Custom contracts
| case 'disabled': | ||
| case 'none': | ||
| case 'no': |
There was a problem hiding this comment.
Is there a need to support these different aliases?
There was a problem hiding this comment.
Only to prevent mistakes with param name when running the script locally
| case 'roles-dar-default': | ||
| case 'roles_dar_default': |
There was a problem hiding this comment.
Is there a need to support these different aliases?
There was a problem hiding this comment.
Only to prevent mistakes with param name when running the script locally
| case 'roles-dar-custom': | ||
| case 'roles_dar_custom': |
There was a problem hiding this comment.
Is there a need to support these different aliases?
There was a problem hiding this comment.
Only to prevent mistakes with param name when running the script locally
| throw new Error(`Invalid argument format: ${arg}`); | ||
| } | ||
| } else if (i + 1 < args.length) { | ||
| // Handle --arg value format |
There was a problem hiding this comment.
This script can be run locally or as a part of CI workflow. The CI workflow uses --arg=arg syntax, but I decided to support both common ways to pass named CLI args
There was a problem hiding this comment.
That sounds fine, although I have a slight preference for being stricter (and failing early if a wrong input is provided), just so there's less code to maintain. Same for the aliases below.
If we do want to support different input formats or aliases, perhaps those can be refactored into helpers to make this part of the code cleaner.
| throw new Error(`Invalid argument format: ${arg}`); | ||
| } | ||
| } else if (i + 1 < args.length) { | ||
| // Handle --arg value format |
There was a problem hiding this comment.
That sounds fine, although I have a slight preference for being stricter (and failing early if a wrong input is provided), just so there's less code to maintain. Same for the aliases below.
If we do want to support different input formats or aliases, perhaps those can be refactored into helpers to make this part of the code cleaner.
3 participants
Adds support for
AccessControlDefaultAdminRulescomponent by introducing an additional option to the access module:roles-dar. Refactorsaccessto be an object with type and additional options instead of being value-based. Introduces UI controls for configuring theAccessControlDefaultAdminRulessettings.