offchain - remove dependency on pg.qopts #629
Conversation
| @@ -332,13 +332,13 @@ func (d *Delegate) cleanupEVM(jb job.Job, q pg.Queryer, relayID relay.ID) error | |||
| d.lggr.Errorw("failed to derive ocr2keeper filter names from spec", "err", err, "spec", spec) | |||
| } | |||
| case types.CCIPCommit: | |||
| err = ccipcommit.UnregisterCommitPluginLpFilters(context.Background(), d.lggr, jb, d.legacyChains, pg.WithQueryer(q)) | |||
There was a problem hiding this comment.
Is it important to preserve this Queryer? i.e. is it a transaction?
There was a problem hiding this comment.
Would be nice to have but I wouldn't say it's important internally in ccip.
That's part of the transaction opened at:
ocr2: OnDeleteJob(jb job.Job, q pg.Queryer)
delegate: cleanup EVM(jb job.Job, <<q pg.Queryer>>, relayID relay.ID) error
Is there a way to keep it?
There was a problem hiding this comment.
Yes, the new helpers support the old QOpts behavior one way or another. This one may not be ready yet though. We have some PRs in progress.
There was a problem hiding this comment.
After sync with @PiotrTrzpil seems that this is required by CLO to preserve it.
Do we already support this? How can I get a ctx instance.
There was a problem hiding this comment.
Are you up to date with develop? You may have to wait for me to make more progress on https://smartcontract-it.atlassian.net/browse/BCF-2978, but it looks like the EVM side should be done? https://smartcontract-it.atlassian.net/browse/BCI-2644
I'm currently working on this PR: smartcontractkit/chainlink#12456
There was a problem hiding this comment.
No we haven't synced our fork with this updates, probably this task gets blocked until we do.
| @@ -242,8 +242,8 @@ func (c *CommitStore) ChangeConfig(onchainConfig []byte, offchainConfig []byte) | |||
| return cciptypes.Address(onchainConfigParsed.PriceRegistry.String()), nil | |||
| } | |||
|
|
|||
| func (c *CommitStore) Close(qopts ...pg.QOpt) error { | |||
| return logpollerutil.UnregisterLpFilters(c.lp, c.filters, qopts...) | |||
| func (c *CommitStore) Close() error { | |||
There was a problem hiding this comment.
Are we okay with these changes considering all the past issues with CLO? Can we end up in some weird state in which some filters are registered and some not in case of failing to approve job spec?
How about something like this
- Feeds tries to Approve the job spec
- It starts with deleting existing job spec and unregistering LogPoller filters
- It wants to create a new plugin instance but then something fails (e.g. flaky RPC in commit/exec init logic)
We end up with a job half-approved. Previous job definition will still be present in the database, because of db tx, but LogPoller filters are already gone because they are unregistered/registered separately.
There was a problem hiding this comment.
We do not intend to change behavior when moving away from pg.QOpts. We have equivalent support for every feature.
|
will open a new PR |
The core repo has removed the concept of
pg.Optsfor enabling EVM extraction into it’s own repo. CCIP was keep usingpg.Opts, specifically when creating jobs.This PR Updates CCIP to the new signatures that use
ctx.