[WIP] Apply Shield Wallet Interaction Part 2 #355

CyonAlexRDX · 2025-01-23T08:22:11Z

Caution

Sargon breaking change in Profile format - only Wallet team devs affected since models are MFA models end users have not been able to produce yet.
renamed "auto_confirm" to "time_based_confirmation" instead since "auto" was indeed very misleading! Since we MUST call confirm in a manifest and submit to the network. Is does not happen "automatically"!

Tip

Not ready to be merged but I need early input from protocol team on logic

Note

Part 2 of N
Part 1 merged into main

Update Factors of Securified Entities

For securified entities we need six different manifests to sign depending on which roles the user is able to exercise at the moment of applying the shield.

Host will display the variant which is exercising all roles with confirmation rule using quick confirm - but if user slides to sign, Sargon will under the hood try to sign all six (maybe less if we can finish early). If she does not have access to some factor belonging to Primary role she can skip signing with that factor and all manifest variants out of those six which requires Primary will be invalid. We can still withdraw XRD from AccessController XRD vault so we can continue trying sign manifests requiring Recovery role. Just that we cannot top up the XRD vault of the access controller. If she manages to sign with the required factors of Recovery role we proceed trying to sign with factors of ConfirmationRole. Which can fail too, then we use time based fallback, if user was unable to exercise Primary Role and Reocery role we fail.

This PR adds logic for creating all six flavours:

pub enum RolesExercisableInTransactionManifestCombination {

InitiateWithPrimaryCompleteWithRecovery,
    InitiateWithPrimaryCompleteWithConfirmation,
    InitiateWithRecoveryCompleteWithConfirmation,

    InitiateWithRecoveryDelayedCompletion,

   
    /// ‼️ REQUIRES "Dugong" ‼️
    InitiateWithPrimaryDelayedCompletion,
}

Sajjon · 2025-01-24T09:12:06Z

...nsaction/manifests/src/manifests_security_shield/access_controller_factors_and_time_input.rs

+};
+
+#[derive(Debug, Clone)]
+pub struct AccessControllerFactorsAndTimeInput {


shared type to create:

AccessControllerInitiateRecoveryAsPrimaryInput

AccessControllerInitiateRecoveryAsRecoveryInput

AccessControllerQuickConfirmPrimaryRoleRecoveryProposalInput

AccessControllerQuickConfirmRecoveryRoleRecoveryProposalInput

AccessControllerTimedConfirmRecoveryInput

Input to call_method instruction, using SecurityStructureOfFactorInstances - which is Into<RuleSet> (and time u32).

Sajjon · 2025-01-24T09:13:10Z

...ction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs

+        let entity_address = securified_entity.entity.address();
+
+        // ACCESS_CONTROLLER_CREATE_PROOF_IDENT
+        let mut builder = TransactionManifest::produce_owner_badge(


This does:

builder.call_method( access_controller_address.scrypto(), ACCESS_CONTROLLER_CREATE_PROOF_IDENT, (), );

Sajjon · 2025-01-24T09:15:28Z

...ction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs

+
+        // INITIATE RECOVERY
+        let (init_method, init_input) =
+            kind.input_for_initialization(factors_and_time_input);


Depending on selector of which of the six TransactionManifest flavours as selected by kind: TransactionManifestApplySecurityShieldKind we use different roles for initialization of recovery (and the correct concrete scrypto "Input"-type (Box dyn-ed))

For details see transaction_manifest_apply_security_shield_kind.rs which was added by this PR

codecov · 2025-01-24T09:16:27Z

Codecov Report

Attention: Patch coverage is 88.94009% with 24 lines in your changes missing coverage. Please review.

Project coverage is 92.40%. Comparing base (22a4d57) to head (15e0558).

Files with missing lines	Patch %	Lines
...eld/sargon_os_apply_security_shield_interaction.rs	0.00%	6 Missing ⚠️
...exercisable_in_transaction_manifest_combination.rs	80.00%	5 Missing ⚠️
...eld/manifests_securify_shield_securified_entity.rs	89.18%	4 Missing ⚠️
...shield/access_controller_factors_and_time_input.rs	85.71%	3 Missing ⚠️
.../models/supporting-types/src/securified_persona.rs	66.66%	2 Missing ⚠️
...urity_shield/top_up_access_controller_xrd_vault.rs	91.66%	2 Missing ⚠️
...ces_structures/matrices/builder/matrix_template.rs	75.00%	1 Missing ⚠️
...structures/matrices/matrix_of_factor_source_ids.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   92.59%   92.40%   -0.20%     
==========================================
  Files        1169      890     -279     
  Lines       26153    22859    -3294     
  Branches       81       81              
==========================================
- Hits        24217    21122    -3095     
+ Misses       1925     1726     -199     
  Partials       11       11

Flag	Coverage Δ
kotlin	`97.73% <ø> (ø)`
rust	`92.07% <88.94%> (-0.14%)`	⬇️
swift	`?`

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.

...ction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs

Sajjon · 2025-01-24T09:16:49Z

...ction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs

+        // Set Rola Key
+        let should_set_rola_key = security_structure_of_factor_instances
+            .authentication_signing_factor_instance
+            != securified_entity


dont set ROLA key if it is unchanged

Sajjon · 2025-01-24T09:18:24Z

...ion/manifests/src/manifests_security_shield/manifests_securify_shield_unsecurified_entity.rs

-                owner_badge_bucket_name,
-            );
-        };
+        let (mut builder, owner_badge_bucket) = Self::put_owner_badge_in_bucket(


still calls "securify" - just refactored a bit.

Sajjon · 2025-01-24T09:19:29Z

...n/manifests/src/manifests_security_shield/transaction_manifest_apply_security_shield_kind.rs

+    PrimaryAndExplicitConfirmation,
+
+    /// (Primary Time) ‼️ REQUIRES "Dugong" ‼️
+    PrimaryWithTimedAutoConfirm,


@GhenadieVP I think Matt said we would have Dugong available. So we will remove this comment once Dugong is live. Maybe I should comment out PrimaryWithTimedAutoConfirm for now...

Sajjon · 2025-01-24T09:22:19Z

...n/manifests/src/manifests_security_shield/transaction_manifest_apply_security_shield_kind.rs

+    pub fn input_for_initialization(
+        &self,
+        factors_and_time: &AccessControllerFactorsAndTimeInput,
+    ) -> (&'static str, Box<dyn CallMethodInput>) {


@dhedey naivly I tried returning (&'static str, impl ResolvableArguments) which did not compile. Then I tried Box<dyn ResolvableArguments> which compiled here, but did not compile when I tried passing it to call_method directly, so I tried .deref() on the box to get the "inner" dyn ResolvableArguments which did not compile... bah. But when I introduced this "dummy trait" (see above):

// Trickery to allow `Box<dyn ResolvableArguments>` - which is not allowed it seems, // but this solves it, since in Scrypto they impl ResolvableArguments for ManifestEncode + ManifestSborTuple pub trait CallMethodInput: ManifestEncode + ManifestSborTuple {} impl<T: ManifestEncode + ManifestSborTuple> CallMethodInput for T {}

then everything works. It is just 2 lines of code so fine for now. But maybe you have ideas on how it can be removed? :)

Love it, the reason why that trick works is subtle (basically T: CallMethodInput => T: ManifestEncode + ManifestSborTuple => Box<T>: ManifestEncode + ManifestSborTuple(via blanket impls on Box<T> of ManifestEncode and ManifestSborTuple) => Box<T>: ResolvableArguments).

The trick seems fine to me.

Other fixes in the repo would be to re-work the ResolvableArguments trait in the scrypto repo, either:

To itself have the : ManifestEncode + ManifestSborTuple bounds

To be dyn-friendly, e.g. changing the self to &self. Currently Box<dyn ResolvableArguments> has no methods because the resolve method isn't dyn-friendly.

pub trait ResolvableArguments { fn resolve(self) -> ManifestValue; }

Sajjon · 2025-01-24T09:23:22Z

...n/manifests/src/manifests_security_shield/transaction_manifest_apply_security_shield_kind.rs

+                (SCRYPTO_ACCESS_CONTROLLER_QUICK_CONFIRM_PRIMARY_ROLE_RECOVERY_PROPOSAL_IDENT, Box::new(ScryptoAccessControllerQuickConfirmPrimaryRoleRecoveryProposalInput::from(factors_and_time)))
+            }
+        } else {
+            // Time based


See https://github.com/radixdlt/sargon/pull/355/files#r1928373228 I think this is wrong, should probably update this method to return Option<(&'static str, Box<dyn CallMethodInput>)> and then None if we cannot quick confirm.

Sajjon · 2025-01-24T09:23:52Z

...action/manifests/src/manifests_security_shield/transaction_manifest_owner_badge_producing.rs

+            EntitySecurityState::Securified { value } => {
+                builder = builder.call_method(
+                    value.access_controller_address.scrypto(),
+                    ACCESS_CONTROLLER_CREATE_PROOF_IDENT,


Is ACCESS_CONTROLLER_CREATE_PROOF_IDENT correct for when we are performing recovery?

Maybe this question does not make any sense... but I thought we always needed to invoke an instruction which triggers requirement to "auth". but maybe the INITIATE recovery methods will do that...?

…56_part2

CyonAlexRDX · 2025-01-24T10:01:16Z

...ction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs

+        let access_controller_address = securified_entity
+            .securified_entity_control
+            .access_controller_address;
+


I have forgotten to add the withdrawal of XRD from AccessControllera XRD vault.

I’m adding it now

Right we need to add the locking of fee against XRD vault of AccessController later (like the top up) - since we dont know how big a fee to lock. Ive added modify_manifest_add_lock_fee_against_xrd_vault_of_access_controller. Take a look at it 4d8fc6d

Sajjon · 2025-01-24T11:13:14Z

...n/manifests/src/manifests_security_shield/lock_fee_against_xrd_vault_of_access_controller.rs

+            .securified_entity_control
+            .access_controller_address;
+        // Lock fee against XRD vault of the access controller
+        builder = builder.call_method(


@0xOmarA here I'm appending this instruction last. Is that OK? Or must this be prepended, index 0 of the manifest? if so I need to do a lot of trickery to do that. Since I cannot create the instruction in rust code "manually" and since calling buider.call_method will put it last...

@GhenadieVP alternatively we just lock a "large" amount. Which ""should work"" for most shields... and if we do that we can lock fee in apply_security_shield_for_securified_entity at index 0 https://github.com/radixdlt/sargon/blob/ac/wallet_interaction_applying_shield_abw-4056_part2/crates/transaction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs#L37

Sajjon · 2025-01-24T13:21:31Z

crates/transaction/manifests/src/manifests_security_shield/confirm_timed_recovery.rs

+    /// be quick confirmed. We need to figure out how we best represent this
+    /// in Profile. Perhaps a new variant on ProvisionalSecurityConfig? Something
+    /// like:
+    /// `WaitingForTimedRecovery(SecurityStructureOfFactorInstances)`


@GhenadieVP good thing we kept ProvisionalSecurifiedConfig an enum! I think we need a second variant - WaitingForTimedRecovery(SecurityStructureOfFactorInstances)

…ear that it does not happen automagically.

Sajjon · 2025-01-24T13:51:06Z

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

@@ -247,7 +247,7 @@ mod tests {
                                    }
                                ]
                            },
-                            "numberOfDaysUntilAutoConfirm": 14


Breaking profile change! But ok since only Radix Wallet team devs are affected.

numberOfDaysUntilAutoConfirm made it sound like confirmation would happen automagically. Which perhaps even I believed they would when I named this variable...

Sajjon · 2025-01-24T13:51:15Z

...curity-structures/src/roles_matrices_structures/matrices/abstract_matrix_builder_or_built.rs

@@ -39,13 +39,14 @@ pub struct AbstractMatrixBuilderOrBuilt<
    pub(crate) confirmation_role:
        AbstractRoleBuilderOrBuilt<{ ROLE_CONFIRMATION }, MODE_OF_ROLE, FACTOR>,

-    pub number_of_days_until_auto_confirm: u16,
+    pub number_of_days_until_timed_confirmation_is_callable: u16,


Breaking profile change! But ok since only Radix Wallet team devs are affected.

numberOfDaysUntilAutoConfirm made it sound like confirmation would happen automagically. Which perhaps even I believed they would when I named this variable...

Sajjon · 2025-01-24T13:51:43Z

...s/profile/models/security-structures/src/roles_matrices_structures/matrices/builder/error.rs

-    #[error("The number of days until auto confirm must be greater than zero")]
-    NumberOfDaysUntilAutoConfirmMustBeGreaterThanZero,
+    #[error("The number of days until timed confirm is callable must be greater than zero")]
+    NumberOfDaysUntilTimeBasedConfirmationMustBeGreaterThanZero,


Breaking Profile format change number 2 - @sergiupuhalschi-rdx / @danvleju-rdx / @matiasbzurovski might get affected in host

Sajjon · 2025-01-24T13:52:12Z

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

@@ -379,17 +379,20 @@ impl MatrixBuilder {
        self.primary_role.get_threshold()
    }

-    pub fn set_number_of_days_until_auto_confirm(
+    pub fn set_number_of_days_until_timed_confirmation_is_callable(


Breaking API change - @sergiupuhalschi-rdx / @danvleju-rdx / @matiasbzurovski will get affected in host

Sajjon · 2025-01-24T13:55:27Z

...ction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs

+        );
+
+        // QUICK CONFIRM RECOVERY - Only if we can exercise the confirmation role explicitly.
+        if let Some((confirm_method, confirm_input)) =


Only call "quick confirm" if we can exercise confirmation role explicitly. Else we skip quick confirm and we will LATER (number_of_days_until_timed_confirmation_is_callable later) make another transaction.

Sajjon · 2025-01-24T13:57:11Z

crates/uniffi/uniffi_SPLIT_ME/src/profile/mfa/security_structures/matrices/decl_matrix_macro.rs

@@ -25,7 +25,7 @@ macro_rules! matrix_conversion {
                pub recovery_role: #recovery_role_type,
                pub confirmation_role: #confirmation_role_type,

-                pub number_of_days_until_auto_confirm: u16,
+                pub number_of_days_until_timed_confirmation_is_callable: u16,


@GhenadieVP we should probably change this to number_of_minutes though! Which easily support days. But if we ever wanted to allow less than a day in the future we cannot. i.e. the host and builders will continue using Day as smallest granularity, but Profile probably SHOULD use Minute as time unit - since that is what rest of Radix Stack uses. Right? If you agree, I will make that change now - so we dont have to have two consequetive breaking profile changes on top of each other.

dhedey · 2025-01-24T14:21:44Z

...n/manifests/src/manifests_security_shield/transaction_manifest_apply_security_shield_kind.rs

+#[derive(Clone, Debug, PartialEq, Eq, Hash, enum_iterator::Sequence)]
+pub enum TransactionManifestApplySecurityShieldKind {
+    /// (Primary Recovery Confirmation)
+    PrimaryAndRecoveryWithExplicitConfirmation,


Under the two of three model, lots of these don't make sense to me.

I'd expect something like:

InitiateWithPrimaryCompleteWithRecovery

InitiateWithPrimaryCompleteWithConfirmation

InitiateWithRecoveryCompleteWithConfirmation

InitiateWithPrimaryDelayedCompletion // Either Timed or (in future) external factors

InitiateWithRecoveryDelayedCompletion // Either Timed or (in future) external factors

And then in future, there might be modes where the user can't initiate themselves and needs to send a request to an external source (e.g. a friend or custodian) to do, which might look like:

ExternalInitiateWithPrimary

ExternalInitiateWithRecovery

Yes good point.

Thanks!

I answered on Slack too

Sajjon and others added 4 commits January 23, 2025 09:20

[no ci] WIP

1a52f44

[no ci] wip

fdea748

[no ci] wip

9015925

first initial impl of manifests for updating securified entities

e7fd884

Sajjon reviewed Jan 24, 2025

View reviewed changes

...ction/manifests/src/manifests_security_shield/manifests_securify_shield_securified_entity.rs Outdated Show resolved Hide resolved

Sajjon reviewed Jan 24, 2025

View reviewed changes

CyonAlexRDX marked this pull request as ready for review January 24, 2025 09:25

CyonAlexRDX requested review from dhedey, 0xOmarA, GhenadieVP and iamyulong January 24, 2025 09:27

Sajjon added 3 commits January 24, 2025 10:27

remove unused input type

54e9967

remove unused input types

20cbf2a

Merge branch 'main' into ac/wallet_interaction_applying_shield_abw-40…

25188e9

…56_part2

CyonAlexRDX commented Jan 24, 2025

View reviewed changes

add modify_manifest_add_lock_fee_against_xrd_vault_of_access_controller

4d8fc6d

Sajjon reviewed Jan 24, 2025

View reviewed changes

Sajjon added 3 commits January 24, 2025 13:40

fix typos

f2babd1

doc

12704e2

split out confirm of time based to be done later.

58fc8cb

Sajjon reviewed Jan 24, 2025

View reviewed changes

Replace bad wording 'auto_confirm' with 'timed_confirm' to make it cl…

4007bae

…ear that it does not happen automagically.

Sajjon reviewed Jan 24, 2025

View reviewed changes

fix typo

0ce2b35

dhedey reviewed Jan 24, 2025

View reviewed changes

Sajjon added 2 commits January 24, 2025 19:21

review comments

5529715

refactor and assert manifests

15e0558

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Apply Shield Wallet Interaction Part 2 #355

[WIP] Apply Shield Wallet Interaction Part 2 #355

CyonAlexRDX commented Jan 23, 2025 •

edited

Loading

Sajjon Jan 24, 2025 •

edited

Loading

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

codecov bot commented Jan 24, 2025 •

edited

Loading

Sajjon Jan 24, 2025 •

edited

Loading

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

dhedey Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

CyonAlexRDX Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025 •

edited

Loading

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

Sajjon Jan 24, 2025

dhedey Jan 24, 2025

CyonAlexRDX Jan 24, 2025 •

edited

Loading

[WIP] Apply Shield Wallet Interaction Part 2 #355

Are you sure you want to change the base?

[WIP] Apply Shield Wallet Interaction Part 2 #355

Conversation

CyonAlexRDX commented Jan 23, 2025 • edited Loading

Update Factors of Securified Entities

Sajjon Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2025 • edited Loading

Codecov Report

Sajjon Jan 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sajjon Jan 24, 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

CyonAlexRDX Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

CyonAlexRDX commented Jan 23, 2025 •

edited

Loading

Sajjon Jan 24, 2025 •

edited

Loading

codecov bot commented Jan 24, 2025 •

edited

Loading

Sajjon Jan 24, 2025 •

edited

Loading

Sajjon Jan 24, 2025 •

edited

Loading

CyonAlexRDX Jan 24, 2025 •

edited

Loading