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.

Sign up for GitHub

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

Jump to bottom

create permissioned signer example #14469

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

create permissioned signer example #14469

wants to merge 1 commit into from
+2,684 −0

Conversation

runtian-zhou
Copy link
Contributor

Description

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

Key Areas to Review

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io Trunk.io
Copy link

trunk-io bot commented Aug 29, 2024 •
edited
Loading

⏱️ 1h 46m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
rust-move-unit-coverage 14m 🟩 
rust-move-unit-coverage 13m 🟩 
rust-move-tests 9m 🟥 
general-lints 9m 🟩 🟩 🟩 🟩 🟩 
rust-move-unit-coverage 9m 🟩 
rust-move-tests 9m 🟥 
rust-cargo-deny 9m 🟩 🟩 🟩 🟩 🟩 
rust-move-unit-coverage 7m 🟩 
check-dynamic-deps 7m 🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-unit-coverage 6m 🟩 
rust-move-tests 3m 🟥 
rust-move-tests 3m 🟥 
rust-move-tests 3m 🟥 
semgrep/ci 2m 🟩 🟩 🟩 🟩 🟩 
file_change_determinator 1m 🟩 🟩 🟩 🟩 🟩 

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@runtian-zhou Graphite App
Copy link
Contributor Author

runtian-zhou commented Aug 29, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codecov Codecov
Copy link

codecov bot commented Aug 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.1%. Comparing base (c155fde) to head (e7391d6).
Report is 30 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff            @@
##             main   #14469    +/-   ##
========================================
  Coverage    60.0%    60.1%            
========================================
  Files         856      856            
  Lines      210625   211026   +401     
========================================
+ Hits       126555   126865   +310     
- Misses      84070    84161    +91

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 2 times, most recently from 887d7a2 to 456f5f4 Compare September 4, 2024 08:55
@runtian-zhou runtian-zhou mentioned this pull request Sep 4, 2024
Draft
21 tasks
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 2 times, most recently from 55771fc to 070a951 Compare September 5, 2024 00:41
@runtian-zhou runtian-zhou mentioned this pull request Sep 5, 2024
Draft
21 tasks
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 3 times, most recently from 6ddb481 to 8b92b98 Compare September 9, 2024 17:09
@runtian-zhou runtian-zhou mentioned this pull request Sep 9, 2024
Draft
21 tasks
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch from 8b92b98 to fa646c8 Compare September 10, 2024 17:24
@runtian-zhou runtian-zhou mentioned this pull request Sep 10, 2024
Draft
21 tasks
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 2 times, most recently from c3aba3a to 50b556b Compare September 12, 2024 01:49
@runtian-zhou runtian-zhou mentioned this pull request Sep 12, 2024
Open
21 tasks
lightmark
lightmark reviewed Sep 16, 2024
View reviewed changes
aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated
signer::address_of(master) == signer::address_of(permissioned),
error::permission_denied(ECANNOT_AUTHORIZE)
);
let permission_signer = permission_signer(permissioned);
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is so confusing.

@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 3 times, most recently from 1b8ce69 to c7fcc0e Compare September 19, 2024 08:08
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 3 times, most recently from ae32259 to a12a4c1 Compare October 7, 2024 22:56
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 2 times, most recently from 23ab901 to e963d83 Compare October 9, 2024 21:36
@runtian-zhou runtian-zhou mentioned this pull request Oct 9, 2024
Draft
22 tasks
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch from e963d83 to e7391d6 Compare October 15, 2024 19:03
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 4 times, most recently from a56d141 to 48e06fb Compare November 19, 2024 21:22
igor-aptos
igor-aptos requested changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@igor-aptos igor-aptos left a comment

Choose a reason for hiding this comment

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

this also needs more documentation. will we have a documentation page as well - on how to use it?

aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/permissioned_signer.move

/// Authorizes `permissioned` with the given permission. This requires to have access to the `master`
/// signer.
public fun authorize<PermKey: copy + drop + store>(
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we need copy on PermKey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why I needed copy initially is because smart table need copy for the key. Not sure what would be the generic requirement for your container.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, keys. in OrderedMap, there is no restriction. in BigOrderedMap keys need to be copy, values don't.

aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated
Comment on lines 213 to 321
if(simple_map::contains_key(perms, &key)) {
let entry = simple_map::borrow_mut(perms, &key);
entry.capacity = entry.capacity + capacity;
} else {
simple_map::add(perms, key, StoredPermission::V1 { capacity });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should have two different functions for setting capacity and incrementing amount.

I would also make whole section of Permission Management to be public(friend) in this release, so that we can iterate on it later (and anyways we don't want third-party permissions for in v0)

aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated Show resolved Hide resolved
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch 4 times, most recently from 8fcdf7c to f8073f6 Compare November 22, 2024 21:29
rahxephon89
rahxephon89 reviewed Nov 24, 2024
View reviewed changes
aptos-move/framework/aptos-framework/sources/permissioned_signer.move
#[test_only]
friend aptos_framework::permissioned_signer_tests;

/// Trying to grant permission using master signer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Trying to grant permission using non-master signer?

aptos-move/framework/aptos-framework/sources/permissioned_signer.move Outdated
)
}

/// Return the permission handle address so that it could be used for revokation purpose.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "revokation" -> "revocation"

aptos-move/framework/aptos-framework/sources/permissioned_signer.move Show resolved Hide resolved
@runtian-zhou runtian-zhou force-pushed the 08-29-create_permissioned_signer_example branch from f8073f6 to 69db8e1 Compare November 25, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igor-aptos igor-aptos igor-aptos requested changes

@rahxephon89 rahxephon89 rahxephon89 left review comments

@lightmark lightmark lightmark left review comments

@davidiw davidiw Awaiting requested review from davidiw davidiw will be requested when the pull request is marked ready for review davidiw is a code owner

@movekevin movekevin Awaiting requested review from movekevin movekevin will be requested when the pull request is marked ready for review movekevin is a code owner

@wrwg wrwg Awaiting requested review from wrwg wrwg will be requested when the pull request is marked ready for review wrwg is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@runtian-zhou @lightmark @rahxephon89 @igor-aptos