Change keepers to use the default contract transmitter #11308
Conversation
ferglor
requested review from
a team,
jmank88,
krehermann,
connorwstein and
samsondav
as code owners
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
ferglor
force-pushed
the
feature/AUTO-7289
branch
4 times, most recently
from
e0dd30f to
4d1a809
Compare
ferglor
force-pushed
the
feature/AUTO-7289
branch
from
df9bff0 to
cb6a9f6
Compare
ferglor
requested review from
krehermann,
jmank88 and
cedric-cordenier
and removed request for
krehermann and
jmank88
ferglor
force-pushed
the
feature/AUTO-7289
branch
from
b5754f1 to
08af62a
Compare
| expectedPostRecover := runs + emits | ||
| waitPipelineRuns(t, nodes, expectedPostRecover, testutils.WaitTimeout(t), cltest.DBPollingInterval) |
There was a problem hiding this comment.
We should find some other way to assert wanted expectations. Removing this is weakening this test
core/services/relay/evm/evm.go
Outdated
| @@ -373,7 +371,7 @@ func newConfigProvider(lggr logger.Logger, chain legacyevm.Chain, opts *types.Re | |||
| return newConfigWatcher(lggr, aggregatorAddress, contractABI, offchainConfigDigester, cp, chain, relayConfig.FromBlock, opts.New), nil | |||
| } | |||
|
|
|||
| func newContractTransmitter(lggr logger.Logger, rargs commontypes.RelayArgs, transmitterID string, configWatcher *configWatcher, ethKeystore keystore.Eth) (*contractTransmitter, error) { | |||
| func newContractTransmitter(lggr logger.Logger, rargs commontypes.RelayArgs, transmitterID string, configWatcher *configWatcher, ethKeystore keystore.Eth, gasLimit uint32) (*contractTransmitter, error) { | |||
There was a problem hiding this comment.
i'd rather this this implemented with a new opts struct than a explicit argument.
type configTransmitterOpts struct {
configWatcher *configWatcher
// override the gas limit default provided in the config watcher
gasLimit uint32
}
func newContractTransmitter (..., opts configTransmitterOpts) {
...
gasLimit := configWatcher.chain.Config().EVM().GasEstimator().LimitDefault()
if opts.gasLimit != 0 {
gasLimit = opt.gasLimit
}
}
all the call site need to be updated, but only the automation call site needs to explicitly set a gasLimit option.
this implementation seems more inline with the intention of passing a *configwatcher and deriving relevant options/configuration from it
| @@ -229,8 +226,6 @@ func TestIntegration_KeeperPluginLogUpkeep(t *testing.T) { | |||
| g.Eventually(listener, testutils.WaitTimeout(t), cltest.DBPollingInterval).Should(gomega.BeTrue()) | |||
| done() | |||
|
|
|||
| runs := checkPipelineRuns(t, nodes, 1) | |||
There was a problem hiding this comment.
Have we explored any alternate assertions for non pipeline transmitter? Or do we feel they are covered by existing assertions
There was a problem hiding this comment.
I wasn't able to see how the other services assert their transmits, so I just assumed the existing assertions would be enough, but I can look into this further
There was a problem hiding this comment.
Verifying the upkeeps are actually performed i think should give similar coverage but I'm not sure that's done everywhere (e.g. the other place where this is removed)
ferglor
force-pushed
the
feature/AUTO-7289
branch
from
91b3495 to
8fd92dd
Compare
ferglor
force-pushed
the
feature/AUTO-7289
branch
from
4f813c7 to
c12cab2
Compare
ferglor
requested review from
jmank88,
krehermann and
amirylm
and removed request for
krehermann
core/services/relay/evm/evm.go
Outdated
| @@ -390,6 +394,8 @@ func newContractTransmitter(lggr logger.Logger, rargs commontypes.RelayArgs, tra | |||
| return nil, pkgerrors.New("no sending keys provided") | |||
| } | |||
|
|
|||
| configWatcher := opts.configWatcher | |||
There was a problem hiding this comment.
nit. this seems like a required option (code below will fail if this is nil) so not sure if it made more sense as a parameter before (unlike pluginGasLimit)
|
SonarQube Quality Gate
|
* develop: (56 commits) [TT-367] [TT-745] Quick and Dirty OCRv2 Soak Test (#11487) [FUN-990] s4 observability improvements (#11512) fix health monitoring (#11558) Removes Optimism Goerli from Scheduled Tests (#11559) bump Foundry to the December release (#11540) Standardize LP filter logging (#11515) Change keepers to use the default contract transmitter (#11308) bump toml/v2 and prometheus to latest patch (#11541) Remove big from core utils (#11511) Handle edge case involving blocks not being found in the db (#11298) [DEPLOY-178]: Adds Scroll L2EP Contracts (#11405) disable kaniko fallback, increase deploy wait timeout (#11548) Use multiple EL clients with ocrv2 median smoke test (#11399) Remove core utils dependencies from common (#11425) [BCF-2760] Flakey test detection improvements (#11470) go.mods: rm libp2p; rm btcd replace (#11502) wrap devspace commands (#11530) small improvements based on comments (#11491) (test): Remove unnecessary fuzzing from Functions OnTokenTransfer tests (#11517) core/scripts/common: rm ava-labs/coreth; lint (#11451) ...
* Switch keepers to use the default contract transmitter Pass the gas limit into the transmitter constructor so we can specify the automation gas limit Update tests * Fix streams import * Clean up function calls * Remove pipeline runner dependency for automation * Use contractTransmitterOpts to specify a pluginGasLimit * Make the pluginGasLimit a pointer * Clean up the pipeline transmitter * Attempt to listen for transmits Clean up linter Extract function Intentionally fail test Rework transmit listen Try filtering for transmits Update test * Revert "Attempt to listen for transmits" This reverts commit 198e6669a6f64768c84acb82e01b2773c89426ce. * Listen for performed events * Goimports * Add wrapper function that lets us specify a count of performed * Update integration test * Pass the configWatcher as a parameter to indicate that its required
As part of the LOOPP work, we discovered that the keepers service was using a different contract transmitter to the other services (the pipeline contract transmitter)
To enable the LOOPP work to progress for keepers, this PR switches the keeper service to using the default contract transmitter, and also removes the pipeline contract transmitter
The integration tests were failing on this PR initially due to the fact that the gas limit for the automation service is pulled from a different function vs other services. To fix this, the contract transmitter constructor now accepts a gas limit as a param.