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

Communities: Share airdrop address #18505

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Conversation

ajayesivan
Copy link
Contributor

fixes #18081

Test Note

  • To test this, change the community-accounts-selection-enabled? flag in src/status_im2/config.cljs to true.

This feature is currently under a flag not enabled in the development branch, so these changes do not require manual QA.

@ajayesivan ajayesivan self-assigned this Jan 15, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Jan 15, 2024

Jenkins Builds

Click to see older builds (24)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f76fe68 #1 2024-01-15 12:32:29 ~5 min ios 📱ipa 📲
✔️ f76fe68 #1 2024-01-15 12:32:48 ~6 min tests 📄log
✔️ f76fe68 #1 2024-01-15 12:33:36 ~6 min android-e2e 🤖apk 📲
✔️ f76fe68 #1 2024-01-15 12:34:48 ~8 min android 🤖apk 📲
58aa3f6 #2 2024-01-15 13:59:03 ~1 min tests 📄log
✔️ 58aa3f6 #2 2024-01-15 14:03:24 ~6 min ios 📱ipa 📲
✔️ 58aa3f6 #2 2024-01-15 14:03:48 ~6 min android-e2e 🤖apk 📲
✔️ 58aa3f6 #2 2024-01-15 14:04:58 ~7 min android 🤖apk 📲
✔️ a37c433 #3 2024-01-15 15:09:18 ~5 min tests 📄log
✔️ a37c433 #3 2024-01-15 15:09:20 ~5 min ios 📱ipa 📲
✔️ a37c433 #3 2024-01-15 15:10:25 ~6 min android 🤖apk 📲
✔️ a37c433 #3 2024-01-15 15:11:02 ~7 min android-e2e 🤖apk 📲
✔️ 3476ae9 #4 2024-01-16 08:13:58 ~5 min ios 📱ipa 📲
✔️ 3476ae9 #4 2024-01-16 08:14:07 ~5 min tests 📄log
✔️ 3476ae9 #4 2024-01-16 08:16:35 ~8 min android-e2e 🤖apk 📲
✔️ 3476ae9 #4 2024-01-16 08:16:36 ~8 min android 🤖apk 📲
533e860 #5 2024-01-16 09:00:20 ~1 min tests 📄log
✔️ 533e860 #5 2024-01-16 09:04:36 ~5 min ios 📱ipa 📲
✔️ 533e860 #5 2024-01-16 09:05:46 ~6 min android-e2e 🤖apk 📲
✔️ 533e860 #5 2024-01-16 09:05:52 ~6 min android 🤖apk 📲
✔️ 3dcf187 #6 2024-01-16 09:11:42 ~5 min tests 📄log
✔️ 3dcf187 #6 2024-01-16 09:13:27 ~7 min android-e2e 🤖apk 📲
✔️ 3dcf187 #6 2024-01-16 09:13:39 ~7 min android 🤖apk 📲
✔️ 3dcf187 #6 2024-01-16 09:13:43 ~7 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 97d39d2 #8 2024-01-17 11:39:27 ~4 min tests 📄log
✔️ 97d39d2 #8 2024-01-17 11:41:20 ~6 min ios 📱ipa 📲
✔️ 97d39d2 #8 2024-01-17 11:43:44 ~9 min android 🤖apk 📲
✔️ 97d39d2 #8 2024-01-17 11:43:44 ~9 min android-e2e 🤖apk 📲
✔️ 63927e2 #10 2024-01-17 18:15:35 ~5 min tests 📄log
✔️ 63927e2 #10 2024-01-17 18:16:46 ~6 min ios 📱ipa 📲
✔️ 63927e2 #10 2024-01-17 18:17:06 ~6 min android-e2e 🤖apk 📲
✔️ 63927e2 #10 2024-01-17 18:18:19 ~8 min android 🤖apk 📲

[quo/account-item
{:account-props item
:state (if (= airdrop-address (:address item)) :selected nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use when' here since it returns 'nil if not
(when (= airdrop-address) (:address-item) :selected)

accounts (rf/sub [:wallet/accounts-with-customization-color])]
accounts (rf/sub [:wallet/accounts-with-customization-color])
selected-addresses (rf/sub [:communities/selected-permission-addresses])
selected-accounts (filter #(contains? selected-addresses (:address %)) accounts)
Copy link
Contributor

Choose a reason for hiding this comment

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

The binding selected-accounts is computed solely from the result of two subs :communities/selected-permission-addresses and :wallet/accounts-with-customization-color that don't require any argument (data fully stored in the app-db).

With that in mind, we should have a separate layer-3 subscription and unit test it. This is kind of a general strategy in re-frame to keep views dumb.

:params [(map #(assoc % :password password) sign-params)]
:on-success [:communities/request-to-join-with-signatures
{:community-id community-id
;; NOTE: After completely migrating to the new reqest to join flow we can
Copy link
Contributor

Choose a reason for hiding this comment

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

I read and re-read this comment, but I couldn't understand it. Could you elaborate a bit? Shouldn't we be using the feature flag to decide which path to take to make the events backwards compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I completely forgot that we can rely on the flag instead of using the passed value to make a decision. However, as I was running the tests, I realized the current approach of making a decision inside the event is a bad idea. The tests passed when the flag is off and failed when it is on. Relying on an outside value will make it hard to test. I think we should write new events to handle the new flow and keep the old ones untouched. What are your thoughts on this approach?

@@ -86,7 +89,9 @@
:track-text (i18n/label :t/slide-to-request-to-join)
:track-icon :i/face-id
:customization-color color
:on-complete #(join-community-and-navigate-back id selected-permission-addresses)}]]]))
:on-complete #(join-community-and-navigate-back id
selected-permission-addresses
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 to pass values that are already stored in the app-db to the event :communities/request-to-join. For example, the value of airdrop-address is computed solely from the subscription :communities/airdrop-address, which means the event can easily grab this value from the app-db. The same idea applies to selected-permission-addresses.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe is ok in this case, but in general I think on-press event should try to use parameters (or closures in this case) that are passed in the render, rather than relying on the state of the database, to the extent of where that's useful and feasible and does not occur in extra subscriptions for example.

I think it's a better to make events more independent of the database, that makes it more explicit and clearer, as otherwise there's implicit parameters being passed and it's bit harder to inspect, also there's always the possibility that the value could change after the user confirms (not in this case I guess).

In this case might be totally fine, since as far as I can see, we only subscribe to that piece of data to pass it as a parameter, but as a general rule, I think it's clearer if we tend to be more declarative and less dependent on the current state of the database in events (We have similar issues with current-chat-id, which is always an implicit parameter and we struggle to keep it synchronized with the events, although of course it's a much more complex scenario).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the events to use values from the database, but I think it's better to write new events to keep the old and new flow separate. Do you have any resource suggestions regarding best practices on this topic (events, subscriptions, etc.)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ajayesivan, in terms of best practices, I think the re-frame docs generally explain and document why certain choices should be made, but there's more than one truth, so it's up to each developer to judge each context. Based on my experience, good Redux code also translates well to re-frame, so if you have experience with it, perhaps there are good Redux sources out there.

@cammellos, I don't know the specifics of the issues with current-chat-id, but I can see why we may have/had bugs related to this. Probably places that should've received an explicit ID, instead looked for the current chat ID (?). But this is a problem we can't have here, because there's only one flow the user can select addresses to join and no other screen should refer to this app-db state for anything else.

also there's always the possibility that the value could change after the user confirms (not in this case I guess).

That's why I think we should pretty much always strive to only pass stable identifiers to subscriptions and events (like IDs, or addresses). Anything that can be derived from them should not be passed because then we can have this mid-flight state change problem you mentioned, among other problems.

And you're correct, for Ajay's PR it's fine because we are passing addresses.

as otherwise there's implicit parameters being passed and it's bit harder to inspect

👍🏼 Yes, sometimes this is a pain. Other times, when events receive fewer arguments, it makes debugging them with the REPL easier because I can check what they do without having to use re-frisk or other techniques to print the arguments being passed from views.

@ajayesivan ajayesivan force-pushed the 18081-share-airdrop-address branch 2 times, most recently from a37c433 to 3476ae9 Compare January 16, 2024 08:08
@ajayesivan
Copy link
Contributor Author

Hi @ilmotta & @cammellos,
I've implemented your suggestions and updated the PR.

Created new events for the new flow and using the values from db directly. These events will only be called when the flag is true, from the new screen. While the events themselves remain largely unchanged, I think having separate events for the new flow is better than making decisions within the event based on a flag. Would appreciate it if you could take another look. Thanks!

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.

Hi @ilmotta & @cammellos, I've implemented your suggestions and updated the PR.

Created new events for the new flow and using the values from db directly. These events will only be called when the flag is true, from the new screen. While the events themselves remain largely unchanged, I think having separate events for the new flow is better than making decisions within the event based on a flag. Would appreciate it if you could take another look. Thanks!

Thanks @ajayesivan. Given that we will replace the old flows with new flows in the not so distant future and that nobody else in the mobile team is changing the existing joining flow, to me it means it's totally safe to decouple and duplicate events/subs/etc. And we'll definitely come back to delete the current (old) code.

:<- [:communities/selected-permission-addresses]
:<- [:wallet/accounts-with-customization-color]
(fn [[selected-permission-addresses accounts]]
(filter #(contains? selected-permission-addresses (:address %)) accounts)))
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to write tests for layer-3 subs @ajayesivan. Layer-3 subs are the ones that do more than just extract data (often extractors just use get or get-in). Guideline https://github.com/status-im/status-mobile/blob/a2bf23cc1281fa10a5b44e65c4cf41c2efeda293/doc/new-guidelines.md#subscription-tests

By using deftest-sub we also make sure changes in subscriptions are interconnected. For example, introducing breaking changes in any sub that's a dependency of another sub should theoretically break both of their subscription tests. That's why it's so important to test all layer-3 subs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ilmotta, Thanks for the review and suggestion. I have added tests, could you please have a look?

Thank you!

@ajayesivan ajayesivan force-pushed the 18081-share-airdrop-address branch from cbe173d to 97d39d2 Compare January 17, 2024 11:34
@@ -459,3 +459,83 @@
(let [{:keys [formatted-balance tokens]} (rf/sub [sub-name])]
(is (match? 2 (count tokens)))
(is (match? "$4506.00" formatted-balance)))))

(h/deftest-sub :wallet/accounts-with-customization-color
Copy link
Contributor

Choose a reason for hiding this comment

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

@ajayesivan, I think it's kind of hard to understand what's being tested due to the amount of setup data. The subscription only does one assoc that copies color into a new key customization-color, so the test should try to reflect that simple behavior.

While reviewing this test I just noticed match? has a bug because = works perfectly. cc @yqrashawn if you're not aware already.

(h/deftest-sub :wallet/accounts-with-customization-color
  [sub-name]
  (testing "returns all accounts with `customization-color` copied from `color`"
    (swap! rf-db/app-db
      (fn [db]
        (-> db
            (assoc-in [:wallet :accounts] accounts)
            (assoc-in [:wallet :networks] network-data))))
    (is (= [(-> accounts
                (get "0x1")
                (assoc :customization-color :blue)
                (assoc :network-preferences-names #{:ethereum :arbitrum :optimism}))
            (-> accounts
                (get "0x2")
                (assoc :customization-color :purple)
                (assoc :network-preferences-names #{:ethereum :arbitrum :optimism}))
            (-> accounts
                (get "0x3")
                (assoc :customization-color :magenta)
                (assoc :network-preferences-names #{}))]
           (rf/sub [sub-name])))))

"0x3" {:address "0x3"
:color :purple
:name "account3"}}})
(is (= (list {:address "0x1"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the normal vector syntax here instead of list.

"0x2" {:address "0x2"
:color :orange
:name "account2"}}})
(is (= {:address "0x1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested, and match? works here, it's better than is for larger data structures.

@ajayesivan ajayesivan force-pushed the 18081-share-airdrop-address branch from 458728a to 63927e2 Compare January 17, 2024 18:09
@ajayesivan ajayesivan merged commit c888c51 into develop Jan 17, 2024
6 checks passed
@ajayesivan ajayesivan deleted the 18081-share-airdrop-address branch January 17, 2024 18:23
@ajayesivan ajayesivan restored the 18081-share-airdrop-address branch January 17, 2024 18:26
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.

Communities: Share airdrop address in non token-gated community
4 participants