Skip to content
This repository was archived by the owner on Mar 14, 2025. It is now read-only.

Move price registry sync to start of Observation #586

Merged
jarnaud merged 3 commits into from
Mar 7, 2024
Merged

Move price registry sync to start of Observation #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8fd87ef
Move price registry sync to start of Observation
jarnaud Mar 6, 2024
bd21a39
Const for config set event
jarnaud Mar 6, 2024
cf190c6
Fix test
jarnaud Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions core/services/ocr2/plugins/ccip/ccipexec/ocr2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@ func (r *ExecutionReportingPlugin) Observation(ctx context.Context, timestamp ty
if err := ccipcommon.VerifyNotDown(ctx, r.lggr, r.commitStoreReader, r.onRampReader); err != nil {
return nil, err
}

// Ensure that the source price registry is synchronized with the onRamp.
if err := r.ensurePriceRegistrySynchronization(ctx); err != nil {
return nil, fmt.Errorf("ensuring price registry synchronization: %w", err)
}

// Expire any inflight reports.
r.inflightReports.expire(lggr)
inFlight := r.inflightReports.getAll()
Expand Down Expand Up @@ -1042,11 +1048,6 @@ func (r *ExecutionReportingPlugin) prepareTokenExecData(ctx context.Context) (ex
return execTokenData{}, err
}

// Ensure that the source price registry is synchronized with the onRamp.
if err = r.ensurePriceRegistrySynchronization(ctx); err != nil {
Copy link
Contributor

@mateusz-sekara mateusz-sekara Mar 7, 2024

Choose a reason for hiding this comment

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

Mentioned that in a previous PR so copypasting :D

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 a stale object which is bad. On the other hand, we will keep refreshing cache with every observation even if we don't need to do anything with tokens, but maybe this is not a bad thing

Copy link
Contributor

@dimkouv dimkouv Mar 7, 2024

Choose a reason for hiding this comment

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

It's not really problem, but in order to figure this out (if there's a problem) I had to look at the call stack to make sure the price registry is not used before the ensurePriceRegistrySynchronization call.

Results are cached so I believe it's good to make the logic cleaner to avoid future bugs.

return execTokenData{}, fmt.Errorf("ensuring price registry synchronization: %w", err)
}

sourceFeeTokens, err := r.sourcePriceRegistry.GetFeeTokens(ctx)
if err != nil {
return execTokenData{}, fmt.Errorf("get source fee tokens: %w", err)
Expand Down
2 changes: 1 addition & 1 deletion core/services/ocr2/plugins/ccip/ccipexec/ocr2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,7 +1855,7 @@ func Test_prepareTokenExecData(t *testing.T) {
reportingPlugin := ExecutionReportingPlugin{
onRampReader: onrampReader,
offRampReader: offrampReader,
sourcePriceRegistry: nil, // will be updated at first use.
sourcePriceRegistry: sourcePriceRegistry,
sourcePriceRegistryProvider: sourcePriceRegistryProvider,
destPriceRegistry: destPriceRegistry,
gasPriceEstimator: gasPriceEstimator,
Expand Down
6 changes: 4 additions & 2 deletions core/services/ocr2/plugins/ccip/internal/ccipdata/v1_0_0/onramp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

const (
CCIPSendRequestedEventName = "CCIPSendRequested"
ConfigSetEventName = "ConfigSet"
)

var _ ccipdata.OnRampReader = &OnRamp{}
Expand Down Expand Up @@ -52,6 +53,7 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
}
onRampABI := abihelpers.MustParseABI(evm_2_evm_onramp_1_0_0.EVM2EVMOnRampABI)
eventSig := abihelpers.MustGetEventID(CCIPSendRequestedEventName, onRampABI)
configSetEventSig := abihelpers.MustGetEventID(ConfigSetEventName, onRampABI)
filters := []logpoller.Filter{
{
Name: logpoller.FilterName(ccipdata.COMMIT_CCIP_SENDS, onRampAddress),
Expand All @@ -60,7 +62,7 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
},
{
Name: logpoller.FilterName(ccipdata.CONFIG_CHANGED, onRampAddress),
EventSigs: []common.Hash{abihelpers.MustGetEventID("ConfigSet", onRampABI)},
EventSigs: []common.Hash{configSetEventSig},
Addresses: []common.Address{onRampAddress},
},
}
Expand All @@ -80,7 +82,7 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
sendRequestedEventSig: eventSig,
cachedSourcePriceRegistryAddress: cache.NewLogpollerEventsBased[cciptypes.Address](
sourceLP,
[]common.Hash{abihelpers.MustGetEventID("ConfigSet", onRampABI)},
[]common.Hash{configSetEventSig},
onRampAddress,
),
cachedStaticConfig: cachedStaticConfig,
Expand Down
8 changes: 5 additions & 3 deletions core/services/ocr2/plugins/ccip/internal/ccipdata/v1_2_0/onramp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ import (
var (
// Backwards compat for integration tests
CCIPSendRequestEventSig common.Hash
ConfigSetEventSig common.Hash
)

const (
CCIPSendRequestSeqNumIndex = 4
CCIPSendRequestedEventName = "CCIPSendRequested"
ConfigSetEventName = "ConfigSet"
)

func init() {
Expand All @@ -40,6 +42,7 @@ func init() {
panic(err)
}
CCIPSendRequestEventSig = abihelpers.MustGetEventID(CCIPSendRequestedEventName, onRampABI)
ConfigSetEventSig = abihelpers.MustGetEventID(ConfigSetEventName, onRampABI)
}

var _ ccipdata.OnRampReader = &OnRamp{}
Expand Down Expand Up @@ -67,7 +70,6 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
if err != nil {
return nil, err
}
onRampABI := abihelpers.MustParseABI(evm_2_evm_onramp_1_2_0.EVM2EVMOnRampABI)
// Subscribe to the relevant logs
// Note we can keep the same prefix across 1.0/1.1 and 1.2 because the onramp addresses will be different
filters := []logpoller.Filter{
Expand All @@ -78,7 +80,7 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
},
{
Name: logpoller.FilterName(ccipdata.CONFIG_CHANGED, onRampAddress),
EventSigs: []common.Hash{abihelpers.MustGetEventID("ConfigSet", onRampABI)},
EventSigs: []common.Hash{ConfigSetEventSig},
Addresses: []common.Address{onRampAddress},
},
}
Expand All @@ -97,7 +99,7 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
sendRequestedEventSig: CCIPSendRequestEventSig,
cachedSourcePriceRegistryAddress: cache.NewLogpollerEventsBased[cciptypes.Address](
sourceLP,
[]common.Hash{abihelpers.MustGetEventID("ConfigSet", onRampABI)},
[]common.Hash{ConfigSetEventSig},
onRampAddress,
),
cachedStaticConfig: cachedStaticConfig,
Expand Down
8 changes: 5 additions & 3 deletions core/services/ocr2/plugins/ccip/internal/ccipdata/v1_5_0/onramp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ import (
var (
// Backwards compat for integration tests
CCIPSendRequestEventSig common.Hash
ConfigSetEventSig common.Hash
)

const (
CCIPSendRequestSeqNumIndex = 4
CCIPSendRequestedEventName = "CCIPSendRequested"
ConfigSetEventName = "ConfigSet"
)

func init() {
Expand All @@ -41,6 +43,7 @@ func init() {
panic(err)
}
CCIPSendRequestEventSig = abihelpers.MustGetEventID(CCIPSendRequestedEventName, onRampABI)
ConfigSetEventSig = abihelpers.MustGetEventID(ConfigSetEventName, onRampABI)
}

var _ ccipdata.OnRampReader = &OnRamp{}
Expand Down Expand Up @@ -68,7 +71,6 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
if err != nil {
return nil, err
}
onRampABI := abihelpers.MustParseABI(evm_2_evm_onramp.EVM2EVMOnRampABI)
// Subscribe to the relevant logs
// Note we can keep the same prefix across 1.0/1.1 and 1.2 because the onramp addresses will be different
filters := []logpoller.Filter{
Expand All @@ -79,7 +81,7 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
},
{
Name: logpoller.FilterName(ccipdata.CONFIG_CHANGED, onRampAddress),
EventSigs: []common.Hash{abihelpers.MustGetEventID("ConfigSet", onRampABI)},
EventSigs: []common.Hash{ConfigSetEventSig},
Addresses: []common.Address{onRampAddress},
},
}
Expand All @@ -98,7 +100,7 @@ func NewOnRamp(lggr logger.Logger, sourceSelector, destSelector uint64, onRampAd
sendRequestedEventSig: CCIPSendRequestEventSig,
cachedSourcePriceRegistryAddress: cache.NewLogpollerEventsBased[cciptypes.Address](
sourceLP,
[]common.Hash{abihelpers.MustGetEventID("ConfigSet", onRampABI)},
[]common.Hash{ConfigSetEventSig},
onRampAddress,
),
cachedStaticConfig: cachedStaticConfig,
Expand Down