diff --git a/CHANGELOG.md b/CHANGELOG.md index d8493802..c77715de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,19 +8,6 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ## Unreleased -### `openzeppelin_access` - -#### Added - -- `access_control` module for role-based access control with typed `Auth` capabilities and timelocked root transfer. - -### `openzeppelin_fp_math` - -#### Added - -- `sqrt` for `UD30x9` and `SD29x9` with round-down semantics. (#286) -- `log2`, `ln`, `log10` for `UD30x9` (rounds down) and `SD29x9` (rounds toward zero). (#320) - ### `openzeppelin_math` #### Added @@ -37,6 +24,21 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) - New `rate_limiter` module: an embeddable rate-limiting primitive with three strategies behind one enum. (#315) +## 1.2.0 (03-06-2026) + +### `openzeppelin_access` + +#### Added + +- `access_control` module for role-based access control with typed `Auth` capabilities and timelocked root transfer. + +### `openzeppelin_fp_math` + +#### Added + +- `sqrt` for `UD30x9` and `SD29x9` with round-down semantics. (#286) +- `log2`, `ln`, `log10` for `UD30x9` (rounds down) and `SD29x9` (rounds toward zero). (#320) + ## 1.1.0 (21-04-2026) ### `openzeppelin_fp_math` diff --git a/audits/2026-06-v1.2.0-access-control.pdf b/audits/2026-06-v1.2.0-access-control.pdf new file mode 100644 index 00000000..70f7fc8e Binary files /dev/null and b/audits/2026-06-v1.2.0-access-control.pdf differ diff --git a/audits/2026-06-v1.2.0-fp-math-diff.pdf b/audits/2026-06-v1.2.0-fp-math-diff.pdf new file mode 100644 index 00000000..8b3877ec Binary files /dev/null and b/audits/2026-06-v1.2.0-fp-math-diff.pdf differ diff --git a/audits/README.md b/audits/README.md index 4928d07a..6d00c75e 100644 --- a/audits/README.md +++ b/audits/README.md @@ -6,3 +6,5 @@ | March 2026 | PR #183 | [`7d3dd29`](https://github.com/OpenZeppelin/contracts-sui/tree/7d3dd290e8950acdda87eee644cf286aa7075661) | OpenZeppelin | Two-step transfer flow | [πŸ”—](./2026-03-PR183.pdf) | | April 2026 | v1.1.0 | [`7cfc07c`](https://github.com/OpenZeppelin/contracts-sui/tree/7cfc07c528f8b4da7cdb69b8ed1246e705afbbeb) | OpenZeppelin | `openzeppelin_fp_math` package | [πŸ”—](./2026-04-v1.1.0-fp-math.pdf) | | April 2026 | v1.1.0 | [`v1.0.0 β†’ v1.1.0`](https://github.com/OpenZeppelin/contracts-sui/compare/v1.0.0-rc.1...v1.1.0-rc.1) | OpenZeppelin | v1.0.0 β†’ v1.1.0 diff | [πŸ”—](./2026-04-v1.1.0-diff.pdf) | +| June 2026 | v1.2.0 | [`7f7a300`](https://github.com/OpenZeppelin/contracts-sui/tree/7f7a300308a55a4cae1df2026c2e45d6cd8ee91c) | OpenZeppelin | `openzeppelin_access` AccessControl module | [πŸ”—](./2026-06-v1.2.0-access-control.pdf) | +| June 2026 | v1.2.0 | [`af09be7 β†’ 55a7971`](https://github.com/OpenZeppelin/contracts-sui/compare/af09be74eaac71094b47e9795890651e0259ac88...55a79713a7e51a142de017e57bd7901ce8198212) | OpenZeppelin | Fixed-point math diff | [πŸ”—](./2026-06-v1.2.0-fp-math-diff.pdf) | diff --git a/contracts/access/Published.toml b/contracts/access/Published.toml index cdff527d..4b4f1f41 100644 --- a/contracts/access/Published.toml +++ b/contracts/access/Published.toml @@ -4,18 +4,18 @@ [published.mainnet] chain-id = "35834a8a" -published-at = "0x0a031c162f9982ee32b199b98fbfbb6561051f2c4d2e17d358b09beafc20ce45" +published-at = "0x9ed727e318f8372868534146e38e0eab50fff6bb1c64ed41dbce1be023330115" original-id = "0x0a031c162f9982ee32b199b98fbfbb6561051f2c4d2e17d358b09beafc20ce45" -version = 1 -toolchain-version = "1.66.2" +version = 2 +toolchain-version = "1.73.0" build-config = { flavor = "sui", edition = "2024" } upgrade-capability = "0x84f250a13ea278987fd27cf25b10df9e74468cf4dbb1229acb45a6d237cc648b" [published.testnet] chain-id = "4c78adac" -published-at = "0xa7ea17bd36ee0df5473c2b0ea1ede707bae0c25da9bebffc30b02858cace244b" +published-at = "0xb357701a05fd1e26b42b167dcadc1c3cf5e521448ceb8fdb088402f7390465d7" original-id = "0xa7ea17bd36ee0df5473c2b0ea1ede707bae0c25da9bebffc30b02858cace244b" -version = 1 -toolchain-version = "1.69.2" +version = 2 +toolchain-version = "1.73.0" build-config = { flavor = "sui", edition = "2024" } upgrade-capability = "0x8fe40c0781aee3a57e0872d89be2b51288f3457dec7fe40ce4f9acbe2e1f2e09" diff --git a/contracts/access/README.md b/contracts/access/README.md index 87dc3a5a..e4bd9029 100644 --- a/contracts/access/README.md +++ b/contracts/access/README.md @@ -29,7 +29,10 @@ Initialize the registry from the module's `init` function when the package is first published. Sui does not run `init` for modules added in a later package upgrade, so an already-published package cannot adopt AccessControl by adding a new module that expects its OTW to be delivered. Publish a fresh package with -its own initializing module and AccessControl registry instead. +its own initializing module and AccessControl registry instead. Use +`access_control::new` when the publisher should be the default admin, or +`access_control::new_with_admin` when the initial default admin should be an +explicit governance or multisig address. ```move use openzeppelin_access::access_control::{Self, AccessControl, Auth}; diff --git a/contracts/access/sources/access_control.move b/contracts/access/sources/access_control.move index 8a9c40f0..1052658e 100644 --- a/contracts/access/sources/access_control.move +++ b/contracts/access/sources/access_control.move @@ -12,16 +12,17 @@ /// that initializes its own registry instead of adding a new module to the /// existing package. /// -/// 2. **Only home-module roles.** Every role-typed entry point β€” the four -/// mutating functions (`grant_role`, `revoke_role`, `renounce_role`, -/// `set_role_admin`) plus the auth-mint path (`new_auth`) β€” checks that -/// the role type's home module matches the root role's home module. +/// 2. **Only home-module roles.** Every role-typed path that mutates state, +/// mints auth, or queries admin configuration (`grant_role`, `revoke_role`, +/// `renounce_role`, `set_role_admin`, `new_auth`, `get_role_admin`) checks +/// that the role type's home module matches the root role's home module. /// Foreign role types are rejected at the boundary; they cannot be -/// introduced into the bag. The check uses `type_name::with_original_ids`, -/// which compares the package's *original* publish address β€” so role -/// types introduced in later upgrades to the same module in the same -/// package pass the check too. The home-module rule restricts roles to the -/// same original package and module, not to the same package version. +/// introduced into the non-root role map. The check uses +/// `type_name::with_original_ids`, which compares the package's *original* +/// publish address β€” so role types introduced in later upgrades to the same +/// module in the same package pass the check too. The home-module rule +/// restricts roles to the same original package and module, not to the same +/// package version. /// /// Together these guarantees make `Auth` a self-validating capability. /// `Role` can only live in the registry of its home module, that registry is @@ -45,12 +46,15 @@ /// transfer::public_share_object(registry); /// } /// ``` +/// +/// Use `new_with_admin` instead when the initial default admin should be an +/// explicit governance or multisig address rather than `ctx.sender()`. module openzeppelin_access::access_control; use std::type_name::{TypeName, with_original_ids}; -use sui::bag::{Self, Bag}; use sui::clock::Clock; use sui::event; +use sui::table::{Self, Table}; use sui::types; use sui::vec_set::{Self, VecSet}; @@ -79,41 +83,41 @@ const ENotPendingAdmin: vector = "Caller is not the pending admin"; #[error(code = 4)] const EDelayNotElapsed: vector = "Pending action's timelock has not elapsed"; -/// Role renounce attempted for an account other than the transaction sender. -#[error(code = 5)] -const ECannotRenounceForOtherAccount: vector = "Can only renounce role for own account"; - /// Default admin delay value exceeds the allowed maximum. -#[error(code = 6)] +#[error(code = 5)] const EDelayTooLarge: vector = "Admin delay exceeds the maximum allowed value"; /// Root role value is not a genuine One-Time Witness. -#[error(code = 7)] +#[error(code = 6)] const ENotOneTimeWitness: vector = "Root role must be a One-Time Witness"; /// A role type from a module other than the root role's home module was used /// in a write path. -#[error(code = 8)] +#[error(code = 7)] const EForeignRole: vector = "Role type must be defined in the same module as the root role"; /// Pending root role action is a renounce, not a transfer. -#[error(code = 9)] +#[error(code = 8)] const ENotPendingTransfer: vector = "Pending action is a renounce, not a transfer"; /// Pending root role action is a transfer, not a renounce. -#[error(code = 10)] +#[error(code = 9)] const ENotPendingRenounce: vector = "Pending action is not a renounce"; /// No pending default admin delay change exists. -#[error(code = 11)] +#[error(code = 10)] const ENoPendingDelayChange: vector = "No pending default admin delay change"; /// Zero address was used as a role holder or root transfer target. The zero /// address has no signing key, so a role granted to it can never be exercised /// and a transfer scheduled to it can never be accepted. -#[error(code = 12)] +#[error(code = 11)] const EZeroAddress: vector = "Cannot use the zero address as a role holder or transfer target"; +/// The current default admin tried to schedule a no-op transfer to itself. +#[error(code = 12)] +const EDefaultAdminTransferToSelf: vector = "Cannot transfer default admin to self"; + // === Constants === /// Upper bound on `default_admin_delay_ms` β€” the configured timelock for @@ -124,7 +128,7 @@ const MAX_DEFAULT_ADMIN_DELAY_MS: u64 = 60 * 24 * 60 * 60 * 1_000; /// Maximum wait before a scheduled delay *increase* takes effect. /// -/// When the root admin schedules an increase via +/// When the default admin schedules an increase via /// `begin_default_admin_delay_change`, the new (larger) delay applies after /// a wait of `min(new_delay_ms, MAX_DELAY_INCREASE_WAIT_MS)`. This cap /// prevents the wait from being unreasonably long when scheduling a large @@ -146,6 +150,8 @@ const MAX_DELAY_INCREASE_WAIT_MS: u64 = 48 * 60 * 60 * 1_000; /// can ever be registered in it (Invariant 2), every `Auth` that exists was /// produced by that one registry, against a sender that genuinely held `Role`. /// Consumers can take `&Auth` as authorization without any body checks. +/// This is a PTB-local snapshot: if the holder later renounces `Role` in the +/// same PTB, the witness remains valid but no longer reflects live membership. public struct Auth has drop { addr: address, } @@ -160,19 +166,26 @@ public struct AccessControl has key, store { id: UID, /// Per-role membership and admin mapping. Keyed by `TypeName`. /// Entries are created lazily on first grant or `set_role_admin`. - roles: Bag, + /// Does NOT contain an entry for the root role; see `default_admin`. + roles: Table, /// `TypeName` of the root role (= `RootRole`'s OTW). Direct /// `grant_role` / `revoke_role` / `renounce_role` / `set_role_admin` on /// this role is blocked (`ECannotManageRootRole`); use the timelocked /// transfer or renounce flow. protected_root: TypeName, - /// Pending change to the root role holder β€” either a transfer to a - /// specific new admin or a renounce. See `PendingAdminTransfer`. + /// The current default admin, or `none` after a renounce. + /// This address is the sole holder of the root role. + /// Stored as a dedicated field (not inside `roles`) to express the + /// singleton constraint directly and simplify transfer and renounce paths. + default_admin: Option
, + /// Pending change to the default admin β€” either a transfer to a specific + /// new admin or a renounce. See `PendingAdminTransfer`. pending_default_admin: Option, - /// Pending change to `default_admin_delay_ms`. The change becomes - /// effective only after `accept_default_admin_delay_change` is called - /// past `schedule_after_ms`; an in-flight `pending_default_admin` is - /// unaffected (it locks in the delay it was scheduled under). + /// Pending change to `default_admin_delay_ms`. The change becomes effective + /// once `schedule_after_ms` has elapsed. Until a delay-refreshing entrypoint + /// clears this slot, it may remain stored here even though public pending + /// getters no longer report it as pending. An in-flight `pending_default_admin` + /// is unaffected (it locks in the delay it was scheduled under). pending_default_admin_delay_change: Option, /// Current minimum delay (ms) for root role transfer / renounce. /// Mutable through the delayed change flow only. @@ -186,23 +199,23 @@ public struct RoleData has store { admin_role: TypeName, } -/// Snapshot of a pending change to the root role holder. +/// Snapshot of a pending change to the default admin. /// /// `new_admin = some(addr)` represents a pending transfer β€” `addr` accepts /// after the delay via `accept_default_admin_transfer`. /// -/// `new_admin = none` represents a pending renounce β€” the current root holder +/// `new_admin = none` represents a pending renounce β€” the current default admin /// finalizes after the delay via `accept_default_admin_renounce`. The same /// pending slot is reused so a transfer and a renounce are mutually -/// exclusive: scheduling either overwrites the other. +/// exclusive: scheduling either cancels and overwrites the other. public struct PendingAdminTransfer has drop, store { new_admin: Option
, execute_after_ms: u64, } -/// Snapshot of a pending change to `default_admin_delay_ms`. Becomes -/// effective only after `accept_default_admin_delay_change` runs at or past -/// `schedule_after_ms`. +/// Snapshot of a pending change to `default_admin_delay_ms`. Becomes effective +/// once `schedule_after_ms` has elapsed and is later cleared by a +/// delay-refreshing entrypoint. public struct PendingDelayChange has drop, store { new_delay_ms: u64, schedule_after_ms: u64, @@ -210,15 +223,20 @@ public struct PendingDelayChange has drop, store { // === Events === +// Access-control events are parameterized by `RootRole` so the event type +// identifies the registry without duplicating registry fields in every +// payload. Event attribution is available from the Sui event envelope +// (`SuiEvent.sender`) and is intentionally not duplicated in the Move event +// payloads. + /// Emitted when a role is granted to an account. /// /// Role identifiers are `TypeName`s β€” they embed the defining package address /// and module name of the role struct, so events from independently-published /// consumer protocols are distinguishable off-chain. -public struct RoleGranted has copy, drop { +public struct RoleGranted has copy, drop { role: TypeName, account: address, - sender: address, } /// Emitted when a role is removed from an account. @@ -226,10 +244,9 @@ public struct RoleGranted has copy, drop { /// Role identifiers are `TypeName`s β€” they embed the defining package address /// and module name of the role struct, so events from independently-published /// consumer protocols are distinguishable off-chain. -public struct RoleRevoked has copy, drop { +public struct RoleRevoked has copy, drop { role: TypeName, account: address, - sender: address, } /// Emitted when a role's admin role is reconfigured. @@ -237,7 +254,7 @@ public struct RoleRevoked has copy, drop { /// Role identifiers are `TypeName`s β€” they embed the defining package address /// and module name of each role struct, so events from independently-published /// consumer protocols are distinguishable off-chain. -public struct RoleAdminChanged has copy, drop { +public struct RoleAdminChanged has copy, drop { role: TypeName, previous_admin_role: TypeName, new_admin_role: TypeName, @@ -246,7 +263,7 @@ public struct RoleAdminChanged has copy, drop { /// Emitted when a transfer of the root role is scheduled. Distinct from /// `DefaultAdminRenounceScheduled` so off-chain consumers can tell the two /// kinds of pending state apart without inspecting payload. -public struct DefaultAdminTransferScheduled has copy, drop { +public struct DefaultAdminTransferScheduled has copy, drop { new_admin: address, execute_after_ms: u64, } @@ -254,32 +271,41 @@ public struct DefaultAdminTransferScheduled has copy, drop { /// Emitted when a renounce of the root role is scheduled. Distinct from /// `DefaultAdminTransferScheduled` so off-chain consumers can tell the two /// kinds of pending state apart without inspecting payload. -public struct DefaultAdminRenounceScheduled has copy, drop { +public struct DefaultAdminRenounceScheduled has copy, drop { execute_after_ms: u64, } -/// Emitted when a pending root role transfer or renounce is cancelled. Indexers -/// can correlate with the prior `DefaultAdminTransferScheduled` or -/// `DefaultAdminRenounceScheduled` event to know which kind was cancelled. -public struct DefaultAdminTransferCancelled has copy, drop {} +/// Emitted when a pending root role transfer is cancelled, either explicitly or +/// because it is overwritten by a new pending action. +public struct DefaultAdminTransferCancelled has copy, drop {} + +/// Emitted when a pending root role renounce is cancelled, either explicitly or +/// because it is overwritten by a new pending action. +public struct DefaultAdminRenounceCancelled has copy, drop {} /// Emitted when a change to `default_admin_delay_ms` is scheduled. -public struct DefaultAdminDelayChangeScheduled has copy, drop { +public struct DefaultAdminDelayChangeScheduled has copy, drop { new_delay_ms: u64, schedule_after_ms: u64, } -/// Emitted when a pending `default_admin_delay_ms` change is cancelled. -public struct DefaultAdminDelayChangeCancelled has copy, drop {} +/// Emitted when an unelapsed pending `default_admin_delay_ms` change is +/// cancelled, either explicitly or because it is overwritten by a new pending +/// delay change. +public struct DefaultAdminDelayChangeCancelled has copy, drop {} // === Constructor === -/// Create the singleton `AccessControl` for a module. +/// Create the singleton `AccessControl` for a module, with the transaction +/// sender as the initial default admin. /// /// The caller passes their module's One-Time Witness as `otw`. The runtime /// `is_one_time_witness` check confirms the value is genuine β€” this is what /// enforces Invariant 1 (one registry per module, initialized at first -/// publish). The transaction sender automatically becomes the root role holder. +/// publish). The transaction sender automatically becomes the default admin. +/// Prefer this constructor when the publishing transaction sender should be the +/// initial default admin. It derives the admin from `ctx.sender()`, so `init` +/// does not need to hard-code or forward an explicit address. /// /// The expected call site is the consumer module's `init` function, where /// the VM has already produced exactly one OTW value. That `init` function is @@ -303,34 +329,55 @@ public fun new( otw: RootRole, default_admin_delay_ms: u64, ctx: &mut TxContext, +): AccessControl { + new_with_admin(otw, ctx.sender(), default_admin_delay_ms, ctx) +} + +/// Create the singleton `AccessControl` for a module, with an explicit initial +/// default admin. +/// +/// Use this variant only when the initial default admin intentionally differs +/// from the publishing transaction sender, for example a multisig or governance +/// address. Otherwise prefer `new`, which derives the default admin from +/// `ctx.sender()`. +/// +/// #### Parameters +/// - `otw`: a VM-issued One-Time Witness of type `RootRole`. Consumed by the call. +/// - `initial_admin`: the address that receives the root role and becomes the default admin. +/// - `default_admin_delay_ms`: timelock (ms) that applies to root role transfer and renounce. Mutable post-creation via `begin_default_admin_delay_change`. +/// - `ctx`: transaction context. +/// +/// #### Returns +/// - The freshly-minted singleton `AccessControl` registry. The caller +/// is responsible for sharing, embedding, or otherwise positioning it. +/// +/// #### Aborts +/// - `ENotOneTimeWitness` if `otw` is not a true One-Time Witness. +/// - `EDelayTooLarge` if `default_admin_delay_ms` exceeds `MAX_DEFAULT_ADMIN_DELAY_MS`. +/// - `EZeroAddress` if `initial_admin` is `@0x0`. +public fun new_with_admin( + otw: RootRole, + initial_admin: address, + default_admin_delay_ms: u64, + ctx: &mut TxContext, ): AccessControl { assert!(types::is_one_time_witness(&otw), ENotOneTimeWitness); assert!(default_admin_delay_ms <= MAX_DEFAULT_ADMIN_DELAY_MS, EDelayTooLarge); + assert!(initial_admin != @0x0, EZeroAddress); - let sender = ctx.sender(); let root_type = with_original_ids(); - let mut ac = AccessControl { + let ac = AccessControl { id: object::new(ctx), - roles: bag::new(ctx), + roles: table::new(ctx), protected_root: root_type, + default_admin: option::some(initial_admin), pending_default_admin: option::none(), pending_default_admin_delay_change: option::none(), default_admin_delay_ms, }; - ac - .roles - .add( - root_type, - RoleData { - members: vec_set::singleton(sender), - // Root role is self-administering. - admin_role: root_type, - }, - ); - - event::emit(RoleGranted { role: root_type, account: sender, sender }); + event::emit(RoleGranted { role: root_type, account: initial_admin }); ac } @@ -345,8 +392,9 @@ public fun new( /// - `account`: the address being checked. /// /// #### Returns -/// - `true` if `account` currently holds `Role`, `false` otherwise. Returns -/// `false` for roles that have never been registered. +/// - `true` if `account` currently holds `Role`, `false` otherwise. Root role +/// membership is read from the dedicated `default_admin` field; non-root roles +/// that have never been registered return `false`. public fun has_role(ac: &AccessControl, account: address): bool { ac.has_role_by_name(with_original_ids(), account) } @@ -359,7 +407,9 @@ public fun has_role(ac: &AccessControl, account: addre /// /// #### Returns /// - The `TypeName` of `Role`'s admin role. Defaults to the root role's -/// `TypeName` for roles that have no entry yet. +/// `TypeName` for roles that have no entry yet. The root role itself is +/// self-administering, so calling this with `Role = RootRole` also returns the +/// root role's `TypeName`. /// /// #### Aborts /// - `EForeignRole` if `Role`'s home module differs from `RootRole`'s. @@ -418,11 +468,11 @@ public fun grant_role( ); }; - let role_data = ac.roles.borrow_mut<_, RoleData>(role_name); + let role_data = ac.roles.borrow_mut(role_name); if (role_data.members.contains(&account)) return; role_data.members.insert(account); - event::emit(RoleGranted { role: role_name, account, sender: ctx.sender() }); + event::emit(RoleGranted { role: role_name, account }); } /// Revoke role `Role` from `account`. Caller must hold the admin role of `Role`. @@ -450,11 +500,11 @@ public fun revoke_role( if (!ac.roles.contains(role_name)) return; - let role_data = ac.roles.borrow_mut<_, RoleData>(role_name); + let role_data = ac.roles.borrow_mut(role_name); if (!role_data.members.contains(&account)) return; role_data.members.remove(&account); - event::emit(RoleRevoked { role: role_name, account, sender: ctx.sender() }); + event::emit(RoleRevoked { role: role_name, account }); } /// Voluntarily relinquish role `Role`. @@ -466,31 +516,25 @@ public fun revoke_role( /// #### Parameters /// - `Role` (type): the role to relinquish. /// - `ac`: the registry to mutate. -/// - `account`: the address relinquishing `Role`. /// - `ctx`: transaction context. /// /// #### Aborts -/// - `ECannotRenounceForOtherAccount` if `account != ctx.sender()`. /// - `EForeignRole` if `Role`'s home module differs from `RootRole`'s. /// - `ECannotManageRootRole` if `Role` is the root role. -public fun renounce_role( - ac: &mut AccessControl, - account: address, - ctx: &mut TxContext, -) { - assert!(account == ctx.sender(), ECannotRenounceForOtherAccount); +public fun renounce_role(ac: &mut AccessControl, ctx: &mut TxContext) { assert_home_module(); + let sender = ctx.sender(); let role_name = with_original_ids(); assert!(role_name != ac.protected_root, ECannotManageRootRole); if (!ac.roles.contains(role_name)) return; - let role_data = ac.roles.borrow_mut<_, RoleData>(role_name); - if (!role_data.members.contains(&account)) return; + let role_data = ac.roles.borrow_mut(role_name); + if (!role_data.members.contains(&sender)) return; - role_data.members.remove(&account); - event::emit(RoleRevoked { role: role_name, account, sender: ctx.sender() }); + role_data.members.remove(&sender); + event::emit(RoleRevoked { role: role_name, account: sender }); } /// Set the admin role of `Role` to `AdminRole`. Caller must hold the current @@ -522,7 +566,7 @@ public fun set_role_admin( let new_admin_name = with_original_ids(); if (ac.roles.contains(role_name)) { - ac.roles.borrow_mut<_, RoleData>(role_name).admin_role = new_admin_name; + ac.roles.borrow_mut(role_name).admin_role = new_admin_name; } else { ac .roles @@ -532,7 +576,7 @@ public fun set_role_admin( ); }; - event::emit(RoleAdminChanged { + event::emit(RoleAdminChanged { role: role_name, previous_admin_role, new_admin_role: new_admin_name, @@ -545,8 +589,9 @@ public fun set_role_admin( /// /// Caller must hold the root role. The transfer cannot be accepted until /// `default_admin_delay_ms` has elapsed. An existing pending transfer or -/// renounce is overwritten β€” the caller can correct a wrong target (or -/// switch between transfer and renounce) without cancelling first. +/// renounce is cancelled and overwritten β€” the caller can correct a wrong +/// target (or switch between transfer and renounce) without cancelling first. +/// If a pending delay change has elapsed, it is applied before scheduling. /// /// #### Parameters /// - `ac`: the registry to mutate. @@ -557,6 +602,7 @@ public fun set_role_admin( /// #### Aborts /// - `EUnauthorized` if the caller does not hold the root role. /// - `EZeroAddress` if `new_admin` is `@0x0`. Use `begin_default_admin_renounce` to lock the registry permanently. +/// - `EDefaultAdminTransferToSelf` if `new_admin` is the current default admin. public fun begin_default_admin_transfer( ac: &mut AccessControl, new_admin: address, @@ -565,15 +611,18 @@ public fun begin_default_admin_transfer( ) { assert!(ac.has_role_by_name(ac.protected_root, ctx.sender()), EUnauthorized); assert!(new_admin != @0x0, EZeroAddress); + assert!(new_admin != ctx.sender(), EDefaultAdminTransferToSelf); + refresh_default_admin_delay(ac, clock); let execute_after_ms = clock.timestamp_ms() + ac.default_admin_delay_ms; + cancel_pending_default_admin(ac); ac.pending_default_admin = option::some(PendingAdminTransfer { new_admin: option::some(new_admin), execute_after_ms, }); - event::emit(DefaultAdminTransferScheduled { new_admin, execute_after_ms }); + event::emit(DefaultAdminTransferScheduled { new_admin, execute_after_ms }); } /// Accept a pending root role transfer. Caller must be the pending new admin @@ -609,19 +658,12 @@ public fun accept_default_admin_transfer( let _ = ac.pending_default_admin.extract(); - let sender = ctx.sender(); let root_type = ac.protected_root; - // Root role has at most one holder; find and revoke atomically. - let keys = ac.roles.borrow<_, RoleData>(root_type).members.keys(); - if (!keys.is_empty()) { - let old_admin = keys[0]; - ac.roles.borrow_mut<_, RoleData>(root_type).members.remove(&old_admin); - event::emit(RoleRevoked { role: root_type, account: old_admin, sender }); - }; - - ac.roles.borrow_mut<_, RoleData>(root_type).members.insert(new_admin); - event::emit(RoleGranted { role: root_type, account: new_admin, sender }); + let old_admin = *ac.default_admin.borrow(); + event::emit(RoleRevoked { role: root_type, account: old_admin }); + ac.default_admin = option::some(new_admin); + event::emit(RoleGranted { role: root_type, account: new_admin }); } /// Initiate a renounce of the root role. @@ -629,7 +671,8 @@ public fun accept_default_admin_transfer( /// Caller must hold the root role. The renounce cannot be finalized until /// `default_admin_delay_ms` has elapsed, giving the protocol a cancel window /// before the registry becomes permanently unmanaged. An existing pending -/// transfer or renounce is overwritten. +/// transfer or renounce is cancelled and overwritten. If a pending delay +/// change has elapsed, it is applied before scheduling. /// /// #### Security Warning /// @@ -650,15 +693,17 @@ public fun begin_default_admin_renounce( ctx: &mut TxContext, ) { assert!(ac.has_role_by_name(ac.protected_root, ctx.sender()), EUnauthorized); + refresh_default_admin_delay(ac, clock); let execute_after_ms = clock.timestamp_ms() + ac.default_admin_delay_ms; + cancel_pending_default_admin(ac); ac.pending_default_admin = option::some(PendingAdminTransfer { new_admin: option::none(), execute_after_ms, }); - event::emit(DefaultAdminRenounceScheduled { execute_after_ms }); + event::emit(DefaultAdminRenounceScheduled { execute_after_ms }); } /// Finalize a pending root role renounce. Caller must currently hold the @@ -695,13 +740,13 @@ public fun accept_default_admin_renounce( let sender = ctx.sender(); let root_type = ac.protected_root; - ac.roles.borrow_mut<_, RoleData>(root_type).members.remove(&sender); - event::emit(RoleRevoked { role: root_type, account: sender, sender }); + ac.default_admin = option::none(); + event::emit(RoleRevoked { role: root_type, account: sender }); } /// Cancel a pending root role transfer or renounce. Caller must hold the -/// root role. The same function clears either kind; off-chain consumers -/// correlate with the prior schedule event. +/// root role. Emits `DefaultAdminTransferCancelled` or +/// `DefaultAdminRenounceCancelled` according to the pending action kind. /// /// #### Parameters /// - `ac`: the registry to mutate. @@ -717,9 +762,7 @@ public fun cancel_default_admin_transfer( assert!(ac.pending_default_admin.is_some(), ENoPendingAdminTransfer); assert!(ac.has_role_by_name(ac.protected_root, ctx.sender()), EUnauthorized); - let _ = ac.pending_default_admin.extract(); - - event::emit(DefaultAdminTransferCancelled {}); + cancel_pending_default_admin(ac); } // === Default Admin Delay Change === @@ -741,8 +784,9 @@ public fun cancel_default_admin_transfer( /// the delay they were scheduled under.) /// - **No change** (`new_delay_ms == current`): wait = 0. /// -/// Caller must hold the root role. An existing pending delay change is -/// overwritten. +/// Caller must hold the root role. An unelapsed pending delay change is +/// cancelled and overwritten. An elapsed pending delay change is applied first, +/// then the new schedule is computed from that applied value. /// /// #### Parameters /// - `ac`: the registry to mutate. @@ -761,6 +805,7 @@ public fun begin_default_admin_delay_change( ) { assert!(ac.has_role_by_name(ac.protected_root, ctx.sender()), EUnauthorized); assert!(new_delay_ms <= MAX_DEFAULT_ADMIN_DELAY_MS, EDelayTooLarge); + refresh_default_admin_delay(ac, clock); let current = ac.default_admin_delay_ms; let wait = if (new_delay_ms > current) { @@ -770,65 +815,42 @@ public fun begin_default_admin_delay_change( }; let schedule_after_ms = clock.timestamp_ms() + wait; + if (ac.pending_default_admin_delay_change.is_some()) { + event::emit(DefaultAdminDelayChangeCancelled {}); + }; ac.pending_default_admin_delay_change = option::some(PendingDelayChange { new_delay_ms, schedule_after_ms, }); - event::emit(DefaultAdminDelayChangeScheduled { new_delay_ms, schedule_after_ms }); + event::emit(DefaultAdminDelayChangeScheduled { new_delay_ms, schedule_after_ms }); } -/// Apply a pending delay change once its schedule has elapsed. +/// Cancel an unelapsed pending delay change. Caller must hold the root role. /// -/// No authorization required β€” the schedule was committed by the root admin -/// at `begin` time; `accept` is just the state transition. Anyone can call -/// it. -/// -/// #### Parameters -/// - `ac`: the registry to mutate. -/// - `clock`: current clock; used to verify the schedule has passed. -/// - `_ctx`: transaction context. -/// -/// #### Aborts -/// - `ENoPendingDelayChange` if no pending change exists. -/// - `EDelayNotElapsed` if `schedule_after_ms` has not been reached. -public fun accept_default_admin_delay_change( - ac: &mut AccessControl, - clock: &Clock, - _ctx: &mut TxContext, -) { - assert!(ac.pending_default_admin_delay_change.is_some(), ENoPendingDelayChange); - - let (new_delay_ms, schedule_after_ms) = { - let pending = ac.pending_default_admin_delay_change.borrow(); - (pending.new_delay_ms, pending.schedule_after_ms) - }; - assert!(clock.timestamp_ms() >= schedule_after_ms, EDelayNotElapsed); - - let _ = ac.pending_default_admin_delay_change.extract(); - ac.default_admin_delay_ms = new_delay_ms; -} - -/// Cancel a pending delay change. Caller must hold the root role. +/// If the pending delay change has already elapsed, this call aborts because +/// there is no active pending change to cancel. /// /// #### Parameters /// - `ac`: the registry to mutate. +/// - `clock`: current clock; used to detect elapsed delay changes. /// - `ctx`: transaction context. /// /// #### Aborts -/// - `ENoPendingDelayChange` if no pending change exists. +/// - `ENoPendingDelayChange` if no active pending change exists. /// - `EUnauthorized` if the caller does not hold the root role. public fun cancel_default_admin_delay_change( ac: &mut AccessControl, + clock: &Clock, ctx: &mut TxContext, ) { - assert!(ac.pending_default_admin_delay_change.is_some(), ENoPendingDelayChange); + assert!(has_active_pending_default_admin_delay_change(ac, clock), ENoPendingDelayChange); assert!(ac.has_role_by_name(ac.protected_root, ctx.sender()), EUnauthorized); let _ = ac.pending_default_admin_delay_change.extract(); - event::emit(DefaultAdminDelayChangeCancelled {}); + event::emit(DefaultAdminDelayChangeCancelled {}); } // === Auth Issuance === @@ -847,7 +869,8 @@ public fun cancel_default_admin_delay_change( /// /// #### Returns /// - An `Auth` capability bound to `ctx.sender()`. Pass it by reference -/// to gated functions in the same PTB. +/// to gated functions in the same PTB. The witness proves membership at mint +/// time, not after a later same-PTB renounce. /// /// #### Aborts /// - `EForeignRole` if `Role`'s home module differs from `RootRole`'s. @@ -879,15 +902,27 @@ public fun protected_root(ac: &AccessControl): TypeName { ac.protected_root } +/// The current default admin, or `none` if the root role has been renounced. +/// +/// #### Parameters +/// - `ac`: the registry to query. +public fun default_admin(ac: &AccessControl): Option
{ + ac.default_admin +} + /// Upper bound on `default_admin_delay_ms`. public fun max_default_admin_delay_ms(): u64 { MAX_DEFAULT_ADMIN_DELAY_MS } -/// Currently-configured timelock (ms) for root role transfer / renounce. +/// Effective timelock (ms) for root role transfer / renounce. +/// +/// If a pending delay change has elapsed, this returns the pending new delay +/// even if the stored pending slot has not been cleared yet. /// /// #### Parameters /// - `ac`: the registry to query. -public fun default_admin_delay_ms(ac: &AccessControl): u64 { - ac.default_admin_delay_ms +/// - `clock`: current clock; used to determine whether a pending change has elapsed. +public fun default_admin_delay_ms(ac: &AccessControl, clock: &Clock): u64 { + effective_default_admin_delay_ms(ac, clock) } /// Whether there is any pending action on the root role β€” either a @@ -954,62 +989,133 @@ public fun max_delay_increase_wait_ms(): u64 { MAX_DELAY_INCREASE_WAIT_MS } -/// Whether a delay change is pending. +/// Whether a delay change is still active and cancellable. /// /// #### Parameters /// - `ac`: the registry to query. -public fun has_pending_default_admin_delay_change(ac: &AccessControl): bool { - ac.pending_default_admin_delay_change.is_some() +/// - `clock`: current clock; used to determine whether a pending change has elapsed. +public fun has_pending_default_admin_delay_change( + ac: &AccessControl, + clock: &Clock, +): bool { + has_active_pending_default_admin_delay_change(ac, clock) } -/// The proposed new delay value of the pending change. +/// The proposed new delay value of the active pending change. /// /// #### Parameters /// - `ac`: the registry to query. +/// - `clock`: current clock; used to determine whether a pending change has elapsed. /// /// #### Returns -/// - `some(new_delay_ms)` with the proposed delay if a change is pending. -/// - `none` if there is no pending change. +/// - `some(new_delay_ms)` with the proposed delay if a change is active. +/// - `none` if there is no active pending change. public fun pending_default_admin_delay_change_new_delay_ms( ac: &AccessControl, + clock: &Clock, ): Option { if (ac.pending_default_admin_delay_change.is_none()) return option::none(); - option::some(ac.pending_default_admin_delay_change.borrow().new_delay_ms) + let pending = ac.pending_default_admin_delay_change.borrow(); + if (clock.timestamp_ms() >= pending.schedule_after_ms) return option::none(); + option::some(pending.new_delay_ms) } -/// The timestamp at which the pending delay change becomes acceptable. +/// The timestamp at which the active pending delay change becomes effective. /// /// #### Parameters /// - `ac`: the registry to query. +/// - `clock`: current clock; used to determine whether a pending change has elapsed. /// /// #### Returns -/// - `some(ts)` with the millisecond timestamp the pending change unlocks at. -/// - `none` if there is no pending change. +/// - `some(ts)` with the millisecond timestamp the pending change takes effect at. +/// - `none` if there is no active pending change. +/// - `none` once `clock.timestamp_ms() >= ts`; the change is then effective and +/// no longer cancellable. public fun pending_default_admin_delay_change_schedule_after_ms( ac: &AccessControl, + clock: &Clock, ): Option { if (ac.pending_default_admin_delay_change.is_none()) return option::none(); - option::some(ac.pending_default_admin_delay_change.borrow().schedule_after_ms) + let pending = ac.pending_default_admin_delay_change.borrow(); + if (clock.timestamp_ms() >= pending.schedule_after_ms) return option::none(); + option::some(pending.schedule_after_ms) } // === Internal Helpers === +/// Returns the delay that should be used at `clock.timestamp_ms()`, including +/// elapsed pending changes whose stored pending slot has not been cleared yet. +fun effective_default_admin_delay_ms(ac: &AccessControl, clock: &Clock): u64 { + if (ac.pending_default_admin_delay_change.is_some()) { + let pending = ac.pending_default_admin_delay_change.borrow(); + if (clock.timestamp_ms() >= pending.schedule_after_ms) { + return pending.new_delay_ms + }; + }; + + ac.default_admin_delay_ms +} + +/// Returns true only while a stored pending delay change has not elapsed yet. +/// Elapsed changes are effective, but no longer active or cancellable. +fun has_active_pending_default_admin_delay_change( + ac: &AccessControl, + clock: &Clock, +): bool { + if (ac.pending_default_admin_delay_change.is_none()) return false; + + clock.timestamp_ms() < ac.pending_default_admin_delay_change.borrow().schedule_after_ms +} + +/// Applies an elapsed pending delay change and clears it. Returns true if a +/// pending change was materialized. +fun refresh_default_admin_delay(ac: &mut AccessControl, clock: &Clock): bool { + if (ac.pending_default_admin_delay_change.is_none()) return false; + + let (new_delay_ms, schedule_after_ms) = { + let pending = ac.pending_default_admin_delay_change.borrow(); + (pending.new_delay_ms, pending.schedule_after_ms) + }; + if (clock.timestamp_ms() < schedule_after_ms) return false; + + let _ = ac.pending_default_admin_delay_change.extract(); + ac.default_admin_delay_ms = new_delay_ms; + true +} + +/// Clears a pending root role action, emitting the cancellation event that +/// matches the action kind. +fun cancel_pending_default_admin(ac: &mut AccessControl) { + if (ac.pending_default_admin.is_none()) return; + + let pending = ac.pending_default_admin.extract(); + if (pending.new_admin.is_some()) { + event::emit(DefaultAdminTransferCancelled {}); + } else { + event::emit(DefaultAdminRenounceCancelled {}); + }; +} + /// Membership check by `TypeName` β€” used internally when the admin role is -/// known only as a `TypeName` value, not as a type parameter. +/// known only as a `TypeName` value, not as a type parameter. Root role +/// membership is stored separately as the current default admin. fun has_role_by_name( ac: &AccessControl, role: TypeName, account: address, ): bool { + if (role == ac.protected_root) { + return ac.default_admin == option::some(account) + }; if (!ac.roles.contains(role)) return false; - ac.roles.borrow<_, RoleData>(role).members.contains(&account) + ac.roles.borrow(role).members.contains(&account) } /// Returns the admin role `TypeName` of `role`. Defaults to the root role for /// roles that have no entry yet. fun get_role_admin_name(ac: &AccessControl, role: TypeName): TypeName { if (!ac.roles.contains(role)) return ac.protected_root; - ac.roles.borrow<_, RoleData>(role).admin_role + ac.roles.borrow(role).admin_role } /// Asserts `Role` and `RootRole` come from the same package + module. @@ -1031,53 +1137,70 @@ fun assert_home_module() { // === Test-Only Helpers === #[test_only] -public fun test_new_role_granted(role: TypeName, account: address, sender: address): RoleGranted { - RoleGranted { role, account, sender } +public fun test_new_role_granted( + role: TypeName, + account: address, +): RoleGranted { + RoleGranted { role, account } } #[test_only] -public fun test_new_role_revoked(role: TypeName, account: address, sender: address): RoleRevoked { - RoleRevoked { role, account, sender } +public fun test_new_role_revoked( + role: TypeName, + account: address, +): RoleRevoked { + RoleRevoked { role, account } } #[test_only] -public fun test_new_role_admin_changed( +public fun test_new_role_admin_changed( role: TypeName, previous_admin_role: TypeName, new_admin_role: TypeName, -): RoleAdminChanged { - RoleAdminChanged { role, previous_admin_role, new_admin_role } +): RoleAdminChanged { + RoleAdminChanged { role, previous_admin_role, new_admin_role } } #[test_only] -public fun test_new_default_admin_transfer_scheduled( +public fun test_new_default_admin_transfer_scheduled( new_admin: address, execute_after_ms: u64, -): DefaultAdminTransferScheduled { - DefaultAdminTransferScheduled { new_admin, execute_after_ms } +): DefaultAdminTransferScheduled { + DefaultAdminTransferScheduled { new_admin, execute_after_ms } +} + +#[test_only] +public fun test_new_default_admin_transfer_cancelled(): DefaultAdminTransferCancelled< + RootRole, +> { + DefaultAdminTransferCancelled {} } #[test_only] -public fun test_new_default_admin_transfer_cancelled(): DefaultAdminTransferCancelled { - DefaultAdminTransferCancelled {} +public fun test_new_default_admin_renounce_cancelled(): DefaultAdminRenounceCancelled< + RootRole, +> { + DefaultAdminRenounceCancelled {} } #[test_only] -public fun test_new_default_admin_renounce_scheduled( +public fun test_new_default_admin_renounce_scheduled( execute_after_ms: u64, -): DefaultAdminRenounceScheduled { - DefaultAdminRenounceScheduled { execute_after_ms } +): DefaultAdminRenounceScheduled { + DefaultAdminRenounceScheduled { execute_after_ms } } #[test_only] -public fun test_new_default_admin_delay_change_scheduled( +public fun test_new_default_admin_delay_change_scheduled( new_delay_ms: u64, schedule_after_ms: u64, -): DefaultAdminDelayChangeScheduled { - DefaultAdminDelayChangeScheduled { new_delay_ms, schedule_after_ms } +): DefaultAdminDelayChangeScheduled { + DefaultAdminDelayChangeScheduled { new_delay_ms, schedule_after_ms } } #[test_only] -public fun test_new_default_admin_delay_change_cancelled(): DefaultAdminDelayChangeCancelled { - DefaultAdminDelayChangeCancelled {} +public fun test_new_default_admin_delay_change_cancelled< + RootRole, +>(): DefaultAdminDelayChangeCancelled { + DefaultAdminDelayChangeCancelled {} } diff --git a/contracts/access/tests/access_control_tests.move b/contracts/access/tests/access_control_tests.move index e8b8b754..f3881bbe 100644 --- a/contracts/access/tests/access_control_tests.move +++ b/contracts/access/tests/access_control_tests.move @@ -72,18 +72,48 @@ fun test_new_with_otw_succeeds() { scenario.ctx(), ); - // Sender becomes the root holder. + // Sender becomes the default admin. assert!(ac.has_role<_, ACCESS_CONTROL_TESTS>(deployer)); + assert_eq!(ac.default_admin(), option::some(deployer)); // Protected root TypeName matches the OTW type. assert_eq!(ac.protected_root(), with_original_ids()); - // RoleGranted emitted for the root holder. Asserted in the same tx as + // RoleGranted emitted for the default admin. Asserted in the same tx as // construction because `events_by_type` is per-transaction. - let granted = event::events_by_type(); + let granted = event::events_by_type>(); assert_eq!(granted.length(), 1); - let expected = access_control::test_new_role_granted( + let expected = access_control::test_new_role_granted( with_original_ids(), deployer, - deployer, + ); + assert_eq!(granted[0], expected); + + transfer::public_share_object(ac); + scenario.end(); +} + +#[test] +#[allow(lint(share_owned))] +fun test_new_with_admin_sets_explicit_root_holder() { + let deployer = @0xA; + let initial_admin = @0xB; + let mut scenario = test_scenario::begin(deployer); + let ac = access_control::new_with_admin( + ACCESS_CONTROL_TESTS {}, + initial_admin, + 0, + scenario.ctx(), + ); + + assert!(!ac.has_role<_, ACCESS_CONTROL_TESTS>(deployer)); + assert!(ac.has_role<_, ACCESS_CONTROL_TESTS>(initial_admin)); + assert_eq!(ac.default_admin(), option::some(initial_admin)); + assert_eq!(ac.protected_root(), with_original_ids()); + + let granted = event::events_by_type>(); + assert_eq!(granted.length(), 1); + let expected = access_control::test_new_role_granted( + with_original_ids(), + initial_admin, ); assert_eq!(granted[0], expected); @@ -111,13 +141,28 @@ fun test_new_rejects_excessive_delay() { abort 999 } +#[test, expected_failure(abort_code = access_control::EZeroAddress)] +fun test_new_with_admin_rejects_zero_address() { + let deployer = @0xA; + let mut scenario = test_scenario::begin(deployer); + let _ac = access_control::new_with_admin( + ACCESS_CONTROL_TESTS {}, + @0x0, + 0, + scenario.ctx(), + ); + abort 999 +} + #[test] fun test_new_accepts_max_delay() { let deployer = @0xA; let max = access_control::max_default_admin_delay_ms(); - let scenario = setup(deployer, max); + let mut scenario = setup(deployer, max); let ac = take_ac(&scenario); - assert_eq!(ac.default_admin_delay_ms(), max); + let clk = clock::create_for_testing(scenario.ctx()); + assert_eq!(ac.default_admin_delay_ms(&clk), max); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } @@ -125,9 +170,11 @@ fun test_new_accepts_max_delay() { #[test] fun test_new_accepts_zero_delay() { let deployer = @0xA; - let scenario = setup(deployer, 0); + let mut scenario = setup(deployer, 0); let ac = take_ac(&scenario); - assert_eq!(ac.default_admin_delay_ms(), 0); + let clk = clock::create_for_testing(scenario.ctx()); + assert_eq!(ac.default_admin_delay_ms(&clk), 0); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } @@ -166,12 +213,11 @@ fun test_grant_role_happy_path() { // `events_by_type` is per-transaction; the construction event from `setup` // lives in the previous tx, so only the grant event shows up here. - let granted = event::events_by_type(); + let granted = event::events_by_type>(); assert_eq!(granted.length(), 1); - let expected = access_control::test_new_role_granted( + let expected = access_control::test_new_role_granted( with_original_ids(), alice, - deployer, ); assert_eq!(granted[0], expected); @@ -187,19 +233,23 @@ fun test_grant_role_idempotent() { let mut ac = take_ac(&scenario); ac.grant_role<_, AdminA>(alice, scenario.ctx()); - let count_after_first = event::events_by_type().length(); + let count_after_first = event::events_by_type< + access_control::RoleGranted, + >().length(); // Second grant to the same account is a no-op β€” no new event, no abort. ac.grant_role<_, AdminA>(alice, scenario.ctx()); - let count_after_second = event::events_by_type().length(); + let count_after_second = event::events_by_type< + access_control::RoleGranted, + >().length(); assert_eq!(count_after_first, count_after_second); test_scenario::return_shared(ac); scenario.end(); } -// Non-root roles have no membership cap (unlike root, which is capped at one -// holder by `accept_default_admin_transfer`'s revoke-then-grant logic). Pin -// that two distinct accounts can simultaneously hold the same non-root role. +// Non-root roles have no membership cap (unlike root, which is stored as a +// single `Option
` holder). Pin that two distinct accounts can +// simultaneously hold the same non-root role. #[test] fun test_grant_role_multiple_holders() { let deployer = @0xA; @@ -272,12 +322,11 @@ fun test_revoke_role_happy_path() { ac.revoke_role<_, AdminA>(alice, scenario.ctx()); assert!(!ac.has_role<_, AdminA>(alice)); - let revoked = event::events_by_type(); + let revoked = event::events_by_type>(); assert_eq!(revoked.length(), 1); - let expected = access_control::test_new_role_revoked( + let expected = access_control::test_new_role_revoked( with_original_ids(), alice, - deployer, ); assert_eq!(revoked[0], expected); @@ -293,11 +342,16 @@ fun test_revoke_role_idempotent_non_member() { let mut ac = take_ac(&scenario); ac.grant_role<_, AdminA>(alice, scenario.ctx()); - let revoked_count = event::events_by_type().length(); + let revoked_count = event::events_by_type< + access_control::RoleRevoked, + >().length(); // Carol never had the role β€” revoking is a no-op. ac.revoke_role<_, AdminA>(@0xC, scenario.ctx()); - assert_eq!(event::events_by_type().length(), revoked_count); + assert_eq!( + event::events_by_type>().length(), + revoked_count, + ); test_scenario::return_shared(ac); scenario.end(); @@ -309,9 +363,12 @@ fun test_revoke_role_idempotent_unknown_role() { let mut scenario = setup(deployer, 0); let mut ac = take_ac(&scenario); - // RoleY was never granted, so it has no bag entry. Revoking is a no-op. + // RoleY was never granted, so it has no role entry. Revoking is a no-op. ac.revoke_role<_, RoleY>(@0xB, scenario.ctx()); - assert_eq!(event::events_by_type().length(), 0); + assert_eq!( + event::events_by_type>().length(), + 0, + ); test_scenario::return_shared(ac); scenario.end(); @@ -365,15 +422,14 @@ fun test_renounce_role_happy_path() { scenario.next_tx(alice); let mut ac = take_ac(&scenario); - ac.renounce_role<_, AdminA>(alice, scenario.ctx()); + ac.renounce_role<_, AdminA>(scenario.ctx()); assert!(!ac.has_role<_, AdminA>(alice)); - let revoked = event::events_by_type(); + let revoked = event::events_by_type>(); let last = revoked[revoked.length() - 1]; - let expected = access_control::test_new_role_revoked( + let expected = access_control::test_new_role_revoked( with_original_ids(), alice, - alice, ); assert_eq!(last, expected); @@ -391,7 +447,7 @@ fun test_renounce_role_rejects_root() { let deployer = @0xA; let mut scenario = setup(deployer, 0); let mut ac = take_ac(&scenario); - ac.renounce_role<_, ACCESS_CONTROL_TESTS>(deployer, scenario.ctx()); + ac.renounce_role<_, ACCESS_CONTROL_TESTS>(scenario.ctx()); abort 999 } @@ -404,55 +460,51 @@ fun test_renounce_role_idempotent_non_member() { let mut ac = take_ac(&scenario); // alice never held AdminA β€” renounce is a no-op, no event. - ac.renounce_role<_, AdminA>(alice, scenario.ctx()); - assert_eq!(event::events_by_type().length(), 0); + ac.renounce_role<_, AdminA>(scenario.ctx()); + assert_eq!( + event::events_by_type>().length(), + 0, + ); test_scenario::return_shared(ac); scenario.end(); } -// Renounce idempotency: account == sender, role in bag, but the caller is not -// a member β€” the second early-return path. Distinct from the "role not in bag" -// path covered by the test above. +// Renounce idempotency: role entry exists, but the caller is not a member β€” +// the second early-return path. Distinct from the "no role entry" path covered +// by the test above. #[test] -fun test_renounce_role_idempotent_role_in_bag_non_member() { +fun test_renounce_role_idempotent_existing_role_non_member() { let deployer = @0xA; let alice = @0xB; let carol = @0xC; let mut scenario = setup(deployer, 0); let mut ac = take_ac(&scenario); - // Grant AdminA to alice β€” the role's bag entry now exists. + // Grant AdminA to alice β€” the role entry now exists. ac.grant_role<_, AdminA>(alice, scenario.ctx()); test_scenario::return_shared(ac); - // Carol is not a member of AdminA β€” exercises the "role in bag, not + // Carol is not a member of AdminA β€” exercises the "role entry exists, not // member" early-return branch. scenario.next_tx(carol); let mut ac = take_ac(&scenario); - ac.renounce_role<_, AdminA>(carol, scenario.ctx()); - assert_eq!(event::events_by_type().length(), 0); + ac.renounce_role<_, AdminA>(scenario.ctx()); + assert_eq!( + event::events_by_type>().length(), + 0, + ); test_scenario::return_shared(ac); scenario.end(); } -#[test, expected_failure(abort_code = access_control::ECannotRenounceForOtherAccount)] -fun test_renounce_role_rejects_other_account() { - let deployer = @0xA; - let mut scenario = setup(deployer, 0); - let mut ac = take_ac(&scenario); - // Sender is deployer; trying to renounce on behalf of @0xB must abort. - ac.renounce_role<_, AdminA>(@0xB, scenario.ctx()); - abort 999 -} - #[test, expected_failure(abort_code = access_control::EForeignRole)] fun test_renounce_role_rejects_foreign() { let deployer = @0xA; let mut scenario = setup(deployer, 0); let mut ac = take_ac(&scenario); - ac.renounce_role<_, ForeignRole>(deployer, scenario.ctx()); + ac.renounce_role<_, ForeignRole>(scenario.ctx()); abort 999 } @@ -472,9 +524,9 @@ fun test_set_role_admin_happy_path() { assert_eq!(ac.get_role_admin<_, RoleX>(), with_original_ids()); // RoleAdminChanged emitted. - let changed = event::events_by_type(); + let changed = event::events_by_type>(); assert_eq!(changed.length(), 1); - let expected = access_control::test_new_role_admin_changed( + let expected = access_control::test_new_role_admin_changed( with_original_ids(), with_original_ids(), with_original_ids(), @@ -509,7 +561,7 @@ fun test_set_role_admin_lazy_create() { scenario.end(); } -// set_role_admin against a role that already has a bag entry β€” exercises +// set_role_admin against a role that already has an entry β€” exercises // the "update existing admin_role" branch (the lazy-create branch is covered // by the test above) and asserts the event reports previous = old admin // rather than empty / fresh-default. @@ -520,7 +572,7 @@ fun test_set_role_admin_updates_existing_role() { let mut scenario = setup(deployer, 0); let mut ac = take_ac(&scenario); - // First grant of RoleX creates the bag entry with admin = root. + // First grant of RoleX creates the role entry with admin = root. ac.grant_role<_, RoleX>(alice, scenario.ctx()); assert_eq!(ac.get_role_admin<_, RoleX>(), with_original_ids()); @@ -533,9 +585,9 @@ fun test_set_role_admin_updates_existing_role() { assert!(ac.has_role<_, RoleX>(alice)); // Event reports previous = root (NOT empty), new = AdminA. - let changed = event::events_by_type(); + let changed = event::events_by_type>(); assert_eq!(changed.length(), 1); - let expected = access_control::test_new_role_admin_changed( + let expected = access_control::test_new_role_admin_changed( with_original_ids(), with_original_ids(), with_original_ids(), @@ -636,7 +688,7 @@ fun test_has_role_unknown_role_returns_false() { let deployer = @0xA; let scenario = setup(deployer, 0); let ac = take_ac(&scenario); - // RoleX has no bag entry at all. + // RoleX has no role entry at all. assert!(!ac.has_role<_, RoleX>(deployer)); test_scenario::return_shared(ac); scenario.end(); @@ -707,9 +759,11 @@ fun test_protected_root_returns_root_typename() { #[test] fun test_default_admin_delay_ms_persisted() { let deployer = @0xA; - let scenario = setup(deployer, 12345); + let mut scenario = setup(deployer, 12345); let ac = take_ac(&scenario); - assert_eq!(ac.default_admin_delay_ms(), 12345); + let clk = clock::create_for_testing(scenario.ctx()); + assert_eq!(ac.default_admin_delay_ms(&clk), 12345); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } @@ -780,10 +834,16 @@ fun test_begin_admin_transfer_happy_path() { assert!(ac.has_pending_default_admin_transfer()); assert_eq!(ac.pending_default_admin_new_admin(), option::some(new_admin)); assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(delay)); + assert_eq!(ac.default_admin(), option::some(deployer)); - let scheduled = event::events_by_type(); + let scheduled = event::events_by_type< + access_control::DefaultAdminTransferScheduled, + >(); assert_eq!(scheduled.length(), 1); - let expected = access_control::test_new_default_admin_transfer_scheduled(new_admin, delay); + let expected = access_control::test_new_default_admin_transfer_scheduled( + new_admin, + delay, + ); assert_eq!(scheduled[0], expected); clock::destroy_for_testing(clk); @@ -812,6 +872,16 @@ fun test_begin_admin_transfer_rejects_zero_address() { abort 999 } +#[test, expected_failure(abort_code = access_control::EDefaultAdminTransferToSelf)] +fun test_begin_admin_transfer_rejects_self() { + let deployer = @0xA; + let mut scenario = setup(deployer, 0); + let mut ac = take_ac(&scenario); + let clk = clock::create_for_testing(scenario.ctx()); + ac.begin_default_admin_transfer(deployer, &clk, scenario.ctx()); + abort 999 +} + #[test] fun test_begin_admin_transfer_overwrites_pending() { let deployer = @0xA; @@ -828,6 +898,21 @@ fun test_begin_admin_transfer_overwrites_pending() { assert_eq!(ac.pending_default_admin_new_admin(), option::some(@0xC)); assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(50)); + let cancelled = event::events_by_type< + access_control::DefaultAdminTransferCancelled, + >(); + assert_eq!(cancelled.length(), 1); + let expected = access_control::test_new_default_admin_transfer_cancelled< + ACCESS_CONTROL_TESTS, + >(); + assert_eq!(cancelled[0], expected); + assert_eq!( + event::events_by_type< + access_control::DefaultAdminRenounceCancelled, + >().length(), + 0, + ); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); @@ -852,42 +937,53 @@ fun test_accept_admin_transfer_happy_path() { scenario.next_tx(new_admin); let mut ac = take_ac(&scenario); clk.set_for_testing(delay); - let granted_before = event::events_by_type().length(); - let revoked_before = event::events_by_type().length(); + let granted_before = event::events_by_type< + access_control::RoleGranted, + >().length(); + let revoked_before = event::events_by_type< + access_control::RoleRevoked, + >().length(); ac.accept_default_admin_transfer(&clk, scenario.ctx()); // Atomic rotation: old admin lost root, new admin gained it. assert!(!ac.has_role<_, ACCESS_CONTROL_TESTS>(deployer)); assert!(ac.has_role<_, ACCESS_CONTROL_TESTS>(new_admin)); + assert_eq!(ac.default_admin(), option::some(new_admin)); + let auth = ac.new_auth<_, ACCESS_CONTROL_TESTS>(scenario.ctx()); + assert_eq!(access_control::auth_addr(&auth), new_admin); // Pending state cleared. assert!(!ac.has_pending_default_admin_transfer()); assert!(ac.pending_default_admin_new_admin().is_none()); assert!(ac.pending_default_admin_execute_after_ms().is_none()); // Both events emitted in this transaction. - assert_eq!(event::events_by_type().length(), granted_before + 1); - assert_eq!(event::events_by_type().length(), revoked_before + 1); + assert_eq!( + event::events_by_type>().length(), + granted_before + 1, + ); + assert_eq!( + event::events_by_type>().length(), + revoked_before + 1, + ); // Field-level event payload assertions for the atomic rotation: the pair // of events (revoke-old, grant-new) must each carry the right role - // TypeName and the right addresses. - let granted = event::events_by_type(); + // TypeName and account address. + let granted = event::events_by_type>(); let last_granted = granted[granted.length() - 1]; assert_eq!( last_granted, - access_control::test_new_role_granted( + access_control::test_new_role_granted( with_original_ids(), new_admin, - new_admin, ), ); - let revoked = event::events_by_type(); + let revoked = event::events_by_type>(); let last_revoked = revoked[revoked.length() - 1]; assert_eq!( last_revoked, - access_control::test_new_role_revoked( + access_control::test_new_role_revoked( with_original_ids(), deployer, - new_admin, ), ); @@ -896,6 +992,54 @@ fun test_accept_admin_transfer_happy_path() { scenario.end(); } +#[test] +fun test_transferred_admin_can_manage_root_role_administered_roles() { + let deployer = @0xA; + let new_admin = @0xB; + let user = @0xC; + let mut scenario = setup(deployer, 0); + let mut ac = take_ac(&scenario); + let clk = clock::create_for_testing(scenario.ctx()); + + ac.begin_default_admin_transfer(new_admin, &clk, scenario.ctx()); + test_scenario::return_shared(ac); + + scenario.next_tx(new_admin); + let mut ac = take_ac(&scenario); + ac.accept_default_admin_transfer(&clk, scenario.ctx()); + ac.grant_role<_, AdminA>(user, scenario.ctx()); + + assert!(ac.has_role<_, AdminA>(user)); + assert_eq!(ac.get_role_admin<_, AdminA>(), with_original_ids()); + + clock::destroy_for_testing(clk); + test_scenario::return_shared(ac); + scenario.end(); +} + +#[test, expected_failure(abort_code = access_control::EUnauthorized)] +fun test_old_admin_cannot_manage_root_role_administered_roles_after_transfer() { + let deployer = @0xA; + let new_admin = @0xB; + let user = @0xC; + let mut scenario = setup(deployer, 0); + let mut ac = take_ac(&scenario); + let clk = clock::create_for_testing(scenario.ctx()); + + ac.begin_default_admin_transfer(new_admin, &clk, scenario.ctx()); + test_scenario::return_shared(ac); + + scenario.next_tx(new_admin); + let mut ac = take_ac(&scenario); + ac.accept_default_admin_transfer(&clk, scenario.ctx()); + test_scenario::return_shared(ac); + + scenario.next_tx(deployer); + let mut ac = take_ac(&scenario); + ac.grant_role<_, AdminA>(user, scenario.ctx()); + abort 999 +} + #[test, expected_failure(abort_code = access_control::ENoPendingAdminTransfer)] fun test_accept_admin_transfer_rejects_no_pending() { let deployer = @0xA; @@ -968,66 +1112,6 @@ fun test_accept_admin_transfer_at_exact_delay() { scenario.end(); } -// Self-transfer edge case: `old_admin == new_admin`. The function does NOT -// short-circuit β€” it emits the full revoke-old + grant-new pair even though -// they reference the same address, and the registry ends with the original -// holder still holding root. This pins the no-special-case decision so a future -// "optimization" to skip the events when `new_admin == sender` can't be made -// silently. -#[test] -fun test_accept_admin_transfer_to_self() { - let deployer = @0xA; - let delay = 100; - let mut scenario = setup(deployer, delay); - let mut ac = take_ac(&scenario); - - let mut clk = clock::create_for_testing(scenario.ctx()); - clk.set_for_testing(0); - ac.begin_default_admin_transfer(deployer, &clk, scenario.ctx()); - test_scenario::return_shared(ac); - - scenario.next_tx(deployer); - let mut ac = take_ac(&scenario); - clk.set_for_testing(delay); - let granted_before = event::events_by_type().length(); - let revoked_before = event::events_by_type().length(); - ac.accept_default_admin_transfer(&clk, scenario.ctx()); - - // Deployer still holds root after rotating to self; pending state cleared. - assert!(ac.has_role<_, ACCESS_CONTROL_TESTS>(deployer)); - assert!(!ac.has_pending_default_admin_transfer()); - assert!(ac.pending_default_admin_new_admin().is_none()); - - // Both events fire β€” function does not short-circuit transfer-to-self. - assert_eq!(event::events_by_type().length(), granted_before + 1); - assert_eq!(event::events_by_type().length(), revoked_before + 1); - - let granted = event::events_by_type(); - let last_granted = granted[granted.length() - 1]; - assert_eq!( - last_granted, - access_control::test_new_role_granted( - with_original_ids(), - deployer, - deployer, - ), - ); - let revoked = event::events_by_type(); - let last_revoked = revoked[revoked.length() - 1]; - assert_eq!( - last_revoked, - access_control::test_new_role_revoked( - with_original_ids(), - deployer, - deployer, - ), - ); - - clock::destroy_for_testing(clk); - test_scenario::return_shared(ac); - scenario.end(); -} - // === cancel_default_admin_transfer === #[test] @@ -1042,9 +1126,14 @@ fun test_cancel_admin_transfer_happy_path() { assert!(!ac.has_pending_default_admin_transfer()); assert!(ac.pending_default_admin_new_admin().is_none()); - let cancelled = event::events_by_type(); + assert_eq!(ac.default_admin(), option::some(deployer)); + let cancelled = event::events_by_type< + access_control::DefaultAdminTransferCancelled, + >(); assert_eq!(cancelled.length(), 1); - let expected = access_control::test_new_default_admin_transfer_cancelled(); + let expected = access_control::test_new_default_admin_transfer_cancelled< + ACCESS_CONTROL_TESTS, + >(); assert_eq!(cancelled[0], expected); clock::destroy_for_testing(clk); @@ -1077,9 +1166,8 @@ fun test_cancel_admin_transfer_rejects_non_root() { abort 999 } -// `cancel_default_admin_transfer` clears either kind of pending action β€” -// the same function and same `DefaultAdminTransferCancelled` event are -// reused for both transfer and renounce cancels. +// `cancel_default_admin_transfer` clears either kind of pending action and +// emits the cancellation event matching the pending action kind. #[test] fun test_cancel_admin_transfer_clears_pending_renounce() { let deployer = @0xA; @@ -1092,10 +1180,20 @@ fun test_cancel_admin_transfer_clears_pending_renounce() { assert!(!ac.has_pending_default_admin_transfer()); assert!(!ac.is_pending_default_admin_renounce()); - // Indexers correlate the cancel event with the prior schedule event to - // know which kind was cleared. - let cancelled = event::events_by_type(); + let cancelled = event::events_by_type< + access_control::DefaultAdminRenounceCancelled, + >(); assert_eq!(cancelled.length(), 1); + let expected = access_control::test_new_default_admin_renounce_cancelled< + ACCESS_CONTROL_TESTS, + >(); + assert_eq!(cancelled[0], expected); + assert_eq!( + event::events_by_type< + access_control::DefaultAdminTransferCancelled, + >().length(), + 0, + ); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); @@ -1221,15 +1319,20 @@ fun test_begin_admin_renounce_happy_path() { assert!(ac.has_pending_default_admin_transfer()); assert!(ac.is_pending_default_admin_renounce()); + assert_eq!(ac.default_admin(), option::some(deployer)); // Renounce has no incoming admin β€” `pending_default_admin_new_admin` // returns `none` for both "no pending" and "pending renounce". Use // `is_pending_default_admin_renounce` to disambiguate (above). assert!(ac.pending_default_admin_new_admin().is_none()); assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(delay)); - let scheduled = event::events_by_type(); + let scheduled = event::events_by_type< + access_control::DefaultAdminRenounceScheduled, + >(); assert_eq!(scheduled.length(), 1); - let expected = access_control::test_new_default_admin_renounce_scheduled(delay); + let expected = access_control::test_new_default_admin_renounce_scheduled( + delay, + ); assert_eq!(scheduled[0], expected); clock::destroy_for_testing(clk); @@ -1237,6 +1340,20 @@ fun test_begin_admin_renounce_happy_path() { scenario.end(); } +#[test, expected_failure(abort_code = access_control::EUnauthorized)] +fun test_renounced_admin_cannot_manage_root_role_administered_roles() { + let deployer = @0xA; + let user = @0xB; + let mut scenario = setup(deployer, 0); + let mut ac = take_ac(&scenario); + let clk = clock::create_for_testing(scenario.ctx()); + + ac.begin_default_admin_renounce(&clk, scenario.ctx()); + ac.accept_default_admin_renounce(&clk, scenario.ctx()); + ac.grant_role<_, AdminA>(user, scenario.ctx()); + abort 999 +} + #[test, expected_failure(abort_code = access_control::EUnauthorized)] fun test_begin_admin_renounce_rejects_non_root() { let deployer = @0xA; @@ -1248,8 +1365,9 @@ fun test_begin_admin_renounce_rejects_non_root() { abort 999 } -// Scheduling a renounce overwrites an existing pending transfer (and vice -// versa). The two are mutually exclusive β€” they share `pending_default_admin`. +// Scheduling a renounce cancels and overwrites an existing pending transfer +// (and vice versa). The two are mutually exclusive β€” they share +// `pending_default_admin`. #[test] fun test_begin_admin_renounce_overwrites_pending_transfer() { let deployer = @0xA; @@ -1266,6 +1384,17 @@ fun test_begin_admin_renounce_overwrites_pending_transfer() { assert!(ac.is_pending_default_admin_renounce()); assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(50)); + let cancelled = event::events_by_type< + access_control::DefaultAdminTransferCancelled, + >(); + assert_eq!(cancelled.length(), 1); + assert_eq!( + event::events_by_type< + access_control::DefaultAdminRenounceCancelled, + >().length(), + 0, + ); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); @@ -1288,14 +1417,25 @@ fun test_begin_admin_transfer_overwrites_pending_renounce() { assert!(!ac.is_pending_default_admin_renounce()); assert_eq!(ac.pending_default_admin_new_admin(), option::some(new_admin)); + let cancelled = event::events_by_type< + access_control::DefaultAdminRenounceCancelled, + >(); + assert_eq!(cancelled.length(), 1); + assert_eq!( + event::events_by_type< + access_control::DefaultAdminTransferCancelled, + >().length(), + 0, + ); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } // Symmetric to the transfer-overwrites-transfer and the cross-kind tests -// above: a second `begin_default_admin_renounce` overwrites the first, -// re-anchoring `execute_after_ms` to the new clock. +// above: a second `begin_default_admin_renounce` cancels and overwrites the +// first, re-anchoring `execute_after_ms` to the new clock. #[test] fun test_begin_admin_renounce_overwrites_pending_renounce() { let deployer = @0xA; @@ -1313,6 +1453,17 @@ fun test_begin_admin_renounce_overwrites_pending_renounce() { assert!(ac.is_pending_default_admin_renounce()); assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(50)); + let cancelled = event::events_by_type< + access_control::DefaultAdminRenounceCancelled, + >(); + assert_eq!(cancelled.length(), 1); + assert_eq!( + event::events_by_type< + access_control::DefaultAdminTransferCancelled, + >().length(), + 0, + ); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); @@ -1331,27 +1482,37 @@ fun test_accept_admin_renounce_happy_path() { ac.begin_default_admin_renounce(&clk, scenario.ctx()); clk.set_for_testing(delay); - let revoked_before = event::events_by_type().length(); - let granted_before = event::events_by_type().length(); + let revoked_before = event::events_by_type< + access_control::RoleRevoked, + >().length(); + let granted_before = event::events_by_type< + access_control::RoleGranted, + >().length(); ac.accept_default_admin_renounce(&clk, scenario.ctx()); - // Caller (current root) is removed from the root role; pending cleared. + // Caller (current default admin) is removed from the root role; pending cleared. assert!(!ac.has_role<_, ACCESS_CONTROL_TESTS>(deployer)); + assert!(ac.default_admin().is_none()); assert!(!ac.has_pending_default_admin_transfer()); assert!(!ac.is_pending_default_admin_renounce()); // Exactly one RoleRevoked and zero RoleGranted (no incoming admin). - assert_eq!(event::events_by_type().length(), revoked_before + 1); - assert_eq!(event::events_by_type().length(), granted_before); + assert_eq!( + event::events_by_type>().length(), + revoked_before + 1, + ); + assert_eq!( + event::events_by_type>().length(), + granted_before, + ); - let revoked = event::events_by_type(); + let revoked = event::events_by_type>(); let last = revoked[revoked.length() - 1]; assert_eq!( last, - access_control::test_new_role_revoked( + access_control::test_new_role_revoked( with_original_ids(), deployer, - deployer, ), ); @@ -1458,13 +1619,20 @@ fun test_begin_delay_change_increase_below_cap() { clk.set_for_testing(0); ac.begin_default_admin_delay_change(two_hours, &clk, scenario.ctx()); - assert!(ac.has_pending_default_admin_delay_change()); - assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(), option::some(two_hours)); - assert_eq!(ac.pending_default_admin_delay_change_schedule_after_ms(), option::some(two_hours)); + assert!(ac.has_pending_default_admin_delay_change(&clk)); + assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(&clk), option::some(two_hours)); + assert_eq!( + ac.pending_default_admin_delay_change_schedule_after_ms(&clk), + option::some(two_hours), + ); - let scheduled = event::events_by_type(); + let scheduled = event::events_by_type< + access_control::DefaultAdminDelayChangeScheduled, + >(); assert_eq!(scheduled.length(), 1); - let expected = access_control::test_new_default_admin_delay_change_scheduled( + let expected = access_control::test_new_default_admin_delay_change_scheduled< + ACCESS_CONTROL_TESTS, + >( two_hours, two_hours, ); @@ -1490,8 +1658,8 @@ fun test_begin_delay_change_increase_above_cap() { clk.set_for_testing(0); ac.begin_default_admin_delay_change(thirty_days, &clk, scenario.ctx()); - assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(), option::some(thirty_days)); - assert_eq!(ac.pending_default_admin_delay_change_schedule_after_ms(), option::some(cap)); + assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(&clk), option::some(thirty_days)); + assert_eq!(ac.pending_default_admin_delay_change_schedule_after_ms(&clk), option::some(cap)); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); @@ -1511,7 +1679,7 @@ fun test_begin_delay_change_increase_at_cap_boundary() { let mut clk = clock::create_for_testing(scenario.ctx()); clk.set_for_testing(0); ac.begin_default_admin_delay_change(cap, &clk, scenario.ctx()); - assert_eq!(ac.pending_default_admin_delay_change_schedule_after_ms(), option::some(cap)); + assert_eq!(ac.pending_default_admin_delay_change_schedule_after_ms(&clk), option::some(cap)); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); @@ -1533,7 +1701,7 @@ fun test_begin_delay_change_decrease() { ac.begin_default_admin_delay_change(one_day, &clk, scenario.ctx()); assert_eq!( - ac.pending_default_admin_delay_change_schedule_after_ms(), + ac.pending_default_admin_delay_change_schedule_after_ms(&clk), option::some(seven_days - one_day), ); @@ -1552,7 +1720,9 @@ fun test_begin_delay_change_no_change() { let mut clk = clock::create_for_testing(scenario.ctx()); clk.set_for_testing(123); ac.begin_default_admin_delay_change(one_hour, &clk, scenario.ctx()); - assert_eq!(ac.pending_default_admin_delay_change_schedule_after_ms(), option::some(123)); + assert!(!ac.has_pending_default_admin_delay_change(&clk)); + assert!(ac.pending_default_admin_delay_change_new_delay_ms(&clk).is_none()); + assert!(ac.pending_default_admin_delay_change_schedule_after_ms(&clk).is_none()); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); @@ -1592,7 +1762,7 @@ fun test_begin_delay_change_at_max_boundary() { let mut clk = clock::create_for_testing(scenario.ctx()); clk.set_for_testing(0); ac.begin_default_admin_delay_change(max, &clk, scenario.ctx()); - assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(), option::some(max)); + assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(&clk), option::some(max)); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); @@ -1610,18 +1780,30 @@ fun test_begin_delay_change_overwrites_pending() { clk.set_for_testing(50); ac.begin_default_admin_delay_change(200, &clk, scenario.ctx()); - assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(), option::some(200)); - assert_eq!(ac.pending_default_admin_delay_change_schedule_after_ms(), option::some(50 + 200)); + assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(&clk), option::some(200)); + assert_eq!( + ac.pending_default_admin_delay_change_schedule_after_ms(&clk), + option::some(50 + 200), + ); + + let cancelled = event::events_by_type< + access_control::DefaultAdminDelayChangeCancelled, + >(); + assert_eq!(cancelled.length(), 1); + let expected = access_control::test_new_default_admin_delay_change_cancelled< + ACCESS_CONTROL_TESTS, + >(); + assert_eq!(cancelled[0], expected); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } -// === accept_default_admin_delay_change === +// === automatic default admin delay application === #[test] -fun test_accept_delay_change_happy_path() { +fun test_default_admin_delay_ms_returns_elapsed_pending_delay() { let deployer = @0xA; let one_hour: u64 = 60 * 60 * 1_000; let two_hours: u64 = 2 * one_hour; @@ -1631,22 +1813,24 @@ fun test_accept_delay_change_happy_path() { clk.set_for_testing(0); ac.begin_default_admin_delay_change(two_hours, &clk, scenario.ctx()); - clk.set_for_testing(two_hours); - ac.accept_default_admin_delay_change(&clk, scenario.ctx()); + clk.set_for_testing(two_hours - 1); + assert_eq!(ac.default_admin_delay_ms(&clk), one_hour); - assert_eq!(ac.default_admin_delay_ms(), two_hours); - assert!(!ac.has_pending_default_admin_delay_change()); + clk.set_for_testing(two_hours); + assert_eq!(ac.default_admin_delay_ms(&clk), two_hours); + assert!(!ac.has_pending_default_admin_delay_change(&clk)); + assert!(ac.pending_default_admin_delay_change_new_delay_ms(&clk).is_none()); + assert!(ac.pending_default_admin_delay_change_schedule_after_ms(&clk).is_none()); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } -// `accept` requires no permission β€” the schedule was committed at `begin`, -// so any caller can trigger the state transition once the schedule passes. #[test] -fun test_accept_delay_change_callable_by_anyone() { +fun test_elapsed_delay_change_applies_to_new_transfer() { let deployer = @0xA; + let new_admin = @0xB; let one_hour: u64 = 60 * 60 * 1_000; let two_hours: u64 = 2 * one_hour; let mut scenario = setup(deployer, one_hour); @@ -1654,32 +1838,48 @@ fun test_accept_delay_change_callable_by_anyone() { let mut clk = clock::create_for_testing(scenario.ctx()); clk.set_for_testing(0); ac.begin_default_admin_delay_change(two_hours, &clk, scenario.ctx()); - test_scenario::return_shared(ac); - scenario.next_tx(@0xB); - let mut ac = take_ac(&scenario); - clk.set_for_testing(two_hours); - ac.accept_default_admin_delay_change(&clk, scenario.ctx()); - assert_eq!(ac.default_admin_delay_ms(), two_hours); + let now = two_hours; + clk.set_for_testing(now); + ac.begin_default_admin_transfer(new_admin, &clk, scenario.ctx()); + + assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(now + two_hours)); + assert_eq!(ac.default_admin_delay_ms(&clk), two_hours); + assert!(!ac.has_pending_default_admin_delay_change(&clk)); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } -#[test, expected_failure(abort_code = access_control::ENoPendingDelayChange)] -fun test_accept_delay_change_rejects_no_pending() { +#[test] +fun test_elapsed_delay_change_applies_to_new_renounce() { let deployer = @0xA; - let mut scenario = setup(deployer, 0); + let one_hour: u64 = 60 * 60 * 1_000; + let two_hours: u64 = 2 * one_hour; + let mut scenario = setup(deployer, one_hour); let mut ac = take_ac(&scenario); - let clk = clock::create_for_testing(scenario.ctx()); - ac.accept_default_admin_delay_change(&clk, scenario.ctx()); - abort 999 + let mut clk = clock::create_for_testing(scenario.ctx()); + clk.set_for_testing(0); + ac.begin_default_admin_delay_change(two_hours, &clk, scenario.ctx()); + + let now = two_hours; + clk.set_for_testing(now); + ac.begin_default_admin_renounce(&clk, scenario.ctx()); + + assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(now + two_hours)); + assert_eq!(ac.default_admin_delay_ms(&clk), two_hours); + assert!(!ac.has_pending_default_admin_delay_change(&clk)); + + clock::destroy_for_testing(clk); + test_scenario::return_shared(ac); + scenario.end(); } -#[test, expected_failure(abort_code = access_control::EDelayNotElapsed)] -fun test_accept_delay_change_rejects_too_early() { +#[test] +fun test_unelapsed_delay_change_does_not_apply_to_new_transfer() { let deployer = @0xA; + let new_admin = @0xB; let one_hour: u64 = 60 * 60 * 1_000; let two_hours: u64 = 2 * one_hour; let mut scenario = setup(deployer, one_hour); @@ -1687,9 +1887,18 @@ fun test_accept_delay_change_rejects_too_early() { let mut clk = clock::create_for_testing(scenario.ctx()); clk.set_for_testing(0); ac.begin_default_admin_delay_change(two_hours, &clk, scenario.ctx()); - clk.set_for_testing(two_hours - 1); - ac.accept_default_admin_delay_change(&clk, scenario.ctx()); - abort 999 + + let now = two_hours - 1; + clk.set_for_testing(now); + ac.begin_default_admin_transfer(new_admin, &clk, scenario.ctx()); + + assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(now + one_hour)); + assert_eq!(ac.default_admin_delay_ms(&clk), one_hour); + assert!(ac.has_pending_default_admin_delay_change(&clk)); + + clock::destroy_for_testing(clk); + test_scenario::return_shared(ac); + scenario.end(); } // === cancel_default_admin_delay_change === @@ -1702,12 +1911,16 @@ fun test_cancel_delay_change_happy_path() { let clk = clock::create_for_testing(scenario.ctx()); ac.begin_default_admin_delay_change(100, &clk, scenario.ctx()); - ac.cancel_default_admin_delay_change(scenario.ctx()); + ac.cancel_default_admin_delay_change(&clk, scenario.ctx()); - assert!(!ac.has_pending_default_admin_delay_change()); - let cancelled = event::events_by_type(); + assert!(!ac.has_pending_default_admin_delay_change(&clk)); + let cancelled = event::events_by_type< + access_control::DefaultAdminDelayChangeCancelled, + >(); assert_eq!(cancelled.length(), 1); - let expected = access_control::test_new_default_admin_delay_change_cancelled(); + let expected = access_control::test_new_default_admin_delay_change_cancelled< + ACCESS_CONTROL_TESTS, + >(); assert_eq!(cancelled[0], expected); clock::destroy_for_testing(clk); @@ -1720,7 +1933,8 @@ fun test_cancel_delay_change_rejects_no_pending() { let deployer = @0xA; let mut scenario = setup(deployer, 0); let mut ac = take_ac(&scenario); - ac.cancel_default_admin_delay_change(scenario.ctx()); + let clk = clock::create_for_testing(scenario.ctx()); + ac.cancel_default_admin_delay_change(&clk, scenario.ctx()); abort 999 } @@ -1735,31 +1949,49 @@ fun test_cancel_delay_change_rejects_non_root() { scenario.next_tx(@0xB); let mut ac = take_ac(&scenario); - ac.cancel_default_admin_delay_change(scenario.ctx()); + ac.cancel_default_admin_delay_change(&clk, scenario.ctx()); clock::destroy_for_testing(clk); abort 999 } +#[test, expected_failure(abort_code = access_control::ENoPendingDelayChange)] +fun test_cancel_delay_change_rejects_elapsed_pending() { + let deployer = @0xA; + let one_hour: u64 = 60 * 60 * 1_000; + let two_hours: u64 = 2 * one_hour; + let mut scenario = setup(deployer, one_hour); + let mut ac = take_ac(&scenario); + let mut clk = clock::create_for_testing(scenario.ctx()); + clk.set_for_testing(0); + ac.begin_default_admin_delay_change(two_hours, &clk, scenario.ctx()); + + clk.set_for_testing(two_hours); + ac.cancel_default_admin_delay_change(&clk, scenario.ctx()); + abort 999 +} + // === Pending getters: delay change === #[test] fun test_delay_change_getters_when_no_pending() { let deployer = @0xA; - let scenario = setup(deployer, 0); + let mut scenario = setup(deployer, 0); let ac = take_ac(&scenario); - assert!(!ac.has_pending_default_admin_delay_change()); - assert!(ac.pending_default_admin_delay_change_new_delay_ms().is_none()); - assert!(ac.pending_default_admin_delay_change_schedule_after_ms().is_none()); + let clk = clock::create_for_testing(scenario.ctx()); + assert!(!ac.has_pending_default_admin_delay_change(&clk)); + assert!(ac.pending_default_admin_delay_change_new_delay_ms(&clk).is_none()); + assert!(ac.pending_default_admin_delay_change_schedule_after_ms(&clk).is_none()); + clock::destroy_for_testing(clk); test_scenario::return_shared(ac); scenario.end(); } // === In-flight noninterference === -// A pending admin transfer was scheduled under the OLD delay. A delay change -// is then scheduled and accepted. The pending transfer's `execute_after_ms` -// must not change β€” in-flight transfers honor the delay they were scheduled -// under, regardless of subsequent delay changes. +// A pending admin transfer was scheduled under the old delay. A delay change +// then becomes effective. The pending transfer's `execute_after_ms` must not +// change β€” in-flight transfers honor the delay they were scheduled under, +// regardless of subsequent delay changes. #[test] fun test_delay_change_does_not_affect_pending_transfer() { let deployer = @0xA; @@ -1775,13 +2007,12 @@ fun test_delay_change_does_not_affect_pending_transfer() { let pending_execute_at = ac.pending_default_admin_execute_after_ms(); assert_eq!(pending_execute_at, option::some(one_hour)); - // Schedule + accept a delay decrease to a much smaller value. + // Schedule a delay decrease to a much smaller value, then let it elapse. ac.begin_default_admin_delay_change(1, &clk, scenario.ctx()); clk.set_for_testing(one_hour); // past the freed-time wait - ac.accept_default_admin_delay_change(&clk, scenario.ctx()); - // Configured delay updated... - assert_eq!(ac.default_admin_delay_ms(), 1); + // Effective delay updated... + assert_eq!(ac.default_admin_delay_ms(&clk), 1); // ...but the in-flight transfer's execute_after_ms is unchanged. assert_eq!(ac.pending_default_admin_execute_after_ms(), pending_execute_at); @@ -1790,26 +2021,38 @@ fun test_delay_change_does_not_affect_pending_transfer() { scenario.end(); } -// After the delay change applies, a NEW transfer uses the new delay. #[test] -fun test_new_transfer_uses_new_delay() { +fun test_begin_delay_change_applies_elapsed_pending_before_new_schedule() { let deployer = @0xA; - let new_admin = @0xB; let one_hour: u64 = 60 * 60 * 1_000; let two_hours: u64 = 2 * one_hour; + let three_hours: u64 = 3 * one_hour; let mut scenario = setup(deployer, one_hour); let mut ac = take_ac(&scenario); let mut clk = clock::create_for_testing(scenario.ctx()); clk.set_for_testing(0); ac.begin_default_admin_delay_change(two_hours, &clk, scenario.ctx()); + + // The first change has elapsed. Scheduling a decrease from 2h to 1h must + // use the 2h effective delay as the current value, so wait = 1h. clk.set_for_testing(two_hours); - ac.accept_default_admin_delay_change(&clk, scenario.ctx()); + let cancelled_before = event::events_by_type< + access_control::DefaultAdminDelayChangeCancelled, + >().length(); + ac.begin_default_admin_delay_change(one_hour, &clk, scenario.ctx()); - // Schedule a transfer; it should use the new (2h) delay. - let now = two_hours; - clk.set_for_testing(now); - ac.begin_default_admin_transfer(new_admin, &clk, scenario.ctx()); - assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(now + two_hours)); + assert_eq!(ac.default_admin_delay_ms(&clk), two_hours); + assert_eq!(ac.pending_default_admin_delay_change_new_delay_ms(&clk), option::some(one_hour)); + assert_eq!( + ac.pending_default_admin_delay_change_schedule_after_ms(&clk), + option::some(three_hours), + ); + assert_eq!( + event::events_by_type< + access_control::DefaultAdminDelayChangeCancelled, + >().length(), + cancelled_before, + ); clock::destroy_for_testing(clk); test_scenario::return_shared(ac); diff --git a/math/fixed_point/Published.toml b/math/fixed_point/Published.toml index 44cd7773..0e38db16 100644 --- a/math/fixed_point/Published.toml +++ b/math/fixed_point/Published.toml @@ -4,18 +4,18 @@ [published.mainnet] chain-id = "35834a8a" -published-at = "0xf18ad29e1549aec2d5ce12d1a461ea0a03b5a113cc04aced0e07963738fd8a84" +published-at = "0xa09088ee34ea6bbb337d5fa7832757ef0d3b76fecc1e3bb3659e22b9930c92a9" original-id = "0xf18ad29e1549aec2d5ce12d1a461ea0a03b5a113cc04aced0e07963738fd8a84" -version = 1 -toolchain-version = "1.69.2" +version = 2 +toolchain-version = "1.73.0" build-config = { flavor = "sui", edition = "2024" } upgrade-capability = "0xef746e8f4285ebf308f54f0ad83e438daf0ee84824abdb334a23e661d6973166" [published.testnet] chain-id = "4c78adac" -published-at = "0xd7cade5babb2de2a99a53ada981cea48adfd8122f3279fc51bb50958c01cb36f" +published-at = "0x9f5aef4f702e164ce1704f2269134509a63937e220bbd63dee72b5900943a78b" original-id = "0xd7cade5babb2de2a99a53ada981cea48adfd8122f3279fc51bb50958c01cb36f" -version = 1 -toolchain-version = "1.69.2" +version = 2 +toolchain-version = "1.73.0" build-config = { flavor = "sui", edition = "2024" } upgrade-capability = "0x0d228096f603de61cc0aed2a8cf3d527db455bb958da6d76dbb0e4f99c16f08c" diff --git a/math/fixed_point/README.md b/math/fixed_point/README.md index 468ac3ae..3bcbbd26 100644 --- a/math/fixed_point/README.md +++ b/math/fixed_point/README.md @@ -92,13 +92,14 @@ single `i128`-style input or output. `log2`, `ln`, and `log10` are computed from a shared `log2` kernel; `ln` and `log10` apply a base-conversion factor on top. -- **UD30x9** aborts on `x < 1` (the result would be negative). - Rounds down. +- **UD30x9** aborts for `0 < x < 1` because the fixed-point logarithm would be + negative; **UD30x9** also aborts for `x == 0` because the log operation is + undefined/invalid. Rounds down. - **SD29x9** aborts on `x <= 0`. Rounds toward zero, matching `mul_trunc`, `div_trunc`, and `pow` in the same module. -Round-down compounding can put the result 1 ulp from the mathematical answer -at irrational identity points (e.g. `log10(10Β·SCALE) == SCALE - 1`). +`log10` is exact on integer powers of ten (including sub-unit `10^-k` on +`SD29x9`): `log10(10^k) == k * SCALE`. ```move use openzeppelin_fp_math::{sd29x9, ud30x9_convert}; diff --git a/math/fixed_point/sources/internal/common.move b/math/fixed_point/sources/internal/common.move index 462ce1f1..e0b189bb 100644 --- a/math/fixed_point/sources/internal/common.move +++ b/math/fixed_point/sources/internal/common.move @@ -15,6 +15,25 @@ use openzeppelin_math::u128; #[error(code = 0)] const ELogOfZero: vector = "Logarithm of zero is undefined"; +// === Constants === + +/// Internal precision used by the fixed-point logarithm kernel: `10^18` +/// (~`2^60`), an order of magnitude finer than the user-facing `10^9` scale. +/// +/// Error analysis: in the squaring loop, error in `y` grows ~2Γ— per iteration +/// (since `y_new = y_old^2 / internal`), but a wrong bit at iteration `i` +/// only perturbs `frac` by `internal / 2^(i+1)` β€” exponentially decaying, so +/// late-iteration errors contribute little. Empirically the total `frac` +/// error stays under `~10^3` at scale `10^18`, well below one user-facing +/// ulp (unit in the last place, `10^9`). A 9-decimal variant would lack the +/// headroom for inputs near `1.0`; 18-decimal preserves precision. +const INTERNAL_LOG_SCALE: u128 = 1_000_000_000_000_000_000; // 10^18 + +/// Scale-correction denominator for `apply_log2_factor`: two scale-`10^18` +/// factors yield a scale-`10^36` product; dividing by `10^27` lands the result +/// at the user-facing scale `10^9`. +const LOG_FACTOR_DENOM_E27: u128 = 1_000_000_000_000_000_000_000_000_000; // 10^27 + // === Package Functions === /// Returns the raw fixed-point scale shared by `UD30x9` and `SD29x9`. @@ -88,6 +107,9 @@ public(package) macro fun max_sd29x9_whole(): u128 { /// /// #### Returns /// - `numerator / denominator` when exact, otherwise that quotient plus one. +/// +/// #### Aborts +/// - When `denominator` is zero (native integer-division abort). public(package) fun div_away_u256(numerator: u256, denominator: u256): u256 { let quotient = numerator / denominator; if (quotient * denominator == numerator) { @@ -113,23 +135,6 @@ public(package) macro fun log10_2_e18(): u128 { 301_029_995_663_981_195 } -/// Internal precision used by the fixed-point logarithm kernel: `10^18` -/// (~`2^60`), an order of magnitude finer than the user-facing `10^9` scale. -/// -/// Error analysis: in the squaring loop, error in `y` grows ~2Γ— per iteration -/// (since `y_new = y_old^2 / internal`), but a wrong bit at iteration `i` -/// only perturbs `frac` by `internal / 2^(i+1)` β€” exponentially decaying, so -/// late-iteration errors contribute little. Empirically the total `frac` -/// error stays under `~10^3` at scale `10^18`, well below one user-facing -/// ulp (unit in the last place, `10^9`). A 9-decimal variant would lack the -/// headroom for inputs near `1.0`; 18-decimal preserves precision. -const INTERNAL_LOG_SCALE: u128 = 1_000_000_000_000_000_000; // 10^18 - -/// Scale-correction denominator for `apply_log2_factor`: two scale-`10^18` -/// factors yield a scale-`10^36` product; dividing by `10^27` lands the result -/// at the user-facing scale `10^9`. -const LOG_FACTOR_DENOM_E27: u128 = 1_000_000_000_000_000_000_000_000_000; // 10^27 - /// Combines a `raw_log2` magnitude with a base-conversion factor and returns /// the result at the user-facing `10^9` scale. /// @@ -172,6 +177,24 @@ public(package) fun apply_log2_factor(log2_mag_e18: u128, factor_e18: u128): u12 /// /// #### Aborts /// - `ELogOfZero` if `x_raw` is zero. +/// +/// #### Precision +/// The error direction differs by branch, because the final assembly is +/// `n_abs * internal + frac` for `x_raw >= scale` but `n_abs * internal - +/// frac` for `x_raw < scale`: +/// +/// - `x_raw >= scale` (`n_abs = floor(log2(x_raw / 10^9))`): the magnitude +/// is at most 2 user-facing ulps below the true magnitude, monotone-down. +/// The dominant loss is the `x_raw >> n_abs` truncation, which discards up +/// to `n_abs` low-order bits β€” so the deficit grows with `n_abs` and +/// reaches the 2-ulp ceiling at `x_raw = u128::MAX`. +/// - `x_raw < scale`: normalization is a lossless left shift, so the only +/// loss is the loop's round-down of `frac`. Since `frac` is subtracted +/// here, that round-down leaves the magnitude at or slightly above the +/// true magnitude β€” a sub-ulp upward bias. +/// +/// The user-facing rounding this induces on negative results (toward zero, +/// with rare 1-ulp edge cases) is documented on `SD29x9::log2`. public(package) fun raw_log2(x_raw: u128): (bool, u128) { assert!(x_raw > 0, ELogOfZero); diff --git a/math/fixed_point/sources/sd29x9/sd29x9.move b/math/fixed_point/sources/sd29x9/sd29x9.move index 63da8c6a..36596826 100644 --- a/math/fixed_point/sources/sd29x9/sd29x9.move +++ b/math/fixed_point/sources/sd29x9/sd29x9.move @@ -47,8 +47,8 @@ public use fun openzeppelin_fp_math::sd29x9_base::mul_away as SD29x9.mul_away; public use fun openzeppelin_fp_math::sd29x9_base::mul_trunc as SD29x9.mul_trunc; public use fun openzeppelin_fp_math::sd29x9_base::negate as SD29x9.negate; public use fun openzeppelin_fp_math::sd29x9_base::pow as SD29x9.pow; -public use fun openzeppelin_fp_math::sd29x9_base::sqrt as SD29x9.sqrt; public use fun openzeppelin_fp_math::sd29x9_base::rem as SD29x9.rem; +public use fun openzeppelin_fp_math::sd29x9_base::sqrt as SD29x9.sqrt; public use fun openzeppelin_fp_math::sd29x9_base::sub as SD29x9.sub; public use fun openzeppelin_fp_math::sd29x9_base::unchecked_add as SD29x9.unchecked_add; public use fun openzeppelin_fp_math::sd29x9_base::unchecked_sub as SD29x9.unchecked_sub; diff --git a/math/fixed_point/sources/sd29x9/sd29x9_base.move b/math/fixed_point/sources/sd29x9/sd29x9_base.move index 595eccfd..e34e1a92 100644 --- a/math/fixed_point/sources/sd29x9/sd29x9_base.move +++ b/math/fixed_point/sources/sd29x9/sd29x9_base.move @@ -228,20 +228,35 @@ public fun ln(x: SD29x9): SD29x9 { /// Computes the base-10 logarithm of an `SD29x9` value. /// -/// Derived from `log2` via `log10(x) = log2(x) * log10(2)`. Rounded toward -/// zero; see `log2` for full rounding semantics on signed results. +/// Exact when `x` is an integer power of ten, including sub-unit powers +/// `10^-k` (`k` in `1..=9`). Otherwise derived from `log2` via +/// `log10(x) = log2(x) * log10(2)` and rounded toward zero; see `log2` +/// for full rounding semantics on signed results. /// /// #### Parameters /// - `x`: Input value. /// /// #### Returns -/// - `log10(x)`, rounded toward zero. +/// - `log10(x)`, rounded toward zero (exact at integer powers of ten). /// /// #### Aborts /// - `ELogUndefined` if `x` is zero or negative. public fun log10(x: SD29x9): SD29x9 { let Components { neg, mag } = decompose(x.unwrap()); assert!(!neg && mag > 0, ELogUndefined); + // Applied on the decomposed magnitude before the sign branch so sub-unit + // `10^-k` inputs (raw magnitudes below `SCALE`) also resolve exactly. + if (u256::is_power_of_ten(mag)) { + // Subtract `9 = log10(SCALE)` to strip the embedded scale; `j < 9` + // means a sub-unit input (`10^-k`), producing a negative result. + let j = u256::log10(mag, rounding::down()); + let (is_neg, result_abs) = if (j >= 9) { + (false, ((j - 9) as u256) * common::scale_u256!()) + } else { + (true, ((9 - j) as u256) * common::scale_u256!()) + }; + return wrap_components(Components { neg: is_neg, mag: result_abs }) + }; let (log_neg, log_mag_internal) = common::raw_log2(mag as u128); let result_mag = common::apply_log2_factor(log_mag_internal, common::log10_2_e18!()); wrap_components(Components { neg: log_neg, mag: result_mag as u256 }) @@ -251,11 +266,13 @@ public fun log10(x: SD29x9): SD29x9 { /// /// The result is rounded toward zero, matching the convention used by /// `mul_trunc`, `div_trunc`, and `pow` in this module. For positive results -/// (inputs `>= 1`) this coincides with rounding down. For negative results -/// (inputs in `(0, 1)`) the signed result usually sits closer to zero than -/// the true value, but in narrow edge cases where the kernel's small upward -/// magnitude bias crosses an integer boundary it may instead be 1 ulp (unit -/// in the last place) further from zero. +/// (inputs `>= 1`) this coincides with rounding down and sits at most 2 +/// ulps below the true value (see `raw_log2` for the kernel's precision +/// bound). For negative results (inputs in `(0, 1)`) the signed result +/// usually sits closer to zero than the true value, but in narrow edge +/// cases where the kernel's small upward magnitude bias crosses an integer +/// boundary it may instead be 1 ulp (unit in the last place) further from +/// zero. /// /// #### Parameters /// - `x`: Input value. @@ -560,7 +577,7 @@ public fun pow(x: SD29x9, exp: u8): SD29x9 { /// value. /// /// #### Aborts -/// - Aborts if `x` is negative. +/// - `ENegativeSqrt` if `x` is negative. public fun sqrt(x: SD29x9): SD29x9 { let Components { neg, mag } = decompose(x.unwrap()); assert!(!neg, ENegativeSqrt); diff --git a/math/fixed_point/sources/ud30x9/ud30x9_base.move b/math/fixed_point/sources/ud30x9/ud30x9_base.move index 89192a8b..fb2e49ff 100644 --- a/math/fixed_point/sources/ud30x9/ud30x9_base.move +++ b/math/fixed_point/sources/ud30x9/ud30x9_base.move @@ -5,6 +5,7 @@ use openzeppelin_fp_math::common; use openzeppelin_fp_math::sd29x9::{Self, SD29x9}; use openzeppelin_fp_math::ud30x9::{UD30x9, wrap, zero, one}; use openzeppelin_math::rounding; +use openzeppelin_math::u128; use openzeppelin_math::u256; // === Errors === @@ -29,10 +30,14 @@ const ECannotBeConvertedToSD29x9: vector = "Value cannot be converted to SD2 #[error(code = 4)] const EInvalidShiftSize: vector = "Shift size is out of range (must be less than 128)"; -/// Logarithm is undefined: input must be greater than or equal to one +/// Logarithm is undefined: input must be non-zero #[error(code = 5)] -const ELogUndefined: vector = - "Logarithm is undefined: input must be greater than or equal to one"; +const ELogUndefined: vector = "Logarithm is undefined: input must be non-zero"; + +/// Logarithm result would be negative and is unrepresentable in `UD30x9` +#[error(code = 6)] +const ELogResultUnrepresentable: vector = + "Logarithm result would be negative and is unrepresentable in UD30x9"; // === Public Functions === @@ -250,8 +255,10 @@ public fun unchecked_lshift(x: UD30x9, bits: u8): UD30x9 { /// Computes the natural logarithm of a `UD30x9` value. /// -/// Derived from `log2` via the identity `ln(x) = log2(x) * ln(2)`. Both factors -/// round toward zero, so the result may sit up to one ulp below the true value. +/// Derived from `log2` via the identity `ln(x) = log2(x) * ln(2)`. Both the +/// `log2` kernel and the base-conversion step round toward zero, so the +/// result may sit up to 2 ulps below the true value; see `raw_log2` for the +/// kernel's precision bound. /// /// #### Parameters /// - `x`: Input value. @@ -260,10 +267,13 @@ public fun unchecked_lshift(x: UD30x9, bits: u8): UD30x9 { /// - `ln(x)`, rounded down to the nearest representable `UD30x9` value. /// /// #### Aborts -/// - `ELogUndefined` if `x` is less than one. +/// - `ELogUndefined` if `x` is zero. +/// - `ELogResultUnrepresentable` if `x` is in `(0, 1)` (the result would be negative +/// and cannot be represented in `UD30x9` β€” use `SD29x9` instead). public fun ln(x: UD30x9): UD30x9 { let raw = x.unwrap(); - assert!(raw >= common::scale!(), ELogUndefined); + assert!(raw > 0, ELogUndefined); + assert!(raw >= common::scale!(), ELogResultUnrepresentable); // The `raw >= scale` precondition guarantees `raw_log2` returns a // non-negative sign, so the discarded sign flag is provably `false`. let (_, mag) = common::raw_log2(raw); @@ -272,22 +282,31 @@ public fun ln(x: UD30x9): UD30x9 { /// Computes the base-10 logarithm of a `UD30x9` value. /// -/// Derived from `log2` via the identity `log10(x) = log2(x) * log10(2)`. Both -/// factors round toward zero, so the result may sit up to one ulp below the -/// true value. In particular, `log10(10) == one() - 1 ulp` under pure -/// round-down arithmetic. +/// Exact when `x` is an integer power of ten (`10^k`, `k >= 0`): the +/// dedicated power-of-ten branch returns `k * SCALE`, so `log10(10)` is +/// exactly `one()`. Other inputs are derived from `log2` via the identity +/// `log10(x) = log2(x) * log10(2)` and rounded down. /// /// #### Parameters /// - `x`: Input value. /// /// #### Returns -/// - `log10(x)`, rounded down to the nearest representable `UD30x9` value. +/// - `log10(x)`, rounded down to the nearest representable `UD30x9` value +/// (exact at integer powers of ten). /// /// #### Aborts -/// - `ELogUndefined` if `x` is less than one. +/// - `ELogUndefined` if `x` is zero. +/// - `ELogResultUnrepresentable` if `x` is in `(0, 1)` (the result would be negative +/// and cannot be represented in `UD30x9` β€” use `SD29x9` instead). public fun log10(x: UD30x9): UD30x9 { let raw = x.unwrap(); - assert!(raw >= common::scale!(), ELogUndefined); + assert!(raw > 0, ELogUndefined); + assert!(raw >= common::scale!(), ELogResultUnrepresentable); + if (u128::is_power_of_ten(raw)) { + // Subtract `9 = log10(SCALE)` to strip the embedded scale. + let j = u128::log10(raw, rounding::down()); + return wrap(((j - 9) as u128) * common::scale!()) + }; // The `raw >= scale` precondition guarantees `raw_log2` returns a // non-negative sign, so the discarded sign flag is provably `false`. let (_, mag) = common::raw_log2(raw); @@ -296,20 +315,24 @@ public fun log10(x: UD30x9): UD30x9 { /// Computes the base-2 logarithm of a `UD30x9` value. /// -/// The result is rounded down to the nearest representable `UD30x9` value: -/// it is the largest `UD30x9` `r` such that `2^r <= x`. +/// The result is rounded down and sits at most 2 ulps below the true +/// `log2(x)`; see `raw_log2` for the kernel's precision bound. For inputs +/// in `[1, 2)` the result is exact. /// /// #### Parameters /// - `x`: Input value. /// /// #### Returns -/// - `log2(x)`, rounded down to the nearest representable `UD30x9` value. +/// - `log2(x)`, at most 2 ulps below the true value. /// /// #### Aborts -/// - `ELogUndefined` if `x` is less than one (result would be negative or undefined). +/// - `ELogUndefined` if `x` is zero. +/// - `ELogResultUnrepresentable` if `x` is in `(0, 1)` (the result would be negative +/// and cannot be represented in `UD30x9` β€” use `SD29x9` instead). public fun log2(x: UD30x9): UD30x9 { let raw = x.unwrap(); - assert!(raw >= common::scale!(), ELogUndefined); + assert!(raw > 0, ELogUndefined); + assert!(raw >= common::scale!(), ELogResultUnrepresentable); // The `raw >= scale` precondition guarantees `raw_log2` returns a // non-negative sign, so the discarded sign flag is provably `false`. let (_, mag) = common::raw_log2(raw); diff --git a/math/fixed_point/tests/sd29x9_tests/log10_tests.move b/math/fixed_point/tests/sd29x9_tests/log10_tests.move index 22a2aeac..e3f29f63 100644 --- a/math/fixed_point/tests/sd29x9_tests/log10_tests.move +++ b/math/fixed_point/tests/sd29x9_tests/log10_tests.move @@ -15,37 +15,25 @@ fun log10_of_one_is_zero() { assert_eq!(sd29x9::one().log10(), sd29x9::zero()); } -// === Powers of ten (within 1 ulp at UD30x9 scale) === +// === Powers of ten (exact) === #[test] -fun log10_of_positive_powers_of_ten_pins_values() { - // log10(1) = 0 exactly. +fun log10_of_positive_powers_of_ten_is_exact() { assert_eq!(pos(SCALE).log10(), pos(0)); - // For k >= 1, log10(10^k) = k exactly, but flooring the floored-constant - // product at user scale lands the result 1 ulp below k * SCALE. let mut k: u8 = 1; while (k <= 11) { let x_raw = std::u128::pow(10, k) * SCALE; - let expected = pos((k as u128) * SCALE - 1); - assert_eq!(pos(x_raw).log10(), expected); + assert_eq!(pos(x_raw).log10(), pos((k as u128) * SCALE)); k = k + 1; }; } #[test] -fun log10_of_negative_powers_of_ten_pins_values() { - // log10(10^-k) = -k exactly. The negative-branch kernel produces a high- - // biased magnitude; combined with the floored `log10(2)` constant, the - // user-scale floor lands at `k * SCALE` for small `k` and one ulp below - // for larger `k`. Empirically: - // k = 1..=4: magnitude = k * SCALE - // k = 5..=9: magnitude = k * SCALE - 1 +fun log10_of_negative_powers_of_ten_is_exact() { let mut k: u8 = 1; while (k <= 9) { let x_raw = SCALE / std::u128::pow(10, k); - let mag = (k as u128) * SCALE; - let expected = neg(if (k <= 4) mag else mag - 1); - assert_eq!(pos(x_raw).log10(), expected); + assert_eq!(pos(x_raw).log10(), neg((k as u128) * SCALE)); k = k + 1; }; } @@ -79,7 +67,6 @@ fun log10_of_min_value_aborts() { #[test] fun log10_of_pos_1_matches_reference() { - // pos(1) represents 10^-9. log10(10^-9) = -9 exactly; flooring at user - // scale lands the magnitude 1 ulp below 9 * SCALE. - assert_eq!(pos(1).log10(), neg(9 * SCALE - 1)); + // pos(1) represents 10^-9. + assert_eq!(pos(1).log10(), neg(9 * SCALE)); } diff --git a/math/fixed_point/tests/ud30x9_tests/ln_tests.move b/math/fixed_point/tests/ud30x9_tests/ln_tests.move index 62f22e38..8955f01d 100644 --- a/math/fixed_point/tests/ud30x9_tests/ln_tests.move +++ b/math/fixed_point/tests/ud30x9_tests/ln_tests.move @@ -50,7 +50,7 @@ fun ln_of_zero_aborts() { ud30x9::zero().ln(); } -#[test, expected_failure(abort_code = ud30x9_base::ELogUndefined)] +#[test, expected_failure(abort_code = ud30x9_base::ELogResultUnrepresentable)] fun ln_of_sub_one_aborts() { fixed(SCALE - 1).ln(); } diff --git a/math/fixed_point/tests/ud30x9_tests/log10_tests.move b/math/fixed_point/tests/ud30x9_tests/log10_tests.move index f5a1a925..999437d8 100644 --- a/math/fixed_point/tests/ud30x9_tests/log10_tests.move +++ b/math/fixed_point/tests/ud30x9_tests/log10_tests.move @@ -15,19 +15,15 @@ fun log10_of_one_is_zero() { assert_eq!(ud30x9::one().log10(), ud30x9::zero()); } -// === Powers of 10 (algorithm floors at user scale, so k >= 1 lands 1 ulp below) === +// === Powers of 10 (exact) === #[test] -fun log10_of_powers_of_ten_pins_values() { - // log10(1) = 0 exactly. +fun log10_of_powers_of_ten_is_exact() { assert_eq!(fixed(SCALE).log10(), fixed(0)); - // For k >= 1, log10(10^k) = k exactly, but flooring the floored-constant - // product at user scale lands the result 1 ulp below k * SCALE. let mut k: u8 = 1; while (k <= 11) { let x_raw = std::u128::pow(10, k) * SCALE; - let expected = (k as u128) * SCALE - 1; - assert_eq!(fixed(x_raw).log10(), fixed(expected)); + assert_eq!(fixed(x_raw).log10(), fixed((k as u128) * SCALE)); k = k + 1; }; } @@ -47,7 +43,7 @@ fun log10_of_zero_aborts() { ud30x9::zero().log10(); } -#[test, expected_failure(abort_code = ud30x9_base::ELogUndefined)] +#[test, expected_failure(abort_code = ud30x9_base::ELogResultUnrepresentable)] fun log10_of_sub_one_aborts() { fixed(SCALE - 1).log10(); } diff --git a/math/fixed_point/tests/ud30x9_tests/log2_tests.move b/math/fixed_point/tests/ud30x9_tests/log2_tests.move index cb6ef5eb..e3386374 100644 --- a/math/fixed_point/tests/ud30x9_tests/log2_tests.move +++ b/math/fixed_point/tests/ud30x9_tests/log2_tests.move @@ -45,7 +45,7 @@ fun log2_of_zero_aborts() { ud30x9::zero().log2(); } -#[test, expected_failure(abort_code = ud30x9_base::ELogUndefined)] +#[test, expected_failure(abort_code = ud30x9_base::ELogResultUnrepresentable)] fun log2_of_sub_one_aborts() { fixed(SCALE - 1).log2(); }