Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exec v2 scaffolding to observes prices/costs #154

Closed
wants to merge 1 commit into from

Conversation

rstout
Copy link
Contributor

@rstout rstout commented Sep 20, 2024

Adds scaffolding to the V2 Exec Plugin to enable observing prices and costs (but does not yet observe these values). Once prices and costs are observed, message fees and costs can be computed, and messages with higher costs than paid fees can be filtered.

The core data type is PriceObservations:

type PriceObservations struct {
	// The price of Juels (the smallest denomination of LINK) in USD. Each CCIP message has a fee denominated in Juels.
	JuelPriceUSD cciptypes.BigInt `json:"juelPriceUSD"`

	// The price of gas in the destination chain, denoted in the native token of the destination chain
	GasPrice cciptypes.BigInt `json:"gasPrice"`

	// The price of the native token of the destination chain in USD
	DestNativeTokenPriceUSD cciptypes.BigInt `json:"destNativeTokenPriceUSD"`

	// Maps message IDs to the estimated gas cost of executing these messages
	MessageExecutionGasCosts map[string]cciptypes.BigInt `json:"messageExecutionCosts"`
}

The message fee in USD will be: message.jeuls * JuelPriceUSD * feeBoostFactor
The message execution cost will be GasPrice * MessageExecutionGasCosts * DestNativeTokenPriceUSD

@rstout rstout requested a review from a team as a code owner September 20, 2024 21:41
@rstout rstout changed the title TODO Exec v2 observes prices Sep 20, 2024
@rstout rstout force-pushed the prices_observation branch 7 times, most recently from 15582d6 to 9c81cb9 Compare September 24, 2024 16:37
@rstout rstout changed the title Exec v2 observes prices Exec v2 scaffolding to observes prices/costs Sep 24, 2024
Adds scaffolding to the V2 Exec Plugin to enable observing prices
and costs (but does not yet observe these values). Once prices and
costs are observed, message fees and costs can be computed, and
messages with higher costs than paid fees can be filtered.
Copy link

Test Coverage

Branch Coverage
prices_observation 72.0%
main 72.0%

Comment on lines +20 to +21
// The price of Juels (the smallest denomination of LINK) in USD. Each CCIP message has a fee denominated in Juels.
JuelPriceUSD cciptypes.BigInt `json:"juelPriceUSD"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add two things to this comment:

  • price is in USD, but how many decimals?
  • where is this price expected to be fetched from? e.g which price registry / feed contract

Comment on lines +23 to +24
// The price of gas in the destination chain, denoted in the native token of the destination chain
GasPrice cciptypes.BigInt `json:"gasPrice"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small rename to be consistent w/ the rest of the codebase:

Suggested change
// The price of gas in the destination chain, denoted in the native token of the destination chain
GasPrice cciptypes.BigInt `json:"gasPrice"`
// The price of gas in the destination chain, denoted in the native token of the destination chain
DestChainFee cciptypes.BigInt `json:"gasPrice"`

Also, is this only execution fees or also data availability fees (like for L2's)? We should also state where this is fetched from, I believe in this case the source price registry. Make sure to update the comment to reflect this information as well.

Comment on lines +26 to +27
// The price of the native token of the destination chain in USD
DestNativeTokenPriceUSD cciptypes.BigInt `json:"destNativeTokenPriceUSD"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to the juels comment, we need the number of decimals and the source of this information (I think in this case dest price registry)

Comment on lines +29 to +30
// Maps message IDs to the estimated gas cost of executing these messages
MessageExecutionGasCosts map[string]cciptypes.BigInt `json:"messageExecutionCosts"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Estimated gas cost is a chain-specific metric, do we want this to be USD instead?

Also, if it is USD, include the decimals count in the comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, for this mapping, I believe we can use cciptypes.Bytes32 as the key type to bring it home that this is a msg ID. JSON encoding/decoding is already implemented for this type.


prices := exectypes.PriceObservations{}
if p.messageExecutionEconomicsEnabled {
prices, err = p.priceObserver.ObservePrices(messages)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really sufficient to just call this here? I believe the price observations are a mix of source and dest information, some nodes may have source info, some may have dest info, wouldn't we need to consensus-ify these values (median, etc.) and only then do the boosting on the medianized values?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or is this stuff going to be done in upcoming PRs?

Copy link
Contributor

@winder winder Sep 25, 2024

Choose a reason for hiding this comment

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

Medianizing is happening in the outcome phase based on these observations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the price observations are a mix of source and dest information

Minor point, but I believe everything should be on the dest chain.

wouldn't we need to consensus-ify these values (median, etc.) and only then do the boosting on the medianized values?

We do this consensus in mergePriceObservations in execute/plugin_functions.go

@@ -391,6 +454,7 @@ func getConsensusObservation(
destChainSelector cciptypes.ChainSelector,
F int,
fChain map[cciptypes.ChainSelector]int,
messageExecutionEconomicsEnabled bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could leave this out. The prices would be empty by default on account of no price observations being present. Add a check to VerifiyObservation that there are no price observations when the flag is false.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

This all looks on track to me.

@rstout rstout closed this Sep 30, 2024
@mateusz-sekara mateusz-sekara deleted the prices_observation branch November 8, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants