[CCIP Merge] Capabilities fix [CCIP-2943] #14048
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asoliman92
changed the title
asoliman92
force-pushed
the
ccip/capabilities-merge-fix
branch
from
afb93d7 to
b089224
Compare
asoliman92
force-pushed
the
ccip/capabilities-directory-only
branch
from
7f010e1 to
7d44b17
Compare
asoliman92
force-pushed
the
ccip/capabilities-merge-fix
branch
from
b089224 to
23dea4c
Compare
makramkd
requested changes
Aug 6, 2024
| capabilityLabelledName string, | ||
| state registrysyncer.State, | ||
| ) (kcr.CapabilitiesRegistryCapabilityInfo, error) { | ||
| capabilityID string, |
There was a problem hiding this comment.
Feels less error prone to make this structured ID an actual structure rather than just a string.
@cedric-cordenier wdyt? Maybe something like:
type CapabilityID struct {
Name string
Version string
}
func (cid CapabilityID) String() string {
// perhaps some semver regex validation using the semver package
return fmt.Sprintf("%s@%s", cid.Name, cid.Version)
}There was a problem hiding this comment.
I said a similar comment on call with @cedric-cordenier last week as well :)
There was a problem hiding this comment.
This should not be a blocker for this PR though.
There was a problem hiding this comment.
Yes, I'm not against this 👍
There was a problem hiding this comment.
@asoliman92 can you create a ticket in the backlog for this if you're not going to do it in this PR
Base automatically changed from
ccip/capabilities-directory-only
to
ccip/capabilities-merge-clean
August 6, 2024 14:34
Make application.go ready for adding more fixes
asoliman92
force-pushed
the
ccip/capabilities-merge-fix
branch
from
9cbff85 to
51e8ad9
Compare
asoliman92
force-pushed
the
ccip/capabilities-merge-fix
branch
from
213a58f to
e35df28
Compare
We want to define and use the appropriate OCR offchain config for each plugin. Requires smartcontractkit/chainlink-ccip#36
makramkd
reviewed
Aug 7, 2024
core/capabilities/ccip/delegate.go
Outdated
| @@ -99,6 +106,25 @@ func (d *Delegate) ServicesForSpec(ctx context.Context, spec job.Job) (services | |||
| return nil, fmt.Errorf("failed to get all p2p keys: %w", err) | |||
| } | |||
|
|
|||
| cfg := d.capabilityConfig | |||
| rid := cfg.ExternalRegistry().RelayID() | |||
| registryAddress := cfg.ExternalRegistry().Address() | |||
Comment on lines
27
to
28
| var defaultCapability = getCapability(ccipCapName, ccipCapVersion) | ||
| var newCapability = getCapability(ccipCapName, ccipCapNewVersion) |
There was a problem hiding this comment.
Merge these var decls into one var decl
Comment on lines
44
to
53
| var p2pID1 = getP2PID(1) | ||
| var p2pID2 = getP2PID(2) | ||
|
|
||
| var defaultCapCfgs = map[string]registrysyncer.CapabilityConfiguration{ | ||
| defaultCapability.ID: registrysyncer.CapabilityConfiguration{}, | ||
| } | ||
| var defaultRegistryDon = registrysyncer.DON{ | ||
| DON: getDON(1, []ragep2ptypes.PeerID{p2pID1}, 0), | ||
| CapabilityConfigurations: defaultCapCfgs, | ||
| } |
There was a problem hiding this comment.
Merge into one var decl at the top of the file for better organization
Comment on lines
19
to
20
| ccipreader "github.com/smartcontractkit/chainlink-ccip/pkg/reader" | ||
| ccipreaderpkg "github.com/smartcontractkit/chainlink-ccip/pkg/reader" |
| @@ -46,6 +50,42 @@ type launcher struct { | |||
| subServices []services.Service | |||
| } | |||
|
|
|||
| func unmarshalCapabilityConfig(data []byte) (capabilities.CapabilityConfiguration, error) { | |||
There was a problem hiding this comment.
Which capability config is this unmarshalling? Not CCIP right?
There was a problem hiding this comment.
Not CCIP AFAIU. @cedric-cordenier right?
There was a problem hiding this comment.
Yeah this is only unmarshaling the Keystone capability configs
| @@ -17,6 +17,8 @@ import ( | |||
| "go.uber.org/multierr" | |||
| "go.uber.org/zap/zapcore" | |||
|
|
|||
| "github.com/smartcontractkit/chainlink/v2/core/capabilities/ccip" | |||
There was a problem hiding this comment.
Nit: group this with other chainlink/v2/ imports?
asoliman92
force-pushed
the
ccip/capabilities-merge-fix
branch
from
a58854b to
c495985
Compare
makramkd
approved these changes
Aug 7, 2024
|
asoliman92
merged commit ba0e8c5
into
ccip/capabilities-merge-clean
128 of 129 checks passed
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 7, 2024
* [CCIP Merge] Add ccip capabilities directory [CCIP-2943] (#14044) * Add ccip capabilities directory * [CCIP Merge] Capabilities fix [CCIP-2943] (#14048) * Fix compilation for launcher, diff Make application.go ready for adding more fixes * Fix launcher tests * ks-409 fix the mock trigger to ensure events are sent (#14047) * Add ccip to job orm * Add capabilities directory under BUSL license * Prep to instantiate separate registrysyncer for CCIP * Move registrySyncer creation into ccip delegate * [chore] Change registrysyncer config to bytes * Fix launcher diff tests after changing structs in syncer * Fix linting * MAke simulated backend client work with chains other than 1337 * core/capabilities/ccip: use OCR offchain config (#1264) We want to define and use the appropriate OCR offchain config for each plugin. Requires smartcontractkit/chainlink-ccip#36 * Cleaning up * Add capabilities types to mockery --------- Co-authored-by: Cedric Cordenier <cedric.cordenier@smartcontract.com> Co-authored-by: Matthew Pendrey <matthew.pendrey@gmail.com> Co-authored-by: Makram <makramkd@users.noreply.github.com> * make modgraph * Add changeset * Fix test with new TxMgr constructor --------- Co-authored-by: Cedric Cordenier <cedric.cordenier@smartcontract.com> Co-authored-by: Matthew Pendrey <matthew.pendrey@gmail.com> Co-authored-by: Makram <makramkd@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixing ccip capabilities to be merged into chainlink