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.

Sign up for GitHub

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

Jump to bottom

Apply Shield Wallet Interaction Part 1 #345

Merged
merged 3 commits into from
Jan 23, 2025
Merged

Apply Shield Wallet Interaction Part 1 #345

merged 3 commits into from
Jan 23, 2025

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented Jan 16, 2025 •
edited
Loading

Jira ABW-4056

Note

Part 1 of N

The first of two (or more) PRs for producing manifests which updates the security config of entities - i.e. applying a security shield (factor instances) to an entity.

This PR specifically only adds support for securifying an unsecurified account.

Other Changes

UnsecurifiedPersona && UnsecurifiedAccount types

Refactored UnsecurifiedEntity to be generic over entity => AbstractUnsecurifiedEntity with type aliases AnyUnsecurifiedEntity, UnsecurifiedAccount and UnsecurifiedPersona-- acting just like their "securified cousins" (AbstractSecurifiedEntity, AnySecurifiedEntity, SecurifiedAccount and SecurifiedPersona)

EntitiesOnNetwork

Refactor the five fields of NextDerivationEntityIndexProfileAnalyzingAssigner into the type EntitiesOnNetwork and let NextDerivationEntityIndexProfileAnalyzingAssigner iml Defef<Target = EntitiesOnNetwork>. The EntitiesOnNetwork is now also returned from apply_security_shield_with_id_to_entities which we make use of when we call that function from the new API: make_interaction_for_applying_security_shield and immediately get a structured object of which entites are securified and which are not (all having a provisional config).

IndexSet -> IdentifiedVecOf

I've changed to use IdentifiedVecOf<Sec/Unse Entity> instead of IndexSet which is an important change preventing bugs. Why? Because IndexSet would allow for two copies of the same account if one single deeply nested value has changed, e.g. a hidden flag being present vs non-present - since IndexSet is Hash-based. So the same AccountAddress would be present twice. This should never be allowed to happen. And could potentially lead to dangerous bugs. And we automatically protect against this by using IdentifiedVecOf!

Future Work

Future PR will add the more complex situation where we must produce 6 different kinds of manifest based on:

pub enum TransactionManifestApplySecurityShieldKindSelector {
    /// (Primary + Recovery + Confirmation)
    PrimaryAndRecoveryWithExplicitConfirmation,

    /// (Primary + Recovery + Time)
    PrimaryAndRecoveryWithTimedAutoConfirm,

    /// (Primary + Confirmation)
    PrimaryAndExplicitConfirmation,

    /// (Primary + Time) 
    PrimaryWithTimedAutoConfirm,

    /// (Recovery + Confirmation)
    RecoveryAndExplicitConfirmation,

    /// (Recovery + Time)
    RecoveryWithTimedAutoConfirm,
}

Base automatically changed from ac/cargo_version_ci_locked to main January 20, 2025 09:20
@CyonAlexRDX CyonAlexRDX marked this pull request as ready for review January 21, 2025 15:56
@codecov Codecov
Copy link

codecov bot commented Jan 21, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 73.77049% with 64 lines in your changes missing coverage. Please review.

Project coverage is 92.68%. Comparing base (0365519) to head (121b413).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...models/supporting-types/src/entities_on_network.rs 59.49% 32 Missing ⚠️
...eld/sargon_os_apply_security_shield_interaction.rs 0.00% 19 Missing ⚠️
...urity_shield/top_up_access_controller_xrd_vault.rs 83.72% 7 Missing ⚠️
.../models/supporting-types/src/securified_account.rs 71.42% 2 Missing ⚠️
.../models/supporting-types/src/securified_persona.rs 71.42% 2 Missing ⚠️
...models/supporting-types/src/unsecurified_entity.rs 94.11% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
- Coverage   92.85%   92.68%   -0.17%     
==========================================
  Files        1161     1166       +5     
  Lines       25867    26078     +211     
  Branches       81       81              
==========================================
+ Hits        24019    24171     +152     
- Misses       1837     1896      +59     
  Partials       11       11
Flag Coverage Δ
kotlin 97.73% <ø> (ø)
rust 92.19% <73.77%> (-0.21%) ⬇️
swift 93.76% <ø> (ø)

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 commented Jan 21, 2025
View reviewed changes
crates/transaction/manifests/src/modify_manifest.rs Outdated
@@ -176,6 +179,7 @@ impl TransactionManifestModifying for TransactionManifest {

pub enum InstructionPosition {
First,
Last,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used actually, so we might remove it

crates/transaction/manifests/src/manifests_security_shield.rs Outdated
/// We allow to pass amount so that we can top of with more or less based on
/// token balance of `payer` and current balance of the access controller (when
/// we use this method for securified entities.)
fn modify_manifest_add_withdraw_of_xrd_for_access_controller_xrd_vault_top_up_payed_by_account(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xOmarA @GhenadieVP N.B. It is my intention we call this both with manifest which Securified an unsecurified entity and for manifests updating factors for already Securified entity.

GhenadieVP
GhenadieVP reviewed Jan 22, 2025
View reviewed changes
crates/transaction/manifests/src/manifests_security_shield.rs Outdated
/// we use this method for securified entities.)
fn modify_manifest_add_withdraw_of_xrd_for_access_controller_xrd_vault_top_up_payed_by_account(
payer: Account,
// TODO: remove `entity_applying_shield`, this should be read out from the manifest in a throwing function, `manifest.get_address_of_entity_applying_shield()` or similar which Omar need to provide us with, oh well we need the account here, so elsewhere, in SargonOS where we have access to Profile we would call `manifest.get_address_of_entity_applying_shield` and then lookup the entity.
Copy link
Contributor

Choose a reason for hiding this comment

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

May be possible based on manifest static analysis, but it should be completely acceptable to receive account as input, after all modify should happen on Wallet action which has the context of the account.

crates/transaction/manifests/src/manifests_security_shield.rs Outdated Show resolved Hide resolved
CyonAlexRDX
CyonAlexRDX commented Jan 22, 2025
View reviewed changes
crates/transaction/manifests/src/manifests_security_shield.rs Outdated
use crate::prelude::*;

#[derive(Clone, Debug, PartialEq, Eq)]
pub enum TransactionManifestApplySecurityShieldKindSelector {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used by this PR, but will be used in the follow up PR

CyonAlexRDX
CyonAlexRDX commented Jan 22, 2025
View reviewed changes
crates/transaction/manifests/src/manifests_security_shield.rs Outdated
let apply_shield_manifest_kind = self
.apply_shield_manifest_kind
.clone()
.ok_or(CommonError::Unknown)?; // TODO: replace with proper error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO replace in next PR

@CyonAlexRDX CyonAlexRDX changed the title Ac/wallet interaction applying shield abw 4056 Apply Shield Wallet Interaction Part 1 Jan 23, 2025
@CyonAlexRDX CyonAlexRDX force-pushed the ac/wallet_interaction_applying_shield_abw-4056 branch from 9f79f13 to be35bc4 Compare January 23, 2025 07:13
@CyonAlexRDX CyonAlexRDX merged commit 40da3cf into main Jan 23, 2025
11 of 13 checks passed
@CyonAlexRDX CyonAlexRDX deleted the ac/wallet_interaction_applying_shield_abw-4056 branch January 23, 2025 08:18
@CyonAlexRDX CyonAlexRDX mentioned this pull request Jan 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GhenadieVP GhenadieVP GhenadieVP approved these changes

@0xOmarA 0xOmarA Awaiting requested review from 0xOmarA

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CyonAlexRDX @GhenadieVP @Sajjon