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 Shield Wallet Interaction Part 2 #355

Merged
merged 28 commits into from
Jan 29, 2025

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented Jan 23, 2025

Jira ABW-4056

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"! I've also changed then integer to TimePeriod

Note

Part 2 of N
Part 1 merged into main

Update Factors of Securified Entities

For securified entities we need many 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 (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 flavours:

pub enum RolesExercisableInTransactionManifestCombination {
    InitiateWithPrimaryCompleteWithRecovery,
    InitiateWithPrimaryCompleteWithConfirmation,
    InitiateWithRecoveryCompleteWithConfirmation,
    InitiateWithRecoveryDelayedCompletion,
    /// ‼️ REQUIRES "Dugong" ‼️
    InitiateWithPrimaryDelayedCompletion,
}

Other Changes

  • Changed from number_of_days_until_auto_confirmation: u16 -> time_until_delayed_confirmation_is_callable: TimePeriod in Profile - two important changes since auto confirm was misleading and since u16 representation in days prevented us from ever changing to hours/minutes if we ever wanted that. The TimePeriod was a perfect type and is made Serde
  • moved incorrectly placed JSON files into /models from /vectors- a vector is a list of small object with input and expected output - often used by cryptographic operations
  • Swift: fix deprecation warnings

};

#[derive(Debug, Clone)]
pub struct AccessControllerFactorsAndTimeInput {
Copy link
Contributor

@Sajjon Sajjon Jan 24, 2025

Choose a reason for hiding this comment

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

shared type to create:

  • AccessControllerInitiateRecoveryAsPrimaryInput
  • AccessControllerInitiateRecoveryAsRecoveryInput
  • AccessControllerQuickConfirmPrimaryRoleRecoveryProposalInput
  • AccessControllerQuickConfirmRecoveryRoleRecoveryProposalInput
  • AccessControllerTimedConfirmRecoveryInput

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

let entity_address = securified_entity.entity.address();

// ACCESS_CONTROLLER_CREATE_PROOF_IDENT
let mut builder = TransactionManifest::produce_owner_badge(
Copy link
Contributor

Choose a reason for hiding this comment

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

This does:

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


// INITIATE RECOVERY
let (init_method, init_input) =
kind.input_for_initialization(factors_and_time_input);
Copy link
Contributor

@Sajjon Sajjon Jan 24, 2025

Choose a reason for hiding this comment

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

Depending on roles_combination: RolesExercisableInTransactionManifestCombination we use different roles for initialization of recovery (and the correct concrete scrypto "Input"-type (Box dyn-ed))

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

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 92.65%. Comparing base (3634378) to head (0de346a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...rc/apply_security_shield/sargon_os_apply_shield.rs 85.18% 4 Missing ⚠️
...eld/manifests_securify_shield_securified_entity.rs 91.11% 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 ⚠️
...exercisable_in_transaction_manifest_combination.rs 95.74% 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.65%   +0.05%     
==========================================
  Files        1172     1181       +9     
  Lines       26255    26628     +373     
  Branches       81       81              
==========================================
+ Hits        24312    24673     +361     
- Misses       1932     1944      +12     
  Partials       11       11              
Flag Coverage Δ
kotlin 97.73% <ø> (ø)
rust 92.28% <93.68%> (+0.07%) ⬆️
swift 93.03% <100.00%> (ø)

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 CyonAlexRDX marked this pull request as ready for review January 24, 2025 09:25
/// 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)`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, a new provisional state should work. I am thinking of this as following:

  • For QuickConfirm, we consider a provisional committed to the network when the transaction is committed on the network, so then we set the provisional config as committed in profile.
  • For TimedConfirmRecovery, we should consider it committed only after the AccessControllerTimedConfirmRecoveryInput manifest is committed to the network.

Copy link

Choose a reason for hiding this comment

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

You could also read the provisional security config from the network (i.e. gateway); but it's possibly safer to save it in the profile...
We should probably at least validate our saved config against the network state before confirming a timed recovery (in case e.g. someone has cancelled the recovery or something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the actual HDKeys which network does not have. So need to use Profile models

/// 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)`
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, a new provisional state should work. I am thinking of this as following:

  • For QuickConfirm, we consider a provisional committed to the network when the transaction is committed on the network, so then we set the provisional config as committed in profile.
  • For TimedConfirmRecovery, we should consider it committed only after the AccessControllerTimedConfirmRecoveryInput manifest is committed to the network.

@CyonAlexRDX CyonAlexRDX requested a review from dhedey January 27, 2025 11:47
Copy link

@dhedey dhedey left a comment

Choose a reason for hiding this comment

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

Nice! 👍

Didn't see any obvious logical issues, but had a few minor comments regarding rust coding norms which I noticed during the review and thought were worth mentioning.

fn validate_combination(&self) -> MatrixBuilderMutateResult {
self.validate_if_primary_has_single_it_must_not_be_used_by_any_other_role()?;
self.validate_primary_cannot_have_multiple_devices()?;
self.validate_no_factor_may_be_used_in_both_primary_threshold_and_override()?;
self.validate_no_factor_may_be_used_in_both_recovery_and_confirmation(
)?;
self.validate_number_of_days_until_auto_confirm()?;
self.validate_time_until_delayed_confirmation_is_callable()?;
Copy link

Choose a reason for hiding this comment

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

Just wanted to flag that these rules were incomplete/wrong, and Matt's reworked them to be simpler. The new rules as per the confluence doc are:

  • Only strong factors can be used in the ACCESS role’s threshold alone, or as an ACCESS role override.
  • No single factor can perform an instant recovery. Put another way, it can’t be used alone to produce any 2 of ACCESS/START/CONFIRM)
  • You can lose any one factor and still recover (in the worst case, using either ACCESS or START plus the timed recovery option)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this code has become a bit messy since requirements has changed 4 times since implementation. I believe @sergiupuhalschi-rdx @danvleju-rdx are in top of things.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dhedey this validation is up to date, and note that we would allow users to build Weak shields which will not follow the rules you specified

Copy link

Choose a reason for hiding this comment

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

👍 OK, so this is the new minimal ruleset; and there's the "recommended" ruleset applied somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

minimal is the recommended, unless you mean something else

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is no longer used as the minimal ruleset but as the recommended one. The minimal ruleset is enforced here when attempting to add a factor to a role.

instructions: impl IntoIterator<Item = ScryptoInstruction>,
) -> ManifestBuilder {
instructions.into_iter().fold(self, |builder, instruction| {
builder.add_instruction_advanced(instruction).0
Copy link

Choose a reason for hiding this comment

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

It's OK to start a manifest with this, but not to use it to add stuff in the middle (in general).

This isn't correct if the manifest already has contents; because you will likely end up with mis-aligned indices in the added manifest. (e.g. they both might reference a Bucket(0) which should resolve to different buckets).

If you want to combine two manifests togetehr, you'll need to take a more comprehensive/surgical approach. I can advise or help build this approach, but maybe we should talk through requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only insert instruction in middle outside of ManifestBuilder - for TX Guarantees - which we can do since we can unit that instruction directly.

And when we must use builder we only insert at index 0 - eg lock fee against AccessControllers XRD Vault

Or we append instruction last - eg contribute to XRD vault of an AC.

So in all those scenarios we should be Ok, I think.

fn default() -> Self {
// we default to a combination containing Primary since it allows
// use to top up XRD vault of AccessController
Self::InitiateWithPrimaryCompleteWithRecovery
Copy link

Choose a reason for hiding this comment

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

What is this default for? I imagine in general, InitiateWithPrimaryCompleteWithConfirmation will be a little easier to perform, but it depends on what default is used for.

In a way, it feels a little weird to have a default for RolesExercisableInTransactionManifestCombination? Perhaps named methods specialized to their use cases might be clearer :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The to be displayed manifest for TX review. Sure can be named ctor instead and will switch to CompleteWithConfirmation

Cc @GhenadieVP

payer_is_entity_applying_shield
&& cannot_exercise_primary_role;
if is_unable_to_top_up_xrd_vault {
// The payer is the entity applying the shield, but the manifest is not classified as
Copy link

Choose a reason for hiding this comment

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

I guess there's an implicit assumption here that:

Because we select manifests wit primary roles first, if we haven't selected a manifest with a primary role, it must be that we don't have access to the primary role, and therefore can't access the account to withdraw some XRD.

Is that 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.

Yeah, for the manifest variant built from init_R_complete_C for example we should not modify the manifest so that it suddenly requires Primary role

let owner_badge_bucket_name = "owner_badge_bucket";
let owner_badge = if is_account {
SCRYPTO_ACCOUNT_OWNER_BADGE
} else {
Copy link

Choose a reason for hiding this comment

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

MINOR: Minor robustness smell here using a boolean - I'd suggest a enum WalletEntityType { Account, Identity } instead - so that if this ever changed in future, it'd be a compiler error here.

&entity_address,
);
} else {
return None; // Nothing has "failed" really, but we cannot proceed with this combination.
Copy link

Choose a reason for hiding this comment

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

I'm not sure I understand what this failure case is - why would kind.can_set_rola_key() fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting owner_keys hashes (to the hash of the authentication_signing_factor_instance aka ROLA key) can only be done by Primary role is my understanding? Is that assumption incorrect?

So any manifest which sets the ROLA key will require Primary Role. Thus a init_R_complete_C manifest variant which sets ROLA would not be signable since it does not exercise Primary.

Copy link

@dhedey dhedey Jan 28, 2025

Choose a reason for hiding this comment

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

Ahh right ... I think this could do with a better comment?

We could work around this limitation a few ways:

  • Signing with the new primary after recovery, if possible, in order to create the proof and update the ROLA badge.
  • If doing a quick-confirm, then we can start by recovering to all ALLOW_ALL, then update the ROLA key, then recover to the correct framework.

Probably the easiest/more sane thing to to do is - if we don't already have a primary available - then require a signature from the new primary as part of confirmation (either quick or delayed) and update the ROLA at that point?

Copy link
Contributor Author

@CyonAlexRDX CyonAlexRDX Jan 29, 2025

Choose a reason for hiding this comment

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

Changed to this:

pub fn order_of_instruction_setting_rola(
&self,
security_structure_of_factor_instances: &SecurityStructureOfFactorInstances,
entity: &AnySecurifiedEntity,
) -> OrderOfInstructionSettingRolaKey {
// unindented body to fit better in Github
if security_structure_of_factor_instances
    .authentication_signing_factor_instance
    == entity.current_authentication_signing_factor_instance()
{
    // The factor of ROLA key is the same as the current factor of the entity
    // => we can skip setting the ROLA key.
    return OrderOfInstructionSettingRolaKey::NotNeeded;
}

if !self.can_quick_confirm() {
    // N.B.
    // In case of role initiating the recovery being `Primary` we CAN in fact
    // set the ROLA key in the same transaction as we initiate recovery, however,
    // if we CANCEL the recovery proposal (instead of CONFIRM) in the future
    // transaction we would be in a bad state (not using the old nor the new shield).
    return OrderOfInstructionSettingRolaKey::MustSetInFutureTxForConfirmRecovery;
}

match self.role_initiating_recovery() {
    RoleInitiatingRecovery::Primary => {
        // We can exercise the Primary Role => no need
        // to set the ROLA key after recovery and use new factors, instead
        // we can use existing factors
        OrderOfInstructionSettingRolaKey::BeforeInitRecovery
    }
    RoleInitiatingRecovery::Recovery => {
        // we can quick confirm so we can always set the ROLA key using
        // the NEW factors - disregarding of which role initiated recovery.
        OrderOfInstructionSettingRolaKey::AfterQuickConfirm
    }
}
// ^ unindented body to fit better in Github ^
}

securified_entity.network_id(),
);

// N.B.
Copy link

Choose a reason for hiding this comment

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

This is fine, although it's generally better to lock a fee of 0 and then edit it later. This provides a more accurate previewed fee.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Harder to modify later

Copy link
Contributor

Choose a reason for hiding this comment

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

This provides a more accurate previewed fee.

@dhedey Wallet has logic to include the cost of the lock fee instruction in the total transaction fee

@CyonAlexRDX CyonAlexRDX changed the title [WIP] Apply Shield Wallet Interaction Part 2 Apply Shield Wallet Interaction Part 2 Jan 29, 2025
@CyonAlexRDX CyonAlexRDX merged commit 19e8256 into main Jan 29, 2025
13 checks passed
@CyonAlexRDX CyonAlexRDX deleted the ac/wallet_interaction_applying_shield_abw-4056_part2 branch January 29, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants