Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support higher tx versions in Account #858

Merged
merged 25 commits into from Jan 20, 2024

Conversation

andrew-fleming
Copy link
Collaborator

@andrew-fleming andrew-fleming commented Dec 18, 2023

Fixes #830.

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

@andrew-fleming andrew-fleming marked this pull request as ready for review December 26, 2023 15:41
@martriay martriay requested review from ericnordelo and removed request for ericnordelo December 28, 2023 01:34
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good andrew! Left a small suggestion.

src/tests/account/test_account.cairo Show resolved Hide resolved
.github/workflows/test.yml Show resolved Hide resolved
src/account/utils.cairo Outdated Show resolved Hide resolved
src/account/account.cairo Outdated Show resolved Hide resolved
src/account/account.cairo Outdated Show resolved Hide resolved
src/account/account.cairo Show resolved Hide resolved
src/tests/utils/constants.cairo Outdated Show resolved Hide resolved
@martriay martriay mentioned this pull request Dec 29, 2023
4 tasks
src/account/account.cairo Outdated Show resolved Hide resolved
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@andrew-fleming
Copy link
Collaborator Author

andrew-fleming commented Jan 19, 2024

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
Copy link
Contributor

martriay commented Jan 19, 2024

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

src/account/account.cairo Outdated Show resolved Hide resolved
@martriay martriay merged commit 8372bc9 into OpenZeppelin:main Jan 20, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support higher tx versions in AccountComponent
3 participants