Aptos p2p map to cap reg (generalising)#21672
Aptos p2p map to cap reg (generalising)#21672yashnevatia wants to merge 1 commit intoaptos-p2p-map-to-capRegfrom
Conversation
yashnevatia
changed the title
product-security-plaid-production
bot
requested review from
DimitriosNaikopoulos and
michael-cll
|
👋 yashnevatia, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM — introduces new per-DON config enrichment logic (Aptos-specific) that depends on Job Distributor metadata and mutates capability configs prior to on-chain DON updates.
This PR adds an enrichment layer to the capabilities-registry v2 “add capabilities” flow so Aptos capabilities can automatically inject a p2pToTransmitterMap into specConfig based on JD node metadata.
Changes:
- Add a per-DON capability-config enrichment step in the
AddCapabilitiessequence (used beforeUpdateDON). - Introduce a new
enricherpackage with an Aptos enricher that builds and injectsspecConfig.p2pToTransmitterMap. - Add an integration-style test ensuring the Aptos
specConfiginjection lands on-chain and can be decoded.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
deployment/cre/capabilities_registry/v2/changeset/sequences/add_capabilities.go |
Clones per-DON capability configs and runs enrichers before updating the DON. |
deployment/cre/capabilities_registry/v2/changeset/enricher/enricher.go |
New enrichment framework + Aptos implementation that pulls transmit accounts from JD and injects specConfig. |
deployment/cre/capabilities_registry/v2/changeset/add_capabilities_test.go |
Adds Aptos apply test asserting specConfig.p2pToTransmitterMap presence after Apply. |
Scrupulous human review recommended for:
buildAptosP2PToTransmitterMap/mergeAptosP2PToTransmitterIntoConfigcorrectness and completeness guarantees (especially around missing nodes / partial JD responses).- The sequencing in
AddCapabilitiesto avoid partially-applied state in the non-MCMS (immediate execution) strategy.
Suggested reviewers (per .github/CODEOWNERS for /deployment/cre):
@smartcontractkit/keystone@smartcontractkit/operations-platform
Comments suppressed due to low confidence (5)
deployment/cre/capabilities_registry/v2/changeset/enricher/enricher.go:19
- The import block isn’t gofmt-compliant (standard library imports are split and not sorted, with extra blank lines). This will typically fail gofmt/lint checks—please run gofmt so imports are grouped/sorted correctly.
deployment/cre/capabilities_registry/v2/changeset/enricher/enricher.go:24 - The doc comment for
CapabilityConfigEnrichContextstarts with a different type name ("DonCapabilityEnrichContext"), so it won’t be associated correctly by Go doc tooling. Please update the comment to start withCapabilityConfigEnrichContext(and keep the wording consistent with the type name).
deployment/cre/capabilities_registry/v2/changeset/enricher/enricher.go:84 - The doc comment for
parseAptosChainSelectorFromCapabilityIDbegins withParseAptos..., but the function is unexported and namedparseAptos.... This mismatch makes the comment misleading and can trigger doc/lint issues; please align the comment prefix with the actual function name (or export the function if you intend it to be public).
deployment/cre/capabilities_registry/v2/changeset/enricher/enricher.go:141 deployment.NodeInfois documented as “optimistic” and may return only the subset of nodes that exist in JD. This function currently builds the p2p→transmitter map from whatever is returned, which can silently omit entries for DON peer IDs that weren’t returned. That would produce an incompletep2pToTransmitterMapand likely misconfigure the DON. Please add an explicit validation that all requesteddonPeerIDsare present in the NodeInfo result (and return a clear error listing missing peer IDs).
deployment/cre/capabilities_registry/v2/changeset/enricher/enricher.go:57- The error message here hard-codes the caller name ("AddCapabilities") even though the check lives in the enricher layer, and it doesn’t include the DON name/capability ID. Consider rewording to be caller-agnostic and include relevant context (e.g., DON + capability) so failures are easier to diagnose.
| donCapabilityConfigs := cloneCapabilityConfigsShallowCopyConfigMaps(input.CapabilityConfigs) | ||
| enrichCtx := enricher.CapabilityConfigEnrichContext{ | ||
| Env: deps.Env, | ||
| DonName: donName, | ||
| P2PIDs: p2pIDs, | ||
| Configs: donCapabilityConfigs, | ||
| } | ||
| for _, e := range enricher.DefaultCapabilityConfigEnrichers() { | ||
| if err := e.Enrich(enrichCtx); err != nil { | ||
| return AddCapabilitiesOutput{}, fmt.Errorf("enrich capability configs for DON %s: %w", donName, err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new enrichment step can fail after UpdateNodes has already executed (in the non-MCMS/simple strategy path), which would leave the registry in a partially-updated state (nodes updated, DON config not). To reduce this risk, consider performing capability-config enrichment before executing contracts.UpdateNodes, since enrichment only depends on Env, donName, and the DON’s p2pIDs (already available here).
|
Requires
Supports