From 2d5b4588f0d28c02c70ff15f94abf7fc38b4aa90 Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Tue, 19 Nov 2024 14:46:21 +0100 Subject: [PATCH 1/5] feat: bump versions (#1222) --- Scarb.toml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Scarb.toml b/Scarb.toml index fc3e07e4f..3480b9487 100644 --- a/Scarb.toml +++ b/Scarb.toml @@ -24,8 +24,8 @@ keywords.workspace = true [workspace.package] version = "0.19.0" edition = "2024_07" -cairo-version = "2.8.4" -scarb-version = "2.8.4" +cairo-version = "2.8.5" +scarb-version = "2.8.5" authors = ["OpenZeppelin Community "] description = "OpenZeppelin Contracts written in Cairo for Starknet, a decentralized ZK Rollup" documentation = "https://docs.openzeppelin.com/contracts-cairo" @@ -40,8 +40,8 @@ keywords = [ ] [workspace.dependencies] -assert_macros = "2.8.4" -starknet = "2.8.4" +assert_macros = "2.8.5" +starknet = "2.8.5" snforge_std = "0.33.0" [dependencies] From c77b4b1f7f48efbe11149ba3b47545396204bd03 Mon Sep 17 00:00:00 2001 From: Andrew Fleming Date: Tue, 19 Nov 2024 09:53:51 -0600 Subject: [PATCH 2/5] Minor improvements to Utils package (#1206) * change to for loop * minor doc fix * add further in-code docs for snip12 * remove mut from compute_hash param * fix comment * add changelog entry * fix comment * add domain separator comment * add tests for average * add u256 average test * tidy up test * Apply suggestions from code review Co-authored-by: Eric Nordelo --------- Co-authored-by: Eric Nordelo --- CHANGELOG.md | 4 ++ docs/modules/ROOT/pages/api/utilities.adoc | 2 +- packages/utils/src/cryptography/snip12.cairo | 12 ++++ packages/utils/src/deployments.cairo | 17 ++--- packages/utils/src/structs/checkpoint.cairo | 4 +- packages/utils/src/tests.cairo | 1 + packages/utils/src/tests/test_math.cairo | 75 ++++++++++++++++++++ 7 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 packages/utils/src/tests/test_math.cairo diff --git a/CHANGELOG.md b/CHANGELOG.md index 184176278..8f3b159a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - SRC9 (Outside Execution) integration to account presets (#1201) +### Changed + +- Remove `mut` from `data` param in `compute_hash_on_elements` (#1206) + ### Changed (Breaking) - Add `verifying_contract` member to the `Delegation` struct used in Votes `delegate_by_sig` (#1214) diff --git a/docs/modules/ROOT/pages/api/utilities.adoc b/docs/modules/ROOT/pages/api/utilities.adoc index 9e0802233..c91e1262b 100644 --- a/docs/modules/ROOT/pages/api/utilities.adoc +++ b/docs/modules/ROOT/pages/api/utilities.adoc @@ -136,7 +136,7 @@ Returns the contract address when passing the given arguments to {deploy_syscall [.contract-item] [[deployments-compute_hash_on_elements]] -==== `[.contract-item-name]#++compute_hash_on_elements++#++(mut data: Span) → felt252++` [.item-kind]#function# +==== `[.contract-item-name]#++compute_hash_on_elements++#++(data: Span) → felt252++` [.item-kind]#function# Creates a Pedersen hash chain with the elements of `data` and returns the finalized hash. diff --git a/packages/utils/src/cryptography/snip12.cairo b/packages/utils/src/cryptography/snip12.cairo index 33a267250..f2f1df794 100644 --- a/packages/utils/src/cryptography/snip12.cairo +++ b/packages/utils/src/cryptography/snip12.cairo @@ -16,6 +16,7 @@ use starknet::{ContractAddress, get_tx_info}; pub const STARKNET_DOMAIN_TYPE_HASH: felt252 = 0x1ff2f602e42168014d405a94f75e8a93d640751d71d16311266e140d8b0a210; +/// Generic Starknet domain separator representation as defined in SNIP-12. #[derive(Drop, Copy, Hash)] pub struct StarknetDomain { pub name: felt252, @@ -24,14 +25,17 @@ pub struct StarknetDomain { pub revision: felt252, } +/// Trait for calculating the hash of a struct. pub trait StructHash { fn hash_struct(self: @T) -> felt252; } +/// Trait for calculating the hash of a message given the passed `signer`. pub trait OffchainMessageHash { fn get_message_hash(self: @T, signer: ContractAddress) -> felt252; } +/// Implementation of `StructHash` that calculates the Poseidon hash of type `StarknetDomain`. pub impl StructHashStarknetDomainImpl of StructHash { fn hash_struct(self: @StarknetDomain) -> felt252 { let hash_state = PoseidonTrait::new(); @@ -47,6 +51,14 @@ pub trait SNIP12Metadata { fn version() -> felt252; } +/// Implementation of OffchainMessageHash that calculates the Poseidon hash of the message. +/// +/// The hash state hashes the following in order: +/// +/// - 'StarkNet Message' short string. +/// - Starknet domain struct hash. +/// - `signer` of the message. +/// - Hashed struct of the message. pub(crate) impl OffchainMessageHashImpl< T, +StructHash, impl metadata: SNIP12Metadata > of OffchainMessageHash { diff --git a/packages/utils/src/deployments.cairo b/packages/utils/src/deployments.cairo index d0efe1c82..26b7ac1cc 100644 --- a/packages/utils/src/deployments.cairo +++ b/packages/utils/src/deployments.cairo @@ -46,20 +46,13 @@ pub fn calculate_contract_address_from_deploy_syscall( } /// Creates a Pedersen hash chain with the elements of `data` and returns the finalized hash. -fn compute_hash_on_elements(mut data: Span) -> felt252 { - let data_len = data.len(); +fn compute_hash_on_elements(data: Span) -> felt252 { let mut state = PedersenTrait::new(0); - let mut hash = 0; - loop { - match data.pop_front() { - Option::Some(elem) => { state = state.update_with(*elem); }, - Option::None => { - hash = state.update_with(data_len).finalize(); - break; - }, - }; + for elem in data { + state = state.update_with(*elem); }; - hash + + state.update_with(data.len()).finalize() } #[derive(Drop)] diff --git a/packages/utils/src/structs/checkpoint.cairo b/packages/utils/src/structs/checkpoint.cairo index 8e3fe2077..c677f805f 100644 --- a/packages/utils/src/structs/checkpoint.cairo +++ b/packages/utils/src/structs/checkpoint.cairo @@ -99,12 +99,12 @@ pub impl TraceImpl of TraceTrait { } } - /// Returns the number of checkpoints. + /// Returns the total number of checkpoints. fn length(self: StoragePath) -> u64 { self.checkpoints.len() } - /// Returns the checkpoint at given position. + /// Returns the checkpoint at the given position. fn at(self: StoragePath, pos: u64) -> Checkpoint { assert(pos < self.length(), 'Vec overflow'); self.checkpoints[pos].read() diff --git a/packages/utils/src/tests.cairo b/packages/utils/src/tests.cairo index 74bd036b1..597d6ddef 100644 --- a/packages/utils/src/tests.cairo +++ b/packages/utils/src/tests.cairo @@ -1,3 +1,4 @@ mod test_checkpoint; +mod test_math; mod test_nonces; mod test_snip12; diff --git a/packages/utils/src/tests/test_math.cairo b/packages/utils/src/tests/test_math.cairo new file mode 100644 index 000000000..b098c9447 --- /dev/null +++ b/packages/utils/src/tests/test_math.cairo @@ -0,0 +1,75 @@ +use core::integer::{u512, u512_safe_div_rem_by_u256}; +use core::num::traits::OverflowingAdd; +use crate::math::average; + +#[test] +fn test_average_u8(a: u8, b: u8) { + let actual = average(a, b); + + let a: u256 = a.into(); + let b: u256 = b.into(); + let expected = (a + b) / 2; + + assert_eq!(actual, expected.try_into().unwrap()); +} + +#[test] +fn test_average_u16(a: u16, b: u16) { + let actual = average(a, b); + + let a: u256 = a.into(); + let b: u256 = b.into(); + let expected = (a + b) / 2; + + assert_eq!(actual, expected.try_into().unwrap()); +} + +#[test] +fn test_average_u32(a: u32, b: u32) { + let actual = average(a, b); + + let a: u256 = a.into(); + let b: u256 = b.into(); + let expected = (a + b) / 2; + + assert_eq!(actual, expected.try_into().unwrap()); +} + +#[test] +fn test_average_u64(a: u64, b: u64) { + let actual = average(a, b); + + let a: u256 = a.into(); + let b: u256 = b.into(); + let expected = (a + b) / 2; + + assert_eq!(actual, expected.try_into().unwrap()); +} + +#[test] +fn test_average_u128(a: u128, b: u128) { + let actual = average(a, b); + + let a: u256 = a.into(); + let b: u256 = b.into(); + let expected = (a + b) / 2; + + assert_eq!(actual, expected.try_into().unwrap()); +} + +#[test] +fn test_average_u256(a: u256, b: u256) { + let actual = average(a, b); + let mut expected = 0; + + let (sum, overflow) = a.overflowing_add(b); + if !overflow { + expected = sum / 2; + } else { + let u512_sum = u512 { limb0: sum.low, limb1: sum.high, limb2: 1, limb3: 0 }; + let (res, _) = u512_safe_div_rem_by_u256(u512_sum, 2); + expected = res.try_into().unwrap(); + }; + + assert_eq!(actual, expected); +} From 6962bbe9f9a6e72c81384e70b1f2db66aa84658d Mon Sep 17 00:00:00 2001 From: immrsd <103599616+immrsd@users.noreply.github.com> Date: Tue, 19 Nov 2024 19:00:38 +0100 Subject: [PATCH 3/5] Apply minor improvements to Upgrades package (#1225) --- packages/upgrades/src/tests/test_upgradeable.cairo | 8 +++----- packages/upgrades/src/upgradeable.cairo | 14 ++++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/upgrades/src/tests/test_upgradeable.cairo b/packages/upgrades/src/tests/test_upgradeable.cairo index f34967437..fa611cf24 100644 --- a/packages/upgrades/src/tests/test_upgradeable.cairo +++ b/packages/upgrades/src/tests/test_upgradeable.cairo @@ -1,6 +1,5 @@ -use openzeppelin_test_common::mocks::upgrades::{ - IUpgradesV1SafeDispatcher, IUpgradesV1SafeDispatcherTrait -}; +use openzeppelin_test_common::mocks::upgrades::IUpgradesV1SafeDispatcher; +use openzeppelin_test_common::mocks::upgrades::IUpgradesV1SafeDispatcherTrait; use openzeppelin_test_common::mocks::upgrades::{IUpgradesV1Dispatcher, IUpgradesV1DispatcherTrait}; use openzeppelin_test_common::mocks::upgrades::{IUpgradesV2Dispatcher, IUpgradesV2DispatcherTrait}; use openzeppelin_test_common::upgrades::UpgradeableSpyHelpers; @@ -26,7 +25,7 @@ fn setup_test() -> (IUpgradesV1Dispatcher, ContractClass) { // #[test] -#[should_panic(expected: ('Class hash cannot be zero',))] +#[should_panic(expected: 'Class hash cannot be zero')] fn test_upgrade_with_class_hash_zero() { let (v1, _) = setup_test(); v1.upgrade(CLASS_HASH_ZERO()); @@ -87,7 +86,6 @@ fn test_remove_selector_fails_in_v2() { ); } - // // upgrade_and_call // diff --git a/packages/upgrades/src/upgradeable.cairo b/packages/upgrades/src/upgradeable.cairo index 9d544349e..43810c83c 100644 --- a/packages/upgrades/src/upgradeable.cairo +++ b/packages/upgrades/src/upgradeable.cairo @@ -7,9 +7,7 @@ #[starknet::component] pub mod UpgradeableComponent { use core::num::traits::Zero; - use starknet::ClassHash; - use starknet::SyscallResultTrait; - use starknet::syscalls::{call_contract_syscall, replace_class_syscall}; + use starknet::{ClassHash, SyscallResultTrait}; #[storage] pub struct Storage {} @@ -30,6 +28,10 @@ pub mod UpgradeableComponent { pub const INVALID_CLASS: felt252 = 'Class hash cannot be zero'; } + // + // Internal + // + #[generate_trait] pub impl InternalImpl< TContractState, +HasComponent @@ -42,8 +44,8 @@ pub mod UpgradeableComponent { /// /// Emits an `Upgraded` event. fn upgrade(ref self: ComponentState, new_class_hash: ClassHash) { - assert(!new_class_hash.is_zero(), Errors::INVALID_CLASS); - replace_class_syscall(new_class_hash).unwrap_syscall(); + assert(new_class_hash.is_non_zero(), Errors::INVALID_CLASS); + starknet::syscalls::replace_class_syscall(new_class_hash).unwrap_syscall(); self.emit(Upgraded { class_hash: new_class_hash }); } @@ -68,7 +70,7 @@ pub mod UpgradeableComponent { // `call_contract_syscall` is used in order to call `selector` from the new class. // See: // https://docs.starknet.io/documentation/architecture_and_concepts/Contracts/system-calls-cairo1/#replace_class - call_contract_syscall(this, selector, calldata).unwrap_syscall() + starknet::syscalls::call_contract_syscall(this, selector, calldata).unwrap_syscall() } } } From f502aeb64d47c1d72b7aacd3094f3351182a9eb7 Mon Sep 17 00:00:00 2001 From: Gustavo Gonzalez Date: Wed, 20 Nov 2024 11:20:52 +0700 Subject: [PATCH 4/5] Clean up and small refactors to the access module (#1221) * refactor access module * improve comment for initializable * remove tests that check caller is not the zero address * Update packages/access/src/accesscontrol/accesscontrol.cairo Co-authored-by: Eric Nordelo * Update packages/access/src/ownable/ownable.cairo Co-authored-by: Eric Nordelo * check owner is not zero on initializer and improve comments * Update packages/access/src/accesscontrol/accesscontrol.cairo Co-authored-by: Eric Nordelo * Update packages/access/src/ownable/ownable.cairo Co-authored-by: Eric Nordelo * remove caller zero address tests from presets * update docs * update changelog * remove tuple syntax from test * Update docs/modules/ROOT/pages/api/access.adoc Co-authored-by: Andrew Fleming * update changelog --------- Co-authored-by: Eric Nordelo Co-authored-by: Andrew Fleming --- CHANGELOG.md | 1 + docs/modules/ROOT/pages/api/access.adoc | 7 ++- .../src/accesscontrol/accesscontrol.cairo | 29 +++++++--- packages/access/src/ownable/ownable.cairo | 33 ++++++----- .../access/src/tests/test_accesscontrol.cairo | 20 +++---- packages/access/src/tests/test_ownable.cairo | 56 +++++-------------- .../src/tests/test_ownable_twostep.cairo | 40 ++----------- packages/presets/src/tests/test_erc1155.cairo | 32 ----------- packages/presets/src/tests/test_erc20.cairo | 32 ----------- packages/presets/src/tests/test_erc721.cairo | 32 ----------- packages/presets/src/tests/test_vesting.cairo | 32 ----------- packages/security/src/initializable.cairo | 2 +- .../src/tests/test_initializable.cairo | 2 +- .../security/src/tests/test_pausable.cairo | 8 +-- .../src/tests/test_reentrancyguard.cairo | 8 +-- 15 files changed, 85 insertions(+), 249 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f3b159a1..b182243de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed (Breaking) +- The initializer in `OwnableComponent` now checks that `owner` is not the zero address (#1221) - Add `verifying_contract` member to the `Delegation` struct used in Votes `delegate_by_sig` (#1214) use crate::votes::VotesComponent::VotingUnitsTrait; - VotingUnitsTrait moved from `openzeppelin_governance::votes::votes` to `openzeppelin_governance::votes::VotesComponent` (#1214) diff --git a/docs/modules/ROOT/pages/api/access.adoc b/docs/modules/ROOT/pages/api/access.adoc index 2b47dd86d..d3547d22c 100644 --- a/docs/modules/ROOT/pages/api/access.adoc +++ b/docs/modules/ROOT/pages/api/access.adoc @@ -156,7 +156,8 @@ Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. [[OwnableComponent-two-step-transfer_ownership]] ==== `[.contract-item-name]#++transfer_ownership++#++(ref self: ContractState, new_owner: ContractAddress)++` [.item-kind]#external# -Starts the two step ownership transfer process, by setting the pending owner. +Starts the two step ownership transfer process, by setting the pending owner. Setting `new_owner` to the zero address is allowed, this can be used to cancel an initiated ownership transfer. + Can only be called by the current owner. Emits an xref:OwnableComponent-OwnershipTransferStarted[OwnershipTransferStarted] event. @@ -211,6 +212,10 @@ See xref:OwnableComponent-two-step-renounce_ownership[renounce_ownership]. Initializes the contract and sets `owner` as the initial owner. +Requirements: + +- `owner` cannot be the zero address. + Emits an xref:OwnableComponent-OwnershipTransferred[OwnershipTransferred] event. [.contract-item] diff --git a/packages/access/src/accesscontrol/accesscontrol.cairo b/packages/access/src/accesscontrol/accesscontrol.cairo index 239306afe..c9d0eb906 100644 --- a/packages/access/src/accesscontrol/accesscontrol.cairo +++ b/packages/access/src/accesscontrol/accesscontrol.cairo @@ -3,8 +3,19 @@ /// # AccessControl Component /// -/// The AccessControl component allows contracts to implement role-based access control mechanisms. -/// Roles are referred to by their `felt252` identifier. +/// The AccessControl component enables role-based access control mechanisms. This is a lightweight +/// implementation that doesn't support on-chain enumeration of role members, though role membership +/// can be tracked off-chain through contract events. +/// +/// Roles can be granted and revoked dynamically via `grant_role` and `revoke_role`. Each role +/// has an associated admin role that controls who can grant and revoke it. By default, all roles +/// use `DEFAULT_ADMIN_ROLE` as their admin role. +/// Accounts can also renounce roles they have been granted by using `renounce_role`. +/// +/// More complex role hierarchies can be created using `set_role_admin`. +/// +/// WARNING: The `DEFAULT_ADMIN_ROLE` is its own admin, meaning it can grant and revoke itself. +/// Extra precautions should be taken to secure accounts with this role. #[starknet::component] pub mod AccessControlComponent { use crate::accesscontrol::interface; @@ -31,8 +42,8 @@ pub mod AccessControlComponent { /// Emitted when `account` is granted `role`. /// - /// `sender` is the account that originated the contract call, an admin role - /// bearer (except if `_grant_role` is called during initialization from the constructor). + /// `sender` is the account that originated the contract call, an account with the admin role + /// or the deployer address if `grant_role` is called from the constructor. #[derive(Drop, PartialEq, starknet::Event)] pub struct RoleGranted { pub role: felt252, @@ -40,7 +51,7 @@ pub mod AccessControlComponent { pub sender: ContractAddress } - /// Emitted when `account` is revoked `role`. + /// Emitted when `role` is revoked for `account`. /// /// `sender` is the account that originated the contract call: /// - If using `revoke_role`, it is the admin role bearer. @@ -55,7 +66,7 @@ pub mod AccessControlComponent { /// Emitted when `new_admin_role` is set as `role`'s admin role, replacing `previous_admin_role` /// /// `DEFAULT_ADMIN_ROLE` is the starting admin for all roles, despite - /// {RoleAdminChanged} not being emitted signaling this. + /// `RoleAdminChanged` not being emitted signaling this. #[derive(Drop, PartialEq, starknet::Event)] pub struct RoleAdminChanged { pub role: felt252, @@ -89,7 +100,7 @@ pub mod AccessControlComponent { /// Grants `role` to `account`. /// - /// If `account` had not been already granted `role`, emits a `RoleGranted` event. + /// If `account` has not been already granted `role`, emits a `RoleGranted` event. /// /// Requirements: /// @@ -104,7 +115,7 @@ pub mod AccessControlComponent { /// Revokes `role` from `account`. /// - /// If `account` had been granted `role`, emits a `RoleRevoked` event. + /// If `account` has been granted `role`, emits a `RoleRevoked` event. /// /// Requirements: /// @@ -132,7 +143,7 @@ pub mod AccessControlComponent { fn renounce_role( ref self: ComponentState, role: felt252, account: ContractAddress ) { - let caller: ContractAddress = get_caller_address(); + let caller = get_caller_address(); assert(caller == account, Errors::INVALID_CALLER); self._revoke_role(role, account); } diff --git a/packages/access/src/ownable/ownable.cairo b/packages/access/src/ownable/ownable.cairo index b001e8dbd..321114793 100644 --- a/packages/access/src/ownable/ownable.cairo +++ b/packages/access/src/ownable/ownable.cairo @@ -35,6 +35,8 @@ pub mod OwnableComponent { OwnershipTransferStarted: OwnershipTransferStarted } + /// Emitted when `new_owner` is set as owner of the contract. + /// `new_owner` can be set to zero only if the ownership is renounced. #[derive(Drop, PartialEq, starknet::Event)] pub struct OwnershipTransferred { #[key] @@ -43,6 +45,9 @@ pub mod OwnableComponent { pub new_owner: ContractAddress, } + /// Emitted when `transfer_ownership` is called on a contract that implements `IOwnableTwoStep`. + /// `previous_owner` is the address of the current owner. + /// `new_owner` is the address of the pending owner. #[derive(Drop, PartialEq, starknet::Event)] pub struct OwnershipTransferStarted { #[key] @@ -54,7 +59,6 @@ pub mod OwnableComponent { pub mod Errors { pub const NOT_OWNER: felt252 = 'Caller is not the owner'; pub const NOT_PENDING_OWNER: felt252 = 'Caller is not the pending owner'; - pub const ZERO_ADDRESS_CALLER: felt252 = 'Caller is the zero address'; pub const ZERO_ADDRESS_OWNER: felt252 = 'New owner is the zero address'; } @@ -83,8 +87,8 @@ pub mod OwnableComponent { self._transfer_ownership(new_owner); } - /// Leaves the contract without owner. It will not be possible to call `assert_only_owner` - /// functions anymore. Can only be called by the current owner. + /// Leaves the contract without an owner. It will not be possible to call + /// `assert_only_owner` functions anymore. Can only be called by the current owner. /// /// Requirements: /// @@ -129,6 +133,8 @@ pub mod OwnableComponent { /// Starts the two-step ownership transfer process by setting the pending owner. /// + /// Setting `new_owner` to the zero address is allowed; this can be used to cancel an + /// initiated ownership transfer. /// Requirements: /// /// - The caller is the contract owner. @@ -150,7 +156,8 @@ pub mod OwnableComponent { /// /// Emits an `OwnershipTransferred` event. fn renounce_ownership(ref self: ComponentState) { - Ownable::renounce_ownership(ref self); + self.assert_only_owner(); + self._transfer_ownership(Zero::zero()); } } @@ -270,8 +277,13 @@ pub mod OwnableComponent { > of InternalTrait { /// Sets the contract's initial owner. /// + /// Requirements: + /// + /// - `owner` is not the zero address. + /// /// This function should be called at construction time. fn initializer(ref self: ComponentState, owner: ContractAddress) { + assert(!owner.is_zero(), Errors::ZERO_ADDRESS_OWNER); self._transfer_ownership(owner); } @@ -280,7 +292,6 @@ pub mod OwnableComponent { fn assert_only_owner(self: @ComponentState) { let owner = self.Ownable_owner.read(); let caller = get_caller_address(); - assert(!caller.is_zero(), Errors::ZERO_ADDRESS_CALLER); assert(caller == owner, Errors::NOT_OWNER); } @@ -297,10 +308,7 @@ pub mod OwnableComponent { let previous_owner: ContractAddress = self.Ownable_owner.read(); self.Ownable_owner.write(new_owner); - self - .emit( - OwnershipTransferred { previous_owner: previous_owner, new_owner: new_owner } - ); + self.emit(OwnershipTransferred { previous_owner, new_owner }); } /// Sets a new pending owner. @@ -311,12 +319,7 @@ pub mod OwnableComponent { fn _propose_owner(ref self: ComponentState, new_owner: ContractAddress) { let previous_owner = self.Ownable_owner.read(); self.Ownable_pending_owner.write(new_owner); - self - .emit( - OwnershipTransferStarted { - previous_owner: previous_owner, new_owner: new_owner - } - ); + self.emit(OwnershipTransferStarted { previous_owner, new_owner }); } } } diff --git a/packages/access/src/tests/test_accesscontrol.cairo b/packages/access/src/tests/test_accesscontrol.cairo index 81adcddfb..3b09a01f7 100644 --- a/packages/access/src/tests/test_accesscontrol.cairo +++ b/packages/access/src/tests/test_accesscontrol.cairo @@ -81,7 +81,7 @@ fn test_assert_only_role() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_assert_only_role_unauthorized() { let state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -89,7 +89,7 @@ fn test_assert_only_role_unauthorized() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_assert_only_role_unauthorized_when_authorized_for_another_role() { let mut state = setup(); state.grant_role(ROLE, AUTHORIZED()); @@ -151,7 +151,7 @@ fn test_grantRole_multiple_times_for_granted_role() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_grant_role_unauthorized() { let mut state = setup(); start_cheat_caller_address(test_address(), AUTHORIZED()); @@ -159,7 +159,7 @@ fn test_grant_role_unauthorized() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_grantRole_unauthorized() { let mut state = setup(); start_cheat_caller_address(test_address(), AUTHORIZED()); @@ -245,7 +245,7 @@ fn test_revokeRole_multiple_times_for_granted_role() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_revoke_role_unauthorized() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -253,7 +253,7 @@ fn test_revoke_role_unauthorized() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_revokeRole_unauthorized() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -345,7 +345,7 @@ fn test_renounceRole_multiple_times_for_granted_role() { } #[test] -#[should_panic(expected: ('Can only renounce role for self',))] +#[should_panic(expected: 'Can only renounce role for self')] fn test_renounce_role_unauthorized() { let mut state = setup(); let contract_address = test_address(); @@ -357,7 +357,7 @@ fn test_renounce_role_unauthorized() { } #[test] -#[should_panic(expected: ('Can only renounce role for self',))] +#[should_panic(expected: 'Can only renounce role for self')] fn test_renounceRole_unauthorized() { let mut state = setup(); start_cheat_caller_address(test_address(), ADMIN()); @@ -423,7 +423,7 @@ fn test_new_admin_can_revoke_roles() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_previous_admin_cannot_grant_roles() { let mut state = setup(); state.set_role_admin(ROLE, OTHER_ROLE); @@ -432,7 +432,7 @@ fn test_previous_admin_cannot_grant_roles() { } #[test] -#[should_panic(expected: ('Caller is missing role',))] +#[should_panic(expected: 'Caller is missing role')] fn test_previous_admin_cannot_revoke_roles() { let mut state = setup(); state.set_role_admin(ROLE, OTHER_ROLE); diff --git a/packages/access/src/tests/test_ownable.cairo b/packages/access/src/tests/test_ownable.cairo index 46c9d56da..329823c81 100644 --- a/packages/access/src/tests/test_ownable.cairo +++ b/packages/access/src/tests/test_ownable.cairo @@ -44,6 +44,13 @@ fn test_initializer_owner() { assert_eq!(new_owner, OWNER()); } +#[test] +#[should_panic(expected: 'New owner is the zero address')] +fn test_initializer_zero_owner() { + let mut state = COMPONENT_STATE(); + state.initializer(ZERO()); +} + // // assert_only_owner // @@ -56,20 +63,13 @@ fn test_assert_only_owner() { } #[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_assert_only_owner_when_not_owner() { let state = setup(); start_cheat_caller_address(test_address(), OTHER()); state.assert_only_owner(); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_assert_only_owner_when_caller_zero() { - let state = setup(); - state.assert_only_owner(); -} - // // _transfer_ownership // @@ -117,7 +117,7 @@ fn test_transfer_ownership() { } #[test] -#[should_panic(expected: ('New owner is the zero address',))] +#[should_panic(expected: 'New owner is the zero address')] fn test_transfer_ownership_to_zero() { let mut state = setup(); start_cheat_caller_address(test_address(), OWNER()); @@ -125,14 +125,7 @@ fn test_transfer_ownership_to_zero() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transfer_ownership_from_zero() { - let mut state = setup(); - state.transfer_ownership(OTHER()); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_transfer_ownership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -152,7 +145,7 @@ fn test_transferOwnership() { } #[test] -#[should_panic(expected: ('New owner is the zero address',))] +#[should_panic(expected: 'New owner is the zero address')] fn test_transferOwnership_to_zero() { let mut state = setup(); start_cheat_caller_address(test_address(), OWNER()); @@ -160,14 +153,7 @@ fn test_transferOwnership_to_zero() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transferOwnership_from_zero() { - let mut state = setup(); - state.transferOwnership(OTHER()); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_transferOwnership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -191,14 +177,7 @@ fn test_renounce_ownership() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounce_ownership_from_zero_address() { - let mut state = setup(); - state.renounce_ownership(); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_renounce_ownership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -218,14 +197,7 @@ fn test_renounceOwnership() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounceOwnership_from_zero_address() { - let mut state = setup(); - state.renounceOwnership(); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_renounceOwnership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); diff --git a/packages/access/src/tests/test_ownable_twostep.cairo b/packages/access/src/tests/test_ownable_twostep.cairo index ec6d6242b..b8ef090cd 100644 --- a/packages/access/src/tests/test_ownable_twostep.cairo +++ b/packages/access/src/tests/test_ownable_twostep.cairo @@ -97,14 +97,7 @@ fn test_transfer_ownership_to_zero() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transfer_ownership_from_zero() { - let mut state = setup(); - state.transfer_ownership(OTHER()); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_transfer_ownership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -145,14 +138,7 @@ fn test_transferOwnership_to_zero() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transferOwnership_from_zero() { - let mut state = setup(); - state.transferOwnership(OTHER()); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_transferOwnership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -179,7 +165,7 @@ fn test_accept_ownership() { } #[test] -#[should_panic(expected: ('Caller is not the pending owner',))] +#[should_panic(expected: 'Caller is not the pending owner')] fn test_accept_ownership_from_nonpending() { let mut state = setup(); state.Ownable_pending_owner.write(NEW_OWNER()); @@ -203,7 +189,7 @@ fn test_acceptOwnership() { } #[test] -#[should_panic(expected: ('Caller is not the pending owner',))] +#[should_panic(expected: 'Caller is not the pending owner')] fn test_acceptOwnership_from_nonpending() { let mut state = setup(); state.Ownable_pending_owner.write(NEW_OWNER()); @@ -245,14 +231,7 @@ fn test_renounce_ownership_resets_pending_owner() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounce_ownership_from_zero_address() { - let mut state = setup(); - state.renounce_ownership(); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_renounce_ownership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); @@ -273,14 +252,7 @@ fn test_renounceOwnership() { } #[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounceOwnership_from_zero_address() { - let mut state = setup(); - state.renounceOwnership(); -} - -#[test] -#[should_panic(expected: ('Caller is not the owner',))] +#[should_panic(expected: 'Caller is not the owner')] fn test_renounceOwnership_from_nonowner() { let mut state = setup(); start_cheat_caller_address(test_address(), OTHER()); diff --git a/packages/presets/src/tests/test_erc1155.cairo b/packages/presets/src/tests/test_erc1155.cairo index 7cfb30f97..0e83b0640 100644 --- a/packages/presets/src/tests/test_erc1155.cairo +++ b/packages/presets/src/tests/test_erc1155.cairo @@ -656,14 +656,6 @@ fn test_transfer_ownership_to_zero() { dispatcher.transfer_ownership(ZERO()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transfer_ownership_from_zero() { - let (_, dispatcher, _) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.transfer_ownership(OTHER()); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_transfer_ownership_from_nonowner() { @@ -690,14 +682,6 @@ fn test_transferOwnership_to_zero() { dispatcher.transferOwnership(ZERO()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transferOwnership_from_zero() { - let (_, dispatcher, _) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.transferOwnership(OTHER()); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_transferOwnership_from_nonowner() { @@ -720,14 +704,6 @@ fn test_renounce_ownership() { assert!(dispatcher.owner().is_zero()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounce_ownership_from_zero_address() { - let (_, dispatcher, _) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.renounce_ownership(); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_renounce_ownership_from_nonowner() { @@ -746,14 +722,6 @@ fn test_renounceOwnership() { assert!(dispatcher.owner().is_zero()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounceOwnership_from_zero_address() { - let (_, dispatcher, _) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.renounceOwnership(); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_renounceOwnership_from_nonowner() { diff --git a/packages/presets/src/tests/test_erc20.cairo b/packages/presets/src/tests/test_erc20.cairo index cd41e83b1..be41473d9 100644 --- a/packages/presets/src/tests/test_erc20.cairo +++ b/packages/presets/src/tests/test_erc20.cairo @@ -328,14 +328,6 @@ fn test_transfer_ownership_to_zero() { dispatcher.transfer_ownership(ZERO()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transfer_ownership_from_zero() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.transfer_ownership(OTHER()); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_transfer_ownership_from_nonowner() { @@ -362,14 +354,6 @@ fn test_transferOwnership_to_zero() { dispatcher.transferOwnership(ZERO()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transferOwnership_from_zero() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.transferOwnership(OTHER()); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_transferOwnership_from_nonowner() { @@ -392,14 +376,6 @@ fn test_renounce_ownership() { assert!(dispatcher.owner().is_zero()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounce_ownership_from_zero_address() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.renounce_ownership(); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_renounce_ownership_from_nonowner() { @@ -418,14 +394,6 @@ fn test_renounceOwnership() { assert!(dispatcher.owner().is_zero()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounceOwnership_from_zero_address() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.renounceOwnership(); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_renounceOwnership_from_nonowner() { diff --git a/packages/presets/src/tests/test_erc721.cairo b/packages/presets/src/tests/test_erc721.cairo index 63a1844f9..c6b02aa65 100644 --- a/packages/presets/src/tests/test_erc721.cairo +++ b/packages/presets/src/tests/test_erc721.cairo @@ -756,14 +756,6 @@ fn test_transfer_ownership_to_zero() { dispatcher.transfer_ownership(ZERO()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transfer_ownership_from_zero() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.transfer_ownership(OTHER()); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_transfer_ownership_from_nonowner() { @@ -790,14 +782,6 @@ fn test_transferOwnership_to_zero() { dispatcher.transferOwnership(ZERO()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_transferOwnership_from_zero() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.transferOwnership(OTHER()); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_transferOwnership_from_nonowner() { @@ -820,14 +804,6 @@ fn test_renounce_ownership() { assert!(dispatcher.owner().is_zero()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounce_ownership_from_zero_address() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.renounce_ownership(); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_renounce_ownership_from_nonowner() { @@ -846,14 +822,6 @@ fn test_renounceOwnership() { assert!(dispatcher.owner().is_zero()); } -#[test] -#[should_panic(expected: ('Caller is the zero address',))] -fn test_renounceOwnership_from_zero_address() { - let (_, mut dispatcher) = setup_dispatcher(); - start_cheat_caller_address(dispatcher.contract_address, ZERO()); - dispatcher.renounceOwnership(); -} - #[test] #[should_panic(expected: ('Caller is not the owner',))] fn test_renounceOwnership_from_nonowner() { diff --git a/packages/presets/src/tests/test_vesting.cairo b/packages/presets/src/tests/test_vesting.cairo index 0cbc9763a..ccaa0758f 100644 --- a/packages/presets/src/tests/test_vesting.cairo +++ b/packages/presets/src/tests/test_vesting.cairo @@ -246,14 +246,6 @@ fn test_transfer_ownership_to_zero() { vesting.transfer_ownership(ZERO()); } -#[test] -#[should_panic(expected: 'Caller is the zero address')] -fn test_transfer_ownership_from_zero() { - let (vesting, _) = setup(TEST_DATA()); - start_cheat_caller_address(vesting.contract_address, ZERO()); - vesting.transfer_ownership(OTHER()); -} - #[test] #[should_panic(expected: 'Caller is not the owner')] fn test_transfer_ownership_from_nonowner() { @@ -281,14 +273,6 @@ fn test_transferOwnership_to_zero() { vesting.transferOwnership(ZERO()); } -#[test] -#[should_panic(expected: 'Caller is the zero address')] -fn test_transferOwnership_from_zero() { - let (vesting, _) = setup(TEST_DATA()); - start_cheat_caller_address(vesting.contract_address, ZERO()); - vesting.transferOwnership(OTHER()); -} - #[test] #[should_panic(expected: 'Caller is not the owner')] fn test_transferOwnership_from_nonowner() { @@ -312,14 +296,6 @@ fn test_renounce_ownership() { assert!(vesting.owner().is_zero()); } -#[test] -#[should_panic(expected: 'Caller is the zero address')] -fn test_renounce_ownership_from_zero_address() { - let (vesting, _) = setup(TEST_DATA()); - start_cheat_caller_address(vesting.contract_address, ZERO()); - vesting.renounce_ownership(); -} - #[test] #[should_panic(expected: 'Caller is not the owner')] fn test_renounce_ownership_from_nonowner() { @@ -339,14 +315,6 @@ fn test_renounceOwnership() { assert!(vesting.owner().is_zero()); } -#[test] -#[should_panic(expected: 'Caller is the zero address')] -fn test_renounceOwnership_from_zero_address() { - let (vesting, _) = setup(TEST_DATA()); - start_cheat_caller_address(vesting.contract_address, ZERO()); - vesting.renounceOwnership(); -} - #[test] #[should_panic(expected: 'Caller is not the owner')] fn test_renounceOwnership_from_nonowner() { diff --git a/packages/security/src/initializable.cairo b/packages/security/src/initializable.cairo index 9b09e91d8..4c7b348ba 100644 --- a/packages/security/src/initializable.cairo +++ b/packages/security/src/initializable.cairo @@ -24,7 +24,7 @@ pub mod InitializableComponent { impl Initializable< TContractState, +HasComponent > of IInitializable> { - /// Returns true if the using contract executed `initialize`. + /// Returns whether the contract has been initialized. fn is_initialized(self: @ComponentState) -> bool { self.Initializable_initialized.read() } diff --git a/packages/security/src/tests/test_initializable.cairo b/packages/security/src/tests/test_initializable.cairo index 0b7ac8fc7..6fbfada0a 100644 --- a/packages/security/src/tests/test_initializable.cairo +++ b/packages/security/src/tests/test_initializable.cairo @@ -17,7 +17,7 @@ fn test_initialize() { } #[test] -#[should_panic(expected: ('Initializable: is initialized',))] +#[should_panic(expected: 'Initializable: is initialized')] fn test_initialize_when_initialized() { let mut state = COMPONENT_STATE(); state.initialize(); diff --git a/packages/security/src/tests/test_pausable.cairo b/packages/security/src/tests/test_pausable.cairo index 486819c0e..6ccbc921a 100644 --- a/packages/security/src/tests/test_pausable.cairo +++ b/packages/security/src/tests/test_pausable.cairo @@ -42,7 +42,7 @@ fn test_assert_paused_when_paused() { } #[test] -#[should_panic(expected: ('Pausable: not paused',))] +#[should_panic(expected: 'Pausable: not paused')] fn test_assert_paused_when_not_paused() { let state = COMPONENT_STATE(); state.assert_paused(); @@ -53,7 +53,7 @@ fn test_assert_paused_when_not_paused() { // #[test] -#[should_panic(expected: ('Pausable: paused',))] +#[should_panic(expected: 'Pausable: paused')] fn test_assert_not_paused_when_paused() { let mut state = COMPONENT_STATE(); state.pause(); @@ -84,7 +84,7 @@ fn test_pause_when_unpaused() { } #[test] -#[should_panic(expected: ('Pausable: paused',))] +#[should_panic(expected: 'Pausable: paused')] fn test_pause_when_paused() { let mut state = COMPONENT_STATE(); state.pause(); @@ -111,7 +111,7 @@ fn test_unpause_when_paused() { } #[test] -#[should_panic(expected: ('Pausable: not paused',))] +#[should_panic(expected: 'Pausable: not paused')] fn test_unpause_when_unpaused() { let mut state = COMPONENT_STATE(); assert!(!state.is_paused()); diff --git a/packages/security/src/tests/test_reentrancyguard.cairo b/packages/security/src/tests/test_reentrancyguard.cairo index 8647c9e9e..3263e77a4 100644 --- a/packages/security/src/tests/test_reentrancyguard.cairo +++ b/packages/security/src/tests/test_reentrancyguard.cairo @@ -36,7 +36,7 @@ fn test_reentrancy_guard_start() { } #[test] -#[should_panic(expected: ('ReentrancyGuard: reentrant call',))] +#[should_panic(expected: 'ReentrancyGuard: reentrant call')] fn test_reentrancy_guard_start_when_started() { let mut state = COMPONENT_STATE(); @@ -64,7 +64,7 @@ fn test_reentrancy_guard_end() { // #[test] -#[should_panic(expected: ('ReentrancyGuard: reentrant call',))] +#[should_panic(expected: 'ReentrancyGuard: reentrant call')] fn test_remote_callback() { let contract = deploy_mock(); @@ -76,14 +76,14 @@ fn test_remote_callback() { } #[test] -#[should_panic(expected: ('ReentrancyGuard: reentrant call',))] +#[should_panic(expected: 'ReentrancyGuard: reentrant call')] fn test_local_recursion() { let contract = deploy_mock(); contract.count_local_recursive(10); } #[test] -#[should_panic(expected: ('ReentrancyGuard: reentrant call',))] +#[should_panic(expected: 'ReentrancyGuard: reentrant call')] fn test_external_recursion() { let contract = deploy_mock(); contract.count_external_recursive(10); From 45b9a720f026a853657fd6f2236ee2c454d46cc7 Mon Sep 17 00:00:00 2001 From: immrsd <103599616+immrsd@users.noreply.github.com> Date: Wed, 20 Nov 2024 12:18:28 +0100 Subject: [PATCH 5/5] Minor improvements to Account package (#1224) * Apply minor improvements to Account package * Improve is_tx_version_valid description * Apply minor improvements to tests * Update changelog --- CHANGELOG.md | 3 + docs/modules/ROOT/pages/accounts.adoc | 6 +- docs/modules/ROOT/pages/api/account.adoc | 6 +- .../src/accesscontrol/accesscontrol.cairo | 2 +- packages/account/src/account.cairo | 38 ++++------- packages/account/src/eth_account.cairo | 40 ++++------- .../account/src/extensions/src9/src9.cairo | 2 +- .../src/tests/extensions/test_src9.cairo | 20 +++--- packages/account/src/tests/test_account.cairo | 66 ++++++++----------- .../account/src/tests/test_eth_account.cairo | 52 ++++++--------- .../account/src/tests/test_signature.cairo | 2 +- packages/account/src/utils.cairo | 25 ++++--- 12 files changed, 112 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b182243de..7c8efd650 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,10 +12,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - SRC9 (Outside Execution) integration to account presets (#1201) +- `is_tx_version_valid` utility function to `openzeppelin_account::utils` (#1224) ### Changed - Remove `mut` from `data` param in `compute_hash_on_elements` (#1206) +- Remove `mut` from `calls` param in `__execute__` function of Account and EthAccount components (#1224) +- Remove `mut` from `calls` param in `__validate__` function of Account and EthAccount components (#1224) ### Changed (Breaking) diff --git a/docs/modules/ROOT/pages/accounts.adoc b/docs/modules/ROOT/pages/accounts.adoc index 077591d95..af4a3dca8 100644 --- a/docs/modules/ROOT/pages/accounts.adoc +++ b/docs/modules/ROOT/pages/accounts.adoc @@ -32,7 +32,7 @@ supporting this execution flow and interoperability with DApps in the ecosystem. struct Call { to: ContractAddress, selector: felt252, - calldata: Array + calldata: Span } /// Standard Account Interface @@ -50,7 +50,7 @@ pub trait ISRC6 { ---- WARNING: The `calldata` member of the `Call` struct in the accounts has been updated to `Span` for optimization -purposes, but the interface Id remains the same for backwards compatibility. This inconsistency will be fixed in future releases. +purposes, but the interface ID remains the same for backwards compatibility. This inconsistency will be fixed in future releases. {snip-6}[SNIP-6] adds the `is_valid_signature` method. This method is not used by the protocol, but it's useful for DApps to verify the validity of signatures, supporting features like Sign In with Starknet. @@ -70,7 +70,7 @@ pub trait ISRC5 { } ---- -{snip-6}[SNIP-6] compliant accounts must return `true` when queried for the ISRC6 interface Id. +{snip-6}[SNIP-6] compliant accounts must return `true` when queried for the ISRC6 interface ID. Even though these interfaces are not enforced by the protocol, it's recommended to implement them for enabling interoperability with the ecosystem. diff --git a/docs/modules/ROOT/pages/api/account.adoc b/docs/modules/ROOT/pages/api/account.adoc index dde7678fa..78944c81b 100644 --- a/docs/modules/ROOT/pages/api/account.adoc +++ b/docs/modules/ROOT/pages/api/account.adoc @@ -252,7 +252,7 @@ See xref:AccountComponent-set_public_key[set_public_key]. [[AccountComponent-initializer]] ==== `[.contract-item-name]#++initializer++#++(ref self: ComponentState, public_key: felt252)++` [.item-kind]#internal# -Initializes the account with the given public key, and registers the ISRC6 interface ID. +Initializes the account with the given public key, and registers the `ISRC6` interface ID. Emits an {OwnerAdded} event. @@ -503,7 +503,7 @@ See xref:EthAccountComponent-set_public_key[set_public_key]. [[EthAccountComponent-initializer]] ==== `[.contract-item-name]#++initializer++#++(ref self: ComponentState, public_key: EthPublicKey)++` [.item-kind]#internal# -Initializes the account with the given public key, and registers the ISRC6 interface ID. +Initializes the account with the given public key, and registers the `ISRC6` interface ID. Emits an {OwnerAdded} event. @@ -692,7 +692,7 @@ Returns the status of a given nonce. `true` if the nonce is available to use. [[SRC9Component-initializer]] ==== `[.contract-item-name]#++initializer++#++(ref self: ComponentState)++` [.item-kind]#internal# -Initializes the account by registering the `ISRC9_V2` interface Id. +Initializes the account by registering the `ISRC9_V2` interface ID. == Presets diff --git a/packages/access/src/accesscontrol/accesscontrol.cairo b/packages/access/src/accesscontrol/accesscontrol.cairo index c9d0eb906..09761ad3a 100644 --- a/packages/access/src/accesscontrol/accesscontrol.cairo +++ b/packages/access/src/accesscontrol/accesscontrol.cairo @@ -193,7 +193,7 @@ pub mod AccessControlComponent { impl SRC5: SRC5Component::HasComponent, +Drop > of InternalTrait { - /// Initializes the contract by registering the IAccessControl interface Id. + /// Initializes the contract by registering the IAccessControl interface ID. fn initializer(ref self: ComponentState) { let mut src5_component = get_dep_component_mut!(ref self, SRC5); src5_component.register_interface(interface::IACCESSCONTROL_ID); diff --git a/packages/account/src/account.cairo b/packages/account/src/account.cairo index a1a67a384..29f96b455 100644 --- a/packages/account/src/account.cairo +++ b/packages/account/src/account.cairo @@ -10,14 +10,12 @@ pub mod AccountComponent { use core::num::traits::Zero; use core::poseidon::PoseidonTrait; use crate::interface; - use crate::utils::{MIN_TRANSACTION_VERSION, QUERY_OFFSET}; - use crate::utils::{execute_calls, is_valid_stark_signature}; + use crate::utils::{is_tx_version_valid, execute_calls, is_valid_stark_signature}; use openzeppelin_introspection::src5::SRC5Component::InternalTrait as SRC5InternalTrait; use openzeppelin_introspection::src5::SRC5Component::SRC5Impl; use openzeppelin_introspection::src5::SRC5Component; use starknet::account::Call; use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess}; - use starknet::{get_caller_address, get_contract_address, get_tx_info}; #[storage] pub struct Storage { @@ -66,34 +64,23 @@ pub mod AccountComponent { /// Requirements: /// /// - The transaction version must be greater than or equal to `MIN_TRANSACTION_VERSION`. - /// - If the transaction is a simulation (version than `QUERY_OFFSET`), it must be + /// - If the transaction is a simulation (version >= `QUERY_OFFSET`), it must be /// greater than or equal to `QUERY_OFFSET` + `MIN_TRANSACTION_VERSION`. fn __execute__( - self: @ComponentState, mut calls: Array + self: @ComponentState, calls: Array ) -> Array> { // Avoid calls from other contracts // https://github.com/OpenZeppelin/cairo-contracts/issues/344 - let sender = get_caller_address(); + let sender = starknet::get_caller_address(); assert(sender.is_zero(), Errors::INVALID_CALLER); - - // Check tx version - let tx_info = get_tx_info().unbox(); - let tx_version: u256 = tx_info.version.into(); - // Check if tx is a query - if (tx_version >= QUERY_OFFSET) { - assert( - QUERY_OFFSET + MIN_TRANSACTION_VERSION <= tx_version, Errors::INVALID_TX_VERSION - ); - } else { - assert(MIN_TRANSACTION_VERSION <= tx_version, Errors::INVALID_TX_VERSION); - } + assert(is_tx_version_valid(), Errors::INVALID_TX_VERSION); execute_calls(calls.span()) } /// Verifies the validity of the signature for the current transaction. /// This function is used by the protocol to verify `invoke` transactions. - fn __validate__(self: @ComponentState, mut calls: Array) -> felt252 { + fn __validate__(self: @ComponentState, calls: Array) -> felt252 { self.validate_transaction() } @@ -309,8 +296,9 @@ pub mod AccountComponent { impl SRC5: SRC5Component::HasComponent, +Drop > of InternalTrait { - /// Initializes the account by setting the initial public key - /// and registering the ISRC6 interface Id. + /// Initializes the account with the given public key, and registers the ISRC6 interface ID. + /// + /// Emits an `OwnerAdded` event. fn initializer(ref self: ComponentState, public_key: felt252) { let mut src5_component = get_dep_component_mut!(ref self, SRC5); src5_component.register_interface(interface::ISRC6_ID); @@ -319,8 +307,8 @@ pub mod AccountComponent { /// Validates that the caller is the account itself. Otherwise it reverts. fn assert_only_self(self: @ComponentState) { - let caller = get_caller_address(); - let self = get_contract_address(); + let caller = starknet::get_caller_address(); + let self = starknet::get_contract_address(); assert(self == caller, Errors::UNAUTHORIZED); } @@ -341,7 +329,7 @@ pub mod AccountComponent { let message_hash = PoseidonTrait::new() .update_with('StarkNet Message') .update_with('accept_ownership') - .update_with(get_contract_address()) + .update_with(starknet::get_contract_address()) .update_with(current_owner) .finalize(); @@ -352,7 +340,7 @@ pub mod AccountComponent { /// Validates the signature for the current transaction. /// Returns the short string `VALID` if valid, otherwise it reverts. fn validate_transaction(self: @ComponentState) -> felt252 { - let tx_info = get_tx_info().unbox(); + let tx_info = starknet::get_tx_info().unbox(); let tx_hash = tx_info.transaction_hash; let signature = tx_info.signature; assert(self._is_valid_signature(tx_hash, signature), Errors::INVALID_SIGNATURE); diff --git a/packages/account/src/eth_account.cairo b/packages/account/src/eth_account.cairo index 3b19610c7..e1c81f19d 100644 --- a/packages/account/src/eth_account.cairo +++ b/packages/account/src/eth_account.cairo @@ -13,16 +13,12 @@ pub mod EthAccountComponent { use crate::interface::EthPublicKey; use crate::interface; use crate::utils::secp256_point::Secp256PointStorePacking; - use crate::utils::{MIN_TRANSACTION_VERSION, QUERY_OFFSET}; - use crate::utils::{execute_calls, is_valid_eth_signature}; + use crate::utils::{is_tx_version_valid, execute_calls, is_valid_eth_signature}; use openzeppelin_introspection::src5::SRC5Component::InternalTrait as SRC5InternalTrait; use openzeppelin_introspection::src5::SRC5Component::SRC5Impl; use openzeppelin_introspection::src5::SRC5Component; use starknet::SyscallResultTrait; use starknet::account::Call; - use starknet::get_caller_address; - use starknet::get_contract_address; - use starknet::get_tx_info; use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess}; #[storage] @@ -72,34 +68,23 @@ pub mod EthAccountComponent { /// Requirements: /// /// - The transaction version must be greater than or equal to `MIN_TRANSACTION_VERSION`. - /// - If the transaction is a simulation (version than `QUERY_OFFSET`), it must be + /// - If the transaction is a simulation (version >= `QUERY_OFFSET`), it must be /// greater than or equal to `QUERY_OFFSET` + `MIN_TRANSACTION_VERSION`. fn __execute__( - self: @ComponentState, mut calls: Array + self: @ComponentState, calls: Array ) -> Array> { // Avoid calls from other contracts // https://github.com/OpenZeppelin/cairo-contracts/issues/344 - let sender = get_caller_address(); + let sender = starknet::get_caller_address(); assert(sender.is_zero(), Errors::INVALID_CALLER); - - // Check tx version - let tx_info = get_tx_info().unbox(); - let tx_version: u256 = tx_info.version.into(); - // Check if tx is a query - if (tx_version >= QUERY_OFFSET) { - assert( - QUERY_OFFSET + MIN_TRANSACTION_VERSION <= tx_version, Errors::INVALID_TX_VERSION - ); - } else { - assert(MIN_TRANSACTION_VERSION <= tx_version, Errors::INVALID_TX_VERSION); - } + assert(is_tx_version_valid(), Errors::INVALID_TX_VERSION); execute_calls(calls.span()) } /// Verifies the validity of the signature for the current transaction. /// This function is used by the protocol to verify `invoke` transactions. - fn __validate__(self: @ComponentState, mut calls: Array) -> felt252 { + fn __validate__(self: @ComponentState, calls: Array) -> felt252 { self.validate_transaction() } @@ -317,8 +302,9 @@ pub mod EthAccountComponent { impl SRC5: SRC5Component::HasComponent, +Drop > of InternalTrait { - /// Initializes the account by setting the initial public key - /// and registering the ISRC6 interface Id. + /// Initializes the account with the given public key, and registers the ISRC6 interface ID. + /// + /// Emits an `OwnerAdded` event. fn initializer(ref self: ComponentState, public_key: EthPublicKey) { let mut src5_component = get_dep_component_mut!(ref self, SRC5); src5_component.register_interface(interface::ISRC6_ID); @@ -327,8 +313,8 @@ pub mod EthAccountComponent { /// Validates that the caller is the account itself. Otherwise it reverts. fn assert_only_self(self: @ComponentState) { - let caller = get_caller_address(); - let self = get_contract_address(); + let caller = starknet::get_caller_address(); + let self = starknet::get_contract_address(); assert(self == caller, Errors::UNAUTHORIZED); } @@ -349,7 +335,7 @@ pub mod EthAccountComponent { let message_hash = PoseidonTrait::new() .update_with('StarkNet Message') .update_with('accept_ownership') - .update_with(get_contract_address()) + .update_with(starknet::get_contract_address()) .update_with(current_owner.get_coordinates().unwrap_syscall()) .finalize(); @@ -360,7 +346,7 @@ pub mod EthAccountComponent { /// Validates the signature for the current transaction. /// Returns the short string `VALID` if valid, otherwise it reverts. fn validate_transaction(self: @ComponentState) -> felt252 { - let tx_info = get_tx_info().unbox(); + let tx_info = starknet::get_tx_info().unbox(); let tx_hash = tx_info.transaction_hash; let signature = tx_info.signature; assert(self._is_valid_signature(tx_hash, signature), Errors::INVALID_SIGNATURE); diff --git a/packages/account/src/extensions/src9/src9.cairo b/packages/account/src/extensions/src9/src9.cairo index 182bf4735..a5d3609af 100644 --- a/packages/account/src/extensions/src9/src9.cairo +++ b/packages/account/src/extensions/src9/src9.cairo @@ -137,7 +137,7 @@ pub mod SRC9Component { impl SRC5: SRC5Component::HasComponent, +Drop > of InternalTrait { - /// Initializes the account by registering the ISRC9_V2 interface Id. + /// Initializes the account by registering the ISRC9_V2 interface ID. fn initializer(ref self: ComponentState) { let mut src5_component = get_dep_component_mut!(ref self, SRC5); src5_component.register_interface(interface::ISRC9_V2_ID); diff --git a/packages/account/src/tests/extensions/test_src9.cairo b/packages/account/src/tests/extensions/test_src9.cairo index 9547ce586..20fe1c9fc 100644 --- a/packages/account/src/tests/extensions/test_src9.cairo +++ b/packages/account/src/tests/extensions/test_src9.cairo @@ -10,10 +10,8 @@ use openzeppelin_testing::constants::{RECIPIENT, OWNER, OTHER, FELT_VALUE}; use openzeppelin_utils::cryptography::snip12::OffchainMessageHash; use snforge_std::signature::KeyPairTrait; use snforge_std::signature::stark_curve::{StarkCurveKeyPairImpl, StarkCurveSignerImpl}; -use snforge_std::{ - start_cheat_caller_address, cheat_caller_address, start_cheat_block_timestamp_global -}; -use snforge_std::{test_address, load, CheatSpan}; +use snforge_std::{start_cheat_block_timestamp_global, test_address, load, CheatSpan}; +use snforge_std::{start_cheat_caller_address, cheat_caller_address}; use starknet::account::Call; use starknet::storage::StorageMapWriteAccess; use starknet::{ContractAddress, contract_address_const}; @@ -124,7 +122,7 @@ fn test_execute_from_outside_v2_uses_nonce() { } #[test] -#[should_panic(expected: ('SRC9: invalid caller',))] +#[should_panic(expected: 'SRC9: invalid caller')] fn test_execute_from_outside_v2_caller_mismatch() { let mut state = setup(); let mut outside_execution = setup_outside_execution(RECIPIENT(), false); @@ -136,7 +134,7 @@ fn test_execute_from_outside_v2_caller_mismatch() { } #[test] -#[should_panic(expected: ('SRC9: now >= execute_before',))] +#[should_panic(expected: 'SRC9: now >= execute_before')] fn test_execute_from_outside_v2_call_after_execute_before() { let mut state = setup(); let outside_execution = setup_outside_execution(RECIPIENT(), false); @@ -147,7 +145,7 @@ fn test_execute_from_outside_v2_call_after_execute_before() { } #[test] -#[should_panic(expected: ('SRC9: now >= execute_before',))] +#[should_panic(expected: 'SRC9: now >= execute_before')] fn test_execute_from_outside_v2_call_equal_to_execute_before() { let mut state = setup(); let outside_execution = setup_outside_execution(RECIPIENT(), false); @@ -158,7 +156,7 @@ fn test_execute_from_outside_v2_call_equal_to_execute_before() { } #[test] -#[should_panic(expected: ('SRC9: now <= execute_after',))] +#[should_panic(expected: 'SRC9: now <= execute_after')] fn test_execute_from_outside_v2_call_before_execute_after() { let mut state = setup(); let outside_execution = setup_outside_execution(RECIPIENT(), false); @@ -169,7 +167,7 @@ fn test_execute_from_outside_v2_call_before_execute_after() { } #[test] -#[should_panic(expected: ('SRC9: now <= execute_after',))] +#[should_panic(expected: 'SRC9: now <= execute_after')] fn test_execute_from_outside_v2_call_equal_to_execute_after() { let mut state = setup(); let outside_execution = setup_outside_execution(RECIPIENT(), false); @@ -180,7 +178,7 @@ fn test_execute_from_outside_v2_call_equal_to_execute_after() { } #[test] -#[should_panic(expected: ('SRC9: duplicated nonce',))] +#[should_panic(expected: 'SRC9: duplicated nonce')] fn test_execute_from_outside_v2_invalid_nonce() { let mut state = setup(); let outside_execution = setup_outside_execution(RECIPIENT(), false); @@ -191,7 +189,7 @@ fn test_execute_from_outside_v2_invalid_nonce() { } #[test] -#[should_panic(expected: ('SRC9: invalid signature',))] +#[should_panic(expected: 'SRC9: invalid signature')] fn test_execute_from_outside_v2_invalid_signature() { let key_pair = KeyPairTrait::generate(); let account = setup_account(key_pair.public_key); diff --git a/packages/account/src/tests/test_account.cairo b/packages/account/src/tests/test_account.cairo index 4924f3ab4..dae79c71f 100644 --- a/packages/account/src/tests/test_account.cairo +++ b/packages/account/src/tests/test_account.cairo @@ -11,15 +11,11 @@ use openzeppelin_test_common::mocks::account::DualCaseAccountMock; use openzeppelin_test_common::mocks::simple::{ISimpleMockDispatcher, ISimpleMockDispatcherTrait}; use openzeppelin_testing as utils; use openzeppelin_testing::constants::stark::{KEY_PAIR, KEY_PAIR_2}; -use openzeppelin_testing::constants::{ - SALT, ZERO, OTHER, CALLER, QUERY_OFFSET, QUERY_VERSION, MIN_TRANSACTION_VERSION -}; +use openzeppelin_testing::constants::{SALT, QUERY_OFFSET, QUERY_VERSION, MIN_TRANSACTION_VERSION}; +use openzeppelin_testing::constants::{ZERO, OTHER, CALLER}; use openzeppelin_testing::signing::StarkKeyPair; -use snforge_std::{ - start_cheat_signature_global, start_cheat_transaction_version_global, - start_cheat_transaction_hash_global -}; -use snforge_std::{spy_events, test_address, start_cheat_caller_address}; +use snforge_std::{spy_events, start_cheat_signature_global, start_cheat_transaction_hash_global}; +use snforge_std::{test_address, start_cheat_caller_address, start_cheat_transaction_version_global}; use starknet::account::Call; // @@ -48,13 +44,13 @@ fn setup_dispatcher( let contract_class = utils::declare_class("DualCaseAccountMock"); let calldata = array![key_pair.public_key]; - let address = utils::deploy(contract_class, calldata); - let dispatcher = AccountABIDispatcher { contract_address: address }; + let contract_address = utils::deploy(contract_class, calldata); + let dispatcher = AccountABIDispatcher { contract_address }; start_cheat_signature_global(array![data.r, data.s].span()); start_cheat_transaction_hash_global(data.tx_hash); start_cheat_transaction_version_global(MIN_TRANSACTION_VERSION); - start_cheat_caller_address(address, ZERO()); + start_cheat_caller_address(contract_address, ZERO()); (dispatcher, contract_class.class_hash.into()) } @@ -114,7 +110,7 @@ fn test_validate_deploy() { } #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_validate_deploy_invalid_signature_data() { let key_pair = KEY_PAIR(); let mut data = SIGNED_TX_DATA(key_pair); @@ -125,7 +121,7 @@ fn test_validate_deploy_invalid_signature_data() { } #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_validate_deploy_invalid_signature_length() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -136,7 +132,7 @@ fn test_validate_deploy_invalid_signature_length() { } #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_validate_deploy_empty_signature() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -159,7 +155,7 @@ fn test_validate_declare() { } #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_validate_declare_invalid_signature_data() { let key_pair = KEY_PAIR(); let mut data = SIGNED_TX_DATA(key_pair); @@ -170,7 +166,7 @@ fn test_validate_declare_invalid_signature_data() { } #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_validate_declare_invalid_signature_length() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -181,7 +177,7 @@ fn test_validate_declare_invalid_signature_length() { } #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_validate_declare_empty_signature() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -197,16 +193,14 @@ fn test_execute_with_version(version: Option) { // Deploy target contract let calldata = array![]; - let address = utils::declare_and_deploy("SimpleMock", calldata); - let simple_mock = ISimpleMockDispatcher { contract_address: address }; + let contract_address = utils::declare_and_deploy("SimpleMock", calldata); + let simple_mock = ISimpleMockDispatcher { contract_address }; // Craft call and add to calls array let amount = 200; let calldata = array![amount]; let call = Call { - to: simple_mock.contract_address, - selector: selector!("increase_balance"), - calldata: calldata.span() + to: contract_address, selector: selector!("increase_balance"), calldata: calldata.span() }; let calls = array![call]; @@ -243,7 +237,7 @@ fn test_execute_query_version() { } #[test] -#[should_panic(expected: ('Account: invalid tx version',))] +#[should_panic(expected: 'Account: invalid tx version')] fn test_execute_invalid_query_version() { test_execute_with_version(Option::Some(QUERY_OFFSET)); } @@ -254,7 +248,7 @@ fn test_execute_future_query_version() { } #[test] -#[should_panic(expected: ('Account: invalid tx version',))] +#[should_panic(expected: 'Account: invalid tx version')] fn test_execute_invalid_version() { test_execute_with_version(Option::Some(MIN_TRANSACTION_VERSION - 1)); } @@ -270,7 +264,7 @@ fn test_validate() { } #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_validate_invalid() { let key_pair = KEY_PAIR(); let mut data = SIGNED_TX_DATA(key_pair); @@ -288,25 +282,21 @@ fn test_multicall() { // Deploy target contract let calldata = array![]; - let address = utils::declare_and_deploy("SimpleMock", calldata); - let simple_mock = ISimpleMockDispatcher { contract_address: address }; + let contract_address = utils::declare_and_deploy("SimpleMock", calldata); + let simple_mock = ISimpleMockDispatcher { contract_address }; // Craft 1st call let amount1 = 300; let calldata1 = array![amount1]; let call1 = Call { - to: simple_mock.contract_address, - selector: selector!("increase_balance"), - calldata: calldata1.span() + to: contract_address, selector: selector!("increase_balance"), calldata: calldata1.span() }; // Craft 2nd call let amount2 = 500; let calldata2 = array![amount2]; let call2 = Call { - to: simple_mock.contract_address, - selector: selector!("increase_balance"), - calldata: calldata2.span() + to: contract_address, selector: selector!("increase_balance"), calldata: calldata2.span() }; // Bundle calls and execute @@ -339,7 +329,7 @@ fn test_multicall_zero_calls() { } #[test] -#[should_panic(expected: ('Account: invalid caller',))] +#[should_panic(expected: 'Account: invalid caller')] fn test_account_called_from_contract() { let state = setup(KEY_PAIR()); let account_address = test_address(); @@ -379,7 +369,7 @@ fn test_public_key_setter_and_getter() { } #[test] -#[should_panic(expected: ('Account: unauthorized',))] +#[should_panic(expected: 'Account: unauthorized')] fn test_public_key_setter_different_account() { let mut state = COMPONENT_STATE(); let account_address = test_address(); @@ -418,7 +408,7 @@ fn test_public_key_setter_and_getter_camel() { } #[test] -#[should_panic(expected: ('Account: unauthorized',))] +#[should_panic(expected: 'Account: unauthorized')] fn test_public_key_setter_different_account_camel() { let mut state = COMPONENT_STATE(); let account_address = test_address(); @@ -462,7 +452,7 @@ fn test_assert_only_self_true() { } #[test] -#[should_panic(expected: ('Account: unauthorized',))] +#[should_panic(expected: 'Account: unauthorized')] fn test_assert_only_self_false() { let mut state = COMPONENT_STATE(); let account_address = test_address(); @@ -487,7 +477,7 @@ fn test_assert_valid_new_owner() { #[test] -#[should_panic(expected: ('Account: invalid signature',))] +#[should_panic(expected: 'Account: invalid signature')] fn test_assert_valid_new_owner_invalid_signature() { let key_pair = KEY_PAIR(); let state = setup(key_pair); diff --git a/packages/account/src/tests/test_eth_account.cairo b/packages/account/src/tests/test_eth_account.cairo index 7c844a075..49ae89e2a 100644 --- a/packages/account/src/tests/test_eth_account.cairo +++ b/packages/account/src/tests/test_eth_account.cairo @@ -14,16 +14,12 @@ use openzeppelin_test_common::mocks::account::DualCaseEthAccountMock; use openzeppelin_test_common::mocks::simple::{ISimpleMockDispatcher, ISimpleMockDispatcherTrait}; use openzeppelin_testing as utils; use openzeppelin_testing::constants::secp256k1::{KEY_PAIR, KEY_PAIR_2}; -use openzeppelin_testing::constants::{ - SALT, ZERO, OTHER, CALLER, QUERY_VERSION, MIN_TRANSACTION_VERSION -}; +use openzeppelin_testing::constants::{SALT, QUERY_VERSION, MIN_TRANSACTION_VERSION}; +use openzeppelin_testing::constants::{ZERO, OTHER, CALLER}; use openzeppelin_testing::signing::Secp256k1KeyPair; use openzeppelin_utils::serde::SerializedAppend; -use snforge_std::{ - start_cheat_signature_global, start_cheat_transaction_version_global, - start_cheat_transaction_hash_global, start_cheat_caller_address -}; -use snforge_std::{spy_events, test_address}; +use snforge_std::{spy_events, start_cheat_signature_global, start_cheat_transaction_hash_global}; +use snforge_std::{test_address, start_cheat_caller_address, start_cheat_transaction_version_global}; use starknet::account::Call; // @@ -135,7 +131,7 @@ fn test_validate_deploy() { } #[test] -#[should_panic(expected: ('EthAccount: invalid signature',))] +#[should_panic(expected: 'EthAccount: invalid signature')] fn test_validate_deploy_invalid_signature_data() { let key_pair = KEY_PAIR(); let mut data = SIGNED_TX_DATA(key_pair); @@ -146,7 +142,7 @@ fn test_validate_deploy_invalid_signature_data() { } #[test] -#[should_panic(expected: ('Signature: Invalid format.',))] +#[should_panic(expected: 'Signature: Invalid format.')] fn test_validate_deploy_invalid_signature_length() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -158,7 +154,7 @@ fn test_validate_deploy_invalid_signature_length() { } #[test] -#[should_panic(expected: ('Signature: Invalid format.',))] +#[should_panic(expected: 'Signature: Invalid format.')] fn test_validate_deploy_empty_signature() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -181,7 +177,7 @@ fn test_validate_declare() { } #[test] -#[should_panic(expected: ('EthAccount: invalid signature',))] +#[should_panic(expected: 'EthAccount: invalid signature')] fn test_validate_declare_invalid_signature_data() { let key_pair = KEY_PAIR(); let mut data = SIGNED_TX_DATA(key_pair); @@ -192,7 +188,7 @@ fn test_validate_declare_invalid_signature_data() { } #[test] -#[should_panic(expected: ('Signature: Invalid format.',))] +#[should_panic(expected: 'Signature: Invalid format.')] fn test_validate_declare_invalid_signature_length() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -204,7 +200,7 @@ fn test_validate_declare_invalid_signature_length() { } #[test] -#[should_panic(expected: ('Signature: Invalid format.',))] +#[should_panic(expected: 'Signature: Invalid format.')] fn test_validate_declare_empty_signature() { let key_pair = KEY_PAIR(); let (account, class_hash) = setup_dispatcher(key_pair, SIGNED_TX_DATA(key_pair)); @@ -222,16 +218,14 @@ fn test_execute_with_version(version: Option) { // Deploy target contract let calldata = array![]; - let address = utils::declare_and_deploy("SimpleMock", calldata); - let simple_mock = ISimpleMockDispatcher { contract_address: address }; + let contract_address = utils::declare_and_deploy("SimpleMock", calldata); + let simple_mock = ISimpleMockDispatcher { contract_address }; // Craft call and add to calls array let amount = 200; let calldata = array![amount]; let call = Call { - to: simple_mock.contract_address, - selector: selector!("increase_balance"), - calldata: calldata.span() + to: contract_address, selector: selector!("increase_balance"), calldata: calldata.span() }; let calls = array![call]; @@ -297,25 +291,21 @@ fn test_multicall() { // Deploy target contract let calldata = array![]; - let address = utils::declare_and_deploy("SimpleMock", calldata); - let simple_mock = ISimpleMockDispatcher { contract_address: address }; + let contract_address = utils::declare_and_deploy("SimpleMock", calldata); + let simple_mock = ISimpleMockDispatcher { contract_address }; // Craft 1st call let amount1 = 300; let calldata1 = array![amount1]; let call1 = Call { - to: simple_mock.contract_address, - selector: selector!("increase_balance"), - calldata: calldata1.span() + to: contract_address, selector: selector!("increase_balance"), calldata: calldata1.span() }; // Craft 2nd call let amount2 = 500; let calldata2 = array![amount2]; let call2 = Call { - to: simple_mock.contract_address, - selector: selector!("increase_balance"), - calldata: calldata2.span() + to: contract_address, selector: selector!("increase_balance"), calldata: calldata2.span() }; // Bundle calls and execute @@ -407,7 +397,7 @@ fn test_public_key_setter_and_getter() { } #[test] -#[should_panic(expected: ('EthAccount: unauthorized',))] +#[should_panic(expected: 'EthAccount: unauthorized')] fn test_public_key_setter_different_account() { let mut state = COMPONENT_STATE(); let key_pair = KEY_PAIR(); @@ -451,7 +441,7 @@ fn test_public_key_setter_and_getter_camel() { } #[test] -#[should_panic(expected: ('EthAccount: unauthorized',))] +#[should_panic(expected: 'EthAccount: unauthorized')] fn test_public_key_setter_different_account_camel() { let mut state = COMPONENT_STATE(); let key_pair = KEY_PAIR(); @@ -499,7 +489,7 @@ fn test_assert_only_self_true() { } #[test] -#[should_panic(expected: ('EthAccount: unauthorized',))] +#[should_panic(expected: 'EthAccount: unauthorized')] fn test_assert_only_self_false() { let state = COMPONENT_STATE(); @@ -522,7 +512,7 @@ fn test_assert_valid_new_owner() { } #[test] -#[should_panic(expected: ('EthAccount: invalid signature',))] +#[should_panic(expected: 'EthAccount: invalid signature')] fn test_assert_valid_new_owner_invalid_signature() { let key_pair = KEY_PAIR(); let state = setup(key_pair); diff --git a/packages/account/src/tests/test_signature.cairo b/packages/account/src/tests/test_signature.cairo index f7841b2b3..0d6e9d648 100644 --- a/packages/account/src/tests/test_signature.cairo +++ b/packages/account/src/tests/test_signature.cairo @@ -83,7 +83,7 @@ fn test_is_valid_eth_signature_bad_sig() { } #[test] -#[should_panic(expected: ('Signature: Invalid format.',))] +#[should_panic(expected: 'Signature: Invalid format.')] fn test_is_valid_eth_signature_invalid_format_sig() { let key_pair = secp256k1::KEY_PAIR(); let data = eth_signature_data(key_pair); diff --git a/packages/account/src/utils.cairo b/packages/account/src/utils.cairo index 4537a1c1f..e31f8779e 100644 --- a/packages/account/src/utils.cairo +++ b/packages/account/src/utils.cairo @@ -14,20 +14,27 @@ pub const QUERY_OFFSET: u256 = 0x100000000000000000000000000000000; // QUERY_OFFSET + TRANSACTION_VERSION pub const QUERY_VERSION: u256 = 0x100000000000000000000000000000001; -pub fn execute_calls(mut calls: Span) -> Array> { +pub fn execute_calls(calls: Span) -> Array> { let mut res = array![]; - loop { - match calls.pop_front() { - Option::Some(call) => { - let _res = execute_single_call(call); - res.append(_res); - }, - Option::None => { break (); }, - }; + for call in calls { + res.append(execute_single_call(call)) }; res } +/// If the transaction is a simulation (version >= `QUERY_OFFSET`), it must be +/// greater than or equal to `QUERY_OFFSET` + `MIN_TRANSACTION_VERSION` to be considered valid. +/// Otherwise, it must be greater than or equal to `MIN_TRANSACTION_VERSION`. +pub fn is_tx_version_valid() -> bool { + let tx_info = starknet::get_tx_info().unbox(); + let tx_version = tx_info.version.into(); + if tx_version >= QUERY_OFFSET { + QUERY_OFFSET + MIN_TRANSACTION_VERSION <= tx_version + } else { + MIN_TRANSACTION_VERSION <= tx_version + } +} + fn execute_single_call(call: @Call) -> Span { let Call { to, selector, calldata } = *call; starknet::syscalls::call_contract_syscall(to, selector, calldata).unwrap_syscall()