Enable clippy --all-features on CI #1760

rozhkovdmitrii · 2023-04-19T09:51:55Z

No description provided.

caglaryucekaya · 2023-04-19T11:40:12Z

This gives an error here:

https://github.com/KomodoPlatform/atomicDEX-API/actions/runs/4742746709/jobs/8421395650?pr=1760

.github/workflows/fmt-and-lint.yml

onur-ozkan · 2023-04-19T13:05:41Z

mm2src/mm2_main/tests/docker_tests/mod.rs

 // dummy test helping IDE to recognize this as test module
 #[test]
-fn dummy() { assert!(true) }
+fn dummy() {}


I don't use IDEs, can anyone test this?

cc @KomodoPlatform/mm2

Works fine. Removed the test and the IDE didn't recognize docker_tests as a test mod
https://github.com/KomodoPlatform/atomicDEX-API/blob/75663fff7f3f052b0b153d2695e7b8b84480513f/mm2src/mm2_main/tests/docker_tests_main.rs#L29
With the change in this PR it's still recognized as a test mod.

Hmmm then why here assert!(true) was left for dummy test help? @rozhkovdmitrii

It was left because clippy didn't recognize it as problem. I think that is outdated workaround, I've got rid of it. Thank you @laruh

shamardy · 2023-04-20T19:25:53Z

@rozhkovdmitrii shouldn't we use the --all-features flag here after the fixes? https://github.com/KomodoPlatform/atomicDEX-API/blob/2e76e0ca9dd61ab3333c7aeeff7b5491252ce4a3/.github/workflows/fmt-and-lint.yml#L32

shamardy

Thanks a lot for this very much needed refactor/fixes! Only some minor comments :)

mm2src/coins/z_coin.rs

mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs

mm2src/mm2_main/tests/docker_tests/mod.rs

mm2src/mm2_main/tests/mm2_tests/mod.rs

rozhkovdmitrii · 2023-04-21T07:21:33Z

@rozhkovdmitrii shouldn't we use the --all-features flag here after the fixes?

Yeah thank you, I lost during solving conflicts

shamardy

Last minor comments related to commits after last review and z_coin_native_tests which I didn't cover in the last review

mm2src/mm2_main/tests/mm2_tests/mod.rs

shamardy · 2023-04-21T16:02:50Z

mm2src/coins/z_coin/z_coin_native_tests.rs

-    let priv_key = [1; 32];
+    let mut conf = zombie_conf();
+    let params = default_zcoin_activation_params();
+    let priv_key = PrivKeyBuildPolicy::IguanaPrivKey(IguanaPrivKey::default());


Please use priv_key = [1; 32]; instead of default as it was done before, This will need changing in all tests that had it.

Index: mm2src/coins/z_coin/z_coin_native_tests.rs IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/mm2src/coins/z_coin/z_coin_native_tests.rs b/mm2src/coins/z_coin/z_coin_native_tests.rs --- a/mm2src/coins/z_coin/z_coin_native_tests.rs (revision 1d6c9afbaeed16f524db6d953f57b589297a5f2f) +++ b/mm2src/coins/z_coin/z_coin_native_tests.rs (date 1682094202752) @@ -17,7 +17,8 @@ let ctx = MmCtxBuilder::default().into_mm_arc(); let mut conf = zombie_conf(); let params = default_zcoin_activation_params(); - let priv_key = PrivKeyBuildPolicy::IguanaPrivKey(IguanaPrivKey::default()); + let private_key_data = [1; 32]; + let priv_key = PrivKeyBuildPolicy::IguanaPrivKey(private_key_data.into()); let db_dir = PathBuf::from("./for_tests"); let z_key = decode_extended_spending_key(z_mainnet_constants::HRP_SAPLING_EXTENDED_SPENDING_KEY, "secret-extended-key-main1q0k2ga2cqqqqpq8m8j6yl0say83cagrqp53zqz54w38ezs8ly9ly5ptamqwfpq85u87w0df4k8t2lwyde3n9v0gcr69nu4ryv60t0kfcsvkr8h83skwqex2nf0vr32794fmzk89cpmjptzc22lgu5wfhhp8lgf3f5vn2l3sge0udvxnm95k6dtxj2jwlfyccnum7nz297ecyhmd5ph526pxndww0rqq0qly84l635mec0x4yedf95hzn6kcgq8yxts26k98j9g32kjc8y83fe").unwrap().unwrap(); let CoinProtocol::ZHTLC(protocol_info) = serde_json::from_value::<CoinProtocol>(conf["protocol"].take()).unwrap() else { unimplemented!() }; @@ -27,7 +28,7 @@ "ZOMBIE", &conf, &params, - priv_key.clone(), + priv_key, db_dir, z_key, protocol_info, @@ -52,7 +53,6 @@ }; let tx = coin.send_maker_payment(args).wait().unwrap(); println!("swap tx {}", hex::encode(tx.tx_hash().0)); - let PrivKeyBuildPolicy::IguanaPrivKey(private_key_data) = priv_key else { unimplemented!() }; let refund_args = RefundPaymentArgs { payment_tx: &tx.tx_hex(), @@ -60,7 +60,7 @@ other_pubkey: taker_pub, secret_hash: &secret_hash, swap_contract_address: &None, - swap_unique_data: private_key_data.as_slice(), + swap_unique_data: &private_key_data, watcher_reward: false, }; let refund_tx = coin.send_maker_refunds_payment(refund_args).wait().unwrap();

As shown in the above patch, this line
https://github.com/KomodoPlatform/atomicDEX-API/blob/2340ad8c2d574f183cfb5f06444e19a6cde7a19b/mm2src/coins/z_coin/z_coin_native_tests.rs#L58
Won't be needed if we initialized private key data variable before using it in the policy. This will also avoid a clone that is currently used.

This is not a blocker, I will approve the PR without it, but it will make code a tiny bit better :)

shamardy · 2023-04-21T16:20:25Z

mm2src/coins/z_coin/z_coin_native_tests.rs

    let z_key = decode_extended_spending_key(z_mainnet_constants::HRP_SAPLING_EXTENDED_SPENDING_KEY, "secret-extended-key-main1q0k2ga2cqqqqpq8m8j6yl0say83cagrqp53zqz54w38ezs8ly9ly5ptamqwfpq85u87w0df4k8t2lwyde3n9v0gcr69nu4ryv60t0kfcsvkr8h83skwqex2nf0vr32794fmzk89cpmjptzc22lgu5wfhhp8lgf3f5vn2l3sge0udvxnm95k6dtxj2jwlfyccnum7nz297ecyhmd5ph526pxndww0rqq0qly84l635mec0x4yedf95hzn6kcgq8yxts26k98j9g32kjc8y83fe").unwrap().unwrap();
+    let CoinProtocol::ZHTLC(protocol_info) = serde_json::from_value::<CoinProtocol>(conf["protocol"].take()).unwrap() else { unimplemented!() };


I prefer panicking here instead of unimplemented to have a clear message of the reason for panic, please change this in all tests that have unimplemented

let protocol_info = match serde_json::from_value::<CoinProtocol>(conf["protocol"].take()).unwrap() { CoinProtocol::ZHTLC(protocol_info) => protocol_info, other_protocol => panic!("Wrong protocol in coin conf {:?}", other_protocol), };

For this panic
https://github.com/KomodoPlatform/atomicDEX-API/blob/2340ad8c2d574f183cfb5f06444e19a6cde7a19b/mm2src/coins/z_coin/z_coin_native_tests.rs#L25
No need to mention zombie_conf since this can happen if a dev editing this test used another conf by mistake, it's also a wrong protocol not a failure to get protocol.
Again not a blocker :)

shamardy

🔥

shamardy

Re-approve

onur-ozkan

Nice work :) This roughly LGTM. I have one suggestion and question:

mm2src/coins/z_coin.rs

mm2src/mm2_main/tests/mm2_tests/mod.rs

laruh

LGTM! Thanks for the fixes.

I have some notes for the future.
Currently module z_coin_native_tests doesnt work, bcz all other tests were moved to .path/mm2_main/tests/ where we keep our test modules. So I couldn't check if the z_coin_native_tests module works.
As for z_coin_tests module, I updated check point blocks for zhtlc coin configs like arrr and zombie and the tests started successfully. However only activation tests passed successfully, the rest failed with errors like not enough balance or there were no transactions in the history. It can be fixed later.

laruh · 2023-04-25T12:22:42Z

@rozhkovdmitrii
I was told that pirate.dragonhound.info:10032 and http://pirate.dragonhound.info:443 dont work anymore, could you make changes in what we have here for pirate urls.
electrum_servers should be node1.chainkeeper.pro:10132 and light_wallet_d_servers should be http://node1.chainkeeper.pro:443 like here

rozhkovdmitrii · 2023-04-25T14:36:19Z

@rozhkovdmitrii I was told that pirate.dragonhound.info:10032 and http://pirate.dragonhound.info:443 dont work anymore, could you make changes in what we have here for pirate urls. electrum_servers should be node1.chainkeeper.pro:10132 and light_wallet_d_servers should be http://node1.chainkeeper.pro:443 like here

done

onur-ozkan

thank you for this enhancement!

can you add an entry to the changelog regarding to this clippy fixes for all features flag? @rozhkovdmitrii

rozhkovdmitrii · 2023-04-26T07:36:35Z

thank you for this enhancement!

can you add an entry to the changelog regarding to this clippy fixes for all features flag? @rozhkovdmitrii

Thank you, @ozkanonur, done

rozhkovdmitrii requested review from caglaryucekaya and laruh April 19, 2023 09:51

rozhkovdmitrii self-assigned this Apr 19, 2023

rozhkovdmitrii mentioned this pull request Apr 19, 2023

Enable clippy --all-features on CI #1744

Closed

rozhkovdmitrii linked an issue Apr 19, 2023 that may be closed by this pull request

Running clippy with --all-features results in a set of warnings #1745

Closed

rozhkovdmitrii requested review from onur-ozkan and removed request for caglaryucekaya April 19, 2023 11:18

caglaryucekaya added the in progress Changes will be made from the author label Apr 19, 2023

onur-ozkan reviewed Apr 19, 2023

View reviewed changes

.github/workflows/fmt-and-lint.yml Outdated Show resolved Hide resolved

onur-ozkan reviewed Apr 19, 2023

View reviewed changes

rozhkovdmitrii marked this pull request as ready for review April 20, 2023 11:30

rozhkovdmitrii marked this pull request as draft April 20, 2023 11:30

rozhkovdmitrii marked this pull request as ready for review April 20, 2023 11:52

rozhkovdmitrii requested a review from shamardy April 20, 2023 11:52

shamardy added bug fixing week 1 and removed in progress Changes will be made from the author labels Apr 20, 2023

shamardy requested changes Apr 20, 2023

View reviewed changes

rozhkovdmitrii requested a review from shamardy April 21, 2023 07:39

shamardy requested changes Apr 21, 2023

View reviewed changes

rozhkovdmitrii requested review from shamardy and onur-ozkan April 22, 2023 11:48

shamardy previously approved these changes Apr 24, 2023

View reviewed changes

rozhkovdmitrii dismissed shamardy’s stale review via 1717207 April 24, 2023 12:19

rozhkovdmitrii requested a review from shamardy April 24, 2023 12:20

shamardy previously approved these changes Apr 24, 2023

View reviewed changes

rozhkovdmitrii dismissed shamardy’s stale review via 1b0cd3a April 24, 2023 14:05

rozhkovdmitrii force-pushed the 1745-clippy-all-features branch from 1717207 to 1b0cd3a Compare April 24, 2023 14:05

rozhkovdmitrii force-pushed the 1745-clippy-all-features branch 3 times, most recently from 14ecd3d to 919fba6 Compare April 24, 2023 14:16

onur-ozkan reviewed Apr 25, 2023

View reviewed changes

mm2src/coins/z_coin.rs Show resolved Hide resolved

mm2src/mm2_main/tests/mm2_tests/mod.rs Show resolved Hide resolved

laruh previously approved these changes Apr 25, 2023

View reviewed changes

rozhkovdmitrii dismissed laruh’s stale review via c1c48dd April 25, 2023 11:04

rozhkovdmitrii force-pushed the 1745-clippy-all-features branch from 919fba6 to c1c48dd Compare April 25, 2023 11:04

rozhkovdmitrii requested a review from onur-ozkan April 25, 2023 11:11

rozhkovdmitrii force-pushed the 1745-clippy-all-features branch 2 times, most recently from 83dd62f to 92dc9d3 Compare April 25, 2023 11:47

rozhkovdmitrii requested review from laruh and shamardy April 25, 2023 14:36

onur-ozkan previously approved these changes Apr 25, 2023

View reviewed changes

onur-ozkan added the under review label Apr 25, 2023

rozhkovdmitrii dismissed onur-ozkan’s stale review via d61f2e9 April 26, 2023 07:29

rozhkovdmitrii added 5 commits April 26, 2023 12:35

add fn is_sapling_state_synced for ZCoin

8dbddde

refactor: Move enable_z_coin function up into mm2_test_module

0b09a51

ci: get rid of clippy warnings on CI with --all-features flag

59492f6

test: update PIRATE test urls

8372f27

doc: Update CHANGELOG.md

d106978

rozhkovdmitrii force-pushed the 1745-clippy-all-features branch from d61f2e9 to d106978 Compare April 26, 2023 07:36

onur-ozkan merged commit ef82cb7 into dev Apr 26, 2023

onur-ozkan deleted the 1745-clippy-all-features branch April 26, 2023 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable clippy --all-features on CI #1760

Enable clippy --all-features on CI #1760

rozhkovdmitrii commented Apr 19, 2023

caglaryucekaya commented Apr 19, 2023

onur-ozkan Apr 19, 2023 •

edited

Loading

shamardy Apr 19, 2023

laruh Apr 19, 2023

rozhkovdmitrii Apr 20, 2023

shamardy commented Apr 20, 2023

shamardy left a comment

rozhkovdmitrii commented Apr 21, 2023

shamardy left a comment

shamardy Apr 21, 2023

rozhkovdmitrii Apr 22, 2023

shamardy Apr 24, 2023

shamardy Apr 24, 2023 •

edited

Loading

rozhkovdmitrii Apr 24, 2023

shamardy Apr 21, 2023

rozhkovdmitrii Apr 22, 2023

shamardy Apr 24, 2023 •

edited

Loading

rozhkovdmitrii Apr 24, 2023

shamardy left a comment

shamardy left a comment

onur-ozkan left a comment

laruh left a comment

laruh commented Apr 25, 2023 •

edited

Loading

rozhkovdmitrii commented Apr 25, 2023

onur-ozkan left a comment •

edited

Loading

rozhkovdmitrii commented Apr 26, 2023

		let z_key = decode_extended_spending_key(z_mainnet_constants::HRP_SAPLING_EXTENDED_SPENDING_KEY, "secret-extended-key-main1q0k2ga2cqqqqpq8m8j6yl0say83cagrqp53zqz54w38ezs8ly9ly5ptamqwfpq85u87w0df4k8t2lwyde3n9v0gcr69nu4ryv60t0kfcsvkr8h83skwqex2nf0vr32794fmzk89cpmjptzc22lgu5wfhhp8lgf3f5vn2l3sge0udvxnm95k6dtxj2jwlfyccnum7nz297ecyhmd5ph526pxndww0rqq0qly84l635mec0x4yedf95hzn6kcgq8yxts26k98j9g32kjc8y83fe").unwrap().unwrap();
		let CoinProtocol::ZHTLC(protocol_info) = serde_json::from_value::<CoinProtocol>(conf["protocol"].take()).unwrap() else { unimplemented!() };

Enable clippy --all-features on CI #1760

Enable clippy --all-features on CI #1760

Conversation

rozhkovdmitrii commented Apr 19, 2023

caglaryucekaya commented Apr 19, 2023

onur-ozkan Apr 19, 2023 • 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

shamardy commented Apr 20, 2023

shamardy left a comment

Choose a reason for hiding this comment

rozhkovdmitrii commented Apr 21, 2023

shamardy left a 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

shamardy Apr 24, 2023 • 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

shamardy Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

laruh commented Apr 25, 2023 • edited Loading

rozhkovdmitrii commented Apr 25, 2023

onur-ozkan left a comment • edited Loading

Choose a reason for hiding this comment

rozhkovdmitrii commented Apr 26, 2023

onur-ozkan Apr 19, 2023 •

edited

Loading

shamardy Apr 24, 2023 •

edited

Loading

shamardy Apr 24, 2023 •

edited

Loading

laruh commented Apr 25, 2023 •

edited

Loading

onur-ozkan left a comment •

edited

Loading