Skip to content

SimpleSigner: change network to interface#106

Merged
y0sher merged 1 commit intomasterfrom
simple-signer-network-interface
May 26, 2025
Merged

SimpleSigner: change network to interface#106
y0sher merged 1 commit intomasterfrom
simple-signer-network-interface

Conversation

@nkryuchkov
Copy link
Copy Markdown
Contributor

Changes:

  • SimpleSigner expects an interface type for the network parameter instead of core.Network

ssvlabs/ssv#1308 introduces support of custom network configs where any value can be overridden. Currently, SimpleSigner expects the network parameter of the type core.Network which means that it supports only values stored in this repository. On any other values, it returns default zero-values or crashes.

Therefore, when the SSV node calls NewSimpleSigner on setting up a KeyManager, if the network name differs from supported, then SimpleSigner would not work as expected.

The SimpleSigner uses two core.Network's methods:

  • EstimatedEpochAtSlot(slot phase0.Slot) phase0.Epoch
  • EstimatedSlotAtTime(time int64) phase0.Slot

EstimatedEpochAtSlot calls only SlotsPerEpoch which returns 32 for any network. However, EstimatedSlotAtTime calls MinGenesisTime which returns 0 for unknown ones.

The PR changes the type of the network parameter to the interface below to allow the SSV node to just pass any network matching the interface.

type network interface {
	EstimatedEpochAtSlot(slot phase0.Slot) phase0.Epoch
	EstimatedSlotAtTime(time int64) phase0.Slot
}

olegshmuelov
olegshmuelov previously approved these changes Mar 19, 2024
y0sher
y0sher previously approved these changes Mar 9, 2025
@nkryuchkov nkryuchkov force-pushed the simple-signer-network-interface branch from 62088d8 to 079eb6f Compare March 21, 2025 00:47
@nkryuchkov nkryuchkov changed the title Change network to interface in SimpleSigner SimpleSigner: change network to interface Mar 21, 2025
iurii-ssv
iurii-ssv previously approved these changes Apr 17, 2025
Copy link
Copy Markdown

@iurii-ssv iurii-ssv left a comment

Choose a reason for hiding this comment

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

Minor suggestions

Comment thread signer/far_future_protection.go Outdated
Comment thread signer/sign_attestation_test.go
oleg-ssvlabs
oleg-ssvlabs previously approved these changes Apr 17, 2025
@y0sher
Copy link
Copy Markdown
Contributor

y0sher commented Apr 17, 2025

@nkryuchkov @oleg-ssvlabs I have actually fixed a bug in this pr in this branch debug/simple-signer-network-interface we should probably pull them here.

@oleg-ssvlabs
Copy link
Copy Markdown
Contributor

@nkryuchkov @oleg-ssvlabs I have actually fixed a bug in this pr in this branch debug/simple-signer-network-interface we should probably pull them here.

do you mean in62dfd67eb8841354eb63ea8f316c6e4e5fced013 commit ?

@nkryuchkov
Copy link
Copy Markdown
Contributor Author

nkryuchkov commented Apr 17, 2025

@nkryuchkov @oleg-ssvlabs I have actually fixed a bug in this pr in this branch debug/simple-signer-network-interface we should probably pull them here.

@y0sher What bug does it fix?

y0sher
y0sher previously approved these changes May 8, 2025
oleg-ssvlabs
oleg-ssvlabs previously approved these changes May 8, 2025
Comment thread signer/validator_signer.go Outdated
moshe-blox
moshe-blox previously approved these changes May 8, 2025
Copy link
Copy Markdown
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.

lgtm 👌

@y0sher y0sher dismissed stale reviews from moshe-blox, oleg-ssvlabs, and themself via d42891d May 8, 2025 12:26
moshe-blox
moshe-blox previously approved these changes May 8, 2025
oleg-ssvlabs
oleg-ssvlabs previously approved these changes May 8, 2025
MatusKysel
MatusKysel previously approved these changes May 9, 2025
@MatusKysel MatusKysel requested a review from Copilot May 9, 2025 06:30
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the SimpleSigner component to accept a more flexible network interface, allowing custom network configurations to be passed in instead of being restricted to the pre-defined core.Network.

  • Changed the network parameter type in SimpleSigner and related functions from core.Network to a new Network interface.
  • Updated time handling in network functions and tests by replacing Unix timestamps with time.Time values and using time.Add for time deltas.
  • Updated tests in signer and core packages to accommodate the new Network interface and time representations.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
signer/validator_signer.go Updated SimpleSigner to use the new Network interface with time.Time-based methods.
signer/sign_beacon_block_test.go Updated time parameter usage for far future validation tests.
signer/sign_attestation_test.go Updated time parameter usage in attestation tests.
signer/far_future_protection.go Refactored far future protection to use time.Time and updated delta variable.
core/networks_test.go Updated tests to use time.Time in slot estimation.
core/networks.go Modified the EstimatedSlotAtTime method signature and logic to use time.Time instead of Unix timestamps.

Comment thread signer/validator_signer.go
Comment thread core/networks.go
iurii-ssv
iurii-ssv previously approved these changes May 12, 2025
y0sher
y0sher previously approved these changes May 25, 2025
@nkryuchkov nkryuchkov force-pushed the simple-signer-network-interface branch from c4795fa to 9389c39 Compare May 26, 2025 12:59
@y0sher y0sher merged commit ed799db into master May 26, 2025
4 checks passed
@MatusKysel MatusKysel deleted the simple-signer-network-interface branch May 26, 2025 13:14
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.

9 participants