Refactor with family specific chain selector support and added solana #70

huangzhen1997 · 2024-11-06T21:53:09Z

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

To generalize plugin component to support Solana, we need to help CCIP team to refactor chain-selector. The existing chainID is uint64, this PR converts it to string since for solana we will use genesis hash as chainID.
Refactor selector.yml to evm specific, and moved evm specific util function into evm.go. Created ymal for solana and test coverage. Chain agnostic function will be put under selector.go
Removed selector_family.yml, family specific yaml file will be used to infer the chain family

huangzhen1997 · 2024-11-06T22:01:21Z

generated_chains_solana.go

+	SOLANA_TESTNET = SolanaChain{ChainID: "4uhcVJyU9pJkvQyS88uRDiswHXSCkY3zQawwpjk2NsNY", Selector: 6302590918974934319, Name: "solana-testnet"}
+)
+
+var SolanaALL = []SolanaChain{


Not used, and not sure that's going to be useful in the future. We might consider just keep the code gen for evm

makramkd · 2024-11-07T08:07:28Z

selectors.yml

Is there a reason we don't rename this to selectors_evm.yml?

Don't believe the file is part of the public API of this package.

makramkd · 2024-11-07T08:07:57Z

test_selectors.yml

test_selectors_evm.yml?

makramkd · 2024-11-07T08:08:49Z

selectors.yml

@@ -1,3 +1,5 @@
+# EVM selectors.
+# File doesn't have a family suffix for backwards compatibility.


Don't fully understand this comment, this file is not part of the public API of this package?

I think for non-Go based applications it is, RMN + some observability afaik @andrevmatos might know more

makramkd · 2024-11-07T08:24:36Z

selectors_solana.yml

@@ -0,0 +1,11 @@
+selectors:
+  "5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d":


What kind of string is this? Are there any docs we can link to in solana.go?

its a base58 encoded 32 byte hash. +1 we should add a link to where it came from and some comments explaining that

makramkd · 2024-11-07T08:46:13Z

evm.go

+	return copyMap
+}
+
+func ChainIdFromSelector(chainSelectorId uint64) (uint64, error) {


Recommendation: mark this function as deprecated and create a parallel func EVMChainIDFromChainSelector to make it clear this is EVM which should be used instead. The "chain ID" concept should now be abstract and be more scoped with the chain family that you want a chain ID for. Feels like someone can easily use this without intending to and run into issues.

func EVMChainIDFromSelector(chainSelector uint64) (*big.Int, error) { // .. impl // btw, chain IDs are technically uint256 } // Deprecated: use EVMChainIDFromSelector instead func ChainIdFromSelector(chainSelectorId uint64) (uint64, error) { // ... }

I wonder if it might be easier conceptually to just leave it as "Chain" is an EVM chain and then otherwise have a family prefix like SolXXX?

makramkd · 2024-11-07T08:46:46Z

evm.go

+	return 0, fmt.Errorf("chain not found for chain selector %d", chainSelectorId)
+}
+
+func SelectorFromChainId(chainId uint64) (uint64, error) {


Ditto here re: deprecation see above.

makramkd · 2024-11-07T08:46:53Z

evm.go

+	return 0, fmt.Errorf("chain selector not found for chain %d", chainId)
+}
+
+func NameFromChainId(chainId uint64) (string, error) {


Ditto here re: deprecation

makramkd · 2024-11-07T08:46:59Z

evm.go

+	return details.ChainName, nil
+}
+
+func ChainIdFromName(name string) (uint64, error) {


Ditto here re: deprecation

makramkd · 2024-11-07T08:47:08Z

evm.go

+	return 0, fmt.Errorf("chain not found for name %s", name)
+}
+
+func TestChainIds() []uint64 {


Ditto here re: deprecation

makramkd · 2024-11-07T08:47:15Z

evm.go

+	return chainIds
+}
+
+func ChainBySelector(sel uint64) (Chain, bool) {


Ditto here re: deprecation

makramkd · 2024-11-07T08:47:49Z

evm.go

+	// We always return true since only evm chains are supported atm.
+	return true, nil


This is incorrect, should return false if its solana

I think the condition is a bit confusing but still looks correct to me. ChainBySelector is EVM only, so if it does exist there it is EVM

Oops you're right, misread ChainBySelector already. Was already thinking it'd be chain agnostic hehe

makramkd · 2024-11-07T08:48:20Z

I think this is OK, v2 package and a clean break seems better to me though.

connorwstein · 2024-11-07T15:18:28Z

solana.go

+	if err != nil {
+		panic(err)
+	}
+	return data.SelectorsBySolanaChainId


let's add a genesis hash validation like:

for genesisHash := range data.SelectorsBySolanaChainId { b, err := base58.Decode(genesisHash) if err != nil { panic(fmt.Errorf("failed to decode base58 genesis hash %s: %w", genesisHash, err)) } if len(b) != 32 { panic(fmt.Errorf("decoded genesis hash %s is not 32 bytes long", genesisHash)) } }

can use github.com/mr-tron/base58

connorwstein

Should add validation for the genesis hashes and documentation for them o/w lgtm.

connorwstein and others added 2 commits November 6, 2024 11:46

Sketch

28a3bb5

add solana and backwards compatibility

392f8fb

huangzhen1997 marked this pull request as ready for review November 6, 2024 21:53

huangzhen1997 requested review from a team as code owners November 6, 2024 21:53

huangzhen1997 mentioned this pull request Nov 6, 2024

Refactor chain-selector to use chain specific chainID #65

Closed

huangzhen1997 changed the title ~~Nonevm 827/add family specific chain selector support~~ Refactor with family specific chain selector support and added solana Nov 6, 2024

huangzhen1997 requested review from connorwstein, makramkd and prashantkumar1982 November 6, 2024 21:58

huangzhen1997 commented Nov 6, 2024

View reviewed changes

makramkd reviewed Nov 7, 2024

View reviewed changes

connorwstein reviewed Nov 7, 2024

View reviewed changes

connorwstein mentioned this pull request Nov 7, 2024

Non-EVM support with backwards compatibility #69

Closed

connorwstein previously approved these changes Nov 7, 2024

View reviewed changes

add hash validation and update readme

d0bc28a

huangzhen1997 dismissed connorwstein’s stale review via d0bc28a November 7, 2024 17:23

huangzhen1997 requested review from makramkd and connorwstein November 7, 2024 17:23

connorwstein approved these changes Nov 7, 2024

View reviewed changes

connorwstein merged commit 611fd57 into main Nov 7, 2024
1 check passed

connorwstein deleted the NONEVM-827/add-family-specific-chain-selector-support branch November 7, 2024 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor with family specific chain selector support and added solana #70

Refactor with family specific chain selector support and added solana #70

huangzhen1997 commented Nov 6, 2024 •

edited

Loading

huangzhen1997 Nov 6, 2024 •

edited

Loading

makramkd Nov 7, 2024

makramkd Nov 7, 2024

makramkd Nov 7, 2024

makramkd Nov 7, 2024

connorwstein Nov 7, 2024

makramkd Nov 7, 2024

connorwstein Nov 7, 2024

makramkd Nov 7, 2024

connorwstein Nov 7, 2024

makramkd Nov 7, 2024

makramkd Nov 7, 2024

makramkd Nov 7, 2024

makramkd Nov 7, 2024

makramkd Nov 7, 2024

makramkd Nov 7, 2024

connorwstein Nov 7, 2024

makramkd Nov 7, 2024

makramkd commented Nov 7, 2024

connorwstein Nov 7, 2024

connorwstein Nov 7, 2024

connorwstein left a comment

		@@ -1,3 +1,5 @@
		# EVM selectors.
		# File doesn't have a family suffix for backwards compatibility.

		@@ -0,0 +1,11 @@
		selectors:
		"5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d":

		// We always return true since only evm chains are supported atm.
		return true, nil

Refactor with family specific chain selector support and added solana #70

Refactor with family specific chain selector support and added solana #70

Conversation

huangzhen1997 commented Nov 6, 2024 • edited Loading

huangzhen1997 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makramkd commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein left a comment

Choose a reason for hiding this comment

huangzhen1997 commented Nov 6, 2024 •

edited

Loading

huangzhen1997 Nov 6, 2024 •

edited

Loading