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

Implement HostInfo struct #189

Merged
merged 6 commits into from
Jul 25, 2024
Merged

Implement HostInfo struct #189

merged 6 commits into from
Jul 25, 2024

Conversation

micbakos-rdx
Copy link
Contributor

@micbakos-rdx micbakos-rdx commented Jul 22, 2024

This PR aims to decouple DeviceInfo which is stored in profile with host information got from the device in realtime. HostInfoDriver aims to construct host info. Previously it used to return DeviceInfo which includes optional information about the newer data like host app version, host os version and vendor. Also DeviceInfo still uses the old description: String which is not helping when it comes to naming factor sources with device and model names. Since I wanted to keep compatibility with Profile's DeviceInfo, I chose to use a new struct that directly describes the state of the host.

  1. HostInfo replaced DeviceInfo in sargon and wherever a host information is used to create new factor sources or relevant data types.
  2. When in need to compare profile device infos we just compare with id and do not perform equals in the whole DeviceInfo model. (Will attach a comment related to this)
  3. Changed the logic on what needs to be stored on secure storage. Now in order to resolve the host information, we need to:
    1. Load if any stored device id along with its creation date. This is the HostId. If none exists, os generates a new one and stores it in secure storage
    2. Resolve the rest attributes of HostInfo on the fly and attach the host id from (i).
  4. HostInfo can be converted to DeviceInfo in order for the latter to be stored in profile, but I didn't want to expose the opposite. Since only the OS can/should query the most recent host information and should not rely on older data stored on profile.

@micbakos-rdx 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 Jul 22, 2024
@micbakos-rdx micbakos-rdx self-assigned this Jul 22, 2024
@@ -57,69 +56,69 @@ class MainActivity : ComponentActivity() {

@Composable
fun WalletContent(modifier: Modifier = Modifier, storage: SecureStorageDriver) {
var walletState: Wallet? by remember { mutableStateOf(null) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't have time to make example app compatible with sargon os. I can do that if needed.

Comment on lines +14 to +15
let was_needed =
profile.header.last_used_on_device.id != device_info.id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we need to check against the IDs and not the whole object.

Ok(Self::claim_provided_profile(&mut p, device_info.clone()))
Ok(Self::claim_provided_profile(
&mut p,
host_info.clone().into(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 99.54128% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.4%. Comparing base (ecd8c96) to head (777f0b3).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           sargon_os    #189    +/-   ##
==========================================
  Coverage       97.4%   97.4%            
==========================================
  Files            895     906    +11     
  Lines          14386   14489   +103     
  Branches          38      38            
==========================================
+ Hits           14016   14119   +103     
  Misses           364     364            
  Partials           6       6            
Flag Coverage Δ
kotlin 96.6% <100.0%> (+<0.1%) ⬆️
rust 97.0% <99.4%> (+<0.1%) ⬆️
swift 99.5% <100.0%> (+<0.1%) ⬆️

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

Files Coverage Δ
...n/Drivers/HostInfo/HostInfoDriver+DeviceInfo.swift 96.2% <100.0%> (-0.4%) ⬇️
...Methods/Crypto/Keys/PrivateHD+Wrap+Functions.swift 100.0% <100.0%> (ø)
...ile/Factor/DeviceFactorSource+Wrap+Functions.swift 100.0% <100.0%> (ø)
...Swiftified/System/Drivers/Drivers+Swiftified.swift 100.0% <100.0%> (ø)
.../Swiftified/System/Drivers/HostOS+Swiftified.swift 100.0% <100.0%> (ø)
.../java/com/radixdlt/sargon/extensions/DeviceInfo.kt 100.0% <100.0%> (ø)
...adixdlt/sargon/extensions/DeviceInfoDescription.kt 100.0% <100.0%> (ø)
...ava/com/radixdlt/sargon/extensions/FactorSource.kt 83.3% <100.0%> (ø)
...main/java/com/radixdlt/sargon/extensions/HostOs.kt 100.0% <100.0%> (ø)
...ns/PrivateHierarchicalDeterministicFactorSource.kt 100.0% <100.0%> (ø)
... and 31 more

... and 1 file with indirect coverage changes

src/profile/supporting_types/host_info.rs Outdated Show resolved Hide resolved
id: DeviceID::from_str("66F07CA2-A9D9-49E5-8152-77ACA3D1DD74")
.unwrap(),
date: Timestamp::sample(),
description: DeviceInfoDescription {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expected DeviceInfoDescription to also implement HasSampleValues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does but does not follow the logic of one sample being for an iOS client and other for Android.

src/profile/supporting_types/host_info.rs Outdated Show resolved Hide resolved
@micbakos-rdx
Copy link
Contributor Author

@GhenadieVP I have pushed the latest commits were

  1. The HostId is separate from HostInfo. Those two together can create an instance of DeviceInfo.
  2. The host info driver, now can return a HostOS which is an enum with various properties according to the operating system.

@micbakos-rdx micbakos-rdx marked this pull request as ready for review July 24, 2024 14:44
Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

Nice, well done!

@micbakos-rdx micbakos-rdx merged commit b1058da into sargon_os Jul 25, 2024
12 checks passed
@micbakos-rdx micbakos-rdx deleted the host_info branch July 25, 2024 11:47
GhenadieVP added a commit that referenced this pull request Jul 29, 2024
* Rust: `SargonOS` &`Drivers` more Profile Logic | Swift: Shared State  (working example app)! (#131)

Introducing SargonOS

* MFA: Sec Structure of Factor <Source | ID | Instance>s (#150)

MFA Batch 1: SecurityStructureOfFactorSources and MatrixOfFactorSoures and also on FactorSourceID and FactorInstance level. Also add some new FactorSourceKinds at least a POC version of them or stubbed out ones.

* Sargon OS undo Profile JSON format change (#177)

Revert Profile JSON format changes in DeviceInfo

* Merge 'main' into 'sargon_os'

* Implement HostInfo struct (#189)

* Implement HostInfo struct

* Store host id in secure storage

* Fix doc

* Separate id and date from host info

* Implement HostOS that will encapsulate the different values and constants of each host os

* Expose uniffi method for creating device info from host info

* Small fix for AppleHostDriver (#192)

wip

---------

Co-authored-by: micbakos-rdx <michael.bakogiannis@rdx.works>
Co-authored-by: micbakos-rdx <125959264+micbakos-rdx@users.noreply.github.com>
Co-authored-by: Ghenadie <118184705+GhenadieVP@users.noreply.github.com>
@micbakos-rdx micbakos-rdx mentioned this pull request Aug 6, 2024
GhenadieVP added a commit that referenced this pull request Aug 12, 2024
* Rust: `SargonOS` &`Drivers` more Profile Logic | Swift: Shared State  (working example app)! (#131)

Introducing SargonOS

* MFA: Sec Structure of Factor <Source | ID | Instance>s (#150)

MFA Batch 1: SecurityStructureOfFactorSources and MatrixOfFactorSoures and also on FactorSourceID and FactorInstance level. Also add some new FactorSourceKinds at least a POC version of them or stubbed out ones.

* Sargon OS undo Profile JSON format change (#177)

Revert Profile JSON format changes in DeviceInfo

* Merge 'main' into 'sargon_os'

* wip

* Implement HostInfo struct (#189)

* Implement HostInfo struct

* Store host id in secure storage

* Fix doc

* Separate id and date from host info

* Implement HostOS that will encapsulate the different values and constants of each host os

* Expose uniffi method for creating device info from host info

* wip

* Sargon os v1 android (#196)

* Implement sargon os drivers

* Boot sargon os in example app

* Remove active profile id

* Add more unit tests

* Fix apple tests

* pr review

* version bump

---------

Co-authored-by: Alexander Cyon <116169792+CyonAlexRDX@users.noreply.github.com>
Co-authored-by: micbakos-rdx <michael.bakogiannis@rdx.works>
Co-authored-by: micbakos-rdx <125959264+micbakos-rdx@users.noreply.github.com>
micbakos-rdx added a commit that referenced this pull request Aug 14, 2024
* Rust: `SargonOS` &`Drivers` more Profile Logic | Swift: Shared State  (working example app)! (#131)

Introducing SargonOS

* MFA: Sec Structure of Factor <Source | ID | Instance>s (#150)

MFA Batch 1: SecurityStructureOfFactorSources and MatrixOfFactorSoures and also on FactorSourceID and FactorInstance level. Also add some new FactorSourceKinds at least a POC version of them or stubbed out ones.

* Sargon OS undo Profile JSON format change (#177)

Revert Profile JSON format changes in DeviceInfo

* Merge 'main' into 'sargon_os'

* Implement HostInfo struct (#189)

* Implement HostInfo struct

* Store host id in secure storage

* Fix doc

* Separate id and date from host info

* Implement HostOS that will encapsulate the different values and constants of each host os

* Expose uniffi method for creating device info from host info

* wip

* Sargon os v1 android (#196)

* Implement sargon os drivers

* Boot sargon os in example app

* Remove active profile id

* Add more unit tests

* Fix apple tests

* Change profile holder to keep profile state instead of profile

* Driver emits profile states instead of profile

* Implement new and delete wallet functions

* Remove unused comment

* Implement import functionality

* Propagate CRUD operations on profile to the profile state change driver.

* Remove unnecessary comment

* Fix swift tests

* Fix doc

* Prioritize bdfs save and then save profile in new wallet

* Implement retry read mechanism for profile snapshot.

This currently exists on Android codebase and needs revisiting.

* Bump version

---------

Co-authored-by: Alexander Cyon <116169792+CyonAlexRDX@users.noreply.github.com>
Co-authored-by: Ghenadie Vasiliev-Pusca <ghenadie.vasiliev-pusca@rdx.works>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kotlin 🤖 Changes in Kotlin Sargon Rust 🦀 Changes in Rust Sargon Swift 🍏 Changes in Swift Sargon UniFFI 🦄 Changes in UniFFI exported APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants