Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(fix): Enforce Consensus Capability config field key_id #892

Merged
merged 16 commits into from
Nov 6, 2024
Merged

(fix): Enforce Consensus Capability config field key_id #892

merged 16 commits into from
Nov 6, 2024

Conversation

justinkaseman
Copy link
Contributor

No description provided.

@justinkaseman justinkaseman requested review from a team as code owners October 25, 2024 00:03
bolekk
bolekk reviewed Oct 25, 2024
View reviewed changes
pkg/capabilities/consensus/ocr3/models.go
@@ -12,7 +12,7 @@ type config struct {
ReportID string `mapstructure:"report_id" json:"report_id" jsonschema:"required,pattern=^[a-f0-9]{4}$"`
RequestTimeoutMS int64 `mapstructure:"request_timeout_ms" json:"request_timeout_ms"`

KeyID string `mapstructure:"key_id" json:"key_id"`
KeyID string `mapstructure:"key_id" json:"key_id,omitempty" jsonschema:"required"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from this, we should also check if the desired key ID is available, probably somewhere very early in the consensus step. I'm not sure if that's easy without some refactoring...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say if the desired key ID is available, we would use this key ID from the workflow spec, where would we be checking against?

Copy link
Contributor

Choose a reason for hiding this comment

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

Against all possible bundle IDs available in OCR3OnchainKeyringMultiChainAdapter (essentially in Keystore). That's why it might be a bit trickier because I don't think we have easy access to that data where we need to run a check... but maybe I'm wrong

@justinkaseman justinkaseman merged commit 3b320ad into main Nov 6, 2024
11 checks passed
@justinkaseman justinkaseman deleted the CAPPL-198 branch November 6, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agparadiso agparadiso agparadiso approved these changes

@bolekk bolekk bolekk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justinkaseman @bolekk @agparadiso