From 130c3622bb50cd9ac6bdcf3c104858ea6134d2c6 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 27 Apr 2026 18:24:15 -0300 Subject: [PATCH 01/11] refactor ownable -> replace pk with sk --- contracts/src/access/Ownable.compact | 169 ++++-- contracts/src/access/test/Ownable.test.ts | 547 ++++++++++++------ .../src/access/test/mocks/MockOwnable.compact | 17 +- .../test/simulators/OwnableSimulator.ts | 65 ++- .../src/access/witnesses/OwnableWitnesses.ts | 75 ++- .../witnesses/test/OwnableWitnesses.test.ts | 132 +++++ 6 files changed, 766 insertions(+), 239 deletions(-) create mode 100644 contracts/src/access/witnesses/test/OwnableWitnesses.test.ts diff --git a/contracts/src/access/Ownable.compact b/contracts/src/access/Ownable.compact index d254d0c9..41964895 100644 --- a/contracts/src/access/Ownable.compact +++ b/contracts/src/access/Ownable.compact @@ -6,14 +6,25 @@ pragma language_version >= 0.21.0; /** * @module Ownable * @description An unshielded Ownable library. - * This modules provides a basic access control mechanism, where there is an owner + * This module provides a basic access control mechanism, where there is an owner * that can be granted exclusive access to specific circuits. * This approach is perfectly reasonable for contracts that have a single administrative user. * + * Authorization is based on a witness-derived identity scheme. The owner proves knowledge + * of a secret key by injecting it via the `wit_OwnableSK` witness. The module computes + * an account identifier as `persistentHash(secretKey)` which is a commitment that hides the key + * while providing a stable, pseudonymous on-chain identity. + * + * Because the account identifier is `H(secretKey)` with no per-deployment salt or domain + * separator, the same secret key produces the same identity across all contracts. This is + * intentional. It provides a linkable pseudonymous identity analogous to Solidity's + * `msg.sender`. Users who desire cross-contract unlinkability can use different secret keys + * per contract at the wallet layer. + * * The initial owner must be set by using the `initialize` circuit during construction. * This can later be changed with `transferOwnership`. * - * @notice Ownership can only be transferred to ZswapCoinPublicKeys + * @notice Ownership can only be transferred to Bytes<32> account identifiers * through the main transfer circuits (`transferOwnership` and `_transferOwnership`). * In other words, ownership transfers to contract addresses are disallowed through these * circuits. @@ -24,52 +35,70 @@ pragma language_version >= 0.21.0; * @notice This module does offer experimental circuits that allow ownership to be granted * to contract addresses (`_unsafeTransferOwnership` and `_unsafeUncheckedTransferOwnership`). * Note that the circuit names are very explicit ("unsafe") with these experimental circuits. - * Until contract-to-contract calls are supported, there is no direct way for a contract - * to authenticate as an owner. + * Until contract-to-contract calls are supported, + * there is no direct way for a contract to call circuits of other contracts + * or transfer ownership back to a user. * * @notice The unsafe circuits are planned to become deprecated once contract-to-contract calls * are supported. + * + * @dev Security Considerations: + * - The `secretKey` must be kept private. Loss of the key prevents the owner + * from proving ownership. Key exposure allows impersonation. + * - It is strongly recommended to use cryptographically secure random values for the secret key + * (e.g., `crypto.getRandomValues()`). Weak or predictable keys can be brute-forced. + * - The `secretKey` is provided via the `wit_OwnableSK` witness. As with all witnesses, + * the contract must not assume that the witness implementation matches the developer's code. + * Any DApp may provide any implementation. The ZK proof system constrains what values + * can produce valid proofs. */ module Ownable { import CompactStandardLibrary; import "../security/Initializable" prefix Initializable_; import "../utils/Utils" prefix Utils_; - export ledger _owner: Either; + export ledger _owner: Either, ContractAddress>; + + /** + * @witness wit_OwnableSK + * @description Returns the caller's secret key used in deriving the account identifier. + * + * The same key produces the same account identifier across all contracts. Users who + * desire cross-contract unlinkability should use different keys per contract. + * + * @returns {Bytes<32>} secretKey - A 32-byte cryptographically secure random value. + */ + witness wit_OwnableSK(): Bytes<32>; /** * @description Initializes the contract by setting the `initialOwner`. * This must be called in the contract's constructor. * - * @circuitInfo k=10, rows=258 - * * Requirements: * * - Contract is not already initialized. * - `initialOwner` is not a ContractAddress. * - `initialOwner` is not the zero address. * - * @param {Either} initialOwner - The initial owner of the contract. + * @param {Either, ContractAddress>} initialOwner - The initial owner of the contract. * @returns {[]} Empty tuple. */ - export circuit initialize(initialOwner: Either): [] { + export circuit initialize(initialOwner: Either, ContractAddress>): [] { Initializable_initialize(); - assert(!Utils_isKeyOrAddressZero(initialOwner), "Ownable: invalid initial owner"); + assert(!_isTargetZero(initialOwner), "Ownable: invalid initial owner"); _transferOwnership(initialOwner); } /** * @description Returns the current contract owner. * - * @circuitInfo k=10, rows=84 - * * Requirements: * * - Contract is initialized. * - * @returns {Either } - The contract owner. + * @returns {Either, ContractAddress>} - The contract owner. */ - export circuit owner(): Either { + export circuit owner(): Either, ContractAddress> { Initializable_assertInitialized(); return _owner; } @@ -77,8 +106,6 @@ module Ownable { /** * @description Transfers ownership of the contract to `newOwner`. * - * @circuitInfo k=10, rows=338 - * * @notice Ownership transfers to contract addresses are currently disallowed until contract-to-contract * interactions are supported in Compact. * This restriction prevents permanently disabling access to a circuit. @@ -90,20 +117,19 @@ module Ownable { * - `newOwner` is not a ContractAddress. * - `newOwner` is not the zero address. * - * @param {Either} newOwner - The new owner. + * @param {Either, ContractAddress>} newOwner - The new owner. * @returns {[]} Empty tuple. */ - export circuit transferOwnership(newOwner: Either): [] { + export circuit transferOwnership(newOwner: Either, ContractAddress>): [] { Initializable_assertInitialized(); - assert(!Utils_isContractAddress(newOwner), "Ownable: unsafe ownership transfer"); + const isContractAddr = !newOwner.is_left; + assert(!isContractAddr, "Ownable: unsafe ownership transfer"); _unsafeTransferOwnership(newOwner); } /** * @description Unsafe variant of `transferOwnership`. * - * @circuitInfo k=10, rows=335 - * * @warning Ownership transfers to contract addresses are considered unsafe because contract-to-contract * calls are not currently supported. * Ownership privileges sent to a contract address may become uncallable. @@ -115,13 +141,13 @@ module Ownable { * - The caller is the current contract owner. * - `newOwner` is not the zero address. * - * @param {Either} newOwner - The new owner. + * @param {Either, ContractAddress>} newOwner - The new owner. * @returns {[]} Empty tuple. */ - export circuit _unsafeTransferOwnership(newOwner: Either): [] { + export circuit _unsafeTransferOwnership(newOwner: Either, ContractAddress>): [] { Initializable_assertInitialized(); assertOnlyOwner(); - assert(!Utils_isKeyOrAddressZero(newOwner), "Ownable: invalid new owner"); + assert(!_isTargetZero(newOwner), "Ownable: invalid new owner"); _unsafeUncheckedTransferOwnership(newOwner); } @@ -130,8 +156,6 @@ module Ownable { * It will not be possible to call `assertOnlyOwner` circuits anymore. * Can only be called by the current owner. * - * @circuitInfo k=10, rows=124 - * * Requirements: * * - Contract is initialized. @@ -142,14 +166,21 @@ module Ownable { export circuit renounceOwnership(): [] { Initializable_assertInitialized(); assertOnlyOwner(); - _transferOwnership(shieldedBurnAddress()); + + // We use `left` here because `assertOnlyOwner` would return a misleading + // error msg (c2c is not currently supported) + const zero = Either, ContractAddress> { + is_left: true, left: default>, right: default + }; + _unsafeUncheckedTransferOwnership(zero); } /** * @description Throws if called by any account other than the owner. * Use this to restrict access of specific circuits to the owner. * - * @circuitInfo k=10, rows=115 + * The caller's identity is derived from the `wit_OwnableSK` witness as + * `persistentHash(secretKey)`. * * Requirements: * @@ -161,8 +192,8 @@ module Ownable { export circuit assertOnlyOwner(): [] { Initializable_assertInitialized(); if (_owner.is_left) { - const caller = ownPublicKey(); - assert(caller == _owner.left, "Ownable: caller is not the owner"); + const callerId = _computeAccountId(); + assert(callerId == _owner.left, "Ownable: caller is not the owner"); } else { assert(false, "Ownable: contract address owner authentication is not yet supported"); } @@ -172,8 +203,6 @@ module Ownable { * @description Transfers ownership of the contract to `newOwner` without * enforcing permission checks on the caller. * - * @circuitInfo k=10, rows=219 - * * @notice Ownership transfers to contract addresses are currently disallowed until contract-to-contract * interactions are supported in Compact. * This restriction prevents circuits from being inadvertently locked in contracts. @@ -183,20 +212,19 @@ module Ownable { * - Contract is initialized. * - `newOwner` is not a ContractAddress. * - * @param {Either} newOwner - The new owner. + * @param {Either, ContractAddress>} newOwner - The new owner. * @returns {[]} Empty tuple. */ - export circuit _transferOwnership(newOwner: Either): [] { + export circuit _transferOwnership(newOwner: Either, ContractAddress>): [] { Initializable_assertInitialized(); - assert(!Utils_isContractAddress(newOwner), "Ownable: unsafe ownership transfer"); + const isContractAddr = !newOwner.is_left; + assert(!isContractAddr, "Ownable: unsafe ownership transfer"); _unsafeUncheckedTransferOwnership(newOwner); } /** * @description Unsafe variant of `_transferOwnership`. * - * @circuitInfo k=10, rows=216 - * * @warning Ownership transfers to contract addresses are considered unsafe because contract-to-contract * calls are not currently supported. * Ownership privileges sent to a contract address may become uncallable. @@ -206,13 +234,74 @@ module Ownable { * * - Contract is initialized. * - * @param {Either} newOwner - The new owner. + * @param {Either, ContractAddress>} newOwner - The new owner. * @returns {[]} Empty tuple. */ export circuit _unsafeUncheckedTransferOwnership( - newOwner: Either + newOwner: Either, ContractAddress> ): [] { Initializable_assertInitialized(); - _owner = disclose(Utils_canonicalize(newOwner)); + const canonAcct = Utils_canonicalize, ContractAddress>(newOwner); + _owner = disclose(canonAcct); + } + + /** + * @description Computes the caller's account identifier from the `wit_OwnableSK` witness. + * + * ## ID Derivation + * `accountId = persistentHash(secretKey)` + * + * The result is a 32-byte commitment that uniquely identifies the caller. + * + * @returns {Bytes<32>} accountId - The computed account identifier. + */ + circuit _computeAccountId(): Bytes<32> { + return computeAccountId(wit_OwnableSK()); + } + + /** + * @description Computes an account identifier without on-chain state, allowing a user to derive + * their identity commitment before submitting it in an ownership transfer. + * This is the off-chain counterpart to {_computeAccountId} and produces an identical result + * given the same inputs. + * + * @warning OpSec: The `secretKey` parameter is a sensitive secret. Mishandling it can + * permanently compromise the security of this system: + * + * - **Never log or persist** the `secretKey` in plaintext — avoid browser devtools, + * application logs, analytics pipelines, or any observable side-channel. + * - **Store offline or in secure enclaves** — hardware security modules (HSMs), + * air-gapped devices, or encrypted vaults are strongly preferred over hot storage. + * - **Use cryptographically secure randomness** — generate keys with `crypto.getRandomValues()` + * or equivalent; weak or predictable keys can be brute-forced to reveal your identity. + * - **Treat key loss as identity loss** — a lost key cannot be recovered. + * - **Avoid calling this circuit in untrusted environments** — executing this in an + * unverified browser extension, compromised runtime, or shared machine may expose + * the key to a malicious observer. + * + * ## ID Derivation + * `accountId = persistentHash(secretKey)` + * + * @param {Bytes<32>} secretKey - A 32-byte cryptographically secure random value. + * + * @returns {Bytes<32>} accountId - The computed account identifier. + */ + export pure circuit computeAccountId(secretKey: Bytes<32>): Bytes<32> { + return persistentHash>>([secretKey]); + } + + /** + * @description Returns `true` if `target`'s active branch (as indicated by `is_left`) + * holds the zero value. + * + * @param {Either, ContractAddress>} target - The value to check. + * @returns {Boolean} - `true` if the active branch is zero, `false` otherwise. + */ + circuit _isTargetZero(target: Either, ContractAddress>): Boolean { + if (target.is_left) { + return target.left == default>; + } else { + return target.right == default; + } } } diff --git a/contracts/src/access/test/Ownable.test.ts b/contracts/src/access/test/Ownable.test.ts index ec4da964..a720a981 100644 --- a/contracts/src/access/test/Ownable.test.ts +++ b/contracts/src/access/test/Ownable.test.ts @@ -1,43 +1,92 @@ +import { + CompactTypeBytes, + CompactTypeVector, + persistentHash, +} from '@midnight-ntwrk/compact-runtime'; import { beforeEach, describe, expect, it } from 'vitest'; import * as utils from '#test-utils/address.js'; import { OwnableSimulator } from './simulators/OwnableSimulator.js'; -// PKs -const [OWNER, Z_OWNER] = utils.generateEitherPubKeyPair('OWNER'); -const [NEW_OWNER, Z_NEW_OWNER] = utils.generateEitherPubKeyPair('NEW_OWNER'); -const [UNAUTHORIZED, _] = utils.generateEitherPubKeyPair('UNAUTHORIZED'); - -// Encoded contract addresses -const Z_OWNER_CONTRACT = - utils.createEitherTestContractAddress('OWNER_CONTRACT'); -const Z_RECIPIENT_CONTRACT = - utils.createEitherTestContractAddress('RECIPIENT_CONTRACT'); - +// Helpers +const buildAccountIdHash = (sk: Uint8Array): Uint8Array => { + const rt_type = new CompactTypeVector(1, new CompactTypeBytes(32)); + return persistentHash(rt_type, [sk]); +}; + +const zeroBytes = utils.zeroUint8Array(); + +const eitherAccountId = (accountId: Uint8Array) => { + return { + is_left: true, + left: accountId, + right: { bytes: zeroBytes }, + }; +}; + +const eitherContract = (address: string) => { + return { + is_left: false, + left: zeroBytes, + right: utils.encodeToAddress(address), + }; +}; + +const createTestSK = (label: string): Uint8Array => { + const sk = new Uint8Array(32); + const encoded = new TextEncoder().encode(label); + sk.set(encoded.slice(0, 32)); + return sk; +}; + +const makeUser = (label: string) => { + const secretKey = createTestSK(label); + const accountId = buildAccountIdHash(secretKey); + const either = eitherAccountId(accountId); + return { secretKey, accountId, either }; +}; + +// Users +const OWNER = makeUser('OWNER'); +const NEW_OWNER = makeUser('NEW_OWNER'); +const UNAUTHORIZED = makeUser('UNAUTHORIZED'); + +// Contract addresses +const OWNER_CONTRACT = eitherContract('OWNER_CONTRACT'); +const RECIPIENT_CONTRACT = eitherContract('RECIPIENT_CONTRACT'); + +// Zero values +const ZERO_ACCOUNT = eitherAccountId(zeroBytes); +const ZERO_CONTRACT = { + is_left: false, + left: zeroBytes, + right: { bytes: zeroBytes }, +}; + +// Init flags const isInit = true; const isBadInit = false; let ownable: OwnableSimulator; const zeroTypes = [ - ['contract', utils.ZERO_ADDRESS], - ['pubkey', utils.ZERO_KEY], -] as const; - -const newOwnerTypes = [ - ['contract', Z_OWNER_CONTRACT], - ['pubkey', Z_NEW_OWNER], + ['contract', ZERO_CONTRACT], + ['accountId', ZERO_ACCOUNT], ] as const; describe('Ownable', () => { describe('before initialized', () => { it('should initialize', () => { - ownable = new OwnableSimulator(Z_OWNER, isInit); - expect(ownable.owner()).toEqual(Z_OWNER); + ownable = new OwnableSimulator(OWNER.either, isInit, { + privateState: { secretKey: OWNER.secretKey }, + }); + expect(ownable.owner()).toEqual(OWNER.either); }); it('should fail to initialize when owner is a contract address', () => { expect(() => { - new OwnableSimulator(Z_OWNER_CONTRACT, isInit); + new OwnableSimulator(OWNER_CONTRACT, isInit, { + privateState: { secretKey: OWNER.secretKey }, + }); }).toThrow('Ownable: unsafe ownership transfer'); }); @@ -45,279 +94,433 @@ describe('Ownable', () => { zeroTypes, )('should fail to initialize when owner is zero (%s)', (_, _zero) => { expect(() => { - ownable = new OwnableSimulator(_zero, isInit); + ownable = new OwnableSimulator(_zero, isInit, { + privateState: { secretKey: OWNER.secretKey }, + }); }).toThrow('Ownable: invalid initial owner'); }); type FailingCircuits = [method: keyof OwnableSimulator, args: unknown[]]; - // Circuit calls should fail before the args are used const circuitsToFail: FailingCircuits[] = [ ['owner', []], ['assertOnlyOwner', []], - ['transferOwnership', [Z_OWNER]], - ['_unsafeTransferOwnership', [Z_OWNER]], + ['transferOwnership', [OWNER.either]], + ['_unsafeTransferOwnership', [OWNER.either]], ['renounceOwnership', []], - ['_transferOwnership', [Z_OWNER]], - ['_unsafeUncheckedTransferOwnership', [Z_OWNER]], + ['_transferOwnership', [OWNER.either]], + ['_unsafeUncheckedTransferOwnership', [OWNER.either]], ]; it.each( circuitsToFail, )('should fail when calling circuit "%s"', (circuitName, args) => { - ownable = new OwnableSimulator(Z_OWNER, isBadInit); + ownable = new OwnableSimulator(OWNER.either, isBadInit, { + privateState: { secretKey: OWNER.secretKey }, + }); expect(() => { (ownable[circuitName] as (...args: unknown[]) => unknown)(...args); }).toThrow('Initializable: contract not initialized'); }); + + it('should canonicalize initial owner', () => { + const nonCanonical = { + is_left: true, + left: OWNER.accountId, + right: utils.encodeToAddress('JUNK_DATA'), + }; + + ownable = new OwnableSimulator(nonCanonical, isInit, { + privateState: { secretKey: OWNER.secretKey }, + }); + + const stored = ownable.owner(); + expect(stored.is_left).toBe(true); + expect(stored.left).toEqual(OWNER.accountId); + expect(stored.right).toEqual({ bytes: zeroBytes }); + }); }); describe('when initialized', () => { beforeEach(() => { - ownable = new OwnableSimulator(Z_OWNER, isInit); + ownable = new OwnableSimulator(OWNER.either, isInit, { + privateState: { secretKey: OWNER.secretKey }, + }); }); describe('owner', () => { it('should return owner', () => { - expect(ownable.owner()).toEqual(Z_OWNER); + expect(ownable.owner()).toEqual(OWNER.either); }); - it('should return zero address when unowned', () => { - ownable._transferOwnership(utils.ZERO_KEY); - expect(ownable.owner()).toEqual(utils.ZERO_KEY); + it('should return zero when unowned', () => { + ownable._transferOwnership(ZERO_ACCOUNT); + expect(ownable.owner()).toEqual(ZERO_ACCOUNT); + }); + }); + + describe('computeAccountId', () => { + it('should match pre-computed accountId', () => { + expect(ownable.computeAccountId(OWNER.secretKey)).toEqual( + OWNER.accountId, + ); + }); + + it('should produce different accountId with different key', () => { + expect(ownable.computeAccountId(UNAUTHORIZED.secretKey)).not.toEqual( + OWNER.accountId, + ); + }); + + it('should match test helper derivation', () => { + expect(ownable.computeAccountId(OWNER.secretKey)).toEqual( + buildAccountIdHash(OWNER.secretKey), + ); }); }); describe('assertOnlyOwner', () => { it('should allow owner to call', () => { - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).not.toThrow(); + expect(() => ownable.assertOnlyOwner()).not.toThrow(); }); it('should fail when called by unauthorized', () => { - expect(() => { - ownable.as(UNAUTHORIZED).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); }); - it('should fail when owner is a contract address', () => { - ownable._unsafeUncheckedTransferOwnership(Z_OWNER_CONTRACT); - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).toThrow( + it('should reject all accountId callers when owner is a contract', () => { + ownable._unsafeTransferOwnership(OWNER_CONTRACT); + + // Original owner rejected + expect(() => ownable.assertOnlyOwner()).toThrow( 'Ownable: contract address owner authentication is not yet supported', ); + + // Sample other keys + for (const label of ['SAMPLE_1', 'SAMPLE_2', 'SAMPLE_3']) { + const sampleUser = makeUser(label); + ownable.privateState.injectSecretKey(sampleUser.secretKey); + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: contract address owner authentication is not yet supported', + ); + } }); }); describe('transferOwnership', () => { it('should transfer ownership', () => { - ownable.as(OWNER).transferOwnership(Z_NEW_OWNER); - expect(ownable.owner()).toEqual(Z_NEW_OWNER); + ownable.transferOwnership(NEW_OWNER.either); + expect(ownable.owner()).toEqual(NEW_OWNER.either); - // Original owner - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); + // Original owner can no longer call + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); - expect(() => { - ownable.as(UNAUTHORIZED).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); + // Unauthorized still can't call + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); - expect(() => { - ownable.as(NEW_OWNER).assertOnlyOwner(); - }).not.toThrow(); + // New owner can call + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + expect(() => ownable.assertOnlyOwner()).not.toThrow(); }); it('should fail when unauthorized transfers ownership', () => { - expect(() => { - ownable.as(UNAUTHORIZED).transferOwnership(Z_NEW_OWNER); - }).toThrow('Ownable: caller is not the owner'); + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => ownable.transferOwnership(NEW_OWNER.either)).toThrow( + 'Ownable: caller is not the owner', + ); }); it('should fail when transferring to a contract address', () => { - expect(() => { - ownable.as(OWNER).transferOwnership(Z_RECIPIENT_CONTRACT); - }).toThrow('Ownable: unsafe ownership transfer'); + expect(() => ownable.transferOwnership(RECIPIENT_CONTRACT)).toThrow( + 'Ownable: unsafe ownership transfer', + ); }); - it('should fail when transferring to zero (pk)', () => { - expect(() => { - ownable.as(OWNER).transferOwnership(utils.ZERO_KEY); - }).toThrow('Ownable: invalid new owner'); + it('should fail when transferring to zero (accountId)', () => { + expect(() => ownable.transferOwnership(ZERO_ACCOUNT)).toThrow( + 'Ownable: invalid new owner', + ); }); it('should fail when transferring to zero (contract)', () => { - expect(() => { - ownable.as(OWNER).transferOwnership(utils.ZERO_ADDRESS); - }).toThrow('Ownable: unsafe ownership transfer'); + expect(() => ownable.transferOwnership(ZERO_CONTRACT)).toThrow( + 'Ownable: unsafe ownership transfer', + ); }); it('should transfer multiple times', () => { - ownable.as(OWNER).transferOwnership(Z_NEW_OWNER); + ownable.transferOwnership(NEW_OWNER.either); - ownable.as(NEW_OWNER).transferOwnership(Z_OWNER); + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + ownable.transferOwnership(OWNER.either); - ownable.as(OWNER).transferOwnership(Z_NEW_OWNER); + ownable.privateState.injectSecretKey(OWNER.secretKey); + ownable.transferOwnership(NEW_OWNER.either); - expect(ownable.owner()).toEqual(Z_NEW_OWNER); + expect(ownable.owner()).toEqual(NEW_OWNER.either); }); }); describe('_unsafeTransferOwnership', () => { - describe.each( - newOwnerTypes, - )('when the new owner is a %s', (type, newOwner) => { - it('should transfer ownership', () => { - ownable.as(OWNER)._unsafeTransferOwnership(newOwner); - expect(ownable.owner()).toEqual(newOwner); - - if (type === 'pubkey') { - expect(() => { - ownable.as(NEW_OWNER).assertOnlyOwner(); - }).not.toThrow(); - } else { - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).toThrow( - 'Ownable: contract address owner authentication is not yet supported', - ); - } - }); + it('should transfer ownership to accountId', () => { + ownable._unsafeTransferOwnership(NEW_OWNER.either); + expect(ownable.owner()).toEqual(NEW_OWNER.either); + + // Original owner rejected + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); + + // New owner can call + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + expect(() => ownable.assertOnlyOwner()).not.toThrow(); + }); + + it('should transfer ownership to contract', () => { + ownable._unsafeTransferOwnership(OWNER_CONTRACT); + expect(ownable.owner()).toEqual(OWNER_CONTRACT); + + // No one can authenticate, c2c not supported + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: contract address owner authentication is not yet supported', + ); }); it('should fail when unauthorized transfers ownership', () => { - expect(() => { - ownable.as(UNAUTHORIZED)._unsafeTransferOwnership(Z_NEW_OWNER); - }).toThrow('Ownable: caller is not the owner'); + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => + ownable._unsafeTransferOwnership(NEW_OWNER.either), + ).toThrow('Ownable: caller is not the owner'); }); - it('should fail when transferring to zero (pk)', () => { - expect(() => { - ownable.as(OWNER)._unsafeTransferOwnership(utils.ZERO_KEY); - }).toThrow('Ownable: invalid new owner'); + it('should fail when transferring to zero (accountId)', () => { + expect(() => ownable._unsafeTransferOwnership(ZERO_ACCOUNT)).toThrow( + 'Ownable: invalid new owner', + ); }); it('should fail when transferring to zero (contract)', () => { - expect(() => { - ownable.as(OWNER)._unsafeTransferOwnership(utils.ZERO_ADDRESS); - }).toThrow('Ownable: invalid new owner'); + expect(() => ownable._unsafeTransferOwnership(ZERO_CONTRACT)).toThrow( + 'Ownable: invalid new owner', + ); }); - it('should canonicalize crafted Either inputs (contract side)', () => { - const crafted = { - is_left: false, - left: Z_NEW_OWNER.left, - right: Z_OWNER_CONTRACT.right, - }; - ownable.as(OWNER)._unsafeTransferOwnership(crafted); - const stored = ownable.owner(); - // left must be zeroed after canonicalization - expect(stored.left).toEqual(utils.ZERO_KEY.left); - expect(stored.right).toEqual(Z_OWNER_CONTRACT.right); + it('should enforce permissions after transfer (accountId)', () => { + ownable._unsafeTransferOwnership(NEW_OWNER.either); + + // Original owner can no longer call + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); + + // Unauthorized still can't call + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); + + // New owner can call + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + expect(() => ownable.assertOnlyOwner()).not.toThrow(); + }); + + it('should transfer multiple times', () => { + ownable._unsafeTransferOwnership(NEW_OWNER.either); + + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + ownable._unsafeTransferOwnership(OWNER.either); + + ownable.privateState.injectSecretKey(OWNER.secretKey); + ownable._unsafeTransferOwnership(OWNER_CONTRACT); + + expect(ownable.owner()).toEqual(OWNER_CONTRACT); }); }); describe('renounceOwnership', () => { it('should renounce ownership', () => { - expect(ownable.owner()).toEqual(Z_OWNER); + expect(ownable.owner()).toEqual(OWNER.either); - ownable.as(OWNER).renounceOwnership(); + ownable.renounceOwnership(); - // Check owner - expect(ownable.owner()).toEqual(utils.ZERO_KEY); + expect(ownable.owner()).toEqual(ZERO_ACCOUNT); // Confirm revoked permissions - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); }); it('should fail when renouncing from unauthorized', () => { - expect(() => { - ownable.as(UNAUTHORIZED).renounceOwnership(); - }).toThrow('Ownable: caller is not the owner'); + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => ownable.renounceOwnership()).toThrow( + 'Ownable: caller is not the owner', + ); + }); + + it('should store canonical zero after renouncing', () => { + ownable.renounceOwnership(); + + const stored = ownable.owner(); + expect(stored.is_left).toBe(true); + expect(stored.left).toEqual(zeroBytes); + expect(stored.right).toEqual({ bytes: zeroBytes }); }); }); describe('_transferOwnership', () => { it('should transfer ownership', () => { - ownable._transferOwnership(Z_NEW_OWNER); - expect(ownable.owner()).toEqual(Z_NEW_OWNER); + ownable._transferOwnership(NEW_OWNER.either); + expect(ownable.owner()).toEqual(NEW_OWNER.either); - // Original owner - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); + // Original owner can no longer call + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); - expect(() => { - ownable.as(UNAUTHORIZED).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); + // Unauthorized still can't call + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); - expect(() => { - ownable.as(NEW_OWNER).assertOnlyOwner(); - }).not.toThrow(); + // New owner can call + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + expect(() => ownable.assertOnlyOwner()).not.toThrow(); }); it('should allow transfers to zero', () => { - ownable._transferOwnership(utils.ZERO_KEY); - expect(ownable.owner()).toEqual(utils.ZERO_KEY); + ownable._transferOwnership(ZERO_ACCOUNT); + expect(ownable.owner()).toEqual(ZERO_ACCOUNT); }); - it('should fail when transferring ownership to contract address zero', () => { - expect(() => { - ownable._transferOwnership(utils.ZERO_ADDRESS); - }).toThrow('Ownable: unsafe ownership transfer'); + it('should fail when transferring to contract address zero', () => { + expect(() => ownable._transferOwnership(ZERO_CONTRACT)).toThrow( + 'Ownable: unsafe ownership transfer', + ); }); - it('should fail when transferring ownership to non-zero contract address', () => { - expect(() => { - ownable._transferOwnership(Z_OWNER_CONTRACT); - }).toThrow('Ownable: unsafe ownership transfer'); + it('should fail when transferring to non-zero contract address', () => { + expect(() => ownable._transferOwnership(OWNER_CONTRACT)).toThrow( + 'Ownable: unsafe ownership transfer', + ); }); it('should transfer multiple times', () => { - ownable.as(OWNER)._transferOwnership(Z_NEW_OWNER); + ownable._transferOwnership(NEW_OWNER.either); - ownable.as(NEW_OWNER)._transferOwnership(Z_OWNER); + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + ownable._transferOwnership(OWNER.either); - ownable.as(OWNER)._transferOwnership(Z_NEW_OWNER); + ownable.privateState.injectSecretKey(OWNER.secretKey); + ownable._transferOwnership(NEW_OWNER.either); - expect(ownable.owner()).toEqual(Z_NEW_OWNER); + expect(ownable.owner()).toEqual(NEW_OWNER.either); + }); + + it('should allow transfers to zero', () => { + ownable._transferOwnership(ZERO_ACCOUNT); + expect(ownable.owner()).toEqual(ZERO_ACCOUNT); + + // No one can authenticate after zeroing + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); }); }); describe('_unsafeUncheckedTransferOwnership', () => { - describe.each( - newOwnerTypes, - )('when the new owner is a %s', (_, newOwner) => { - it('should transfer ownership without caller check', () => { - ownable._unsafeUncheckedTransferOwnership(newOwner); - expect(ownable.owner()).toEqual(newOwner); - }); + it('should transfer ownership to accountId', () => { + ownable._unsafeUncheckedTransferOwnership(NEW_OWNER.either); + expect(ownable.owner()).toEqual(NEW_OWNER.either); + + // Original owner rejected + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); + + // New owner can call + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + expect(() => ownable.assertOnlyOwner()).not.toThrow(); }); - it('should canonicalize crafted Either inputs (contract side)', () => { - const crafted = { - is_left: false, - left: Z_NEW_OWNER.left, - right: Z_OWNER_CONTRACT.right, + it('should transfer ownership to contract', () => { + ownable._unsafeUncheckedTransferOwnership(OWNER_CONTRACT); + expect(ownable.owner()).toEqual(OWNER_CONTRACT); + + // No one can authenticate, c2c not supported + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: contract address owner authentication is not yet supported', + ); + }); + + it('should enforce permissions after transfer (accountId)', () => { + ownable._unsafeUncheckedTransferOwnership(NEW_OWNER.either); + + // Original owner can no longer call + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); + + // Unauthorized still can't call + ownable.privateState.injectSecretKey(UNAUTHORIZED.secretKey); + expect(() => ownable.assertOnlyOwner()).toThrow( + 'Ownable: caller is not the owner', + ); + + // New owner can call + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + expect(() => ownable.assertOnlyOwner()).not.toThrow(); + }); + + it('should transfer multiple times', () => { + ownable._unsafeUncheckedTransferOwnership(NEW_OWNER.either); + + ownable.privateState.injectSecretKey(NEW_OWNER.secretKey); + ownable._unsafeUncheckedTransferOwnership(OWNER.either); + + ownable.privateState.injectSecretKey(OWNER.secretKey); + ownable._unsafeUncheckedTransferOwnership(OWNER_CONTRACT); + + expect(ownable.owner()).toEqual(OWNER_CONTRACT); + }); + + it('should canonicalize accountId (zero out inactive right side)', () => { + // Craft a non-canonical Either: is_left=true but right side has data + const nonCanonical = { + is_left: true, + left: NEW_OWNER.accountId, + right: utils.encodeToAddress('JUNK_DATA'), }; - ownable._unsafeUncheckedTransferOwnership(crafted); + + ownable._unsafeUncheckedTransferOwnership(nonCanonical); + const stored = ownable.owner(); - expect(stored.left).toEqual(utils.ZERO_KEY.left); - expect(stored.right).toEqual(Z_OWNER_CONTRACT.right); + expect(stored.is_left).toBe(true); + expect(stored.left).toEqual(NEW_OWNER.accountId); + expect(stored.right).toEqual({ bytes: zeroBytes }); }); - it('should canonicalize crafted Either inputs (pubkey side)', () => { - const crafted = { - is_left: true, - left: Z_NEW_OWNER.left, - right: Z_OWNER_CONTRACT.right, + it('should canonicalize contract address (zero out inactive left side)', () => { + // Craft a non-canonical Either: is_left=false but left side has data + const nonCanonical = { + is_left: false, + left: NEW_OWNER.accountId, + right: utils.encodeToAddress('OWNER_CONTRACT'), }; - ownable._unsafeUncheckedTransferOwnership(crafted); + + ownable._unsafeUncheckedTransferOwnership(nonCanonical); + const stored = ownable.owner(); - expect(stored.left).toEqual(Z_NEW_OWNER.left); - expect(stored.right).toEqual(utils.ZERO_ADDRESS.right); + expect(stored.is_left).toBe(false); + expect(stored.left).toEqual(zeroBytes); + expect(stored.right).toEqual(utils.encodeToAddress('OWNER_CONTRACT')); }); }); }); diff --git a/contracts/src/access/test/mocks/MockOwnable.compact b/contracts/src/access/test/mocks/MockOwnable.compact index 8294b8df..cac45d21 100644 --- a/contracts/src/access/test/mocks/MockOwnable.compact +++ b/contracts/src/access/test/mocks/MockOwnable.compact @@ -11,7 +11,7 @@ import CompactStandardLibrary; import "../../Ownable" prefix Ownable_; -export { ZswapCoinPublicKey, ContractAddress, Either, Maybe }; +export { ContractAddress, Either, Maybe }; /** * @description `isInit` is a param for testing. @@ -20,21 +20,21 @@ export { ZswapCoinPublicKey, ContractAddress, Either, Maybe }; * This behavior is to test that circuits are not callable unless the * contract is initialized. */ -constructor(initialOwner: Either, isInit: Boolean) { +constructor(initialOwner: Either, ContractAddress>, isInit: Boolean) { if (disclose(isInit)) { Ownable_initialize(initialOwner); } } -export circuit owner(): Either { +export circuit owner(): Either, ContractAddress> { return Ownable_owner(); } -export circuit transferOwnership(newOwner: Either): [] { +export circuit transferOwnership(newOwner: Either, ContractAddress>): [] { return Ownable_transferOwnership(newOwner); } -export circuit _unsafeTransferOwnership(newOwner: Either): [] { +export circuit _unsafeTransferOwnership(newOwner: Either, ContractAddress>): [] { return Ownable__unsafeTransferOwnership(newOwner); } @@ -46,11 +46,14 @@ export circuit assertOnlyOwner(): [] { return Ownable_assertOnlyOwner(); } -export circuit _transferOwnership(newOwner: Either): [] { +export circuit _transferOwnership(newOwner: Either, ContractAddress>): [] { return Ownable__transferOwnership(newOwner); } -export circuit _unsafeUncheckedTransferOwnership(newOwner: Either): [] { +export circuit _unsafeUncheckedTransferOwnership(newOwner: Either, ContractAddress>): [] { return Ownable__unsafeUncheckedTransferOwnership(newOwner); } +export pure circuit computeAccountId(secretKey: Bytes<32>): Bytes<32> { + return Ownable_computeAccountId(secretKey); +} diff --git a/contracts/src/access/test/simulators/OwnableSimulator.ts b/contracts/src/access/test/simulators/OwnableSimulator.ts index 59d0a711..c478460b 100644 --- a/contracts/src/access/test/simulators/OwnableSimulator.ts +++ b/contracts/src/access/test/simulators/OwnableSimulator.ts @@ -7,7 +7,6 @@ import { type Either, ledger, Contract as MockOwnable, - type ZswapCoinPublicKey, } from '../../../../artifacts/MockOwnable/contract/index.js'; import { OwnablePrivateState, @@ -18,7 +17,7 @@ import { * Type constructor args */ type OwnableArgs = readonly [ - initialOwner: Either, + initialOwner: Either, isInit: boolean, ]; @@ -31,7 +30,7 @@ const OwnableSimulatorBase = createSimulator< >({ contractFactory: (witnesses) => new MockOwnable(witnesses), - defaultPrivateState: () => OwnablePrivateState, + defaultPrivateState: () => OwnablePrivateState.generate(), contractArgs: (initialOwner, isInit) => [initialOwner, isInit], ledgerExtractor: (state) => ledger(state), witnessesFactory: () => OwnableWitnesses(), @@ -42,7 +41,7 @@ const OwnableSimulatorBase = createSimulator< */ export class OwnableSimulator extends OwnableSimulatorBase { constructor( - initialOwner: Either, + initialOwner: Either, isInit: boolean, options: BaseSimulatorOptions< OwnablePrivateState, @@ -55,7 +54,7 @@ export class OwnableSimulator extends OwnableSimulatorBase { * @description Returns the current contract owner. * @returns The contract owner. */ - public owner(): Either { + public owner(): Either { return this.circuits.impure.owner(); } @@ -63,19 +62,16 @@ export class OwnableSimulator extends OwnableSimulatorBase { * @description Transfers ownership of the contract to `newOwner`. * @param newOwner - The new owner. */ - public transferOwnership( - newOwner: Either, - ) { + public transferOwnership(newOwner: Either) { this.circuits.impure.transferOwnership(newOwner); } /** - * @description Unsafe variant of `transferOwnership` that allows transferring - * ownership to a contract address. + * @description Unsafe variant of `transferOwnership`. * @param newOwner - The new owner. */ public _unsafeTransferOwnership( - newOwner: Either, + newOwner: Either, ) { this.circuits.impure._unsafeTransferOwnership(newOwner); } @@ -102,20 +98,55 @@ export class OwnableSimulator extends OwnableSimulatorBase { * enforcing permission checks on the caller. * @param newOwner - The new owner. */ - public _transferOwnership( - newOwner: Either, - ) { + public _transferOwnership(newOwner: Either) { this.circuits.impure._transferOwnership(newOwner); } /** - * @description Unsafe variant of `_transferOwnership` without caller checks - * that allows transferring ownership to a contract address. + * @description Unsafe variant of `_transferOwnership`. * @param newOwner - The new owner. */ public _unsafeUncheckedTransferOwnership( - newOwner: Either, + newOwner: Either, ) { this.circuits.impure._unsafeUncheckedTransferOwnership(newOwner); } + + /** + * @description Computes an account identifier without on-chain state, allowing a user to derive + * their identity commitment before submitting it in a grant or revoke operation. + * @param {Bytes<32>} secretKey - A 32-byte cryptographically secure random value. + * @returns {Bytes<32>} accountId - The computed account identifier. + */ + public computeAccountId(secretKey: Uint8Array): Uint8Array { + return this.circuits.pure.computeAccountId(secretKey); + } + + public readonly privateState = { + /** + * @description Replaces the secret key in the private state. Used in tests to + * simulate switching between different user identities or injecting incorrect + * keys to test failure paths. + * @param newSK - The new secret key to set. + * @returns The updated private state. + */ + injectSecretKey: (newSK: Uint8Array): OwnablePrivateState => { + const updatedState = { secretKey: newSK }; + this.circuitContextManager.updatePrivateState(updatedState); + return updatedState; + }, + + /** + * @description Returns the current secret key from the private state. + * @returns The secret key. + * @throws If the secret key is undefined. + */ + getCurrentSecretKey: (): Uint8Array => { + const sk = this.getPrivateState().secretKey; + if (typeof sk === 'undefined') { + throw new Error('Missing secret key'); + } + return sk; + }, + }; } diff --git a/contracts/src/access/witnesses/OwnableWitnesses.ts b/contracts/src/access/witnesses/OwnableWitnesses.ts index a3d39a17..fad40071 100644 --- a/contracts/src/access/witnesses/OwnableWitnesses.ts +++ b/contracts/src/access/witnesses/OwnableWitnesses.ts @@ -1,6 +1,75 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (access/witnesses/OwnableWitnesses.ts) -export type OwnablePrivateState = Record; -export const OwnablePrivateState: OwnablePrivateState = {}; -export const OwnableWitnesses = () => ({}); +import { getRandomValues } from 'node:crypto'; +import type { WitnessContext } from '@midnight-ntwrk/compact-runtime'; +import type { Ledger } from '../../../artifacts/MockOwnable/contract/index.js'; + +/** + * @description Interface defining the witness methods for Ownable operations. + * @template P - The private state type. + */ +export interface IOwnableWitnesses

{ + /** + * Retrieves the secret key from the private state. + * @param context - The witness context containing the private state. + * @returns A tuple of the private state and the secret key as a Uint8Array. + */ + wit_OwnableSK(context: WitnessContext): [P, Uint8Array]; +} + +/** + * @description Represents the private state of an Ownable contract, storing a secret key. + */ +export type OwnablePrivateState = { + /** @description A 32-byte secret key used for creating a public user identifier. */ + secretKey: Uint8Array; +}; + +/** + * @description Utility object for managing the private state of an Ownable contract. + */ +export const OwnablePrivateState = { + /** + * @description Generates a new private state with a random secret key. + * @returns A fresh OwnablePrivateState instance. + */ + generate: (): OwnablePrivateState => { + return { secretKey: getRandomValues(new Uint8Array(32)) }; + }, + + /** + * @description Generates a new private state with a user-defined secret key. + * Useful for deterministic nonce generation or advanced use cases. + * + * @param sk - The 32-byte secret nonce to use. + * @returns A fresh OwnablePrivateState instance with the provided nonce. + * + * @example + * ```typescript + * // For deterministic keys (user-defined scheme) + * const deterministicKey = myDeterministicScheme(...); + * const privateState = OwnablePrivateState.withSecretKey(deterministicKey); + * ``` + */ + withSecretKey: (sk: Uint8Array): OwnablePrivateState => { + if (sk.length !== 32) { + throw new Error( + `withSecretKey: expected 32-byte secret key, received ${sk.length} bytes`, + ); + } + return { secretKey: Uint8Array.from(sk) }; + }, +}; + +/** + * @description Factory function creating witness implementations for Ownable operations. + * @returns An object implementing the Witnesses interface for OwnablePrivateState. + */ +export const OwnableWitnesses = (): IOwnableWitnesses => ({ + wit_OwnableSK( + context: WitnessContext, + ): [OwnablePrivateState, Uint8Array] { + return [context.privateState, context.privateState.secretKey]; + }, +}); diff --git a/contracts/src/access/witnesses/test/OwnableWitnesses.test.ts b/contracts/src/access/witnesses/test/OwnableWitnesses.test.ts new file mode 100644 index 00000000..57bfa7b2 --- /dev/null +++ b/contracts/src/access/witnesses/test/OwnableWitnesses.test.ts @@ -0,0 +1,132 @@ +import type { WitnessContext } from '@midnight-ntwrk/compact-runtime'; +import { describe, expect, it } from 'vitest'; +import type { Ledger } from '../../../../artifacts/MockOwnable/contract/index.js'; +import { OwnablePrivateState, OwnableWitnesses } from '../OwnableWitnesses.js'; + +const SECRET_KEY = new Uint8Array(32).fill(0x34); + +describe('OwnablePrivateState', () => { + describe('generate', () => { + it('should return a state with a 32-byte secretKey', () => { + const state = OwnablePrivateState.generate(); + expect(state.secretKey).toBeInstanceOf(Uint8Array); + expect(state.secretKey.length).toBe(32); + }); + + it('should produce unique secret key on successive calls', () => { + const a = OwnablePrivateState.generate(); + const b = OwnablePrivateState.generate(); + expect(a.secretKey).not.toEqual(b.secretKey); + }); + }); + + describe('withSecretKey', () => { + it('should accept a valid 32-byte secret key', () => { + const state = OwnablePrivateState.withSecretKey(SECRET_KEY); + expect(state.secretKey).toEqual(SECRET_KEY); + }); + + it('should create a defensive copy of the input secret key', () => { + const sk = new Uint8Array(32).fill(0xcc); + const state = OwnablePrivateState.withSecretKey(sk); + + sk.fill(0xff); + expect(state.secretKey).toEqual(new Uint8Array(32).fill(0xcc)); + }); + + it('should throw for a secret key shorter than 32 bytes', () => { + const short = new Uint8Array(16); + expect(() => OwnablePrivateState.withSecretKey(short)).toThrowError( + 'withSecretKey: expected 32-byte secret key, received 16 bytes', + ); + }); + + it('should throw for a secret key longer than 32 bytes', () => { + const long = new Uint8Array(64); + expect(() => OwnablePrivateState.withSecretKey(long)).toThrowError( + 'withSecretKey: expected 32-byte secret key, received 64 bytes', + ); + }); + + it('should throw for an empty array', () => { + expect(() => + OwnablePrivateState.withSecretKey(new Uint8Array(0)), + ).toThrowError( + 'withSecretKey: expected 32-byte secret key, received 0 bytes', + ); + }); + }); +}); + +describe('OwnableWitnesses', () => { + const witnesses = OwnableWitnesses(); + + function makeContext( + privateState: OwnablePrivateState, + ): WitnessContext { + return { privateState } as WitnessContext; + } + + describe('wit_OwnableSK', () => { + it('should return a tuple of [privateState, secretKey]', () => { + const state = OwnablePrivateState.withSecretKey(SECRET_KEY); + const ctx = makeContext(state); + + const [returnedState, returnedSK] = witnesses.wit_OwnableSK(ctx); + + expect(returnedState).toBe(state); + expect(returnedSK).toEqual(SECRET_KEY); + }); + + it('should return the exact same privateState reference', () => { + const state = OwnablePrivateState.generate(); + const ctx = makeContext(state); + + const [returnedState] = witnesses.wit_OwnableSK(ctx); + expect(returnedState).toBe(state); + }); + + it('should return the secretKey as a Uint8Array', () => { + const state = OwnablePrivateState.generate(); + const ctx = makeContext(state); + + const [, returnedSK] = witnesses.wit_OwnableSK(ctx); + expect(returnedSK).toBeInstanceOf(Uint8Array); + expect(returnedSK.length).toBe(32); + }); + + it('should work with a randomly generated state', () => { + const state = OwnablePrivateState.generate(); + const ctx = makeContext(state); + + const [returnedState, returnedSK] = witnesses.wit_OwnableSK(ctx); + + expect(returnedState).toBe(state); + expect(returnedSK).toEqual(state.secretKey); + }); + }); +}); + +describe('OwnableWitnesses factory', () => { + it('should return a fresh witnesses object on each call', () => { + const a = OwnableWitnesses(); + const b = OwnableWitnesses(); + expect(a).not.toBe(b); + }); + + it('should produce witnesses with identical behaviour', () => { + const a = OwnableWitnesses(); + const b = OwnableWitnesses(); + const state = OwnablePrivateState.generate(); + const ctx = { privateState: state } as WitnessContext< + Ledger, + OwnablePrivateState + >; + + const [stateA, skA] = a.wit_OwnableSK(ctx); + const [stateB, skB] = b.wit_OwnableSK(ctx); + + expect(stateA).toBe(stateB); + expect(skA).toEqual(skB); + }); +}); From 2d7ab6e4c92d5b6e841fc7166538b4a4563fc234 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 27 Apr 2026 18:35:00 -0300 Subject: [PATCH 02/11] improve assertOnlyOwner comment --- contracts/src/access/Ownable.compact | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/src/access/Ownable.compact b/contracts/src/access/Ownable.compact index 41964895..69ea391e 100644 --- a/contracts/src/access/Ownable.compact +++ b/contracts/src/access/Ownable.compact @@ -179,8 +179,8 @@ module Ownable { * @description Throws if called by any account other than the owner. * Use this to restrict access of specific circuits to the owner. * - * The caller's identity is derived from the `wit_OwnableSK` witness as - * `persistentHash(secretKey)`. + * In the case of an external (non-contract) caller, the caller’s identity is + * derived from the `wit_OwnableSK` witness as `persistentHash(secretKey)`. * * Requirements: * From 52750c66ba3b40c679b9401565f0bba560559ec7 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 27 Apr 2026 18:52:13 -0300 Subject: [PATCH 03/11] decouple witness file from mock ledger --- contracts/src/access/witnesses/OwnableWitnesses.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/contracts/src/access/witnesses/OwnableWitnesses.ts b/contracts/src/access/witnesses/OwnableWitnesses.ts index fad40071..0a299bac 100644 --- a/contracts/src/access/witnesses/OwnableWitnesses.ts +++ b/contracts/src/access/witnesses/OwnableWitnesses.ts @@ -3,19 +3,18 @@ import { getRandomValues } from 'node:crypto'; import type { WitnessContext } from '@midnight-ntwrk/compact-runtime'; -import type { Ledger } from '../../../artifacts/MockOwnable/contract/index.js'; /** * @description Interface defining the witness methods for Ownable operations. * @template P - The private state type. */ -export interface IOwnableWitnesses

{ +export interface IOwnableWitnesses { /** * Retrieves the secret key from the private state. * @param context - The witness context containing the private state. * @returns A tuple of the private state and the secret key as a Uint8Array. */ - wit_OwnableSK(context: WitnessContext): [P, Uint8Array]; + wit_OwnableSK(context: WitnessContext): [P, Uint8Array]; } /** @@ -66,9 +65,12 @@ export const OwnablePrivateState = { * @description Factory function creating witness implementations for Ownable operations. * @returns An object implementing the Witnesses interface for OwnablePrivateState. */ -export const OwnableWitnesses = (): IOwnableWitnesses => ({ +export const OwnableWitnesses = (): IOwnableWitnesses< + L, + OwnablePrivateState +> => ({ wit_OwnableSK( - context: WitnessContext, + context: WitnessContext, ): [OwnablePrivateState, Uint8Array] { return [context.privateState, context.privateState.secretKey]; }, From 42cd6b789e9aa61e986675243b52339f0b8e1ef4 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 27 Apr 2026 18:53:37 -0300 Subject: [PATCH 04/11] improve ps in sim --- contracts/src/access/test/simulators/OwnableSimulator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/src/access/test/simulators/OwnableSimulator.ts b/contracts/src/access/test/simulators/OwnableSimulator.ts index c478460b..eb3bb2e4 100644 --- a/contracts/src/access/test/simulators/OwnableSimulator.ts +++ b/contracts/src/access/test/simulators/OwnableSimulator.ts @@ -131,7 +131,7 @@ export class OwnableSimulator extends OwnableSimulatorBase { * @returns The updated private state. */ injectSecretKey: (newSK: Uint8Array): OwnablePrivateState => { - const updatedState = { secretKey: newSK }; + const updatedState = OwnablePrivateState.withSecretKey(newSK); this.circuitContextManager.updatePrivateState(updatedState); return updatedState; }, @@ -146,7 +146,7 @@ export class OwnableSimulator extends OwnableSimulatorBase { if (typeof sk === 'undefined') { throw new Error('Missing secret key'); } - return sk; + return Uint8Array.from(sk); }, }; } From 0ba05fb7b8fcc61714ee01be5b14818efab74f23 Mon Sep 17 00:00:00 2001 From: andrew Date: Wed, 29 Apr 2026 01:33:16 -0300 Subject: [PATCH 05/11] return separate buffer in witness --- contracts/src/access/witnesses/OwnableWitnesses.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/src/access/witnesses/OwnableWitnesses.ts b/contracts/src/access/witnesses/OwnableWitnesses.ts index 0a299bac..226eb1c3 100644 --- a/contracts/src/access/witnesses/OwnableWitnesses.ts +++ b/contracts/src/access/witnesses/OwnableWitnesses.ts @@ -72,6 +72,9 @@ export const OwnableWitnesses = (): IOwnableWitnesses< wit_OwnableSK( context: WitnessContext, ): [OwnablePrivateState, Uint8Array] { - return [context.privateState, context.privateState.secretKey]; + return [ + context.privateState, + Uint8Array.from(context.privateState.secretKey), + ]; }, }); From cbb3afb529a5c4f6a9982b819bf8323685840f21 Mon Sep 17 00:00:00 2001 From: andrew Date: Wed, 29 Apr 2026 13:05:56 -0300 Subject: [PATCH 06/11] add canonicalization doc --- contracts/src/access/Ownable.compact | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/contracts/src/access/Ownable.compact b/contracts/src/access/Ownable.compact index 69ea391e..da0b32d1 100644 --- a/contracts/src/access/Ownable.compact +++ b/contracts/src/access/Ownable.compact @@ -42,6 +42,13 @@ pragma language_version >= 0.21.0; * @notice The unsafe circuits are planned to become deprecated once contract-to-contract calls * are supported. * + * @dev Canonicalization + * All `Either, ContractAddress>` values are canonicalized before being written + * to the `_owner` ledger field. Canonicalization zeroes out the inactive branch of the Either, + * ensuring consistent storage regardless of what data the inactive branch carries. + * The single write point is `_unsafeUncheckedTransferOwnership`, through which all + * ownership changes flow. + * * @dev Security Considerations: * - The `secretKey` must be kept private. Loss of the key prevents the owner * from proving ownership. Key exposure allows impersonation. From ddba02d065aa0013d423e069f445baf83de60733 Mon Sep 17 00:00:00 2001 From: Andrew Fleming Date: Thu, 30 Apr 2026 10:08:35 -0500 Subject: [PATCH 07/11] Apply suggestions from code review Co-authored-by: 0xisk <0xisk@proton.me> Signed-off-by: Andrew Fleming --- contracts/src/access/Ownable.compact | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/src/access/Ownable.compact b/contracts/src/access/Ownable.compact index da0b32d1..a77340e3 100644 --- a/contracts/src/access/Ownable.compact +++ b/contracts/src/access/Ownable.compact @@ -199,8 +199,7 @@ module Ownable { export circuit assertOnlyOwner(): [] { Initializable_assertInitialized(); if (_owner.is_left) { - const callerId = _computeAccountId(); - assert(callerId == _owner.left, "Ownable: caller is not the owner"); + assert(_computeAccountId() == _owner.left, "Ownable: caller is not the owner"); } else { assert(false, "Ownable: contract address owner authentication is not yet supported"); } From 4b82fac6e2836bf13fd5a5b5421238e95ce3ba36 Mon Sep 17 00:00:00 2001 From: andrew Date: Thu, 30 Apr 2026 12:26:18 -0300 Subject: [PATCH 08/11] remove duplicate test --- contracts/src/access/test/Ownable.test.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/contracts/src/access/test/Ownable.test.ts b/contracts/src/access/test/Ownable.test.ts index a720a981..fe434341 100644 --- a/contracts/src/access/test/Ownable.test.ts +++ b/contracts/src/access/test/Ownable.test.ts @@ -395,11 +395,6 @@ describe('Ownable', () => { expect(() => ownable.assertOnlyOwner()).not.toThrow(); }); - it('should allow transfers to zero', () => { - ownable._transferOwnership(ZERO_ACCOUNT); - expect(ownable.owner()).toEqual(ZERO_ACCOUNT); - }); - it('should fail when transferring to contract address zero', () => { expect(() => ownable._transferOwnership(ZERO_CONTRACT)).toThrow( 'Ownable: unsafe ownership transfer', From 735cdd684634dfd468e7063516dac0678a92991f Mon Sep 17 00:00:00 2001 From: andrew Date: Thu, 30 Apr 2026 12:27:19 -0300 Subject: [PATCH 09/11] fix comments in witness file --- contracts/src/access/witnesses/OwnableWitnesses.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/src/access/witnesses/OwnableWitnesses.ts b/contracts/src/access/witnesses/OwnableWitnesses.ts index 226eb1c3..c39eeb05 100644 --- a/contracts/src/access/witnesses/OwnableWitnesses.ts +++ b/contracts/src/access/witnesses/OwnableWitnesses.ts @@ -39,10 +39,10 @@ export const OwnablePrivateState = { /** * @description Generates a new private state with a user-defined secret key. - * Useful for deterministic nonce generation or advanced use cases. + * Useful for deterministic key generation or advanced use cases. * - * @param sk - The 32-byte secret nonce to use. - * @returns A fresh OwnablePrivateState instance with the provided nonce. + * @param sk - The 32-byte secret key to use. + * @returns A fresh OwnablePrivateState instance with the provided key. * * @example * ```typescript From f4a18f35fed03b9e0fc9a67cd13ac9fae151f9c7 Mon Sep 17 00:00:00 2001 From: andrew Date: Thu, 30 Apr 2026 14:37:03 -0300 Subject: [PATCH 10/11] add circuitInfo --- contracts/src/access/Ownable.compact | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/contracts/src/access/Ownable.compact b/contracts/src/access/Ownable.compact index a77340e3..e82ba90d 100644 --- a/contracts/src/access/Ownable.compact +++ b/contracts/src/access/Ownable.compact @@ -81,6 +81,8 @@ module Ownable { * @description Initializes the contract by setting the `initialOwner`. * This must be called in the contract's constructor. * + * @circuitInfo k=10, rows=626 + * * Requirements: * * - Contract is not already initialized. @@ -99,6 +101,8 @@ module Ownable { /** * @description Returns the current contract owner. * + * @circuitInfo k=7, rows=76 + * * Requirements: * * - Contract is initialized. @@ -117,6 +121,8 @@ module Ownable { * interactions are supported in Compact. * This restriction prevents permanently disabling access to a circuit. * + * @circuitInfo k=13, rows=2959 + * * Requirements: * * - Contract is initialized. @@ -142,6 +148,8 @@ module Ownable { * Ownership privileges sent to a contract address may become uncallable. * Once contract-to-contract calls are supported, this circuit may be deprecated. * + * @circuitInfo k=13, rows=2956 + * * Requirements: * * - Contract is initialized. @@ -163,6 +171,8 @@ module Ownable { * It will not be possible to call `assertOnlyOwner` circuits anymore. * Can only be called by the current owner. * + * @circuitInfo k=13, rows=2364 + * * Requirements: * * - Contract is initialized. @@ -189,6 +199,8 @@ module Ownable { * In the case of an external (non-contract) caller, the caller’s identity is * derived from the `wit_OwnableSK` witness as `persistentHash(secretKey)`. * + * @circuitInfo k=13, rows=2360 + * * Requirements: * * - Contract is initialized. @@ -213,6 +225,8 @@ module Ownable { * interactions are supported in Compact. * This restriction prevents circuits from being inadvertently locked in contracts. * + * @circuitInfo k=10, rows=600 + * * Requirements: * * - Contract is initialized. @@ -236,6 +250,8 @@ module Ownable { * Ownership privileges sent to a contract address may become uncallable. * Once contract-to-contract calls are supported, this circuit may be deprecated. * + * @circuitInfo k=10, rows=597 + * * Requirements: * * - Contract is initialized. From a96a340a3a263ac311b9d59786608dc31f9c5fed Mon Sep 17 00:00:00 2001 From: Andrew Fleming Date: Thu, 30 Apr 2026 12:38:27 -0500 Subject: [PATCH 11/11] Apply suggestions from code review Co-authored-by: 0xisk <0xisk@proton.me> Signed-off-by: Andrew Fleming --- contracts/src/access/Ownable.compact | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/src/access/Ownable.compact b/contracts/src/access/Ownable.compact index e82ba90d..ba7f5a49 100644 --- a/contracts/src/access/Ownable.compact +++ b/contracts/src/access/Ownable.compact @@ -35,9 +35,8 @@ pragma language_version >= 0.21.0; * @notice This module does offer experimental circuits that allow ownership to be granted * to contract addresses (`_unsafeTransferOwnership` and `_unsafeUncheckedTransferOwnership`). * Note that the circuit names are very explicit ("unsafe") with these experimental circuits. - * Until contract-to-contract calls are supported, - * there is no direct way for a contract to call circuits of other contracts - * or transfer ownership back to a user. + * Until contract-to-contract calls are supported, there is no direct way for a contract + * to authenticate as an owner. * * @notice The unsafe circuits are planned to become deprecated once contract-to-contract calls * are supported.