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

Fix remaining stale operator ID cases #1240

Merged
merged 12 commits into from
Mar 19, 2024
Merged

Conversation

nkryuchkov
Copy link
Contributor

@nkryuchkov nkryuchkov commented Dec 13, 2023

fix passing stale operator ID to

  • message validation
  • beacon client
  • fee recipient controller

This reverts commit 25ebc4f.
@nkryuchkov nkryuchkov marked this pull request as ready for review December 13, 2023 13:35
y0sher
y0sher previously approved these changes Dec 16, 2023
@y0sher
Copy link
Contributor

y0sher commented Dec 16, 2023

@moshe-blox

operator/node.go Outdated Show resolved Hide resolved
Copy link
Contributor

@moshe-blox moshe-blox 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, just 1 comment

…operator-id

# Conflicts:
#	network/p2p/p2p.go
#	operator/validator/controller.go
#	operator/validator/mocks/controller.go
MatusKysel
MatusKysel previously approved these changes Feb 13, 2024
Copy link
Contributor

@olegshmuelov olegshmuelov left a comment

Choose a reason for hiding this comment

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

following your changes, we can adjust

func (eh *EventHandler) validatorAddedEventToShare(
	event *contract.ContractValidatorAdded,
	shareEncryptionKeyProvider ShareEncryptionKeyProvider,
	operatorData *registrystorage.OperatorData,
	sharePublicKeys [][]byte,
	encryptedKeys [][]byte,
) (*ssvtypes.SSVShare, *bls.SecretKey, error)

in eventhandler pkg
to receive getOperatorId instead of operatorData

Copy link
Contributor

@olegshmuelov olegshmuelov left a comment

Choose a reason for hiding this comment

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

Should we merge stage into our current branch? There might be additional changes related to operatorID we haven't accounted for yet.

@y0sher
Copy link
Contributor

y0sher commented Mar 18, 2024

@nkryuchkov , lets fix @olegshmuelov 's comments and merge

@nkryuchkov
Copy link
Contributor Author

@olegshmuelov

Should we merge stage into our current branch? There might be additional changes related to operatorID we haven't accounted for yet.

I think we should. Done.

following your changes, we can adjust

Actually I think we don't even need to pass EventHandler's fields to its methods because they also have access to these fields. Simplified.

@nkryuchkov
Copy link
Contributor Author

@nkryuchkov , lets fix @olegshmuelov 's comments and merge

done

olegshmuelov
olegshmuelov previously approved these changes Mar 19, 2024
MatusKysel
MatusKysel previously approved these changes Mar 19, 2024
@y0sher y0sher requested review from moshe-blox and removed request for moshe-blox March 19, 2024 11:37
@y0sher y0sher dismissed moshe-blox’s stale review March 19, 2024 11:39

comments addressed.

@y0sher y0sher merged commit 97d6c4b into stage Mar 19, 2024
5 checks passed
@nkryuchkov nkryuchkov deleted the fix-remaining-stale-operator-id branch March 19, 2024 11:51
@nkryuchkov nkryuchkov mentioned this pull request Mar 29, 2024
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.

None yet

5 participants