Support higher tx versions in Account (OpenZeppelin#858)

* support higher tx versions in account * update changelog * add utils * add utils * change version to u256 * update version import and cast to felt * update spdx * update scarb and cairo to 2.4.0 * bump to v2.4.1 * add helper fns for testing versions * add scarb bump to changelog * move account consts back to accountcomponent * change const to MIN_TRANSACTION_VERSION * add version consts for tests * change version helper fns to consts * cast tx version to u64 in __execute__ * add query version comment * add future query version test * cast version to u256 * add query tx version check * add requirement in __execute__ comment * fix comment * add comment * fix comment * Update src/account/account.cairo --------- Co-authored-by: Martín Triay <martriay@gmail.com>
andrew-fleming · Jan 20, 2024 · 8372bc9 · 8372bc9
1 parent 4f90c23
commit 8372bc9
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 19 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -16,7 +16,7 @@ jobs:
     - uses: actions/checkout@v3
     - uses: software-mansion/setup-scarb@v1
       with:
-        scarb-version: "2.3.1"
+        scarb-version: "2.4.1"
     - name: Markdown lint
       uses: DavidAnson/markdownlint-cli2-action@5b7c9f74fec47e6b15667b2cc23c63dff11e449e # v9
       with:

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,3 +18,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Use ComponentState in tests (#836)
 - Docsite navbar (#838)
+- Support higher tx versions in Account (#858)
+- Bump scarb to v2.4.1 (#858)
diff --git a/Scarb.toml b/Scarb.toml
@@ -1,7 +1,7 @@
 [package]
 name = "openzeppelin"
 version = "0.8.0"
-cairo-version = "2.3.1"
+cairo-version = "2.4.1"
 authors = ["OpenZeppelin Community <maintainers@openzeppelin.org>"]
 description = "OpenZeppelin Contracts written in Cairo for StarkNet, a decentralized ZK Rollup"
 documentation = "https://docs.openzeppelin.com/contracts-cairo"
@@ -11,7 +11,7 @@ license-file = "LICENSE"
 keywords = ["openzeppelin", "starknet", "cairo", "contracts", "security", "standards"]
 
 [dependencies]
-starknet = "2.3.1"
+starknet = "2.4.1"
 
 [lib]
 

diff --git a/src/account/account.cairo b/src/account/account.cairo
@@ -15,9 +15,8 @@ mod AccountComponent {
     use starknet::get_contract_address;
     use starknet::get_tx_info;
 
-    const TRANSACTION_VERSION: felt252 = 1;
-    // 2**128 + TRANSACTION_VERSION
-    const QUERY_VERSION: felt252 = 0x100000000000000000000000000000001;
+    const MIN_TRANSACTION_VERSION: u256 = 1;
+    const QUERY_OFFSET: u256 = 0x100000000000000000000000000000000;
 
     #[storage]
     struct Storage {
@@ -59,8 +58,9 @@ mod AccountComponent {
         ///
         /// Requirements:
         ///
-        /// - The transaction version must be `TRANSACTION_VERSION` for actual transactions.
-        /// For simulations, the version must be `QUERY_VERSION`.
+        /// - 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
+        /// greater than or equal to `QUERY_OFFSET` + `MIN_TRANSACTION_VERSION`.
         fn __execute__(
             self: @ComponentState<TContractState>, mut calls: Array<Call>
         ) -> Array<Span<felt252>> {
@@ -71,9 +71,14 @@ mod AccountComponent {
 
             // Check tx version
             let tx_info = get_tx_info().unbox();
-            let version = tx_info.version;
-            if version != TRANSACTION_VERSION {
-                assert(version == QUERY_VERSION, Errors::INVALID_TX_VERSION);
+            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);
             }
 
             _execute_calls(calls)

diff --git a/src/tests/account/test_account.cairo b/src/tests/account/test_account.cairo
@@ -1,14 +1,15 @@
 use openzeppelin::account::AccountComponent::{InternalTrait, SRC6CamelOnlyImpl};
 use openzeppelin::account::AccountComponent::{OwnerAdded, OwnerRemoved};
 use openzeppelin::account::AccountComponent::{PublicKeyCamelImpl, PublicKeyImpl};
-use openzeppelin::account::AccountComponent::{TRANSACTION_VERSION, QUERY_VERSION};
 use openzeppelin::account::AccountComponent;
 use openzeppelin::account::interface::{ISRC6, ISRC6_ID};
 use openzeppelin::account::{AccountABIDispatcherTrait, AccountABIDispatcher};
 use openzeppelin::introspection::interface::{ISRC5, ISRC5_ID};
 use openzeppelin::tests::mocks::account_mocks::DualCaseAccountMock;
 use openzeppelin::tests::mocks::erc20_mocks::DualCaseERC20Mock;
-use openzeppelin::tests::utils::constants::{PUBKEY, NEW_PUBKEY, SALT, ZERO};
+use openzeppelin::tests::utils::constants::{
+    PUBKEY, NEW_PUBKEY, SALT, ZERO, QUERY_OFFSET, QUERY_VERSION, MIN_TRANSACTION_VERSION
+};
 use openzeppelin::tests::utils;
 use openzeppelin::token::erc20::interface::{IERC20DispatcherTrait, IERC20Dispatcher};
 use openzeppelin::utils::selectors;
@@ -71,7 +72,7 @@ fn setup() -> ComponentState {
 }
 
 fn setup_dispatcher(data: Option<@SignedTransactionData>) -> AccountABIDispatcher {
-    testing::set_version(TRANSACTION_VERSION);
+    testing::set_version(MIN_TRANSACTION_VERSION);
 
     let mut calldata = array![];
     if data.is_some() {
@@ -287,17 +288,34 @@ fn test_execute() {
     test_execute_with_version(Option::None(()));
 }
 
+#[test]
+fn test_execute_future_version() {
+    test_execute_with_version(Option::Some(MIN_TRANSACTION_VERSION + 1));
+}
+
 #[test]
 #[available_gas(2000000)]
 fn test_execute_query_version() {
     test_execute_with_version(Option::Some(QUERY_VERSION));
 }
 
+#[test]
+#[should_panic(expected: ('Account: invalid tx version', 'ENTRYPOINT_FAILED'))]
+fn test_execute_invalid_query_version() {
+    test_execute_with_version(Option::Some(QUERY_OFFSET));
+}
+
+#[test]
+#[available_gas(2000000)]
+fn test_execute_future_query_version() {
+    test_execute_with_version(Option::Some(QUERY_VERSION + 1));
+}
+
 #[test]
 #[available_gas(2000000)]
 #[should_panic(expected: ('Account: invalid tx version', 'ENTRYPOINT_FAILED'))]
 fn test_execute_invalid_version() {
-    test_execute_with_version(Option::Some(TRANSACTION_VERSION - 1));
+    test_execute_with_version(Option::Some(MIN_TRANSACTION_VERSION - 1));
 }
 
 #[test]

diff --git a/src/tests/presets/test_account.cairo b/src/tests/presets/test_account.cairo
@@ -1,13 +1,14 @@
 use openzeppelin::account::AccountComponent::{OwnerAdded, OwnerRemoved};
-use openzeppelin::account::AccountComponent::{TRANSACTION_VERSION, QUERY_VERSION};
 use openzeppelin::account::interface::ISRC6_ID;
 use openzeppelin::account::{AccountABIDispatcherTrait, AccountABIDispatcher};
 use openzeppelin::introspection::interface::ISRC5_ID;
 use openzeppelin::presets::Account;
 use openzeppelin::tests::account::test_account::{
     deploy_erc20, SIGNED_TX_DATA, SignedTransactionData
 };
-use openzeppelin::tests::utils::constants::{PUBKEY, NEW_PUBKEY, SALT, ZERO, RECIPIENT};
+use openzeppelin::tests::utils::constants::{
+    PUBKEY, NEW_PUBKEY, SALT, ZERO, QUERY_OFFSET, QUERY_VERSION, RECIPIENT, MIN_TRANSACTION_VERSION
+};
 use openzeppelin::tests::utils;
 use openzeppelin::token::erc20::interface::{IERC20DispatcherTrait, IERC20Dispatcher};
 use openzeppelin::utils::selectors;
@@ -34,7 +35,7 @@ fn setup_dispatcher() -> AccountABIDispatcher {
 }
 
 fn setup_dispatcher_with_data(data: Option<@SignedTransactionData>) -> AccountABIDispatcher {
-    testing::set_version(TRANSACTION_VERSION);
+    testing::set_version(MIN_TRANSACTION_VERSION);
 
     let mut calldata = array![];
     if data.is_some() {
@@ -328,17 +329,34 @@ fn test_execute() {
     test_execute_with_version(Option::None(()));
 }
 
+#[test]
+fn test_execute_future_version() {
+    test_execute_with_version(Option::Some(MIN_TRANSACTION_VERSION + 1));
+}
+
 #[test]
 #[available_gas(2000000)]
 fn test_execute_query_version() {
     test_execute_with_version(Option::Some(QUERY_VERSION));
 }
 
+#[test]
+#[should_panic(expected: ('Account: invalid tx version', 'ENTRYPOINT_FAILED'))]
+fn test_execute_invalid_query_version() {
+    test_execute_with_version(Option::Some(QUERY_OFFSET));
+}
+
+#[test]
+#[available_gas(2000000)]
+fn test_execute_future_query_version() {
+    test_execute_with_version(Option::Some(QUERY_VERSION + 1));
+}
+
 #[test]
 #[available_gas(2000000)]
 #[should_panic(expected: ('Account: invalid tx version', 'ENTRYPOINT_FAILED'))]
 fn test_execute_invalid_version() {
-    test_execute_with_version(Option::Some(TRANSACTION_VERSION - 1));
+    test_execute_with_version(Option::Some(MIN_TRANSACTION_VERSION - 1));
 }
 
 #[test]

diff --git a/src/tests/utils/constants.cairo b/src/tests/utils/constants.cairo
@@ -17,6 +17,11 @@ const NEW_PUBKEY: felt252 = 'NEW_PUBKEY';
 const SALT: felt252 = 'SALT';
 const SUCCESS: felt252 = 123123;
 const FAILURE: felt252 = 456456;
+const MIN_TRANSACTION_VERSION: felt252 = 1;
+// 2**128
+const QUERY_OFFSET: felt252 = 0x100000000000000000000000000000000;
+// QUERY_OFFSET + MIN_TRANSACTION_VERSION
+const QUERY_VERSION: felt252 = 0x100000000000000000000000000000001;
 
 fn ADMIN() -> ContractAddress {
     contract_address_const::<'ADMIN'>()