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 another flavor of permission api #15609

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

Conversation

runtian-zhou
Copy link
Contributor

Description

How Has This Been Tested?

Key Areas to Review

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
  • Move Compiler
  • Other (specify)

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

Copy link

trunk-io bot commented Dec 16, 2024

⏱️ 1h 28m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-move-tests 14m 🟩
rust-move-tests 13m 🟩
rust-move-tests 13m 🟩
rust-cargo-deny 12m 🟩🟩🟩🟩🟩 (+2 more)
rust-move-tests 8m 🟥
rust-move-tests 8m 🟥
check-dynamic-deps 7m 🟩🟩🟩🟩🟩 (+2 more)
general-lints 3m 🟩🟩🟩🟩🟩 (+2 more)
semgrep/ci 3m 🟩🟩🟩🟩🟩 (+2 more)
rust-move-tests 3m 🟥
rust-move-tests 3m 🟥
file_change_determinator 1m 🟩🟩🟩🟩🟩 (+2 more)
permission-check 23s 🟩🟩🟩🟩🟩 (+2 more)
permission-check 21s 🟩🟩🟩🟩🟩 (+2 more)
check-branch-prefix 1s 🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

runtian-zhou commented Dec 16, 2024

@runtian-zhou runtian-zhou mentioned this pull request Dec 16, 2024
7 tasks
@runtian-zhou runtian-zhou force-pushed the 10-09-create_benchmark branch from 16346c7 to fc085a0 Compare December 16, 2024 09:29
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch from be4ea04 to 46ddd6f Compare December 16, 2024 09:30
@runtian-zhou runtian-zhou force-pushed the 10-09-create_benchmark branch from fc085a0 to 6a2e87a Compare December 16, 2024 09:53
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch from 46ddd6f to 0ee1c8c Compare December 16, 2024 09:53
@runtian-zhou runtian-zhou force-pushed the 10-09-create_benchmark branch from 6a2e87a to a5e92c0 Compare December 17, 2024 02:28
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch from 0ee1c8c to 851da1d Compare December 17, 2024 02:28
@runtian-zhou runtian-zhou force-pushed the 10-09-create_benchmark branch from a5e92c0 to 60ce91a Compare December 17, 2024 12:02
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch 2 times, most recently from e06ec0c to dd67295 Compare December 17, 2024 15:19
@runtian-zhou runtian-zhou changed the base branch from 10-09-create_benchmark to 12-17-feature_gate_permissioned_signer December 17, 2024 15:19
Comment on lines +618 to +642
if (perm.key != perm_key) {
return false
};
Copy link

Choose a reason for hiding this comment

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

The generic type parameter PermKey only requires copy + drop + store abilities, but the code uses the != operator which may not be supported for all possible types satisfying these constraints. To ensure type safety, consider either:

  1. Adding drop + copy + store + has[std::cmp::Eq] as the ability constraint for PermKey, or
  2. Using a dedicated comparison function that's guaranteed to work for the intended key types

This will prevent potential runtime errors when comparing complex key types that don't implement equality comparison.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@runtian-zhou runtian-zhou force-pushed the 12-17-feature_gate_permissioned_signer branch from 6494f73 to 2bd4613 Compare December 18, 2024 07:37
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch from dd67295 to 287f491 Compare December 18, 2024 07:38
@runtian-zhou runtian-zhou force-pushed the 12-17-feature_gate_permissioned_signer branch from 2bd4613 to 472be55 Compare January 8, 2025 23:56
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch from 287f491 to 9bb1d3b Compare January 8, 2025 23:56
@runtian-zhou runtian-zhou force-pushed the 12-17-feature_gate_permissioned_signer branch from 472be55 to 18a8c9a Compare January 9, 2025 00:55
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch from 9bb1d3b to 84b19ea Compare January 9, 2025 00:56
@runtian-zhou runtian-zhou force-pushed the 12-17-feature_gate_permissioned_signer branch from 18a8c9a to 015bf33 Compare January 9, 2025 03:44
@runtian-zhou runtian-zhou force-pushed the 12-15-add_another_flavor_of_permission_api branch from 84b19ea to 646097a Compare January 9, 2025 03:44
@runtian-zhou runtian-zhou force-pushed the 12-17-feature_gate_permissioned_signer branch from 015bf33 to f9ea87a Compare January 9, 2025 07:58
@runtian-zhou runtian-zhou force-pushed the 01-10-create_object_with_permissioned_signer branch 2 times, most recently from 629e352 to 83738a4 Compare January 15, 2025 03:05
@runtian-zhou runtian-zhou force-pushed the 01-10-create_object_with_permissioned_signer branch 8 times, most recently from f2d1cc7 to 154c5d2 Compare January 15, 2025 15:36
@runtian-zhou runtian-zhou marked this pull request as ready for review January 15, 2025 17:58
@runtian-zhou runtian-zhou force-pushed the 01-10-create_object_with_permissioned_signer branch from 154c5d2 to 7fce8b6 Compare January 15, 2025 17:59
@runtian-zhou runtian-zhou force-pushed the 01-10-create_object_with_permissioned_signer branch 5 times, most recently from 2b01fae to 75508ff Compare January 15, 2025 21:13
Base automatically changed from 01-10-create_object_with_permissioned_signer to 09-09-add_perimission_checks_to_object January 15, 2025 22:06
@runtian-zhou runtian-zhou force-pushed the 09-09-add_perimission_checks_to_object branch from eb4b8f1 to e4a9607 Compare January 15, 2025 23:44
Base automatically changed from 09-09-add_perimission_checks_to_object to main January 16, 2025 01:03
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.

1 participant