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

Alan - Spec alignment #1511

Merged
merged 8 commits into from
Aug 7, 2024
Merged

Alan - Spec alignment #1511

merged 8 commits into from
Aug 7, 2024

Conversation

MatheusFranco99
Copy link
Contributor

Overview

This PR updates the approved changes between ssv and spec, for the spec-alignment script.

Also, it performs a single alignment change.

@MatheusFranco99 MatheusFranco99 self-assigned this Jul 25, 2024
@liorrutenberg liorrutenberg changed the base branch from alan/no-fork to stage July 28, 2024 10:50
@nkryuchkov nkryuchkov force-pushed the alan/spec-alignment branch from 5481828 to 93ee563 Compare July 31, 2024 11:09
@moshe-blox
Copy link
Contributor

image
is this not a significant diff?

@moshe-blox
Copy link
Contributor

image
do we still need the validator stopping logic in the implementation?

@moshe-blox
Copy link
Contributor

moshe-blox commented Aug 7, 2024

image
should return error in implementation (and probably without attester)

@moshe-blox
Copy link
Contributor

moshe-blox commented Aug 7, 2024

image
is this not a significant change?

i think the implementation is sorting commit signers here to avoid creating duplicates, however that should probably also happen in spec if deemed needed, or likely removed from implementation if not.

@MatheusFranco99
Copy link
Contributor Author

@moshe-blox

is this not a significant diff?

(Regarding the msgID attributes) No. We don't have DomainTypeProvider in spec and the CommitteeID is, actually, better as implemented in the node. Although they result the same, if one needs to change, it should be spec.

do we still need the validator stopping logic in the implementation?

This is outside of the scope of spec. And it seems that it's being used in the node.

should return error in implementation (and probably without attester)

I've raised this issue before (this is the function that is never called, remember?). I agree we can change to DomainError and return the error. May I add that?

is this not a significant change? i think the implementation is sorting commit signers here to avoid creating duplicates, however that should probably also happen in spec if deemed needed, or likely removed from implementation if not.

This difference already existed before. Since the problem is in spec, I preferred approving it here and creating the issue in spec. What do you think?

@moshe-blox
Copy link
Contributor

moshe-blox commented Aug 7, 2024

I agree we can change to DomainError and return the error. May I add that?

yes please

This difference already existed before. Since the problem is in spec, I preferred approving it here and creating the issue in spec. What do you think?

+1

agreed on the rest

@liorrutenberg liorrutenberg merged commit 1224419 into stage Aug 7, 2024
4 of 5 checks passed
@liorrutenberg liorrutenberg deleted the alan/spec-alignment branch August 7, 2024 13:50
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