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

ABW-1340 Show dApp detail view from tx review #591

Merged
merged 7 commits into from
Jun 26, 2023

Conversation

kugel3
Copy link
Contributor

@kugel3 kugel3 commented Jun 25, 2023

Jira ticket: ABW-1340

Description

NOW TESTED
With this PR it should be possible to tap on a dApp in the Using dApps section of Transaction review, to show the (simplified) dApp details corresponding to that dApp. I have not been able to test it yet.

Video

RPReplay_Final1687775344.MP4

How to test

  • Send a transaction that sets the encounteredAddresses.componentAddresses.userApplications property on the AnalyzeManifestWithPreviewContextResponse.
  • In transaction review, the corresponding dApp should appear in the Using dApps section.
  • Tap the dApp
    -> A sheet with some information about the dApp should be shown.

PR submission checklist

  • I have tested account to account transfer flow and have confirmed that it works

@@ -1,6 +1,7 @@
import CreateAuthKeyFeature
import EditPersonaFeature
import FeaturePrelude
import TransactionReviewFeature
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is PersonaDetails depending on TransactionReview?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder the same, looks wrong, same in PersonaDetails (reducer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I had to move the simplified version of dapp details to TransactionReview. It's used in both places, but if stays in PersonaDetails and we import THAT from TransactionReview then there will be circular dependencies. I would prefer it to be a variant of the normal dapp details, but that's not possible for the same reason. It's also not possible to let it be a separate feature, for the same reason.

The only two alternatives that seem to work are having SimpleDappDetails in TransactionReview and having it BOTH there and in PersonaDetails.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe do import struct TransactionReviewFeature.SimpleDappDetails.View // Circular dep requires us to have SimpleDapp in TXReview ? and analogously in reducer?

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 ended up having to actually do it in both files, since the two implementations were pretty different it made sense, for now. Rather than spending time making a temporary SimpleDappDetails that can handle both authorized dApps and general dApps.

Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

LGTM, just some small comments.

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

video :) ?

@kugel3
Copy link
Contributor Author

kugel3 commented Jun 26, 2023

This is the PR I'm not able to test, since I can't successfully set dapp definition on sandbox. But Dawid Sowa just posted a walkthrough that works for him so i will try that.

@kugel3 kugel3 merged commit 5382668 into main Jun 26, 2023
2 checks passed
@kugel3 kugel3 deleted the ABW-1340_dApp-detail-view-from-tx-review branch June 26, 2023 13:39
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants