Security Shield builder API updates #343

sergiupuhalschi-rdx · 2025-01-15T17:28:10Z

Updated the API of SecurityShieldBuilder as follows:

Added convenience function to remove all primary override factors
Added TimePeriod and TimePeriodUnit to represent in the UI the time until recovery auto-confirmation
Updated the order of the checks in the validate function to better accommodate the UI requirements. For example, in the first step in the UI, after selecting enough factor sources to build a good shield and auto-assign them to recovery and confirmation roles, and removing all but one primary threshold factor sources and the authentication signing factor, the UI needs to reflect that an authentication signing factor source has to be added before continuing to the recovery / confirmation step.

codecov · 2025-01-15T17:44:45Z

Codecov Report

Attention: Patch coverage is 95.94595% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.84%. Comparing base (50f84ed) to head (5df92fb).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../com/radixdlt/sargon/extensions/SharedConstants.kt	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   92.81%   92.84%   +0.02%     
==========================================
  Files        1154     1160       +6     
  Lines       25738    25797      +59     
  Branches       85       85              
==========================================
+ Hits        23890    23952      +62     
+ Misses       1837     1834       -3     
  Partials       11       11

Flag	Coverage Δ
kotlin	`97.88% <50.00%> (-0.07%)`	⬇️
rust	`92.37% <100.00%> (+0.03%)`	⬆️
swift	`93.76% <100.00%> (+0.01%)`	⬆️

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.

Sajjon · 2025-01-15T21:08:41Z

crates/core/utils/src/constants.rs

+pub const DAYS_PER_WEEK: u16 = 7;
+
+/// Number of days per year.
+pub const DAYS_PER_YEAR: u16 = DAYS_PER_WEEK * WEEKS_PER_YEAR;


Do we want to skip 1 day? Since this is 364.

Right, I'm not sure. Maybe it's best to make it 365 with the downside of being unable to express a year in a fixed number of weeks, but I guess it's fine since we don't need that now anyway.

CyonAlexRDX · 2025-01-16T12:52:25Z

crates/profile/models/security-structures/src/threshold.rs

@@ -23,6 +23,13 @@ impl Threshold {
            Threshold::Specific(value) => *value,
        }
    }
+
+    /// Returns the selectable values for a threshold.
+    pub fn values(max_threshold: u8) -> Vec<Threshold> {


we can do this, but should we? This genarates 9999 elements and then is UniFFI exported. probably not a huge cost but still, it is 10k elements which are uniffi converted, when you could use 2 elements instead by use of a range:

#[derive(uniffi::Record)] pub struct Range16 { pub lower: u16, pub upper: u16 }

And then in Swift Sargon or Kotlin Sargon map the range to Vec<u16>. And we know we always include Threshold::All (prepend with).

Futhermore, current impl allows for 10000 elements of Threshold::All which is weird...

Are you confusing this with the values for the emergency fallback where we have the upper bound of 9999? For the threshold, the array length would be the number of primary threshold factors the user has chosen. Also, we could use IndexSet instead of Vec to ensure the values are unique, but we'll still have to convert to Vec when exporting it through uniffi, so I'm not sure if it's worth making this change since we've covered this with a unit test.

CyonAlexRDX · 2025-01-16T12:54:32Z

crates/core/utils/src/constants.rs

+pub const DAYS_PER_YEAR: u16 = 365;
+
+/// Maximum number of units (days, weeks, years) for the security structure recovery confirmation fallback period.
+pub const MAX_RECOVERY_CONFIRMATION_FALLBACK_PERIOD_UNITS: u16 = 9999;


hmm this seems too specific to be on core crate, ought to into profile-security-structures crate ?

sergiupuhalschi-rdx · 2025-01-16T13:09:42Z

crates/profile/models/security-structures/src/time_period_unit.rs

+}
+
+impl TimePeriodUnit {
+    pub fn values(&self) -> Vec<u16> {


@CyonAlexRDX Here we have an array with 9999 u16 elements which crosses the uniffi boundary, do you think the performance cost of this is too high compared to the benefit of having this logic in a single place?

Is it useful though? the main thing you want to expose is actually the upper bound, there is no reason for this to not be contiguous, so rather maybe this should be a range instead of the actual enumerated values.

right, so here I would use a Range instead.

Right, currently it brings value only in terms of display readiness (there's nothing else to do in UI, just render the values) and in terms of the lower and upper bounds, and if we ever were to display non-consecutive values. But it's ok to have this defined in hosts as it's very UI specific.

GhenadieVP · 2025-01-16T13:44:25Z

.../models/security-structures/src/roles_matrices_structures/matrices/builder/matrix_builder.rs

+        let primary_override_factors =
+            self.primary_role.get_override_factors().clone();
+
+        primary_override_factors.iter().try_for_each(|fsid| {
+            self.remove_factor_from_primary(fsid, FactorListKind::Override)
+        })


Could you instead expose a function on primaryRole to clear all factors in override? similar to primaryRole.reset() function?

GhenadieVP · 2025-01-16T13:51:42Z

crates/profile/models/security-structures/src/time_period_unit.rs

+}
+
+impl TimePeriodUnit {
+    pub fn values(&self) -> Vec<u16> {


Is it useful though? the main thing you want to expose is actually the upper bound, there is no reason for this to not be contiguous, so rather maybe this should be a range instead of the actual enumerated values.

sergiupuhalschi-rdx added 4 commits January 15, 2025 11:17

Uniffi expose primary threshold

f624f61

Add remove all override factors function

8ecf434

Update shield validation

2921733

Add time period as recovery emergency fallback

cc68df6

sergiupuhalschi-rdx self-assigned this Jan 15, 2025

sergiupuhalschi-rdx added 3 commits January 15, 2025 20:10

Add samples. Add test

98cd8fe

Add samples. Add test

f073fe1

Add extensions

d7bdcf8

sergiupuhalschi-rdx added Swift 🍏 Changes in Swift Sargon Rust 🦀 Changes in Rust Sargon Kotlin 🤖 Changes in Kotlin Sargon labels Jan 15, 2025

Add swift tests

3df6f6e

sergiupuhalschi-rdx marked this pull request as ready for review January 15, 2025 19:29

Sajjon reviewed Jan 15, 2025

View reviewed changes

Expose functions and add tests

3ef0c5f

sergiupuhalschi-rdx requested review from GhenadieVP, giannis-rdx, micbakos-rdx, danvleju-rdx and matiasbzurovski January 16, 2025 10:26

CyonAlexRDX reviewed Jan 16, 2025

View reviewed changes

sergiupuhalschi-rdx commented Jan 16, 2025

View reviewed changes

GhenadieVP reviewed Jan 16, 2025

View reviewed changes

sergiupuhalschi-rdx added 2 commits January 16, 2025 17:50

PR review

bbcfd2f

Merge branch 'main' into feature/shield-builder-ui-models

961ebc9

danvleju-rdx approved these changes Jan 17, 2025

View reviewed changes

GhenadieVP approved these changes Jan 17, 2025

View reviewed changes

Version bump

5df92fb

sergiupuhalschi-rdx merged commit 28ac8b8 into main Jan 17, 2025
13 checks passed

sergiupuhalschi-rdx deleted the feature/shield-builder-ui-models branch January 17, 2025 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security Shield builder API updates #343

Security Shield builder API updates #343

sergiupuhalschi-rdx commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

Sajjon Jan 15, 2025

sergiupuhalschi-rdx Jan 16, 2025

CyonAlexRDX Jan 16, 2025 •

edited

Loading

sergiupuhalschi-rdx Jan 16, 2025

CyonAlexRDX Jan 16, 2025

sergiupuhalschi-rdx Jan 16, 2025

GhenadieVP Jan 16, 2025

CyonAlexRDX Jan 16, 2025

sergiupuhalschi-rdx Jan 16, 2025

GhenadieVP Jan 16, 2025

GhenadieVP Jan 16, 2025

Security Shield builder API updates #343

Security Shield builder API updates #343

Conversation

sergiupuhalschi-rdx commented Jan 15, 2025 • edited Loading

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyonAlexRDX Jan 16, 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

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

sergiupuhalschi-rdx commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

CyonAlexRDX Jan 16, 2025 •

edited

Loading