Skip to content

Move ErrNilValidatorsInState from one in each state version to a common one#10074

Merged
prylabs-bulldozer[bot] merged 1 commit into
OffchainLabs:developfrom
leolara:refactor/move-ErrNilValidatorsInState
Jan 18, 2022
Merged

Move ErrNilValidatorsInState from one in each state version to a common one#10074
prylabs-bulldozer[bot] merged 1 commit into
OffchainLabs:developfrom
leolara:refactor/move-ErrNilValidatorsInState

Conversation

@leolara

@leolara leolara commented Jan 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

What type of PR is this?
Refactor

What does this PR do? Why is it needed?

To increase DRY and enable DRY in tests and other users of the Beacon Chain state package,
an error that was duplicated unnecessarily in each version of the state is moved to the root
Beacon Chain state package.

Which issues(s) does this PR fix?

Related and blocks #10047

Other notes for review

None

@leolara leolara requested a review from a team as a code owner January 12, 2022 06:27
nisdas
nisdas previously approved these changes Jan 12, 2022
View reviewed changes

@nisdas nisdas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@nisdas

nisdas commented Jan 12, 2022

Copy link
Copy Markdown
Contributor

This PR is blocked till #10069 is merged in as that PR will be applying a few non trivial changes to the state package and this PR
will conflict with it.

@nisdas nisdas added the blocked label Jan 12, 2022
@nisdas nisdas dismissed their stale review January 12, 2022 13:47

Blocked for now

prestonvanloon
prestonvanloon reviewed Jan 12, 2022
View reviewed changes

@prestonvanloon prestonvanloon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! LGTM

@leolara leolara mentioned this pull request Jan 13, 2022
Closed
@rkapka

rkapka commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

Even though #10069 was merged, let's wait for #9820

@nisdas

nisdas commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

Even though #10069 was merged, let's wait for #9820

This is a minor change, with the large changes having been added in #10069. #9820 will take a while to be fully merged in, we shouldn't do a complete block.

@rkapka

rkapka commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

You are right. Let's at least make me and you required reviewers for any PR that touches state, like we discussed on Discord.

@leolara

leolara commented Jan 14, 2022

Copy link
Copy Markdown
Contributor Author

Ok, so does that mean this can be merged? @rkapka @nisdas

@rkapka rkapka removed the blocked label Jan 14, 2022
rkapka
rkapka requested changes Jan 14, 2022
View reviewed changes

@rkapka rkapka left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need the same changes applied to code inside /state/state-native. This is because the blocking issue introduced temporary state code duplication.

@leolara

leolara commented Jan 15, 2022

Copy link
Copy Markdown
Contributor Author

@rkapka In the base branch (develop) there is no state/state-native, so is this still blocked? Or perhaps I do not understand your instructions?

@nisdas

nisdas commented Jan 15, 2022

Copy link
Copy Markdown
Contributor

Hey @leolara , @rkapka should be referring to this directory:
https://github.com/prysmaticlabs/prysm/tree/develop/beacon-chain/state/state-native

You will have to repeat what was done in this sub-package too, sorry for the inconvenience. This is a temporary thing until we fully roll out our feature.

@leolara

leolara commented Jan 15, 2022

Copy link
Copy Markdown
Contributor Author

@nisdas Thanks 🤦 my bad

@rkapka

rkapka commented Jan 15, 2022

Copy link
Copy Markdown
Contributor

I apologize for the confusion

@leolara leolara force-pushed the refactor/move-ErrNilValidatorsInState branch from 98c638c to 4616424 Compare January 18, 2022 03:44
@leolara

leolara commented Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

@rkapka @nisdas please check again, state-native included

nisdas
nisdas previously approved these changes Jan 18, 2022
View reviewed changes

@nisdas nisdas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding it in @leolara ! LGTM

cc @rkapka

rkapka
rkapka previously approved these changes Jan 18, 2022
View reviewed changes
@leolara
Move ErrNilValidatorsInState from one in each state version to a comm…
54efaa8 
…on one

To increase DRY and enable DRY in tests and other users of the Beacon Chain state package,
an error that was duplicated unnecessarily in each version of the state is moved to the root
Beacon Chain state package.
@leolara leolara dismissed stale reviews from rkapka and nisdas via 54efaa8 January 18, 2022 07:52
@leolara leolara force-pushed the refactor/move-ErrNilValidatorsInState branch from 4616424 to 54efaa8 Compare January 18, 2022 07:52
@leolara

leolara commented Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

@nisdas could you merge? I don't have the rights

@nisdas

nisdas commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

@leolara Our CI is still running the build, it will take 10 -15 minutes more to be merged in.

@prylabs-bulldozer prylabs-bulldozer Bot merged commit 64c02c4 into OffchainLabs:develop Jan 18, 2022
@danceratopz danceratopz mentioned this pull request Jan 15, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prestonvanloon prestonvanloon prestonvanloon left review comments

@rkapka rkapka rkapka left review comments

@terencechain terencechain Awaiting requested review from terencechain terencechain was automatically assigned from prysmaticlabs/core-team

+1 more reviewer

@nisdas nisdas nisdas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@leolara @nisdas @rkapka @prestonvanloon