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

refactor: Rename consensus tasks and split storage (BFT-476) #2357

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Jul 1, 2024

What ❔

  • Renames FetcherTask to ExternalNodeTask
  • Moves run_main_node to mn::run_main_node to match en::run_external_node
  • Splits consensus::storage into consensus::storage::connection and consensus::storage::store

Why ❔

I'm working on #2340 where I made these changes either because the naming was confusing or the module was getting very long and I thought it would make it easier to have it in two before adding more trait implementations to it. The PR was getting huge even before I did any actual work, so I decided to make a pure refactoring PR to make the other one easier to review later.

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.

Copy link
Member

@brunoffranca brunoffranca left a comment

Choose a reason for hiding this comment

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

Looks good!

@brunoffranca brunoffranca added this pull request to the merge queue Jul 1, 2024
Merged via the queue into main with commit 107e1a7 Jul 1, 2024
52 of 53 checks passed
@brunoffranca brunoffranca deleted the bft-476-refactor branch July 1, 2024 18:03
@popzxc
Copy link
Member

popzxc commented Jul 1, 2024

image

Is it expected that the contracts submodule was updated?

@aakoshh
Copy link
Contributor Author

aakoshh commented Jul 1, 2024

@popzxc no, I don't know why it did that. Could it be a side effect of zk init? Sorry, I missed that. It would explain why one of the tests failed with this:

x  Failed to parse config file "/usr/src/zksync/contracts/l1-contracts/script-out/output-deploy-erc20.toml".
|  
!    0: TOML parse error at line 5, column 8
|    |
|  5 | mint = 174085318024506600000000000.0
|    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|  invalid type: floating point `17[408](https://github.com/matter-labs/zksync-era/actions/runs/9744720297/job/26904060256#step:7:409)5318024506600000000000.0`, expected a (both 0x-prefixed or not) hex string or byte array containing between (0; 32] bytes
|  
—  Failed to run command

@popzxc
Copy link
Member

popzxc commented Jul 2, 2024

@aakoshh I think it happened during the merge, e.g. you updated main (which had updated submodule revision) but didn't update it via git submodule update --recursive, and then you committed the (old) revision checked out locally.

@brunoffranca please check the PR for suspicious things; if something doesn't seem to belong to the PR, it's generally better to ask about it.

popzxc added a commit that referenced this pull request Jul 2, 2024
@aakoshh aakoshh restored the bft-476-refactor branch July 2, 2024 08:06
github-merge-queue bot pushed a commit that referenced this pull request Jul 2, 2024
#2364)

Reverts #2357

This PR changed the contracts submodule by mistake.
cc @aakoshh @brunoffranca
github-merge-queue bot pushed a commit that referenced this pull request Jul 2, 2024
## What ❔

This is a 2nd attempt at
#2357 to redo it after
#2364 has reverted it. The
fix was cherry picked from
#2365

## Why ❔

The other PR accidentally changed the contracts submodule.

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [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`.

---------

Co-authored-by: matias-gonz <[email protected]>
irnb pushed a commit to vianetwork/via-server that referenced this pull request Jul 12, 2024
…labs#2357)

## What ❔
* Renames `FetcherTask` to `ExternalNodeTask`
* Moves `run_main_node` to `mn::run_main_node` to match
`en::run_external_node`
* Splits `consensus::storage` into `consensus::storage::connection` and
`consensus::storage::store`

## Why ❔

I'm working on matter-labs#2340 where
I made these changes either because the naming was confusing or the
module was getting very long and I thought it would make it easier to
have it in two before adding more trait implementations to it. The PR
was getting huge even before I did any actual work, so I decided to make
a pure refactoring PR to make the other one easier to review later.

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [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`.
irnb pushed a commit to vianetwork/via-server that referenced this pull request Jul 12, 2024
irnb pushed a commit to vianetwork/via-server that referenced this pull request Jul 12, 2024
…labs#2366)

## What ❔

This is a 2nd attempt at
matter-labs#2357 to redo it after
matter-labs#2364 has reverted it. The
fix was cherry picked from
matter-labs#2365

## Why ❔

The other PR accidentally changed the contracts submodule.

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [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`.

---------

Co-authored-by: matias-gonz <[email protected]>
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.

3 participants