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.

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

Match the changes in collectibles api in status-go #18033

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

vkjr
Copy link
Contributor

@vkjr vkjr commented Nov 30, 2023

fixes #17936

Summary

Collectible api was changed by this status-go PR.
This PR updates the mobile code to match the changes.

Review notes

Latest status-go commit used, not sure if it is better to use the commit from PR with API changes.

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Nov 30, 2023

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
61e9240 #1 2023-11-30 13:36:03 ~6 min tests 📄log
✔️ 61e9240 #1 2023-11-30 13:40:25 ~10 min android 🤖apk 📲
✔️ 61e9240 #1 2023-11-30 13:40:44 ~11 min android-e2e 🤖apk 📲
✔️ 61e9240 #1 2023-11-30 13:44:37 ~15 min ios 📱ipa 📲
e645a2a #2 2023-12-04 15:23:58 ~4 min tests 📄log
✔️ e645a2a #2 2023-12-04 15:25:59 ~6 min android-e2e 🤖apk 📲
✔️ e645a2a #2 2023-12-04 15:26:14 ~6 min android 🤖apk 📲
✔️ e645a2a #2 2023-12-04 15:26:32 ~6 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 642479b #4 2023-12-04 16:27:33 ~5 min android-e2e 🤖apk 📲
✔️ 642479b #4 2023-12-04 16:27:49 ~6 min android 🤖apk 📲
✔️ 642479b #4 2023-12-04 16:28:02 ~6 min ios 📱ipa 📲
✔️ 642479b #4 2023-12-04 16:32:14 ~10 min tests 📄log
✔️ 3c6ebc9 #5 2023-12-04 16:41:09 ~6 min ios 📱ipa 📲
✔️ 3c6ebc9 #5 2023-12-04 16:41:35 ~6 min android 🤖apk 📲
✔️ 3c6ebc9 #5 2023-12-04 16:41:45 ~6 min android-e2e 🤖apk 📲
✔️ 3c6ebc9 #5 2023-12-04 16:44:41 ~9 min tests 📄log

[:wallet/last-collectible-details])
{:keys [id collectible-data preview-url collection-data]} collectible
{:keys [traits description]} collectible-data
chain-id (get-in id [:contract-id :chain-id])]
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have a sub to access this data instead?

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, will update it!

@@ -231,18 +232,37 @@

(rf/reg-event-fx :wallet/store-last-collectible-details store-last-collectible-details)

(def collectible-data-types
Copy link
Contributor

Choose a reason for hiding this comment

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

It's very helpful during refactors to see private vars with the metadata because then we feel much safer to move things around knowing we're not missing usages. clojure-lsp helps a lot, but I don't fully trust it at all times because it's easy to get in a situation where the cache is stale while changing a lot of code.

collectibles-request-batch-size
data-type
fetch-criteria]]
{:json-rpc/call [{:method "wallet_getOwnedCollectiblesAsync"
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed we are having trouble following the re-frame guideline for using only :db and :fx in the returned map (https://github.com/status-im/status-mobile/blob/develop/doc/new-guidelines.md#registering-event-handlers). A lot of events in the repo only dispatch one effect (plus maybe the :db) so maybe we should relax that guideline. I would prefer if we had only one way of doing something and following re-frame's best practices, but it's tedious to comment in PRs to reinforce this guideline, so realistically speaking we should consider relaxing that guideline if we can't properly enforce it.

@vkjr vkjr merged commit 489557c into develop Dec 4, 2023
6 checks passed
@vkjr vkjr deleted the collectibles-api-change branch December 4, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Support traits on collectible info page (upgrade to new API)
4 participants