Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fraschetti/18343- Add contract test for wallet_getDerivedAddresses #19115

Merged
merged 5 commits into from
Mar 12, 2024
Merged

Fraschetti/18343- Add contract test for wallet_getDerivedAddresses #19115

merged 5 commits into from
Mar 12, 2024

Conversation

FFFra
Copy link
Contributor

@FFFra FFFra commented Mar 5, 2024

fixes #18343

Summary

This pull request introduces comprehensive tests for the wallet_getDerivedAddresses endpoint, ensuring its robustness and reliability in our wallet functionality.

Additionally, this work leverages and extends upon the foundational changes introduced by @J-Son89 in PR #18910.

Steps to test

run make test-contracts

status: ready!

@FFFra FFFra requested review from J-Son89 and flexsurfer March 5, 2024 18:16
@FFFra FFFra self-assigned this Mar 5, 2024
@FFFra FFFra added this to the 2.28.0 Alpha milestone Mar 5, 2024
@FFFra FFFra requested a review from ilmotta March 5, 2024 18:17
@status-im-auto
Copy link
Member

status-im-auto commented Mar 5, 2024 •
edited
Loading

Jenkins Builds

Click to see older builds (5)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ c822b1d #1 2024-03-05 18:19:06 ~1 min tests 📄log
✔️ c822b1d #1 2024-03-05 18:23:58 ~6 min android-e2e 🤖apk 📲
✔️ c822b1d #1 2024-03-05 18:24:15 ~7 min android 🤖apk 📲
✔️ c822b1d #1 2024-03-05 18:28:00 ~10 min ios 📱ipa 📲
❌ f8be4d9 #2 2024-03-11 12:52:06 ~2 min tests 📄log
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c0ec094 #4 2024-03-11 13:00:31 ~5 min tests 📄log
✔️ c0ec094 #4 2024-03-11 13:02:11 ~7 min android-e2e 🤖apk 📲
✔️ c0ec094 #4 2024-03-11 13:02:15 ~7 min android 🤖apk 📲
✔️ c0ec094 #4 2024-03-11 13:11:15 ~16 min ios 📱ipa 📲
✔️ 62f2803 #5 2024-03-12 16:11:09 ~6 min tests 📄log
✔️ 62f2803 #5 2024-03-12 16:13:16 ~8 min android-e2e 🤖apk 📲
✔️ 62f2803 #5 2024-03-12 16:13:34 ~8 min android 🤖apk 📲
✔️ 62f2803 #5 2024-03-12 16:18:15 ~13 min ios 📱ipa 📲

@FFFra FFFra requested a review from alaibe March 5, 2024 18:22
J-Son89
J-Son89 reviewed Mar 5, 2024
View reviewed changes
src/tests/contract_test/wallet_test.cljs Outdated
@@ -34,6 +36,7 @@

(defn get-default-account
[accounts]
(println "DEFAULT ACCOUNT" accounts)
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

J-Son89
J-Son89 reviewed Mar 5, 2024
View reviewed changes
src/tests/contract_test/wallet_test.cljs Outdated
{:rpc-endpoint "wallet_getDerivedAddresses"
:params [sha3-pwd main-account derivation-path]
:action assert-derived-account
:on-error (fn [error] (println "RPC Call Failed:" error))}))))))
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need on error in this case 👍

J-Son89
J-Son89 reviewed Mar 5, 2024
View reviewed changes
src/tests/contract_test/wallet_test.cljs Outdated
(is (= (:public-key response) (:public-key response)))
(is (= "m/43'/60'/1581'/0'/0" (:path (first response)))))

(deftest wallet-create-derived-addresses
Copy link
Contributor

Choose a reason for hiding this comment

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

better the test name matches the rpc event.
Can you follow the format I did in the other tests?
e.g wallet-get-derived-addressess-contract

Actually I think we need to adjust the other tests names too - we can do separately though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I will change it

J-Son89
J-Son89 approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@J-Son89 J-Son89 left a comment

Choose a reason for hiding this comment

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

nice one @FFFra

@FFFra
Copy link
Contributor Author

FFFra commented Mar 5, 2024

Thank you @J-Son89!

ilmotta
ilmotta reviewed Mar 5, 2024
View reviewed changes
src/tests/test_utils.cljs
@@ -0,0 +1,136 @@
(ns tests.test-utils
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't be copy & pasting like this. Just so you know @FFFra, this is a strategy we decided a long time ago to avoid because duplicated code leads to a lot of confusion and debt piles up quickly.

The file legacy.status-im.utils-test is only used in a few places, so it's trivial to just move it. This whole namespace is identical except for the lines below:

    :restoreAccountAndLogin
    (fn [request]
      (prn native-status)
      (.restoreAccountAndLogin native-status request))
    :loginAccount
    (fn [request] (.loginAccount native-status request))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the context, Icaro. I'm adapting =)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @FFFra 👍🏼

Copy link
Contributor

Choose a reason for hiding this comment

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

Just an FYI @FFFra As soon as you merge this PR I'll adapt it to the new API coming up in #19208. It will be quick for me, and your PR was opened and reviewed way before, so I think yours should be merged and then I'll adapt the test to the new API.

@FFFra FFFra force-pushed the fraschetti/18343-wallet-add-contract-test-for-wallet_getDerivedAddresses branch 3 times, most recently from 83fb23c to c0ec094 Compare March 11, 2024 12:54
@FFFra FFFra force-pushed the fraschetti/18343-wallet-add-contract-test-for-wallet_getDerivedAddresses branch from c0ec094 to 62f2803 Compare March 12, 2024 16:04
@FFFra FFFra requested a review from ilmotta March 12, 2024 16:04
ilmotta
ilmotta approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@ilmotta ilmotta left a comment

Choose a reason for hiding this comment

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

🚀

@FFFra FFFra merged commit 0828e9a into develop Mar 12, 2024
6 checks passed
@FFFra FFFra deleted the fraschetti/18343-wallet-add-contract-test-for-wallet_getDerivedAddresses branch March 12, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilmotta ilmotta ilmotta approved these changes

@J-Son89 J-Son89 J-Son89 approved these changes

@flexsurfer flexsurfer Awaiting requested review from flexsurfer

@alaibe alaibe Awaiting requested review from alaibe

Assignees

@FFFra FFFra

Labels
skip-manual-qa
Projects
Pipeline for QA
Archived in project
Legacy QA Automation Project
Archived in project
Milestone
2.28.0 Alpha
Development

Successfully merging this pull request may close these issues.

Wallet - Add Contract test for RPC endpoint - wallet_getDerivedAddresses
4 participants
@FFFra @status-im-auto @ilmotta @J-Son89