Skip to content

test(solana): migrate to local/offline containerized node#2168

Closed
0xMMBD wants to merge 3 commits intodevfrom
solana-docker-tests-improvements
Closed

test(solana): migrate to local/offline containerized node#2168
0xMMBD wants to merge 3 commits intodevfrom
solana-docker-tests-improvements

Conversation

@0xMMBD
Copy link
Copy Markdown

@0xMMBD 0xMMBD commented Jul 18, 2024

Add and fix Solana docker tests
Add Dockerfile to run local solana validator for tests

@0xMMBD 0xMMBD self-assigned this Jul 18, 2024
@0xMMBD 0xMMBD requested a review from shamardy July 18, 2024 11:57
@0xMMBD 0xMMBD mentioned this pull request Jul 18, 2024
@laruh
Copy link
Copy Markdown

laruh commented Jul 19, 2024

@0xMMBD please fix conflicts and fmt, lint checks

@0xMMBD
Copy link
Copy Markdown
Author

0xMMBD commented Jul 23, 2024

Hello that's interesting that lint failed, I'll take a look at it (as well as the conflict).

@0xMMBD
Copy link
Copy Markdown
Author

0xMMBD commented Jul 23, 2024

When it comes to the tests unfortunately I'm not sure why some of them are failing, the main scope of this task was to handle solana docker tests which look ok: https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/9991028730/job/27619183380?pr=2168
Let me know if there is anything that can be improved on my side

Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! All solana tests should use dockarized tests and transfers / balance tests should work with it as well!


const SOLANA_CLIENT_URL: &str = "http://localhost:8899";

#[ignore]
Copy link
Copy Markdown
Collaborator

@shamardy shamardy Jul 23, 2024

Choose a reason for hiding this comment

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

I don't think this should be ignored, one of the purposes of having this dockerized node is to fill addresses with balances for tests. We should be able to run swap tests as these https://github.com/KomodoPlatform/komodo-defi-framework/blob/2e0b3ca086f1b2e249bb58e9b211135402b9dd45/mm2src/coins/solana/solana_tests.rs#L342 https://github.com/KomodoPlatform/komodo-defi-framework/blob/2e0b3ca086f1b2e249bb58e9b211135402b9dd45/mm2src/coins/solana/solana_tests.rs#L389 using the dockarized node, so all solana tests should be moved to dockerized tests. To do this, you need to modify the Dockerfile to include a deployment of a token program/contract using https://github.com/solana-labs/solana-program-library where you build the token contract and add it similar to the swap contract or find a prebuilt one in the releases. Then in the test module, you'll need to initialize the token mint and create token accounts. Then you can add functions to transfer funds from the node using the generated key pair to any test addresses used. I can provide detailed code for this if needed, the specs where clear on this where the below was stated in the specs document

Implement Solana Dockerized tests similar to the existing Ethereum and QTUM Dockerized tests.

And I provided the below on DM in the past

For docker tests, here are some code snippets

https://github.com/KomodoPlatform/komodo-defi-framework/blob/af571608c34f38d82e3c421f9bd61c5265db0cf8/mm2src/mm2_main/tests/docker_tests_main.rs#L36-L61
https://github.com/KomodoPlatform/komodo-defi-framework/blob/fa71adb8aadffa7eb38608ea0b6b14893e779d53/mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs#L338-L348
After adding solana devnet test runner as it was done for geth above, you need to specify the URL

https://github.com/KomodoPlatform/komodo-defi-framework/blob/fa71adb8aadffa7eb38608ea0b6b14893e779d53/mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs#L91
Then you can implement functions to fill an address with coins needed for testing

https://github.com/KomodoPlatform/komodo-defi-framework/blob/fa71adb8aadffa7eb38608ea0b6b14893e779d53/mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs#L78-L110
Then you can fill the activated address after activating the coin as such

https://github.com/KomodoPlatform/komodo-defi-framework/blob/fa71adb8aadffa7eb38608ea0b6b14893e779d53/mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs#L265-L299
And this last function can be used in any test to activate a coin that has funds in it's address

Copy link
Copy Markdown
Author

@0xMMBD 0xMMBD Jul 26, 2024

Choose a reason for hiding this comment

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

So that ignore was probably some leftover from my previous commits. If I remember correctly that test was failing for some reason and to not block this task I've disabled it for now.

As for the fill_eth methods, last time we've discussed to try dump account states and programs into the docker container. This way we wont have to handle separate logic related to the Solana as they work differently and it is not that similar to the typical EVM contracts (It can be found here: .docker/Dockerfile.solana-tests). This way we can reduce work needed to handle those tests.
Is it ok for you to do it like that?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

And unfortunately it's impossible for me right now to determine if my work is working as expected as the tests are not really executing: https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/10110629981/job/27960913639?pr=2168

thread 'main' panicked at 'Reached max attempts for <<{docker:?}>>.', mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs:1155:13

The result was the same (failing) with and without #[ignore] that I had on one of my test - its not related to the solana.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As for the fill_eth methods, last time we've discussed to try dump account states and programs into the docker container. This way we wont have to handle separate logic related to the Solana as they work differently and it is not that similar to the typical EVM contracts (It can be found here: .docker/Dockerfile.solana-tests). This way we can reduce work needed to handle those tests.
Is it ok for you to do it like that?

It's fine as long as we have accounts/addresses with funds to test with. solana_coin_send_and_refund_maker_payment and solana_coin_send_and_spend_maker_payment tests among others can't be moved to docker tests now since there are no way to have funds for these tests.

It can be found here: .docker/Dockerfile.solana-tests

This deploys the swap contract only for now. Sure we can deploy token program the same way, but we also need funds on at least one address to test with, but we will probably need more than one address since tests are run in parallel.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And unfortunately it's impossible for me right now to determine if my work is working as expected as the tests are not really executing: https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/10110629981/job/27960913639?pr=2168

thread 'main' panicked at 'Reached max attempts for <<{docker:?}>>.', >mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs:1155:13

The result was the same (failing) with and without #[ignore] that I had on one of my test - its not related to the solana.

Ignored the tendermint tests for now in 4277284, will revert this commit once we resolve this issue from our side.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For the record, I am currently attempting to adjust the scripts according to the suggestions provided. However, the most difficult part seems to be even starting the tests as they currently are. I am investigating the issues with the tests. Even though the Geth container started correctly and is accessible on port 7000, I am getting a spam of logs like the following:

30 07:04:29, docker_tests_common:167] rpc_clients:854] Transport(JsonRpcError { client_info: "coin: MYCOIN", request: JsonRpcRequest { jsonrpc: "1.0", id: "12", method: "getblockcount", params: [] }, error: Transport("Transport 'http://127.0.0.1:7000/' error: connection closed before message completed") })

It's not going too smoothly.

@shamardy
Copy link
Copy Markdown
Collaborator

When it comes to the tests unfortunately I'm not sure why some of them are failing

There are some failing tests due to depending on an external connection / url for moralis for the NFT feature, they can be ignored. That's one of the reasons we are trying to move to local test nodes for all implementations, to have a stable CI for devs.

@0xMMBD 0xMMBD force-pushed the solana-docker-tests-improvements branch 2 times, most recently from bde54f0 to 6c948d7 Compare July 26, 2024 11:38
@0xMMBD
Copy link
Copy Markdown
Author

0xMMBD commented Jul 26, 2024

@laruh thats correct, lint check is fixed now

end conflict too

@0xMMBD 0xMMBD force-pushed the solana-docker-tests-improvements branch from 6c948d7 to ca8a1f6 Compare July 26, 2024 11:51
0xMMBD added 2 commits July 26, 2024 13:54
Add Dockerfile to run local solana validator for tests
unignore test test_solana_and_spl_balance_enable_spl_v2
@0xMMBD 0xMMBD force-pushed the solana-docker-tests-improvements branch from ca8a1f6 to 47861f8 Compare July 26, 2024 11:54
@shamardy shamardy changed the title Solana docker tests test(solana): migrate to local/offline containerized node Jul 26, 2024
@shamardy
Copy link
Copy Markdown
Collaborator

shamardy commented Aug 6, 2024

Superseded by #2187

@shamardy shamardy closed this Aug 6, 2024
@shamardy shamardy deleted the solana-docker-tests-improvements branch August 6, 2024 13:02
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.

4 participants