-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add DA cost estimation to CCIPMessageExecCostUSD18Calculator #275
Conversation
ad24a2b
to
e7913cf
Compare
} | ||
|
||
// calculateMessageMaxDAGas calculates the total DA gas needed for a CCIP message | ||
func calculateMessageMaxDAGas( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DA is quite specific to EVM, I'm not expert of v2 code base, wondering if you want to put it under an EVM folder somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You right. We will work on making this logic chain abstracted in a second step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The priority is to unblock the itests right now, so our plan is to merge this, then move this logic to chainlink with the other EVM specific logic
} | ||
|
||
messageGas := calculateMessageMaxDAGas(message, daConfig) | ||
return big.NewInt(0).Mul(messageGas, dataAvailabilityFee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this actually returning fee in USD or fee in the native currency?
there is a pre-existing comment that says executionFee (USD18/gas)
so I presume dataAvailabilityFee is denoted in USD/gas as opposed to native/gas, but after reading the code, it seems feeComponents
is read straight from the ChainWriter, does chain writer return native/gas (which I thought should be the case and will make this impl incorrect). or actually usd/gas, please double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value is read from https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/ccip/FeeQuoter.sol#L303
which is denominated in USD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like USD/gas: https://github.com/smartcontractkit/chainlink/blob/a1e4f8e960d4b5bf05e353dec3254d13c9a3b4c8/contracts/src/v0.8/ccip/FeeQuoter.sol#L303
function getDestinationChainGasPrice(
uint64 destChainSelector
) external view returns (Internal.TimestampedPackedUint224 memory) {
return s_usdPerUnitGasByDestChainSelector[destChainSelector];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still confused, isn't the value fetched from ChainWriter, maybe I missed it, where is it read from FeeQuoter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving this PR, with the expectation that a conversion from native in units of wei to usd will be added later
3603c72
to
5a53390
Compare
|
No description provided.