Detect onramp dynamic config changes#544
Conversation
jarnaud
added
do not merge
mateusz-sekara
left a comment
mateusz-sekara
left a comment
There was a problem hiding this comment.
I would love to see some tests for that ;)
core/services/ocr2/plugins/ccip/internal/ccipdata/v1_0_0/onramp.go
Outdated
Show resolved
Hide resolved
mateusz-sekara
left a comment
mateusz-sekara
left a comment
There was a problem hiding this comment.
Can we have a dedicated test suite for that logic?
core/services/ocr2/plugins/ccip/internal/ccipdata/v1_0_0/onramp.go
Outdated
Show resolved
Hide resolved
core/services/ocr2/plugins/ccip/internal/ccipdata/v1_0_0/onramp.go
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Ensure that the source price registry is synchronized with the onRamp. | ||
| if err = r.ensurePriceRegistrySynchronization(ctx); err != nil { |
There was a problem hiding this comment.
Shouldn't this call be made when the observation round starts?
Otherwise we might miss price registry getting synchronized.
For example going one step back:
if len(unexpiredReports) == 0 {
return []ccip.ObservedMessage{}, nil
}
getExecTokenData := cache.LazyFunction[execTokenData](func() (execTokenData, error) {
return r.prepareTokenExecData(ctx)
})
in that case prepareTokenExecData and eventually ensurePriceRegistrySynchronization are never called.
There was a problem hiding this comment.
Created another PR to move the sync code: https://github.com/smartcontractkit/ccip/pull/586/files
There was a problem hiding this comment.
in that case prepareTokenExecData and eventually ensurePriceRegistrySynchronization are never called.
@dimkouv @jarnaud why is that a problem? At this moment, the source price registry is used only in a single place, so what's the point of eagerly synchronizing that? I understand that in case of adding any other reference to the source price registry might end up trying to call stale object.
| GetDynamicConfig() (OnRampDynamicConfig, error) | ||
|
|
||
| // SourcePriceRegistryAddress returns the address of the current price registry configured on the onRamp. | ||
| SourcePriceRegistryAddress(ctx context.Context) (Address, error) |
There was a problem hiding this comment.
This should be include under chainlink-common:
https://github.com/smartcontractkit/chainlink-common/tree/main/pkg/types/ccip
I will get rid of cciptypes package and import chainlink-common very soon
There was a problem hiding this comment.
This refactoring doesn't seem immediately related to this ticket
| sendRequestedEventSig: CCIPSendRequestEventSig, | ||
| cachedSourcePriceRegistryAddress: cache.NewLogpollerEventsBased[cciptypes.Address]( | ||
| sourceLP, | ||
| []common.Hash{abihelpers.MustGetEventID("ConfigSet", onRampABI)}, |
There was a problem hiding this comment.
Lot's of "ConfigSet". Can we use a constant?
## Motivation If onRamp price getter configuration changes, the exec plugin won't be notified and we will need to restart the plugin manually. ## Solution - dynamically get the proper priceregistry from the onChain when needed (if its address changed in the onchain config). - replaced the sourcePriceRegistry by a provider in the exec report plugin.
3 participants
Motivation
If onRamp price getter configuration changes, the exec plugin won't be notified and we will need to restart the plugin manually.
Solution