Royalty Info External Traits #1173
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1173 +/- ##
==========================================
- Coverage 91.89% 91.57% -0.32%
==========================================
Files 47 47
Lines 1197 1223 +26
==========================================
+ Hits 1100 1120 +20
- Misses 97 103 +6
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Looking great @immrsd! This is certainly very helpful and the code looks neat. Left a few small suggestions but is pretty much ready to go.
packages/token/src/tests/erc2981/test_erc2981_accesscontrol.cairo
Outdated
Show resolved
Hide resolved
| // Role for the admin responsible for managing royalty settings. | ||
| pub const ROYALTY_ADMIN_ROLE: felt252 = 'ROYALTY_ADMIN_ROLE'; |
There was a problem hiding this comment.
| // Role for the admin responsible for managing royalty settings. | |
| pub const ROYALTY_ADMIN_ROLE: felt252 = 'ROYALTY_ADMIN_ROLE'; | |
| // Role for the admin responsible for managing royalty settings. | |
| pub const ROYALTY_ADMIN_ROLE: felt252 = selector!("ROYALTY_ADMIN_ROLE"); |
For consistency with how we've been calculating role ids
There was a problem hiding this comment.
Btw, what's the rationale behind that? From my point of view felt-strings would be a better choice since they are easier to work with (automatically decoded in block scanners) and the only drawback of 31 char limit is not actually bad for defining a role
And as an additional minor neat thing: no collision between two different roles can happen
packages/token/src/tests/erc2981/test_erc2981_accesscontrol.cairo
Outdated
Show resolved
Hide resolved
packages/token/src/tests/erc2981/test_erc2981_accesscontrol.cairo
Outdated
Show resolved
Hide resolved
| [.contract-item] | ||
| [[IERC2981AdminAccessControlImpl-ROYALTY_ADMIN_ROLE]] | ||
| ==== `[.contract-item-name]#++ROYALTY_ADMIN_ROLE:++#++ felt252++` [.item-kind]#constant# | ||
|
|
||
| Role for the admin responsible for managing royalty settings. |
There was a problem hiding this comment.
Though this is only supposed to be used within the context of the impl, it's accessible from the base 2981 component. AFAIK we have't defined component constants in the other APIs so we should probably come up with something universal (if not now then sometime soon). I imagine we should have a dedicated constants block or something more intentional
There was a problem hiding this comment.
it's accessible from the base 2981 component
I think it's fine, the ERC2981AdminAccessControlImpl is accessible from the base component as well
What do you suggest in this case? Placing the role constant in a constants block this way?
//
// AccessControl-based admin functions
//
//
// Constants
//
// Role for the admin responsible for managing royalty settings.
pub const ROYALTY_ADMIN_ROLE: felt252 = selector!("ROYALTY_ADMIN_ROLE");
#[embeddable_as(ERC2981AdminAccessControlImpl)]
impl ERC2981AdminAccessControl<
...
Fixes #1172
Key Changes:
IERC2981StateInfoandIERC2981Admintraits with ERC2981 external functionsIERC2981Admin: withOwnableorAccessControl)