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

Consolidate ApplyDeposit for Altair and Electra #14535

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

terencechain
Copy link
Member

The following code was completely duplicated:

  • ProcessDeposits (moved to block package)
  • ProcessDeposit (moved to block package)
  • The only difference was in ApplyDeposit (moved to block package)

This PR consolidates ApplyDeposit between Altair and Electra to eliminate duplicated code. The ApplyDeposit function processes a deposit in the beacon chain by checking if a validator exists based on their public key. If the validator exists and the beacon state version is before Electra, it increases their balance. If the validator doesn't exist, it validates the deposit signature (unless all signatures have already been verified) and adds the validator to the registry if valid. For Electra and later versions, it processes deposits by appending them to the pending deposits list, without directly increasing balances.

@terencechain terencechain added the Ready For Review A pull request ready for code review label Oct 15, 2024
@terencechain terencechain marked this pull request as ready for review October 15, 2024 01:10
@terencechain terencechain requested a review from a team as a code owner October 15, 2024 01:10
@terencechain terencechain force-pushed the rm-electra-process-deposits branch 2 times, most recently from 28e6d53 to 94c1788 Compare October 15, 2024 01:35
// withdrawable_epoch=FAR_FUTURE_EPOCH,
// effective_balance=effective_balance,
// )
func GetValidatorFromDeposit(pubKey []byte, withdrawalCredentials []byte, amount uint64) *ethpb.Validator {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should have forking logic as it lives in the blocks package, it was updated in electra

@@ -23,5 +23,5 @@ func blockWithDeposit(ssz []byte) (interfaces.SignedBeaconBlock, error) {
}

func RunDepositTest(t *testing.T, config string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a doubt if we are making it common for all forks shouldn't we keep the tests for it inside blocks pkg only as it doesn't depends on version ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants