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

follow up the chain selector refactor for CCIP plugin, fix potential bug in CR/CW config #15482

Merged

Conversation

huangzhen1997
Copy link
Contributor

@huangzhen1997 huangzhen1997 commented Dec 2, 2024

https://smartcontract-it.atlassian.net/browse/NONEVM-826

Follow up PR for PR for chain selector refactor work in CCIP plugin change, there's a mistake that potentially causes mis-configuration in CR/CW config

@huangzhen1997 huangzhen1997 marked this pull request as ready for review December 2, 2024 23:53
@huangzhen1997 huangzhen1997 requested a review from a team as a code owner December 2, 2024 23:53
Copy link
Contributor

github-actions bot commented Dec 3, 2024

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

silaslenihan
silaslenihan previously approved these changes Dec 3, 2024
@@ -344,7 +344,7 @@ func (i *pluginOracleCreator) createReadersAndWriters(
return nil, nil, err1
}

if chainID == destChainID {
if chainID == destChainID && destChainFamily == relayChainFamily {
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the difference between a chainID and a chain selector?

Is it true that this extra check is to ensure we don't mis-match in case the same chainID is used in different chain families?

Copy link
Contributor Author

@huangzhen1997 huangzhen1997 Dec 4, 2024

Choose a reason for hiding this comment

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

Yes, chain selector is a randomly generated uint64 and used by CCIP, and it is unique across all chains/families.

The chain id is specific to a chain family, so it doesn't provide this guarantee. For example chainid: 1 can either beethereum-mainnet or aptos-mainnet

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I was mixing up chainID and chainSelector. They're both differen't ways to ID a chain so it's hard to reason about, some kind of (chainID, chainFamily) tuple type and/or converting to one or the other rather than passing them around is needed. Having an ID with type string doesn't help.

I believe getChainReaderConfig has this same bug.

@huangzhen1997 huangzhen1997 requested a review from winder December 4, 2024 23:44
@prashantkumar1982 prashantkumar1982 added this pull request to the merge queue Dec 5, 2024
Merged via the queue into develop with commit 35c2f05 Dec 5, 2024
179 of 181 checks passed
@prashantkumar1982 prashantkumar1982 deleted the NONEVM-826/update-chain-family-logic-plugin branch December 5, 2024 13:29
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.

4 participants