Batch token price updates #623
Conversation
|
I see you updated files related to core. Please run |
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| destRouterOffRamps, err := destRouter.GetOffRamps(nil) |
There was a problem hiding this comment.
Can use the ctx here instead of nil
There was a problem hiding this comment.
Actually @dimkouv shouldn't this be added to one of the interfaces? I think this calls the method on the gethwrapper directly.
There was a problem hiding this comment.
Do you mean like abstracting this in a RouterReader interface? We can add that in a follow up PR to this one, where we will read from multiple source router instances to fetch source natives
There was a problem hiding this comment.
Since that's going to only be part of the evm initialization it shouldn't really be behind a reader interface.
There was a problem hiding this comment.
In the followup we will be reading from multiple Routers in Observation() to fetch all the source natives, we can make a Reader with caching for that.
| }) | ||
|
|
||
| var batchCounter uint = 0 | ||
| for _, o := range offRamps { |
There was a problem hiding this comment.
How does it scale with the number of chains we add? What's the expected performance impact after introducing that? It might become a bottleneck at some, it's called in both the Observation and Report phases
There was a problem hiding this comment.
OffRamps caches the tokens, most of the performance impact will be the 1st OCR round read after booting up the CommitDON.
This is a temp solution before we fold the ramps in V2. The impact is capped by amount of chains we can add before v2 transition, 1 extra chain translates to 1 extra OffRamp in this loop. Gut estimate is less than 30 chains before V2.
core/services/ocr2/plugins/ccip/internal/ccipcommon/shortcuts.go
Outdated
Show resolved
Hide resolved
|
|
||
| var destFeeTokens []cciptypes.Address | ||
| var destBridgeableTokens []cciptypes.Address | ||
| lock := &sync.RWMutex{} |
core/services/ocr2/plugins/ccip/internal/ccipcommon/shortcuts.go
Outdated
Show resolved
Hide resolved
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| destRouterOffRamps, err := destRouter.GetOffRamps(&bind.CallOpts{Context: ctx}) |
There was a problem hiding this comment.
On initialization we call the router to get the configured offramp addresses.
If the configuration on the router changes how is the plugin notified about the change in order to initialize the new offramp or delete the reference to the removed off ramp?
There was a problem hiding this comment.
This is a limitation of the current iteration, right now if we add/remove lanes or tokens, the leader lane will need to accept new jobspecs, in additional to the impacted lane as is today; Connor has an idea that may avoid this, I'm looking into if it's feasible.
…ot configured on pricegetter (#686) ## Motivation Self service token pools will enable many new TransferTokens to be used across CCIP, most of which may not have a readily available price. ## Context The current procedure (as of [batched price updates](#623)) is for a single lane to be designated as the "leader lane". It reports all prices that other lanes use. Other lanes have their price reporting disabled. To enable this, the leader lane has all supported tokens configured in the CommitJobSpec. The source of truth for which tokens are supported comes from on-chain: queried from the leader lane's destination price registry (FeeTokens) and from each OffRamp's `supportedTokens`. The combined set is given to the `price getter`, and if the number of resulting prices is different from the CommitJobSpec's tokens then the observation is thrown out. ## Solution ### _For v1.4 (current) contracts:_ Use the `CommitJobSpec`'s configured tokens as the source of truth for which TransferTokens need a price update. Filter out TransferTokens that are not configured on the `pricegetter` before they reach `pricegetter.TokenPricesUSD()`. Risks: - Compatability with [shared job specs](#683) ### _For v1.5 (next) contracts:_ Return to on-chain being the source of truth for which tokens need a price update. The list of TransferTokens that need pricing will be created from querying the OnRamps for tokens that have either BPS set and/or Aggregate Rate Limits. FeeTokens will still be taken from the destination Price Registry.
This reverts commit 2963a90
This reverts commit 2963a90
This reverts commit 2963a90
This reverts commit 2963a90
This effectively reverts the multi-offramp feature introduced in commit 2963a90. ## Motivation The shared jobspec approach has been abandoned due to it being incompatible with LOOPPS migration. Reverting was not trivial due to changes in chainlink-common, and PriceRegistry exclusion building on top.
This effectively reverts the multi-offramp feature introduced in commit 2963a90. ## Motivation The shared jobspec approach has been abandoned due to it being incompatible with LOOPPS migration. Reverting was not trivial due to changes in chainlink-common, and PriceRegistry exclusion building on top.
## Motivation Batch all token price updates on leader lane so save price update costs ## Solution Leader lane: 1. finds out all dest tokens from the Router 2. queries and reports prices for all dest tokens
…ot configured on pricegetter (#686) ## Motivation Self service token pools will enable many new TransferTokens to be used across CCIP, most of which may not have a readily available price. ## Context The current procedure (as of [batched price updates](#623)) is for a single lane to be designated as the "leader lane". It reports all prices that other lanes use. Other lanes have their price reporting disabled. To enable this, the leader lane has all supported tokens configured in the CommitJobSpec. The source of truth for which tokens are supported comes from on-chain: queried from the leader lane's destination price registry (FeeTokens) and from each OffRamp's `supportedTokens`. The combined set is given to the `price getter`, and if the number of resulting prices is different from the CommitJobSpec's tokens then the observation is thrown out. ## Solution ### _For v1.4 (current) contracts:_ Use the `CommitJobSpec`'s configured tokens as the source of truth for which TransferTokens need a price update. Filter out TransferTokens that are not configured on the `pricegetter` before they reach `pricegetter.TokenPricesUSD()`. Risks: - Compatability with [shared job specs](#683) ### _For v1.5 (next) contracts:_ Return to on-chain being the source of truth for which tokens need a price update. The list of TransferTokens that need pricing will be created from querying the OnRamps for tokens that have either BPS set and/or Aggregate Rate Limits. FeeTokens will still be taken from the destination Price Registry.
This effectively reverts the multi-offramp feature introduced in commit 23a3cb8. ## Motivation The shared jobspec approach has been abandoned due to it being incompatible with LOOPPS migration. Reverting was not trivial due to changes in chainlink-common, and PriceRegistry exclusion building on top.
Motivation
Batch all token price updates on leader lane so save price update costs
Solution
Leader lane: