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

Apply security shield on single entity TX Review #1334

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

micbakos-rdx
Copy link
Contributor

@micbakos-rdx micbakos-rdx commented Feb 13, 2025

Description

This PR includes both the UI for showing the transaction summary for applying a security shield to an account or persona.

  • New processor was added to handle the securify classification.
  • The processor resolves the security structure by referencing the id of that security structure in the received metadata from the classifier.
  • Previously I had implemented some logic to show a bottom sheet that applies a shield to a single entity, but deleted it since now when creating a shield we can apply it to entities.
  • Glued the manifest preparation to the ApplyShield flow. Beware that a tx review can be spawned only if we try to securify a single entity. Many entities will be supported when we implement batch transactions and tx queue.
  • When securify tx succeeds, just like in deleting account, the entity is marked as securified.

How to test

  1. Create a shield
  2. Apply it to an account
  3. Create another shield (We have no ui to select already created shields)
  4. Apply it to a persona

Make sure you have XRD in an account on your profile.

Video

slack

@micbakos-rdx micbakos-rdx self-assigned this Feb 13, 2025
@micbakos-rdx micbakos-rdx force-pushed the feature/ABW-4064-transaction-review-screen branch from 6d4007b to 0f2580d Compare February 14, 2025 08:47
Copy link
Contributor

@giannis-rdx giannis-rdx left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

Choose a reason for hiding this comment

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

minor: maybe to move this to signing package? or create a new one securityshields ?

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 think we need a specific package for profile related mutations.

_state.update { state -> state.copy(isLoading = false) }
sendEvent(Event.ShieldApplied)

incomingRequestRepository.add(request)
Copy link
Contributor

Choose a reason for hiding this comment

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

just to be sure, this request gets requestHandled in the TransactionReviewViewModel, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

requestHandled is invoked either:

  • when user dismisses the transaction
  • by the tx status dialog.

Copy link
Contributor

@sergiupuhalschi-rdx sergiupuhalschi-rdx left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -568,7 +568,7 @@ private fun UnsafeCombinationInfoDialog(
}

@Composable
private fun TimePeriod.title(): String {
fun TimePeriod.title(): String {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this composable should be moved to a common package

@micbakos-rdx
Copy link
Contributor Author

Keeping the PR open, to add a withdrawing box for securifying account.

@micbakos-rdx micbakos-rdx force-pushed the feature/ABW-4064-transaction-review-screen branch from b796d26 to 481d76f Compare February 21, 2025 13:00
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube Cloud

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