Solana changeset support for e2e testing #15892
Conversation
This reverts commit aaab52e.
…k into solana-link-deploy
…k into solana-link-deploy
…k into solana-updates
| var signer [20]uint8 | ||
| copy(signer[:], node.SignerKey[:20]) | ||
| signerAddresses = append(signerAddresses, signer) | ||
| transmitterAddresses = append(transmitterAddresses, solana.MustPublicKeyFromBase58(string(node.TransmitterKey))) |
There was a problem hiding this comment.
Don't think this is correct. string(node.TransmitterKey) doesn't base58 encode the bytes, it tries to convert the byte-slice into a string by interpreting the bytes as UTF-8 encoded characters. You should encode to base58 first and then do the decoding.
Also MustPublicKeyFromBase58 looks like it'll panic - rather we should just catch and error gracefully.
There was a problem hiding this comment.
So it took me a bit to remember why we did this, but it does work and the tests do pass with this. The reason is because of the changes in delegate.go https://github.com/smartcontractkit/chainlink/pull/15892/files#diff-5a6126b485353ba3fd28b03f608fa8bd9b1c8ae744845f019eff1fdbc9a6775cR278
Calling String() on a solana key actually base58 encodes the key, so doing a string cast and decode to base58 does work.
We will add the error check rather than the must though to avoid the panic.
There was a problem hiding this comment.
That code you linked is completely unrelated to this code right here. Can you link what test you're referring to? node.TransmitterKey is just []byte so there is no Solana stringer going on here, as far as I can tell.
There was a problem hiding this comment.
Is it unrelated? The transmitter key should be encoded in the delegate and decoded here, no? That appears to be what's happening here.
The test that invokes this line is TestDeployCCIPContracts (in the solana folder) -> DoDeployCCIPContracts -> NewMemoryEnvironment -> NewEnvironmentWithJobsAndContracts -> AddCCIPContractsToEnvironment -> SetOCR3ConfigSolana
There was a problem hiding this comment.
There's no common exec context b/w the code in the delegate (run in the CL node ctx) and the code here. So there is no encoding in the delegate and then decoding here, there's simply no relationship / call stack of that kind. Nowadays we use JD to fetch a nops' solana OCR keys and use those keys to create the proper OCR config. That delegate is only run when the job on the CL node is created (also by JD).
Also, that kind of test doesn't test that this decoding is working as expected, because the string([]byte) doesn't actually return a base58 encoded string. Here's a test that fails pretty often with the msg "decode: high-bit set on invalid digit":
func TestDecodeBase58(t *testing.T) {
transmitterKeyBytes := utils.RandomBytes32()
key, err := solana.PublicKeyFromBase58(string(transmitterKeyBytes[:20]))
require.NoError(t, err)
require.Equal(t, transmitterKeyBytes, key[:])
}To properly do this you have to encode the transmitterKeyBytes as base58 first then decode it, or just load the solana public key from the bytes directly
func TestDecodeBase58(t *testing.T) {
transmitterKeyBytes := utils.RandomBytes32()
_, err := solana.PublicKeyFromBase58(string(transmitterKeyBytes[:])) // no base58 encode
require.Error(t, err)
keyFromBase58, err := solana.PublicKeyFromBase58(base58.Encode(transmitterKeyBytes[:]))
require.NoError(t, err)
require.Equal(t, keyFromBase58.Bytes(), transmitterKeyBytes[:])
keyFromBytes := solana.PublicKeyFromBytes(transmitterKeyBytes[:])
require.Equal(t, keyFromBytes.Bytes(), transmitterKeyBytes[:])
}There was a problem hiding this comment.
We took this offline, but the base58 encoding is happening when we serialize the transmitter key
Under the covers it calls
which base58 encodes.There was a problem hiding this comment.
Yeah we should have a follow up ticket here where, in order to maintain consistence within the OCRConfig itself (both router address + transmitter addresses represented as 32 byte arrays vs. base58 encoded - right now it seems like router is just 32 bytes but transmitters are base58 encoded addresses as bytes)
// in state.GetOffRamp(..)
offRampAddress = s.SolChains[chainSelector].Router.Bytes()
There was a problem hiding this comment.
Added a ticket and comment in the code https://smartcontract-it.atlassian.net/browse/NONEVM-1254
| @@ -456,16 +451,14 @@ func (p SetCandidatePluginInfo) Validate(state CCIPOnChainState, homeChain uint6 | |||
| return errors.New("PluginType must be set to either CCIPCommit or CCIPExec") | |||
| } | |||
| for chainSelector, params := range p.OCRConfigPerRemoteChainSelector { | |||
| _, ok := state.Chains[chainSelector] | |||
| if !ok { | |||
| if _, exists := state.SupportedChains()[chainSelector]; !exists { | |||
There was a problem hiding this comment.
Small nit: cleaner API seems to be state.IsChainSupported(chainSelector)
There was a problem hiding this comment.
Will handle in a followup that just changes the fn name. Don't disagree but the fn already existed for us.
| @@ -145,3 +156,18 @@ func GetEvmDestChainStatePDA(ccipRouterProgramID solana.PublicKey, evmChainSelec | |||
| ) | |||
| return pda | |||
| } | |||
|
|
|||
| func GetReceiverTargetAccountPDA(ccipReceiverProgram solana.PublicKey) solana.PublicKey { | |||
| pda, _, _ := solana.FindProgramAddress([][]byte{[]byte("counter")}, ccipReceiverProgram) | |||
There was a problem hiding this comment.
Also these strings "counter", "external_execution_config", "token_admin_registry" are fetched from where? Are there any constants that can be used instead, perhaps from Go bindings?
There was a problem hiding this comment.
If not, please link where these strings are defined originally / referenced.
There was a problem hiding this comment.
We actually refactored this in chainlink-ccip last week because the programs were hardcoded to test programs. smartcontractkit/chainlink-ccip@5b8bdc1
We can't incorporate the changes in this PR because chainlink-ccip introduced breaking contract changes. We're bumping chainlink-ccip and using the helpers in a followup. https://github.com/smartcontractkit/chainlink/pull/16088/files#diff-b7f8a498937d8fcd6b2d3850cfe63f19fec5932061406345cf5f22e9c8433b11R565
| maxRetries := 10 | ||
| var url, wsURL string |
There was a problem hiding this comment.
I don't really wanna retcon memory here to really mean docker for solana. Why not only spin up solana in the docker env? cc @AnieeG
There was a problem hiding this comment.
Because the in-memory environment is a lot simpler and faster, it avoids spinning up docker containers for all the chainlink nodes (and job distributor). Memory env is the main way a lot of these tests are ran so we'd have to make changes on CI to accommodate from what I understand
There was a problem hiding this comment.
Sure, but I think now memory will no longer be as simple and fast due to the Solana docker container spinup (which also seems to be flakey). Overall not great, and just more burden on folks to continue maintaining flakey envs (of which we already have regularly flaking tests)
There was a problem hiding this comment.
Solana only spins up in tests that ask for solana chains. I do think we need to introduce tags though so that tests for a specific chain type can be ran locally
There was a problem hiding this comment.
Solana's local validator is rust based so it shouldn't take more than a second or two to boot. I'm hoping we can address any flakes and make the env reliable
| @@ -538,6 +533,7 @@ func deployChainContractsEVM(e deployment.Environment, chain deployment.Chain, a | |||
| return nil | |||
| } | |||
|
|
|||
| // TODO: move everything below to solana file | |||
There was a problem hiding this comment.
We doing this in a follow-up?
There was a problem hiding this comment.
Yes already in progress. https://github.com/smartcontractkit/chainlink/pull/16088/files
| // TODO: what if chain family is solana ? | ||
| // bytes4(keccak256("CCIP ChainFamilySelector EVM")) | ||
| ChainFamilySelector: [4]uint8{40, 18, 213, 44}, |
There was a problem hiding this comment.
Isn't this provided by the update object? Why hardcode?
There was a problem hiding this comment.
Yep we are now passing it in via a test config in the followup http://github.com/smartcontractkit/chainlink/pull/16088/files#diff-63c14720b1d34153c81bdcd83289635be7603869be209d97a1c988ae2fea5854R54
|
|
just for context, we're already working on a followup to add full token pool support. This is addressing a lot of the issues here with hardcoded configs or TODOs in code #16088 Some ccip contracts changed (including token pools) and wanted to handle that separately rather than all in one. |
Requires
Supports