diff --git a/contracts/src/access/Ownable.compact b/contracts/src/access/Ownable.compact index 5c15f5a5..ceffd72b 100644 --- a/contracts/src/access/Ownable.compact +++ b/contracts/src/access/Ownable.compact @@ -21,15 +21,10 @@ pragma language_version >= 0.21.0; * if a contract is granted ownership, the owner contract cannot directly call the protected * circuit. * - * @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. - * - * @notice The unsafe circuits are planned to become deprecated once contract-to-contract calls - * are supported. + * @notice Ownership transfers to contract addresses are not currently supported because + * `assertOnlyOwner` cannot authenticate contract address callers. The platform does not + * provide a mechanism to identify the calling contract. Until contract-to-contract calls + * are supported, ownership is restricted to public key accounts. */ module Ownable { import CompactStandardLibrary; @@ -119,7 +114,7 @@ module Ownable { * @param {Either} newOwner - The new owner. * @returns {[]} Empty tuple. */ - export circuit _unsafeTransferOwnership(newOwner: Either): [] { + circuit _unsafeTransferOwnership(newOwner: Either): [] { Initializable_assertInitialized(); assertOnlyOwner(); assert(!Utils_isKeyOrAddressZero(newOwner), "Ownable: invalid new owner"); @@ -206,7 +201,7 @@ module Ownable { * @param {Either} newOwner - The new owner. * @returns {[]} Empty tuple. */ - export circuit _unsafeUncheckedTransferOwnership( + circuit _unsafeUncheckedTransferOwnership( newOwner: Either ): [] { Initializable_assertInitialized(); diff --git a/contracts/src/access/test/Ownable.test.ts b/contracts/src/access/test/Ownable.test.ts index af2716a0..52b2f99c 100644 --- a/contracts/src/access/test/Ownable.test.ts +++ b/contracts/src/access/test/Ownable.test.ts @@ -18,11 +18,6 @@ const isBadInit = false; let ownable: OwnableSimulator; -const newOwnerTypes = [ - ['contract', Z_OWNER_CONTRACT], - ['pubkey', Z_NEW_OWNER], -] as const; - const zeroTypes = [ ['contract', utils.ZERO_ADDRESS], ['pubkey', utils.ZERO_KEY], @@ -55,10 +50,8 @@ describe('Ownable', () => { ['owner', []], ['assertOnlyOwner', []], ['transferOwnership', [Z_OWNER]], - ['_unsafeTransferOwnership', [Z_OWNER]], ['renounceOwnership', []], ['_transferOwnership', [Z_OWNER]], - ['_unsafeUncheckedTransferOwnership', [Z_OWNER]], ]; it.each( circuitsToFail, @@ -154,57 +147,6 @@ describe('Ownable', () => { }); }); - describe('_unsafeTransferOwnership', () => { - describe.each( - newOwnerTypes, - )('when the owner is a %s', (type, newOwner) => { - it('should transfer ownership', () => { - ownable.as(OWNER)._unsafeTransferOwnership(newOwner); - expect(ownable.owner()).toEqual(newOwner); - - // Original owner - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); - - if (type === 'pubkey') { - // New owner - expect(() => { - ownable.as(NEW_OWNER).assertOnlyOwner(); - }).not.toThrow(); - } - }); - }); - - it('should fail when unauthorized transfers ownership', () => { - expect(() => { - ownable.as(UNAUTHORIZED)._unsafeTransferOwnership(Z_NEW_OWNER); - }).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 (contract)', () => { - expect(() => { - ownable.as(OWNER)._unsafeTransferOwnership(utils.ZERO_ADDRESS); - }).toThrow('Ownable: invalid new owner'); - }); - - it('should transfer multiple times', () => { - ownable.as(OWNER)._unsafeTransferOwnership(Z_NEW_OWNER); - - ownable.as(NEW_OWNER)._unsafeTransferOwnership(Z_OWNER); - - ownable.as(OWNER)._unsafeTransferOwnership(Z_OWNER_CONTRACT); - - expect(ownable.owner()).toEqual(Z_OWNER_CONTRACT); - }); - }); - describe('renounceOwnership', () => { it('should renounce ownership', () => { expect(ownable.owner()).toEqual(Z_OWNER); @@ -274,41 +216,5 @@ describe('Ownable', () => { }); }); - describe('_unsafeUncheckedTransferOwnership', () => { - describe.each(newOwnerTypes)('when the owner is a %s', (_, newOwner) => { - it('should transfer ownership', () => { - ownable._unsafeUncheckedTransferOwnership(newOwner); - expect(ownable.owner()).toEqual(newOwner); - }); - }); - - it('should enforce permissions after transfer (pk)', () => { - ownable._unsafeUncheckedTransferOwnership(Z_NEW_OWNER); - - // Original owner - expect(() => { - ownable.as(OWNER).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); - - expect(() => { - ownable.as(UNAUTHORIZED).assertOnlyOwner(); - }).toThrow('Ownable: caller is not the owner'); - - // New owner - expect(() => { - ownable.as(NEW_OWNER).assertOnlyOwner(); - }).not.toThrow(); - }); - - it('should transfer multiple times', () => { - ownable.as(OWNER)._unsafeUncheckedTransferOwnership(Z_NEW_OWNER); - - ownable.as(NEW_OWNER)._unsafeUncheckedTransferOwnership(Z_OWNER); - - ownable.as(OWNER)._unsafeUncheckedTransferOwnership(Z_OWNER_CONTRACT); - - expect(ownable.owner()).toEqual(Z_OWNER_CONTRACT); - }); - }); }); }); diff --git a/contracts/src/access/test/mocks/MockOwnable.compact b/contracts/src/access/test/mocks/MockOwnable.compact index ebbc6110..827fe06b 100644 --- a/contracts/src/access/test/mocks/MockOwnable.compact +++ b/contracts/src/access/test/mocks/MockOwnable.compact @@ -29,10 +29,6 @@ export circuit transferOwnership(newOwner: Either): [] { - return Ownable__unsafeTransferOwnership(newOwner); -} - export circuit renounceOwnership(): [] { return Ownable_renounceOwnership(); } @@ -45,6 +41,3 @@ export circuit _transferOwnership(newOwner: Either): [] { - return Ownable__unsafeUncheckedTransferOwnership(newOwner); -} diff --git a/contracts/src/access/test/simulators/OwnableSimulator.ts b/contracts/src/access/test/simulators/OwnableSimulator.ts index a7bfb447..c4074573 100644 --- a/contracts/src/access/test/simulators/OwnableSimulator.ts +++ b/contracts/src/access/test/simulators/OwnableSimulator.ts @@ -69,16 +69,6 @@ export class OwnableSimulator extends OwnableSimulatorBase { this.circuits.impure.transferOwnership(newOwner); } - /** - * @description Unsafe variant of `transferOwnership`. - * @param newOwner - The new owner. - */ - public _unsafeTransferOwnership( - newOwner: Either, - ) { - this.circuits.impure._unsafeTransferOwnership(newOwner); - } - /** * @description Leaves the contract without an owner. * It will not be possible to call `assertOnlyOnwer` circuits anymore. @@ -107,13 +97,4 @@ export class OwnableSimulator extends OwnableSimulatorBase { this.circuits.impure._transferOwnership(newOwner); } - /** - * @description Unsafe variant of `_transferOwnership`. - * @param newOwner - The new owner. - */ - public _unsafeUncheckedTransferOwnership( - newOwner: Either, - ) { - this.circuits.impure._unsafeUncheckedTransferOwnership(newOwner); - } }