Use bootstrap.JobSpec in apps and devenv.#1007
Conversation
1e1ba22 to
08e0c50
Compare
| GeneratedJobSpecs []string `toml:"-"` | ||
| GeneratedJobSpecs []bootstrap.JobSpec `toml:"-"` |
There was a problem hiding this comment.
Strong types allow the compiler to help detect incompatible parameters.
It also avoids superfluous calls to marshal/unmarshal the same string.
| // TODO: Use bootstrap.JobSpec in CLD to avoid this conversion here | ||
| var executorSpec ExecutorJobSpec | ||
| { | ||
| _, err = toml.Decode(job.Spec, &executorSpec) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode verifier job spec for %s: %w", exec.ContainerName, err) | ||
| } | ||
| executorJobSpecs[exec.ContainerName] = executorSpec.ToBootstrapJobSpec() | ||
| } |
There was a problem hiding this comment.
Allow the devenv to use bootstrap.JobSpec unconditionally by converting away from the other type immediately after calling ccipOffchain.GetJob.
| // TODO: Use bootstrap.JobSpec in CLD to avoid this conversion here | ||
| var verifierJobSpec VerifierJobSpec | ||
| if _, err := toml.Decode(job.Spec, &verifierJobSpec); err != nil { | ||
| return nil, fmt.Errorf("failed to decode verifier job spec for %s: %w", ver.ContainerName, err) | ||
| } | ||
|
|
||
| allJobSpecs = append(allJobSpecs, verifierJobSpec.ToBootstrapJobSpec()) |
There was a problem hiding this comment.
Same as above, allow the devenv to use bootstrap.JobSpec unconditionally by converting away from the other type immediately after calling ccipOffchain.GetJob.
| chainsel.FamilyEVM, | ||
| deprecatedCreateAccessorFactory), | ||
| bootstrap.WithLogLevel[commit.JobSpec](zapcore.InfoLevel), | ||
| bootstrap.WithLogLevel[bootstrap.JobSpec](zapcore.InfoLevel), |
There was a problem hiding this comment.
Standalone mode now uses bootstrap.JobSpec for all JD-Mode apps, I'll be able to remove generics in a followup PR.
| // blockchain.Info for EVM). Strict decode is applied: any unknown key in the config | ||
| // (including under blockchain_infos.<selector>) causes an error. | ||
| func LoadConfigWithBlockchainInfos[T any](spec JobSpec) (*Config, chainaccess.Infos[T], error) { | ||
| func LoadConfigWithBlockchainInfos[T any](cfg string) (*Config, chainaccess.Infos[T], error) { |
There was a problem hiding this comment.
It should be possible to remove all references to BlockchainInfos from the "library" apps in a followup.
This reverts commit 3f2041a.
31184fb to
a801144
Compare
|
Code coverage report:
|
There was a problem hiding this comment.
Pull request overview
This PR standardizes JD job spec handling by replacing app-specific VerifierJobSpec / ExecutorJobSpec types with a shared bootstrap.JobSpec, so bootstrapped services can consume a consistent outer spec while keeping per-app config in AppConfig.
Changes:
- Introduces
bootstrap.JobSpecand updates executor/verifier bootstrapped service factories to accept it. - Refactors executor/verifier config loaders to decode directly from the inner config string (
spec.AppConfig) instead of app-specific job spec wrappers. - Updates devenv job generation/proposal flow to convert legacy job specs into
bootstrap.JobSpecand injectblockchain_infosintoAppConfig.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
bootstrap/job.go |
Adds the shared bootstrap.JobSpec type used across bootstrapped apps. |
verifier/pkg/commit/load.go |
Changes config loading API to accept inner TOML config string. |
verifier/pkg/commit/load_test.go |
Updates tests to call the new config loading signature. |
verifier/pkg/commit/job.go |
Removes verifier-specific job spec wrapper type. |
verifier/cmd/servicefactory.go |
Switches verifier bootstrap factory to bootstrap.JobSpec and reads spec.AppConfig. |
verifier/cmd/committee/main.go |
Updates bootstrap runner options to use bootstrap.JobSpec. |
executor/load.go |
Changes config loading API to accept inner TOML config string. |
executor/job.go |
Removes executor-specific job spec wrapper type. |
cmd/executor/servicefactory.go |
Switches executor bootstrap factory to bootstrap.JobSpec and reads spec.AppConfig. |
build/devenv/services/executor/base.go |
Updates job spec rebuilding to operate on bootstrap.JobSpec and rewrite AppConfig. |
build/devenv/services/committeeverifier/base.go |
Updates verifier job spec rebuilding to operate on bootstrap.JobSpec and rewrite AppConfig. |
build/devenv/environment.go |
Updates job spec generation/proposal plumbing to use bootstrap.JobSpec and introduces conversion helpers. |
integration/pkg/accessors/evm/evm_source_reader.go |
Adds raw message bytes to decode-failure log fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| md, err := toml.Decode(job.Spec, &executorSpec) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode verifier job spec for %s: %w", exec.ContainerName, err) |
There was a problem hiding this comment.
The returned error message says "failed to decode verifier job spec" but this code path is decoding an executor job spec. This makes troubleshooting confusing; update the error text to refer to the executor job spec (and keep terminology consistent with the surrounding errors).
| return nil, fmt.Errorf("failed to decode verifier job spec for %s: %w", exec.ContainerName, err) | |
| return nil, fmt.Errorf("failed to decode executor job spec for %s: %w", exec.ContainerName, err) |
| L.Warn(). | ||
| Str("spec", job.Spec). | ||
| Str("undecoded fields", fmt.Sprintf("%v", md.Undecoded())). | ||
| Msg("Undecoded fields in executor job spec") |
There was a problem hiding this comment.
This warning message is emitted while decoding a verifier job spec, but the log text says "Undecoded fields in executor job spec". Update the log message to reference the verifier job spec so on-call/debugging can attribute the problem correctly.
| Msg("Undecoded fields in executor job spec") | |
| Msg("Undecoded fields in verifier job spec") |
| if err != nil { | ||
| r.lggr.Errorw("Failed to decode message", "error", err) | ||
| r.lggr.Errorw("Failed to decode message", "error", err, "rawMessage", event.EncodedMessage) | ||
| continue // to next message |
There was a problem hiding this comment.
Logging the full raw encoded message bytes on decode failure can significantly bloat logs and may leak sensitive payloads. Consider logging a safe representation instead (e.g., length + a truncated hex prefix or a hash) to keep diagnostics useful without dumping the entire message.
Convert
VerifierJobSpecandExecutorJobSpecto a genericbootstrap.JobSpecthat can be used on all app types.This will simplify the AccessorRegistry by avoiding generics with config types and allowing shared config handling.