Skip to content

containers: mv produce_attestation to Validator - #188

Merged
tcoratger merged 5 commits into
leanEthereum:mainfrom
tcoratger:produce_attestation
Nov 25, 2025
Merged

containers: mv produce_attestation to Validator#188
tcoratger merged 5 commits into
leanEthereum:mainfrom
tcoratger:produce_attestation

Conversation

@tcoratger

Copy link
Copy Markdown
Collaborator

🗒️ Description

🔗 Related Issues or PRs

Part of #128

✅ Checklist

  • Ran tox checks to avoid unnecessary CI fails:
    uvx tox
  • Considered adding appropriate tests for the changes.
  • Considered updating the online docs in the ./docs/ directory.

@KolbyML KolbyML 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.

This PR feels weird, as the function is more dependent on Store, then it does to the validator_index.

Anyways I think this function should remain in Store, but instead of returning an Attestation, it returns AttestationData, then a validator_id/u64 isn't needed at all.

Then the function in Validator can construct the Attestation and take AttestationData as an argument.

I think this would achieve the goals of this PR, well removing the validator_id context as apart of Store

@tcoratger

Copy link
Copy Markdown
Collaborator Author

This PR feels weird, as the function is more dependent on Store, then it does to the validator_index.

Anyways I think this function should remain in Store, but instead of returning an Attestation, it returns AttestationData, then a validator_id/u64 isn't needed at all.

Then the function in Validator can construct the Attestation and take AttestationData as an argument.

I think this would achieve the goals of this PR, well removing the validator_id context as apart of Store

Just fixed, looks like a good idea, let me know what you think about it.

@KolbyML KolbyML 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.

:shipit: the PR looks a lot better,

Image ^ my only concern is I am not sure if this was supposed to be commited, but other then that no concerns

Comment thread tests/lean_spec/subspecs/forkchoice/test_validator.py.bak Outdated
@tcoratger
tcoratger merged commit 22f2e44 into leanEthereum:main Nov 25, 2025
10 checks passed
@tcoratger tcoratger mentioned this pull request Nov 25, 2025
5 tasks
@unnawut unnawut added this to the pq-devnet-1 milestone Nov 28, 2025
@unnawut unnawut added the specs Scope: Changes to the specifications label Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

specs Scope: Changes to the specifications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants