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

Exposing chain names together with selectors #9

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Conversation

mateusz-sekara
Copy link
Collaborator

@mateusz-sekara mateusz-sekara commented Oct 19, 2023

Exposing chain name together with selectors. It will replace hardcoded values used by EngOps and the CCIP

  • Chain name is an optional parameter, so it's not defined for chains (e.g., simulated or test ones). In that case, we return just chainId as a string.
  • We use the same yml file for both chain names and selectors

We use the following pattern for all names:

<blockchain>-<type>-<network_name>-<parachain>-<rollup>-<rollup_instance>

selectors.yml Outdated Show resolved Hide resolved
@mateusz-sekara mateusz-sekara changed the title Exposing chain names together with selector Exposing chain names together with selectors Dec 6, 2023
andrevmatos
andrevmatos previously approved these changes Dec 6, 2023
Copy link
Contributor

@andrevmatos andrevmatos left a comment

Choose a reason for hiding this comment

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

🎉

@@ -46,14 +52,14 @@ func parseYml(ymlFile []byte) map[uint64]uint64 {
func EvmChainIdToChainSelector() map[uint64]uint64 {
copyMap := make(map[uint64]uint64, len(evmChainIdToChainSelector))
for k, v := range evmChainIdToChainSelector {
copyMap[k] = v
copyMap[k] = v.ChainSelector
}
return copyMap
}

func ChainIdFromSelector(chainSelectorId uint64) (uint64, error) {
for k, v := range evmChainIdToChainSelector {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: since these mappings are all internal/private anyway, should we pre-calculate a chainSelectorToEvmChainId at init time, and use here? The memory increase should be minimal, but those may be accessed very often

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think lib users should use accessing functions like ChainIdFromSelector instead of accessing the map directly. However, if they for some reason need that map then it's rebuilt every time requested because this is a defensive way to prevent altering the internal state. Returning a single precomputed map may lead to a problem in which if users want to add something to this map, they update it for all. A map is passed by a pointer/reference, not a value.

Copy link
Contributor

@andrevmatos andrevmatos Dec 7, 2023

Choose a reason for hiding this comment

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

Yes, I expect this to be accessed from the functions. I was not telling about exposing/returning the map, but just pre-calculating a private one from selectors to chainIds (and maybe also chainIds to names for the other function), instead of having to for-loop it on every function call. It's the classic memory-cpu tradeoff.

@mateusz-sekara mateusz-sekara marked this pull request as ready for review December 7, 2023 12:10
@mateusz-sekara mateusz-sekara requested a review from a team as a code owner December 7, 2023 12:10
Copy link
Contributor

@polds polds left a comment

Choose a reason for hiding this comment

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

Nice! Thank you.

@mateusz-sekara mateusz-sekara merged commit b4a7ca0 into main Dec 12, 2023
1 check passed
@mateusz-sekara mateusz-sekara deleted the chain-names branch December 12, 2023 07:38
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