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

Add DA cost estimation to CCIPMessageExecCostUSD18Calculator #275

Merged
merged 11 commits into from
Nov 1, 2024

Conversation

rstout
Copy link
Contributor

@rstout rstout commented Oct 29, 2024

No description provided.

execute/exectypes/costly_messages.go Outdated Show resolved Hide resolved
pkg/reader/ccip.go Show resolved Hide resolved
pkg/reader/ccip.go Show resolved Hide resolved
execute/exectypes/costly_messages.go Show resolved Hide resolved
@0xnogo 0xnogo requested a review from matYang October 30, 2024 14:08
}

// calculateMessageMaxDAGas calculates the total DA gas needed for a CCIP message
func calculateMessageMaxDAGas(
Copy link

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?

Copy link
Contributor

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

Copy link
Contributor Author

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

execute/exectypes/costly_messages.go Outdated Show resolved Hide resolved
execute/exectypes/costly_messages.go Outdated Show resolved Hide resolved
execute/exectypes/costly_messages.go Outdated Show resolved Hide resolved
}

messageGas := calculateMessageMaxDAGas(message, daConfig)
return big.NewInt(0).Mul(messageGas, dataAvailabilityFee)
Copy link

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

Copy link
Contributor

@0xnogo 0xnogo Oct 31, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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];
  }

Copy link

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?

Copy link

@matYang matYang left a 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

Copy link

github-actions bot commented Nov 1, 2024

Metric rs/exec-da-cost main
Coverage 72.5% 72.3%

@0xnogo 0xnogo merged commit e4eab06 into main Nov 1, 2024
4 checks passed
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.

5 participants