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

feat: add state override for gas estimates #1358

Merged
merged 49 commits into from
Jul 22, 2024

Conversation

aon
Copy link
Contributor

@aon aon commented Mar 5, 2024

What ❔

  • Adds state override for gas estimates

Why ❔

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.
  • Spellcheck has been run via zk spellcheck.
  • Linkcheck has been run via zk linkcheck.

@aon aon requested a review from Deniallugo March 6, 2024 20:14
Copy link
Contributor

@Deniallugo Deniallugo left a comment

Choose a reason for hiding this comment

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

I'm waiting for a full implementation..
I'm a bit of concerned, that we have take into account such a lot of places outside vm, probably it does make sense to introduce new abstraction where we do override on abstraction level which is deeper than storage view and use this abstraction instead of direct connection to the database

core/lib/types/src/api/mod.rs Outdated Show resolved Hide resolved
core/lib/types/src/api/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/api_server/tx_sender/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/api_server/web3/tests/vm.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Deniallugo Deniallugo left a comment

Choose a reason for hiding this comment

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

Mostly nits.
Please address the issues with getting balance and setting new value in storage.

core/lib/state/src/storage_overrides.rs Outdated Show resolved Hide resolved
core/lib/state/src/storage_overrides.rs Outdated Show resolved Hide resolved
core/lib/types/src/api/mod.rs Outdated Show resolved Hide resolved
@Jrigada Jrigada marked this pull request as ready for review March 14, 2024 17:26
@Jrigada Jrigada requested review from Deniallugo and Jrigada March 14, 2024 19:36
@Deniallugo Deniallugo requested a review from perekopskiy July 1, 2024 10:38
Copy link
Contributor

@perekopskiy perekopskiy left a comment

Choose a reason for hiding this comment

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

Please add tests for state and stateDiff overrides after fixing the issue with stateDiff

core/lib/state/src/storage_overrides.rs Outdated Show resolved Hide resolved
core/lib/state/src/storage_overrides.rs Outdated Show resolved Hide resolved
core/lib/state/src/storage_view.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/Cargo.toml Outdated Show resolved Hide resolved
core/tests/ts-integration/tests/api/web3.test.ts Outdated Show resolved Hide resolved
@Jrigada Jrigada requested review from Deniallugo and perekopskiy July 4, 2024 14:41
Copy link
Contributor

@perekopskiy perekopskiy left a comment

Choose a reason for hiding this comment

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

Your tests don't cover semantical difference between state and stateDiff. Please fix code and change tests so they actually test it.

core/lib/state/src/storage_overrides.rs Outdated Show resolved Hide resolved
core/lib/state/src/storage_overrides.rs Outdated Show resolved Hide resolved
core/tests/ts-integration/tests/api/web3.test.ts Outdated Show resolved Hide resolved
@Jrigada Jrigada requested a review from perekopskiy July 19, 2024 15:36
@Deniallugo Deniallugo enabled auto-merge July 22, 2024 13:08
Copy link
Contributor

No performance difference detected (anymore)

@Deniallugo Deniallugo added this pull request to the merge queue Jul 22, 2024
Merged via the queue into main with commit 761bda1 Jul 22, 2024
47 checks passed
@Deniallugo Deniallugo deleted the aon-add-override-estimate-gas-tracer branch July 22, 2024 13:47
github-merge-queue bot pushed a commit that referenced this pull request Jul 24, 2024
🤖 I have created a release *beep* *boop*
---


##
[24.11.0](core-v24.10.0...core-v24.11.0)
(2024-07-23)


### Features

* add revert tests (external node) to zk_toolbox
([#2408](#2408))
([3fbbee1](3fbbee1))
* add state override for gas estimates
([#1358](#1358))
([761bda1](761bda1))
* added consensus_config to general config
([#2462](#2462))
([c5650a4](c5650a4))
* added key generation command to EN
([#2461](#2461))
([9861415](9861415))
* remove leftovers after BWIP
([#2456](#2456))
([990676c](990676c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: zksync-era-bot <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Jul 24, 2024
## What ❔

Brush up VM storage overrides as introduced in
#1358.

## Why ❔

The overrides implementation looks overly complex and isn't correctly
localized by domain (located in the `state` crate, while the
functionality is API server-specific). This worsens maintainability.

## Checklist

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Tests for the changes have been added / updated.
- [x] Documentation comments have been added / updated.
- [x] Code has been formatted via `zk fmt` and `zk lint`.
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.

4 participants