Adding solana test selectors #86
Conversation
|
|
||
| "github.com/mr-tron/base58" |
There was a problem hiding this comment.
From what I see here from this lib readme, the only advantage they present vs the previous approach is speed.
Considering that this part of the system doesn't go through a particularly high throughput, I don't think a nano-second level improvement is gonna fly with security in terms of advantage over adding a particular new repo as a dependency.
I'm not gonna oppose it, but wanted to raise that as a concern
generated_chains_solana.go
Outdated
| ) | ||
|
|
||
| var SolanaALL = []SolanaChain{ | ||
| SOLANA_DEVNET, | ||
| SOLANA_MAINNET, | ||
| SOLANA_TESTNET, | ||
| TEST_7ZWZQPZ5RQNJE9ZGHWEZ6KVUFBGQYXHH6T7DJHDU6IXY, |
There was a problem hiding this comment.
Shouldn't we make these names more human-friendly? Something like SOLANA_TEST_XYZ where XYZ in [1..3]
There was a problem hiding this comment.
I think adding the SOLANA_TEST part makes sense.
But changing the hash to [1,2,3..] will deviate from how we are defining chain selectors for testnet/mainnet. And I thought that will create more confusion.
@mateusz-sekara
There was a problem hiding this comment.
I'm afraid that these random values in the enums' names are not usable because they are not human friendly. My knowledge about Solana is very limited so it's hard for me to suggest an alternative approach.
There was a problem hiding this comment.
Maybe these should not be random, but instead sth like this (if allowed)
"11111111111111111111111111111111111111111111":
selector: 12463857294658392847There was a problem hiding this comment.
Not a blocker for merge imo
No description provided.