OpenZeppelin / cairo-contracts

OpenZeppelin Contracts written in Cairo for Starknet, a decentralized ZK Rollup
https://docs.openzeppelin.com/contracts-cairo
MIT License
813 stars 329 forks source link

Support higher tx versions in Account #858

Closed andrew-fleming closed 8 months ago

andrew-fleming commented 9 months ago

Fixes #830.

PR Checklist

andrew-fleming commented 8 months ago

Benchmarking gas in query tx version check

The following snippets show the different logic with checking the transaction versions within AccountComponent's __execute__. "Without query version check" does not check if a query version is valid. This means that a user would receive a false positive on a queried transaction if the transaction is not supported by AccountComponent e.g. querying a tx with version 0 would pass; however, executing the transaction would fail.

This PR now proposes to check if the query version is a supported version, but there is a small increase in gas. The table at the bottom of this comment compares the gas estimates from the test runner.

Without query version check:

fn __execute__(...){

    (...)

  // 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);
    }

    (...)
}

With query version check:

fn __execute__(...){

    (...)

  // Check tx version
  let tx_info = get_tx_info().unbox();
  let tx_version: u256 = tx_info.version.into();
    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);
  }

    (...)

}
Account __execute__ tests
Without check With check Diff Diff %
test_execute 1491640 1500620 8980 .60%
test_execute_invalid_version 847910 856890 - 1.05%
test_execute_future_version 1491740 1500720 - .60%
test_execute_query_version 1491640 1500620 - .60%
test_execute_future_query_version 1491740 1500720 - .60%
test_multicall 2078650 2087630 - .43%
test_multicall_zero_calls 385430 394410 - 2.33%

martriay commented 8 months ago

That's very low, and it will get proportionally smaller the bigger is the Array<Call>. Let's add the check.