-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
permission for framework #14605
base: 09-09-add_perimission_checks_to_object
Are you sure you want to change the base?
permission for framework #14605
Conversation
⏱️ 2h 55m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 09-09-add_perimission_checks_to_object #14605 +/- ##
=========================================================================
+ Coverage 59.4% 60.0% +0.6%
=========================================================================
Files 857 857
Lines 210762 210762
=========================================================================
+ Hits 125197 126586 +1389
+ Misses 85565 84176 -1389 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add the initialization with intent code?
9037ce3
to
843150a
Compare
b2c0a3d
to
8ab6836
Compare
843150a
to
33a73d7
Compare
8ab6836
to
fe7cd59
Compare
33a73d7
to
9123ceb
Compare
fe7cd59
to
d48d186
Compare
9123ceb
to
206e782
Compare
d48d186
to
3c1467e
Compare
206e782
to
9c81c83
Compare
3c1467e
to
ac81f59
Compare
9c81c83
to
75362c4
Compare
ac81f59
to
2695494
Compare
daea4f8
to
76ad6f0
Compare
729cd2a
to
638ddbb
Compare
dffbce5
to
d8dad95
Compare
8837896
to
876e84b
Compare
d8dad95
to
1e81ef4
Compare
876e84b
to
e2ca0c2
Compare
1e81ef4
to
db21dee
Compare
e2ca0c2
to
0bf87f0
Compare
db21dee
to
9cab1df
Compare
0bf87f0
to
8de6de8
Compare
9cab1df
to
bac9d91
Compare
8de6de8
to
b10828f
Compare
bac9d91
to
253e2f2
Compare
b10828f
to
5a5ba7d
Compare
253e2f2
to
9bc9130
Compare
5a5ba7d
to
7ab440e
Compare
9bc9130
to
a471067
Compare
7ab440e
to
cee59bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also ask for reviews from folks that wrote these files, to make sure we understand the use and make permission sets reasonable
struct GovernancePermission has copy, drop, store {} | ||
|
||
/// Permissions | ||
inline fun check_signer_permission(s: &signer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better to name these by the permission - i.e. check_governance_permission vs check_signer_permission
/// Current permissioned signer cannot publish codes. | ||
const ENO_CODE_PERMISSION: u64 = 0xB; | ||
|
||
struct CodePermission has copy, drop, store {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe CodePublishingPermission?
@@ -346,6 +350,8 @@ module aptos_framework::delegation_pool { | |||
allowlist: SmartTable<address, bool>, | |||
} | |||
|
|||
struct DelegationPermission has copy, drop, store {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly , there are (at least) two different roles here:
DelegationPoolManagementPermission (i.e. for delegation pool owners)
and
StakingToDelegationPoolPermission (i.e. for folks staking their fudns to delegation pool nodes. Also maybe this should be the same permission as regular staking, i.e. differentiating between StakingToDelegationPoolPermission and StakingPermission might be unnecessary
@@ -75,6 +78,21 @@ module aptos_framework::object_code_deployment { | |||
object_address: address, | |||
} | |||
|
|||
struct ObjectCodePermission has copy, drop, store {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need a separate permission from code.move? I am not sure I understand the difference between the two files fully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
0e65f1b
to
925457f
Compare
cee59bb
to
7d27d69
Compare
925457f
to
6851ffb
Compare
e00ba7b
to
a234dbf
Compare
6851ffb
to
d4a929c
Compare
a234dbf
to
81c1691
Compare
d4a929c
to
6fd4575
Compare
81c1691
to
6db859a
Compare
Description
Implement checks for all framework related privilege operations.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Added relevant tests.
Key Areas to Review
Whether there's any priviledged operation need to be gated.
Checklist