-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add keystore service to standardcapabilities delegate #18274
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
Conversation
There was a problem hiding this 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 injects a keystore into the StandardCapabilities service and updates related delegates and tests to use live P2P keys (p2p/key_v2) instead of generated peer IDs, ensuring the testing node’s P2P key matches its peer ID.
- Added a
keystoreparameter toStandardCapabilitiesand wired it through delegate and application constructors. - Delegate now builds a
MultiAccountSignerfrom the P2P key and passes it to capabilities. - Integration tests and DON framework refactored to use
p2pkey.KeyV2and include OCR signer alongside peer IDs. - Updated various
go.modfiles to bumpchainlink-commonto the latest commit.
Reviewed Changes
Copilot reviewed 19 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| system-tests/tests/go.mod | Update chainlink-common to new commit |
| system-tests/lib/go.mod | Update chainlink-common to new commit |
| plugins/cmd/capabilities/log-event-trigger/main.go | Added keystore parameter to Initialise |
| integration-tests/load/go.mod | Update chainlink-common to new commit |
| integration-tests/go.mod | Update chainlink-common to new commit |
| go.mod | Update chainlink-common to new commit |
| deployment/go.mod | Update chainlink-common to new commit |
| core/services/workflows/cmd/cre/utils.go | Pass KeystoreMock to NewStandardCapabilities |
| core/services/standardcapabilities/standard_capabilities_test.go | Added keystoreMock and included in tests |
| core/services/standardcapabilities/standard_capabilities.go | Added keystore field and constructor parameter |
| core/services/standardcapabilities/delegate.go | Generate MultiAccountSigner and pass keystore |
| core/services/chainlink/application.go | Supply externalPeerWrapper into NewDelegate |
| core/scripts/go.mod | Update chainlink-common to new commit |
| core/capabilities/integration_tests/keystone/contracts_setup.go | Use GetPeerIDsAndOCRSigners() for signer extraction |
| core/capabilities/integration_tests/framework/peer.go | Refactor to peerIDAndOCRSigner and p2pkey usage |
| core/capabilities/integration_tests/framework/don_configuration.go | Replace peerIDs with p2pKeys in DonConfiguration |
| core/capabilities/integration_tests/framework/don.go | Update DON setup to use P2P keys and OCR signers |
| core/capabilities/integration_tests/framework/capabilities_registry.go | Pass raw peer ID bytes to AddDON |
| core/capabilities/fakes/standard_capabilities_mocks.go | Added KeystoreMock implementation |
Comments suppressed due to low confidence (5)
core/services/standardcapabilities/delegate.go:132
- [nitpick] The local variable
keystoreshadows the importedkeystorepackage (orcore.Keystoreinterface); consider renaming it to a more descriptive name, e.g.,multiAccountSigner.
keystore := core.NewMultiAccountSigner(accountIDs, signers)
core/capabilities/integration_tests/framework/peer.go:19
- [nitpick] The type name
peerIDAndOCRSigneris verbose and uses mixed casing; consider renaming it to something likeocrPeerInfoorPeerInfofor clarity.
type peerIDAndOCRSigner struct {
core/capabilities/integration_tests/framework/capabilities_registry.go:92
- [nitpick] The variable
peerIDscontains raw byte-form peer IDs; consider renaming it topeerIDBytesorpeerIDArraysto reflect its contents more accurately.
peerIDs := make([][32]byte, 0, len(donInfo.p2pKeys))
core/services/workflows/cmd/cre/utils.go:235
- [nitpick] Consider adding a comment explaining why a
KeystoreMockis passed here, to clarify how the keystore dependency is used in CRE loop tests.
&fakes.RelayerSetMock{}, &fakes.OracleFactoryMock{}, &fakes.KeystoreMock{})
core/services/standardcapabilities/standard_capabilities_test.go:39
- Add tests covering scenarios where multiple keystore accounts are provided to
NewStandardCapabilities, verifying that the multi-account signer behaves as expected.
standardCapability := NewStandardCapabilities(lggr, spec, pluginRegistrar, &telemetryServiceMock{}, &kvstoreMock{}, registry, &errorLogMock{}, &pipelineRunnerServiceMock{}, &relayerSetMock{}, &oracleFactoryMock{}, &keystoreMock{})
core/capabilities/integration_tests/keystone/contracts_setup.go
Outdated
Show resolved
Hide resolved
| ks: ks, | ||
| peerWrapper: peerWrapper, | ||
| externalPeerWrapper: externalPeerWrapper, | ||
| ocrPeerWrapper: ocrPeerWrapper, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vreff Why do you need the ocrPeerWrapper here? It should be enough to use the externalPeerWrapper 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't, but the OCR peer wrapper was just called "peerWrapper" so I've renamed it to add the distinction between the wrappers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question was more -- why are we adding the dependency? It didn't used to be there, so why do you need it now 🤔 ?
EDIT: sorry I see what you did now, you actually added the externalPeerWrapper
| if d.ks.P2P() != nil && d.externalPeerWrapper != nil { | ||
| key, err := d.ks.P2P().GetOrFirst(p2pkey.PeerID(d.externalPeerWrapper.GetPeer().ID())) | ||
| if err != nil { | ||
| log.Warnw("Failed to get P2P key", "error", err, "peerID", d.externalPeerWrapper.GetPeer().ID()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically should never happen right? If so I would bump the Warn to an Error and make it clear that this is an invariant violation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, we should never have an external peer wrapper without possession of an underlying P2P key.
| signers = append(signers, key) | ||
| } | ||
| } | ||
| keystore := core.NewMultiAccountSigner(accountIDs, signers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vreff Can you explain more what your intention is here? Do you need access to multiple keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The existing Keystore interface I am using supports multiple accounts. While this PR only adds one account (P2P key), using an underlying struct that supports multiple accounts may easily permit future capability authors to use other keys.
That being said, the Keystore interface itself already allows this so really I don't need the implementation of that interface to overtly support multiple accounts at present.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>
* Add keystore service to standardcapabilities delegate * Update chainlink-common version, fix trigger initialise func * fix nil pointer * fix lint * update gomodtidies * add p2p key use * undo removal of function * Update core/capabilities/integration_tests/keystone/contracts_setup.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * lowercase getSignerStringFromOCRKeyBundle * add error for no p2p key * fix merge conflict * lint * add changeset * add error check * lint * lint * lint * lint * Update chainlink-common, use single account signer. * resolve merge conflict * resolve merge conflict 2 * resolve merge conflict 2 * Fixes chainlink version in CI --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Hamrick <adam.hamrick@smartcontract.com>




Initialisefunction.integration_test/framework/to usep2p/key_v2rather than generating its own peerIDs, and adds the p2pkey to the node being tested. This allows the testing node to have a live p2p key running on it that matches its peerID.