Skip to content

ssvsigner: break circular dependency with ssv-node#2710

Open
nkryuchkov wants to merge 20 commits intostagefrom
ssvsigner-circ-dep
Open

ssvsigner: break circular dependency with ssv-node#2710
nkryuchkov wants to merge 20 commits intostagefrom
ssvsigner-circ-dep

Conversation

@nkryuchkov
Copy link
Contributor

@nkryuchkov nkryuchkov commented Mar 3, 2026

as per title

@codecov
Copy link

codecov bot commented Mar 3, 2026

Codecov Report

❌ Patch coverage is 38.19742% with 144 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.8%. Comparing base (20ffb01) to head (c696e7a).

Files with missing lines Patch % Lines
ssvsigner/internal/testdb/testdb.go 0.0% 80 Missing ⚠️
ekmadapter/database.go 0.0% 41 Missing ⚠️
ssvsigner/internal/beaconcfg/config.go 77.2% 7 Missing and 3 partials ⚠️
cli/operator/node.go 0.0% 8 Missing ⚠️
ssvsigner/cmd/purge-keys/purge-keys.go 0.0% 5 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nkryuchkov
Copy link
Contributor Author

@greptileai please review the PR

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Greptile Summary

This PR successfully completes the work of breaking the circular import dependency between ssvsigner and the root ssv module, which previously required a replace github.com/ssvlabs/ssv => ../ directive in ssvsigner/go.mod.

Core architectural changes:

  • Introduces ssvsigner/ekm/contracts.go with minimal Database, ReadTxn, ReadWriteTxn, BeaconNetwork, and NetworkContext interfaces — all free of root-module types.
  • Adds ekmadapter/database.go in the root module to bridge basedb.Databaseekm.Database, with compile-time guards confirming basedb.Txn satisfies both transaction interfaces.
  • Extracts ssvsigner/internal/beaconcfg as a self-contained beacon configuration (duplicating only the fields ekm actually needs) and ssvsigner/internal/testdb as a lightweight JSON-backed test store replacing Badger in ssvsigner tests.
  • Adds scripts/ssvsigner_boundary.sh (hooked into the default make lint target) to enforce the import boundary going forward.

Side effects:

  • Several heavyweight transitive dependencies (Badger, libp2p, Prometheus, opencensus, etc.) are removed from ssvsigner's dependency graph, significantly reducing its build surface.
  • Log field names in ssvsigner/client.go changed (fields.Countzap.Int("count", …), fields.Tookzap.Duration("took", …), fields.PubKeyzap.Stringer("share_pubkey", …)); any log-parsing tooling targeting the old key names will need updating.

Confidence Score: 4/5

  • The architectural changes are clean and well-tested with compile-time guards and boundary enforcement in place. The refactoring correctly preserves all semantics from the previous implementation.
  • The PR implements a significant architectural refactoring to eliminate a circular dependency. The new interfaces are minimal and correctly designed, with compile-time guards confirming basedb.Txn satisfies both transaction interfaces. The boundary enforcement script prevents regression. File modifications across ssvsigner and the root module are internally consistent and properly tested.
  • No files require special attention. The main architectural components (contracts.go, ekmadapter/database.go, beaconcfg, and testdb) are well-designed and thoroughly tested.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph root["Root SSV Module"]
        node["cli/operator/node.go"]
        adapter["ekmadapter\nNewDatabaseAdapter(basedb.Database)"]
        basedb["storage/basedb\n(Database, Txn, ReadTxn)"]
        node --> adapter
        adapter --> basedb
    end

    subgraph ssvsigner["ssvsigner Module (separate go.mod)"]
        contracts["ekm/contracts.go\n(Database, ReadTxn, ReadWriteTxn,\nBeaconNetwork, NetworkContext)"]
        lkm["ekm/LocalKeyManager"]
        rkm["ekm/RemoteKeyManager"]
        storage["ekm/SignerStorage"]
        protector["ekm/SlashingProtector"]
        beaconcfg["internal/beaconcfg/Config"]
        testdb["internal/testdb/Store\n(tests only)"]

        lkm --> contracts
        rkm --> contracts
        storage --> contracts
        protector --> contracts
        beaconcfg -->|implements BeaconNetwork| contracts
    end

    node -->|NetworkContext\nbeacon = networkConfig.Beacon| contracts
    adapter -->|implements ekm.Database| contracts

    classDef new fill:#d4edda,stroke:#28a745
    class adapter,contracts,beaconcfg,testdb new
Loading

Last reviewed commit: c696e7a

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

39 files reviewed, 6 comments

Edit Code Review Agent Settings | Greptile

@nkryuchkov
Copy link
Contributor Author

@greptileai review this again, considering all comments on this PR

@nkryuchkov
Copy link
Contributor Author

@greptileai review this again, considering all comments on this PR

@nkryuchkov nkryuchkov marked this pull request as ready for review March 4, 2026 10:04
@nkryuchkov nkryuchkov requested review from a team as code owners March 4, 2026 10:04
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.

1 participant