Add shield for display API #374

giannis-rdx · 2025-02-06T13:59:13Z

This PR is the last part of the ABW-4146.

It adds an API to

get entities linked to a given security structure
get shields for display

codecov · 2025-02-06T14:10:31Z

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.68%. Comparing base (059e84b) to head (9945b92).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ferences/security_structures/shield_for_display.rs	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #374      +/-   ##
==========================================
+ Coverage   92.66%   92.68%   +0.02%     
==========================================
  Files        1187     1190       +3     
  Lines       26781    26861      +80     
  Branches       85       85              
==========================================
+ Hits        24816    24896      +80     
  Misses       1954     1954              
  Partials       11       11

Flag	Coverage Δ
kotlin	`98.64% <ø> (ø)`
rust	`92.32% <97.50%> (+0.02%)`	⬆️
swift	`92.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CyonAlexRDX · 2025-02-06T14:15:03Z

..._SPLIT_ME/src/logic/profile_network/profile_network_entities_linked_to_security_structure.rs

+        &self,
+        shield_id: SecurityStructureID,
+    ) -> Result<EntitiesLinkedToSecurityStructure> {
+        let accounts = self


nit: does this work:

fn filter<E>(entity: &E) -> bool where E: HasSecurityState { entity.is_linked_to_security_structure(shield_id) }

and then .filter(filter) in the four places below.

or does compiler complain about shield_id being captured?

can you do `let filter = |entity: &(impl HasSecurityState)| { .. }; ? or does that not work?

mostly curiuos...

this might help as guide

I saw it and indeed it works, but I didn't see any value, that's why I passed.

If you want I can add it.

DRY

But only truly helps IMO if one can pass .filter(filter) which is possible in Swift, but trickier in Rust since the fn or closure captures the shield_id variable

CyonAlexRDX · 2025-02-06T14:20:55Z

crates/profile/models/base-entity/src/base_entity/has_security_state.rs

+                {
+                    return provisional_security_structure
+                        .security_structure_id
+                        == shield_id;


undefined behaviour here... if the provisional is different, then we return false here, even though the actual ("committed") is linked. So you need to doc this method what is it suppose to do? what does linked mean? I think linked means "provisionally_referenced_or_de_facto_in_use" ?

I would actually split this into three methods:

trait HasSecurityState { fn is_currently_or_provisionally_securified_with(&self, shield_id: SecurityStructureID) { self.is_currently_securified_with(shield_id) || self.is_provisionally_securified_with(shield_id) } fn is_currently_securified_with(&self, shield_id: SecurityStructureID) { self.security_state().as_securified().map(|s| s.shield_id == shield_id).unwrap_or(false) } fn is_provisionally_securified_with(&self, shield_id: SecurityStructureID) { self.provisional_security_config.map(|s| s.shield_id() == shield_id).unwrap_or(false) } }

impl ProvisionalSecurityConfig { fn shield_id(&self) -> bool { match self { ... } } }

@giannis-rdx furthermore, this should be in profile-logic crate. All Profile logic goes into profile-logic crate :)

if the provisional is different, then we return false here, even though the actual ("committed") is linked

That is the behaviour we want, or at least agreed to - if an account has a provisional shield set, then that is shown throughout the app. What you are proposing implies showing users that an account is linked to two shields at the same time, no?

@GhenadieVP my point is that is_linked is too broad a term. so the method should be renamed and it should it should be documented.

CyonAlexRDX · 2025-02-06T14:21:19Z

crates/profile/models/base-entity/src/base_entity/has_security_state.rs

+                {
+                    return provisional_security_structure
+                        .security_structure_id
+                        == shield_id;


undefined behaviour here... if the provisional is different, then we return false here, even though the actual ("committed") is linked. So you need to doc this method what is it suppose to do? what does linked mean? I think linked means "provisionally_referenced_or_de_facto_in_use" ? if so, you should provisional_security_structure .security_structure_id == shield_id || value.security_structure.security_structure_id == shield_id

crates/uniffi/uniffi_SPLIT_ME/src/profile/supporting_types/shield_for_display.rs

...iffi/uniffi_SPLIT_ME/src/system/sargon_os/sargon_os_entities_linked_to_security_structure.rs

crates/system/os/factors/src/sargon_os_security_structures.rs

crates/profile/models/app-preferences/src/shield_for_display.rs

..._SPLIT_ME/src/logic/profile_network/profile_network_entities_linked_to_security_structure.rs

Sajjon · 2025-02-06T17:35:23Z

crates/profile/models/app-preferences/src/shield_for_display.rs

+#[display("{}", self.metadata.display_name)]
+pub struct ShieldForDisplay {
+    pub metadata: SecurityStructureMetadata,
+    pub number_of_linked_accounts: usize,


this can probably be put in profile-logic crate. it is not a model used by the Profile.

I agree with you on that
However, the reasons I placed it there are as follows:

there is already an existing account_for_display which also is not used by the Profile. For consistency, I added shield_for_display here.

I am not sure where exactly I should place it in the profile-logic crate. We do not have a model directory there... Oops! model ≠ logic :/

account_for_display should be moved to profile-logic too. :) The "logic" in profile-logic does not mean "cannot contain structs or enums". It means "here is code that DOES stuff using models from Profile models crate OR using models declared in this crate". Where as profile model crates should as much as possible just be the Serde impl of Profile model. Without logic. Why? Well since we might need to migrate v100 Profile to v101 Profile someday and then we need to copy paste all profile models and do some trait IntoLatest and impl that for v100 models. But we should not duplicate logic, neither should the logic operate on v100 models then, only v101 models.

ok where do you think we should place it in profile-logic?
under app_preferences?
or create a new directory security_structures and place it there?

either is fine :)

Sajjon · 2025-02-06T17:36:47Z

...T_ME/src/v100/entities_linked_to_security_structure/entities_linked_to_security_structure.rs

+
+/// This is the result of checking what entities are linked to a given `SecurityStructure`.
+#[derive(Clone, Debug, PartialEq)]
+pub struct EntitiesLinkedToSecurityStructure {


Not Profile model so move to profile-logic crate I think.

Furthermore, this model does not contain which Shield ID, which looks like an oversight! ~~If you include it the code later can be more functional.~~

If you include the SecurityStructureMetadata then you can do more functional stuff below:

https://github.com/radixdlt/sargon/pull/374/files#r1945169455

Not Profile model so move to profile-logic crate I think.

now that i see this then better to have a security_structures in profile-logic crate and place the EntitiesLinkedToSecurityStructure and ShieldForDisplay there.

right?

Not Profile model so move to profile-logic crate I think.

done

Furthermore, this model does not contain which Shield ID, which looks like an oversight! If you include it the code later can be more functional.
If you include the SecurityStructureMetadata ...

if I include the SecurityStructureMetadata in the EntitiesLinkedToSecurityStructure, I need:

either update the API

pub async fn entities_linked_to_security_structure( &self, metadata: SecurityStructureMetadata, // from shield_id: SecurityStructureID profile_to_check: ProfileToCheck, ) // so I can pass the metadata all the way down to build the `EntitiesLinkedToSecurityStructure` struct.

or leave the API as it is with shield_id: SecurityStructureID and internally fetch the SecurityStructureMetadata.

I think the latter is more appropriate.
what do you think @CyonAlexRDX ?

edit: done

Sajjon · 2025-02-06T17:44:15Z

crates/system/os/factors/src/sargon_os_security_structures.rs

+                )
+                .await?;
+
+            shields_for_display.insert(ShieldForDisplay {


Should use constructor ShieldForDisplay::new also prefer functional:

let security_structures = self.security_structures_of_factor_source_ids()?; let shields_for_display = security_structures .iter() .map(|shield| shield.metadata) .map(|metadata| self.entities_linked_to_security_structure(metadata, ProfileToCheck::Current)) .map(ShieldForDisplay::new) .collect::<ShieldsForDisplay>(); Ok(shields_for_display)

which I think is much more clean

although I updated the EntitiesLinkedToSecurityStructure struct by adding the metadata: SecurityStructureMetadata, this functional code doesn't work because map doesn't support async

Unless there is another way that I am not aware of.

@giannis-rdx you should be able to use futures::future::join_all, so:

let security_structures = self.security_structures_of_factor_source_ids()?; let shields_for_display = security_structures .iter() .map(|shield| shield.metadata) .map(|metadata| self.entities_linked_to_security_structure(metadata, ProfileToCheck::Current)) .map(ShieldForDisplay::new) let shields_for_display = futures::future::join_all(shields_for_display) .await .collect::<ShieldsForDisplay>(); Ok(shields_for_display)

I think that should work.

See join_all, which does preserve order:

The returned future will drive execution for all of its underlying futures, collecting the results into a destination Vec in the same order as they were provided.

no it doesn't work. I get a couple of errors:

function is expected to take 1 argument, but it takes 5 arguments .map(ShieldForDisplay::new); Vec<_> is not an iterator --> crates/system/os/factors/src/sargon_os_security_structures.rs:262:14 | 260 | let shields_for_display = futures::future::join_all(shields_for_display)

the only way to make it work is this:

let security_structures = self.security_structures_of_factor_source_ids()?; let shields_for_display_futures = security_structures.iter().map(|security_structure| { let metadata = security_structure.metadata.clone(); async { let linked_entities = self .entities_linked_to_security_structure( metadata.id, ProfileToCheck::Current, ) .await?; Ok(ShieldForDisplay::new( metadata, linked_entities.accounts.len(), linked_entities.hidden_accounts.len(), linked_entities.personas.len(), linked_entities.hidden_personas.len(), )) } }); let shields_for_display = futures::future::join_all(shields_for_display_futures) .await .into_iter() .collect::<Result<ShieldsForDisplay>>()?; Ok(shields_for_display) }

which is quite ugly.

I think I will stick to my original solution. Way simpler and clean.

What I mean by ShieldForDisplay::new if ofc a constructor which takes a EntitiesLinkedToSecurityStructure... maybe fn with_linked and use .map(ShieldForDisplay::with_linked) :)

Sajjon

Great first start! Some crate admin needed. Logic into logic :)

CyonAlexRDX

LGTM! Hope join_all works as I thought, else lets skip it.

CyonAlexRDX · 2025-02-11T06:37:17Z

crates/system/os/factors/Cargo.toml

@@ -32,3 +32,4 @@ preinterpret = { workspace = true }
 pretty_assertions = { workspace = true }
 serde = { workspace = true }
 serde_json = { workspace = true }
+futures = "0.3.31"


Use workspace, always

get_shields_for_display for SargonOS

…struct

LinkedToSecurityStructure trait/functions

updated ProfileNetworkQueryEntitiesLinkedToSecurityStructure

CyonAlexRDX

Good job

giannis-rdx requested review from CyonAlexRDX, GhenadieVP, micbakos-rdx, danvleju-rdx, sergiupuhalschi-rdx and matiasbzurovski February 6, 2025 13:59

giannis-rdx self-assigned this Feb 6, 2025

giannis-rdx changed the title ~~Ass shield for display API~~ Add shield for display API Feb 6, 2025

CyonAlexRDX reviewed Feb 6, 2025

View reviewed changes

danvleju-rdx reviewed Feb 6, 2025

View reviewed changes

Sajjon reviewed Feb 6, 2025

View reviewed changes

crates/profile/models/app-preferences/src/shield_for_display.rs Outdated Show resolved Hide resolved

Sajjon reviewed Feb 6, 2025

View reviewed changes

..._SPLIT_ME/src/logic/profile_network/profile_network_entities_linked_to_security_structure.rs Show resolved Hide resolved

Sajjon reviewed Feb 6, 2025

View reviewed changes

giannis-rdx added Rust 🦀 Changes in Rust Sargon UniFFI 🦄 Changes in UniFFI exported APIs labels Feb 10, 2025

CyonAlexRDX approved these changes Feb 10, 2025

View reviewed changes

matiasbzurovski mentioned this pull request Feb 10, 2025

[ABW-4080] Shields List + Change main shield radixdlt/babylon-wallet-ios#1456

Merged

CyonAlexRDX reviewed Feb 11, 2025

View reviewed changes

danvleju-rdx approved these changes Feb 11, 2025

View reviewed changes

giannis-rdx added 5 commits February 11, 2025 10:24

added EntitiesLinkedToSecurityStructure struct

c101ec7

added is_linked_to_security_structure logic in has_security_state

44690e8

added ProfileNetworkQueryEntitiesLinkedToSecurityStructure

ed85a54

added ShieldForDisplay struct

976e0bc

added entities_linked_to_security_structure and

8b8fe7f

get_shields_for_display for SargonOS

giannis-rdx added 9 commits February 11, 2025 10:24

added missing uniffi functions and modules

0478c8c

moved functions and models not related to the profile to profile-logic

34553b4

fixed import in entities_linked_to_security_structure

7b6888d

added SecurityStructureMetadata in EntitiesLinkedToSecurityStructure …

c14348e

…struct

this is magic

f9ac255

moved query_security_structures to security structures crate and

0f33860

LinkedToSecurityStructure trait/functions

removed is_linked_to_security_structure from has_security_state and

33ae62e

updated ProfileNetworkQueryEntitiesLinkedToSecurityStructure

added one more test in sargon_os_security_structures

18834e7

use workspace

829526a

giannis-rdx force-pushed the gt/add-shield-for-display-api-abw4146 branch from 1c50e93 to 829526a Compare February 11, 2025 08:37

giannis-rdx added 3 commits February 11, 2025 10:45

manually bump cargo to 1.1.136

e19407a

manually bump cargo to 1.1.136 and bytes too

9945b92

CyonAlexRDX approved these changes Feb 11, 2025

View reviewed changes

giannis-rdx merged commit 8b3f5c7 into main Feb 11, 2025
13 checks passed

giannis-rdx deleted the gt/add-shield-for-display-api-abw4146 branch February 11, 2025 09:26

Add shield for display API #374

Add shield for display API #374

Conversation

giannis-rdx commented Feb 6, 2025 • edited by jira bot Loading

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sajjon Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyonAlexRDX Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannis-rdx Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sajjon Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sajjon Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

giannis-rdx Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannis-rdx Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

giannis-rdx Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

Sajjon Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannis-rdx Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sajjon left a comment

Choose a reason for hiding this comment

CyonAlexRDX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyonAlexRDX left a comment

Choose a reason for hiding this comment

giannis-rdx commented Feb 6, 2025 •

edited by jira bot

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

Sajjon Feb 6, 2025 •

edited

Loading

CyonAlexRDX Feb 6, 2025 •

edited

Loading

giannis-rdx Feb 7, 2025 •

edited

Loading

Sajjon Feb 7, 2025 •

edited

Loading

Sajjon Feb 6, 2025 •

edited

Loading

giannis-rdx Feb 7, 2025 •

edited

Loading

giannis-rdx Feb 7, 2025 •

edited

Loading

giannis-rdx Feb 9, 2025 •

edited

Loading

Sajjon Feb 6, 2025 •

edited

Loading

giannis-rdx Feb 10, 2025 •

edited

Loading