ROLA using Signatures Collector #310

micbakos-rdx · 2024-12-19T09:00:09Z

This PR integrates signing auth with signatures collector. There are some steps to achieve this:

Deleted the old implementation which was only capable of signing only one entity at a time.
Implemented a Signable version for signing auth:
- AuthIntent became a Signable and it is the request made to signatures collector when signing auth.
- Since AuthIntent's footprint is small, the Signable::Payload is still an AuthIntent (No need for CompiledAuthIntent. If you feel this is necessary, I could amend.
- AuthIntentHash is the SignableID. This is essentially the rola challenge.
- SignedAuthIntent follows the same logic as SignedIntent but it also carries the signatures of each owner (AddressOfAccountOrPersona), since such information is needed to the hosts.
Removed the sample methods from the Signable trait and moved them to a separate trait. There was no need to be in the same trait before. It was my lack of experience in rust.
Introduced SigningPurpose into SignaturesCollector which can be either SignTX or ROLA.
@GhenadieVP According to your latest changes when resolving the signers, I followed the same logic of just ignoring the signers from input that could not be resolved in profile.

micbakos-rdx · 2024-12-19T09:04:58Z

crates/sargon-uniffi/src/signing/transaction_sign_request_input.rs

+decl_transaction_sign_request_input!(
+    signable_and_payload: AuthIntent
+);


Special case for this one. AuthIntent can act as a payload too.

micbakos-rdx · 2024-12-19T09:06:06Z

...let_interaction/dapp_wallet_interaction/dapp_to_wallet/dapp_metadata/interaction_metadata.rs

@@ -37,7 +37,7 @@ impl HasSampleValues for DappToWalletInteractionMetadata {
    fn sample() -> Self {
        Self::new(
            WalletInteractionVersion::sample(),
-            NetworkID::Stokenet,
+            NetworkID::Mainnet,


These samples were invalid. The dAppDefinigionAddress was in mainnet, but the NetworkID was in Stokenet.

micbakos-rdx · 2024-12-19T09:06:47Z

crates/sargon/src/signing/authentication/auth_intent.rs

+        let origin = TryInto::<Url>::try_into(metadata.origin.clone())?;
+
+        if metadata.network_id != metadata.dapp_definition_address.network_id()
+        {
+            return Err(CommonError::NetworkDiscrepancy {
+                expected: metadata.network_id,
+                actual: metadata.dapp_definition_address.network_id(),
+            });
+        }
+
+        for entity in &entities_to_sign {
+            if entity.network_id() != metadata.network_id {
+                return Err(CommonError::NetworkDiscrepancy {
+                    expected: metadata.network_id,
+                    actual: entity.network_id(),
+                });
+            }
+        }


Added some validation when generating an AuthIntent. I think this is what we need.

micbakos-rdx · 2024-12-19T09:09:00Z

crates/sargon/src/signing/signables/signable_auth_intent.rs

+        let entities = self
+            .entities_to_sign
+            .iter()
+            .filter_map(|address| match address {


@GhenadieVP Ignoring the entities which cannot be resolved in profile.

codecov · 2024-12-19T09:13:46Z

Codecov Report

Attention: Patch coverage is 94.80519% with 8 lines in your changes missing coverage. Please review.

Project coverage is 93.3%. Comparing base (a04b2c3) to head (99a5a41).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/sargon/src/signing/authentication/auth_intent.rs	93.5%	2 Missing ⚠️
...s/sargon/src/system/sargon_os/sargon_os_signing.rs	83.3%	2 Missing ⚠️
...argonOS/ThrowingHostInteractor+Static+Shared.swift	0.0%	1 Missing ⚠️
...ctor_of_instances_required_to_sign_transactions.rs	0.0%	1 Missing ⚠️
...rgon/src/signing/collector/signatures_collector.rs	66.6%	1 Missing ⚠️
.../src/signing/petition_types/petition_for_entity.rs	92.8%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #310   +/-   ##
=====================================
  Coverage   93.3%   93.3%           
=====================================
  Files       1109    1110    +1     
  Lines      23567   23595   +28     
  Branches      79      79           
=====================================
+ Hits       21994   22025   +31     
+ Misses      1558    1555    -3     
  Partials      15      15

Flag	Coverage Δ
kotlin	`97.1% <100.0%> (ø)`
rust	`92.8% <95.3%> (+<0.1%)`	⬆️
swift	`94.8% <0.0%> (ø)`

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

Files with missing lines	Coverage Δ
...es/sargon/src/radix_connect/interaction_version.rs	`100.0% <ø> (ø)`
...pp_to_wallet/dapp_metadata/interaction_metadata.rs	`100.0% <100.0%> (ø)`
...gon/src/signing/authentication/auth_intent_hash.rs	`100.0% <100.0%> (ø)`
...n/src/signing/authentication/signed_auth_intent.rs	`100.0% <100.0%> (ø)`
...ing/collector/signatures_collector_preprocessor.rs	`93.4% <100.0%> (ø)`
...signing/petition_types/petition_for_transaction.rs	`95.7% <ø> (ø)`
...tes/sargon/src/signing/petition_types/petitions.rs	`98.3% <ø> (ø)`
...rgon/src/signing/petition_types/signing_purpose.rs	`100.0% <100.0%> (ø)`
...rates/sargon/src/signing/signable_with_entities.rs	`95.6% <ø> (ø)`
crates/sargon/src/signing/signables/signable.rs	`96.5% <ø> (ø)`
... and 14 more

CyonAlexRDX · 2024-12-19T09:36:37Z

crates/sargon-uniffi/src/signing/transaction_sign_request_input.rs

@@ -38,6 +38,19 @@ macro_rules! decl_transaction_sign_request_input {
            );
        }
    };
+    (signable_and_payload: $signable:ty) => {
+        paste! {


Please use new macro system - preinterpret. See the matrix macro

https://github.com/radixdlt/sargon/blob/main/crates/sargon-uniffi/src/profile/mfa/security_structures/matrices/decl_matrix_macro.rs

Rationale: crate paste is no longer maintained and is gonna be deprecated.

preinterpret is also better.

Created by @dhedey so ask in Rust channel for help from him

But also we have bigger fish to fry... feel free to ignore

CyonAlexRDX · 2024-12-19T10:22:31Z

crates/sargon/src/signing/petition_types/petition_for_entity.rs

+                                (value.security_structure.matrix_of_factors, role_kind)
+                            ).unwrap();
+
+                    PetitionForEntity::new_securified(


Can you change PetitionForEntity -> Self (probably my old code)

CyonAlexRDX · 2024-12-19T10:22:41Z

crates/sargon/src/signing/petition_types/petition_for_entity.rs

+                            vec![value.authentication_signing_factor_instance()],
+                            1,
+                        );
+                    PetitionForEntity::new(


PetitionForEntity -> Self

CyonAlexRDX · 2024-12-19T10:38:10Z

crates/sargon/src/signing/authentication/auth_intent.rs

+    pub fn new_from_request(
+        challenge_nonce: DappToWalletInteractionAuthChallengeNonce,
+        metadata: DappToWalletInteractionMetadata,
+        entities_to_sign: Vec<AddressOfAccountOrPersona>,


prefer impl IntoIterator<Item = AddressOfAccountOrPersona>

CyonAlexRDX · 2024-12-19T10:38:30Z

crates/sargon/src/system/sargon_os/sargon_os_signing.rs

+        let auth_intent = AuthIntent::new_from_request(
+            nonce,
+            metadata,
+            vec![AddressOfAccountOrPersona::Account(account.address)],


vec! can be removed if: https://github.com/radixdlt/sargon/pull/310/files#r1891586418

CyonAlexRDX · 2024-12-19T10:56:32Z

crates/sargon/src/signing/signables/signable_auth_intent.rs

+            .entities_to_sign
+            .iter()
+            .filter_map(|address| match address {
+                AddressOfAccountOrPersona::Account(account_address) => profile


this should be a method on Profile - getting of entities

CyonAlexRDX · 2024-12-19T11:04:17Z

crates/sargon/src/signing/authentication/auth_intent_hash.rs

+        let intent = AuthIntent::new_from_request(
+            DappToWalletInteractionAuthChallengeNonce(nonce),
+            metadata.clone(),
+            vec![AddressOfAccountOrPersona::Account(AccountAddress::random(


can drop vec! if ctor uses impl intoiterator

CyonAlexRDX

Good job!

micbakos-rdx · 2024-12-19T12:38:36Z

Added some tests regarding Rola. I don't understand why coverage thinks that some code is not tested. The tests clearly pass from such places.

micbakos-rdx added 9 commits December 19, 2024 10:48

Introduce signing purpose

69a6bf4

Impl Signable for AuthIntent

fd6867d

Filter out addresses not in the profile and not fail

4b9e3e9

Separate sample building from manifest with a trait

9f721ba

Integrate sign_auth with signatures collector

d46a530

Fix kotlin extensions

3633a01

Fix ios tests

3b8991b

Expose owner per signature for sign auth

0d0f507

Bump version

715f38b

micbakos-rdx added Swift 🍏 Changes in Swift Sargon Rust 🦀 Changes in Rust Sargon Kotlin 🤖 Changes in Kotlin Sargon UniFFI 🦄 Changes in UniFFI exported APIs labels Dec 19, 2024

micbakos-rdx requested review from CyonAlexRDX, GhenadieVP, giannis-rdx, danvleju-rdx, sergiupuhalschi-rdx and matiasbzurovski December 19, 2024 09:00

micbakos-rdx self-assigned this Dec 19, 2024

micbakos-rdx commented Dec 19, 2024

View reviewed changes

CyonAlexRDX reviewed Dec 19, 2024

View reviewed changes

Add some tests

743e533

CyonAlexRDX reviewed Dec 19, 2024

View reviewed changes

CyonAlexRDX approved these changes Dec 19, 2024

View reviewed changes

micbakos-rdx added 2 commits December 19, 2024 13:42

Add test for rola petitions

8cd553b

PR comments

6ad0a5d

micbakos-rdx added 2 commits December 19, 2024 14:39

Make internal auth intent to keep a set of addresses to sign

0b4024f

Clippy fixes

99a5a41

micbakos-rdx merged commit c4ed26a into main Dec 19, 2024
13 checks passed

micbakos-rdx deleted the feature/ABW-4017-signatures-collector-auth branch December 19, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROLA using Signatures Collector #310

ROLA using Signatures Collector #310

micbakos-rdx commented Dec 19, 2024

micbakos-rdx Dec 19, 2024

micbakos-rdx Dec 19, 2024

micbakos-rdx Dec 19, 2024

micbakos-rdx Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024 •

edited

Loading

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024

CyonAlexRDX Dec 19, 2024

CyonAlexRDX left a comment

micbakos-rdx commented Dec 19, 2024

ROLA using Signatures Collector #310

ROLA using Signatures Collector #310

Conversation

micbakos-rdx commented Dec 19, 2024

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

codecov bot commented Dec 19, 2024 • 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

Choose a reason for hiding this comment

CyonAlexRDX Dec 19, 2024 • 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

CyonAlexRDX left a comment

Choose a reason for hiding this comment

micbakos-rdx commented Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

CyonAlexRDX Dec 19, 2024 •

edited

Loading