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

Recover securified entities #10

Merged
35 commits merged into from
Sep 20, 2024
Merged

Recover securified entities #10

35 commits merged into from
Sep 20, 2024

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented Sep 10, 2024

Working POC of recovery of securified entities, using Canonical Entity Indexing (CEI) - strategy 1 as Derivation Entity Indexing heuristics.

Can quite easily be generalized to support generic Derivation Indexing Heuristics to try Individual Entity Indexing (IEI).

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 78.24773% with 72 lines in your changes missing coverage. Please review.

Project coverage is 92.8%. Comparing base (29b33b9) to head (ad38187).
Report is 80 commits behind head on main.

Files with missing lines Patch % Lines
src/recovery/recover_entity.rs 70.8% 60 Missing ⚠️
src/securify/next_free_index_assigner.rs 81.8% 8 Missing ⚠️
src/securify/securify_entity.rs 92.3% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main     #10     +/-   ##
=======================================
- Coverage   97.5%   92.8%   -4.7%     
=======================================
  Files         45      48      +3     
  Lines       1093    1391    +298     
=======================================
+ Hits        1066    1292    +226     
- Misses        27      99     +72     
Files with missing lines Coverage Δ
src/signing/collector/signatures_collector.rs 96.6% <100.0%> (ø)
...ing/collector/signatures_collector_preprocessor.rs 100.0% <100.0%> (ø)
...raction/requests/mono_factor_sign_request_input.rs 100.0% <100.0%> (ø)
...t_interaction/requests/poly_factor_sign_request.rs 100.0% <ø> (ø)
...raction/requests/transaction_sign_request_input.rs 100.0% <100.0%> (ø)
src/signing/petition_types/petition_for_entity.rs 95.5% <100.0%> (-0.2%) ⬇️
...petition_for_factors/petition_for_factors_state.rs 100.0% <ø> (ø)
...for_factors/petition_for_factors_state_snapshot.rs 100.0% <100.0%> (ø)
...signing/petition_types/petition_for_transaction.rs 97.7% <100.0%> (ø)
src/signing/petition_types/petitions.rs 96.8% <100.0%> (ø)
... and 7 more

... and 1 file with indirect coverage changes

use crate::prelude::*;

impl Profile {
async fn new_entity<E: IsEntity + std::fmt::Debug + std::hash::Hash + Eq>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a WEIRD impl, using gateway, which it should not, it should take a pre_derive_keys_cache instead! and use the "next key"! but I have not implemented that....

relevant thread: https://rdxworks.slack.com/archives/C06EBEA0SGY/p1726135124592619?thread_ts=1726133029.389349&cid=C06EBEA0SGY

HierarchicalDeterministicFactorInstance,
E::Address,
)> = None;
for index in base..(base.add_n(50)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this whole loop should go away, should just use the next factor from the pre_derive_keys_cache (not implemented)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YES review this file, but you can skip down to fn recover_entity

/// L. Return the results, which is three sets: recovered_unsecurified, recovered_securified, unrecovered
///
/// [doc]: https://radixdlt.atlassian.net/wiki/spaces/AT/pages/3640655873/Yet+Another+Page+about+Derivation+Indices
pub async fn recover_entity<E: IsEntity + Sync + Hash + Eq>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the big value add of this PR! a POC of recovery!

}

#[cfg(test)]
pub struct TestGateway {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TestGateway needed to "mock" gateway, enabling writing of POC of reovery and tests.

pub network_id: NetworkID,
}

pub struct NextFreeIndexAssigner {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

💡 This is MUCH simplified. A proper one would be async, and use pre_derived_keys_cache and gateway for IEI!

) -> Result<SecurifiedEntityControl> {
let account = profile.account_by_address(address.clone())?;

let keys_collector = KeysCollector::securifying(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is simplified, would actually try to first load factor instances from pre_derived_keys_cache

KeySpace::Securified => self.is_securified(),
}
}
pub fn new_in_key_space(value: HDPathValue, key_space: KeySpace) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

💡 HDPathComponent gets the notion of the two different key spaces.

impl TryFrom<ScryptoAccessRule> for MatrixOfKeyHashes {
type Error = CommonError;

fn try_from(value: ScryptoAccessRule) -> Result<Self> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SHOULD only fail if a NON-Radix-Wallet client has implemented different ScryptoAccessRules - not adhering to our standard.

@CyonAlexRDX CyonAlexRDX marked this pull request as draft September 13, 2024 13:04
@CyonAlexRDX CyonAlexRDX closed this pull request by merging all changes into main in c7839f4 Sep 20, 2024
@CyonAlexRDX CyonAlexRDX deleted the recover_securified_entities branch September 20, 2024 07:59
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.

2 participants