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

Tag evm only get selector as deprecated #78

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

jlaveracll
Copy link
Contributor

Tag the ChainIdFromSelector method as deprecated since it only supports evm chains, and we are expanding support to non evm.

If needed, people can still use it and we don't immediately need to migrate away from it.

If someone needs to also validate that a given chain is evm, they can also use the GetSelectorFamily with the same selector.

@jlaveracll jlaveracll requested review from a team as code owners November 21, 2024 15:00
Copy link

@krehermann krehermann left a comment

Choose a reason for hiding this comment

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

great, thanks. i was tripping over this yesterday

Copy link

@krehermann krehermann left a comment

Choose a reason for hiding this comment

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

actually, please also depreciate the other funcs that are EVM specific and superceded by GetXXX

SelectorFromChainId
NameFromChainId
???```

@@ -69,6 +69,7 @@ func EvmChainIdToChainSelector() map[uint64]uint64 {
return copyMap
}

// Deprecated, this only supports EVM chains, use the chain agnostic `GetChainIDFromSelector` instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm are we sure we want to direct to that and not some new EVMChainIdFromSelector which does have the uint64 typing? I think if ChainIdFromSelector was used in an EVM specific context (say in the EVM relayer) it'd be annoying to have to strconv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in that case I guess users can still rely on the evm specific getter. That's why I think it will be important to at some point create submodules for all these methods, to make the switch explicit.

I'm going to move forward with the merge, however I'm happy to continue this topic in slack

@jlaveracll jlaveracll merged commit 502f846 into main Nov 21, 2024
1 check passed
@jlaveracll jlaveracll deleted the deprecate-evm-only-get-selector branch November 21, 2024 17:45
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