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

Rust: SargonOS &Drivers more Profile Logic | Swift: Shared State (working example app)! #131

Merged
merged 141 commits into from
Jun 28, 2024

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented May 9, 2024

Note

Target branch is not main it is sargon_os branch...

Abstract

Important

Read this description in full, it is more important than looking at the code. Since the PR is big it is much more important you read all of this text.

Introducing SargonOS - the backend for wallets, with Profile CRUD operations as well as persisting profile into secure storage and scaffolding for async stream of changes so that wallet can subscribe to an event stream of updates/changes and update the UI in a reactive manner.

Through this document when I say "host client" I mean iOS or Android Radix wallet app. When I talk about ***Client in the context of Rust code, I mean a type capable of doing something using a driver, e.g. SecureStorageClient. I do not mean Swifts TCA style dependency when I write Client, always Rust (unless explicitly stated).

Note

Tarpaulin incorrectly treats all await lines as "missed", I've raised a GH issue for this accuracy bug. So the PATCH coverage by this PR should in fact be close to 100%.

Tip

You jump to a section using the Table-Of-Contents (ToC) directly and you can jump back to the ToC from the section header by pressing the ^ button next to each section header.

Table Of Contents

Introduction ^

Tip

Please start by looking at this demo, a recording of the iOS example app in this repo, which is built using TCA and new "share state" features1.

In the video you will see me:

  1. Launch app after a clean install
  2. SargonOS creates a new Profile (and a new BDFS)
  3. Name first account
  4. Reveal mnemonic of BDFS
  5. Home screen reacts to async event stream of AccountsForDisplay, and show account(s).
  6. Open account details of first account and change gradient of account.
  7. Restart app, to show profile was persisted
  8. Create another Mainnet account
  9. In settings switch to Stokenet, then go home
  10. Back to settings, batch create 200 Stokenet accounts
  11. Back to home, smooth scrolling of accounts
  12. Restart app, to show profile was persisted
  13. Smooth scrolling again of all accounts
  14. In settings switch to Mainnet, back home.
  15. Create 3rd Mainnet account.
  16. Delete wallet, which creates a new empty one.
  17. Import and decrypt encrypted Profile.
  18. Home, see Zabanet (not Stokenet) account.
  19. In settings switch to Mainnet, back home, no accounts (correct)
  20. Restart app, to show imported profile was persisted
  21. Switch to Stokenet, back home, no accounts (correct)
trim.6F9F5BA4-F56F-4055-9159-87A401CAD87A.MOV

Note

I really recommend you checkout this branch, build it ./scripts/ios/build-sargon.sh and open examples/iOS/Planbok.xcworkspace and play around a bit to understand it.

Description ^

This is a not a production ready implementation of SargonOS - replacement of iOS/Kotlin wallets ProfileStore and clients - since many features are missing, e.g. everything related to Personas, AuthorizedDapps and LegacyAccount import to just name a few examples. However, the features which have been implemented are very near production ready. See Supported Features section below.

If the analogy in this PR does not work well, we ought to change it, here it is:

  • Hosts pass Drivers to Bios
  • Bios boots SargonOS
  • During boot SargonOS loads existing Profile or creates a new BDFS and Profile
  • Hosts commands SargonOS to perform actions e.g. createAndSaveNewAccount
  • SargonOS uses Drivers to perform the action and emits EventNotifications (using a Driver)
  • Hosts have subscribed to EventNotifications (using a Driver) and react to the events (updating UI).

SargonOS ^

The SargonOS is the heart of the Sargon project and will be the heart of the host clients (iOS / Android wallet apps). There should always only be one instance, which you boot from a Bios (to which you have passed Drivers). The SargonOS is #[uniffi::Object], a reference type and should host clients should use it to react to changes and command it to mutate profile, e.g. create and save accounts and SargonOS then persists the modified profile into secure storage, using the SecureStorageClient (see Clients section below), which internally uses a SecureStorageDriver passed to the SargonOS during boot from the BIOS.

SargonOS has a throwing async boot constructor, in we try to load a saved profile from secure storage, which is async for flexibility reasons so that implementing clients can let it be async, which Android might need (@michaelrdx?).

Important

If we realize that SecureStorageDriver for saving and loading of Profile specifically does not need to be async, we should change that and this might allow use to change many functions in SargonOS to not be async. We I say " Profile specifically" since we might need saving and loading of Mnemonics to be async (on iOS at least), since those items requires Authentication to be read, thus must be async.

SargonOS is implemented like this:

#[derive(Debug, uniffi::Object)]
pub struct SargonOS {
    pub(crate) profile_holder: ProfileHolder,
    pub(crate) clients: Clients,
}

Which is an implementation I must say I'm quite happy with. The Clients type contain wrappers and convenience around each driver and most prominently has pub secure_storage: SecureStorageClient. Clients implements Defer, making it possible for us to call self.secure_storage(whereselfis&SargonOS), instead of having to write self.clients.secure_storage`, greatly simplifying the code.

The ProfileHolder is a mutable accessor around a Profile, doing a bit of UniFFI interior mutability dance using RwLock, this allows us to access and modify profile without &mut SargonOS - which does not work on a UniFFI object.

Supported Features ^

This PR does not implement all features and logic relating to Profile which iOS/Android wallets do (then it would not be a 10k LOC PR but rather a 30k LOC one.... and would take several more weeks...), but it does, however, provide a decent amount of features for immediate use (or possibly with minor modifications). Here is a non-exhaustive / best effort attempt at enumeration of implemented features:

  • Load saved profile from secure storage (user starts wallet with existing profile)
  • Create new DeviceInfo - a struct containing information about the device (upgraded with more fields).
  • Create new Profile (fresh install of app, or re-init after having deleted wallet)
  • Delete wallet, which creates a new one, and a new BDFS (just like previous execution path, with the exception of DeviceInfo being intact)
  • Import profile (hosts decrypts encrypted ones separately, before...) and claim (setting last_used_on_device to the host clients device info)
  • Create Unsaved Account
  • Batch Create Unsaved Accounts
  • Add account(s) (append and save previously unsaved account(s))
  • Create And Save Account
  • Batch Create And Save Account
  • Update account (e.g. appearance ID as seen in video)
  • Save by host clients modified Profile, i.e. apps can modify profile and tell SargonOS to save it (temporary "loop hole", allowing us to do gradual adoption of migration to `SargonOS).
  • Change current gateway (network)

Furthermore, using the EventBusDriver, an instance of which host clients passes into the BIOS upon booting SargonOS, host clients can listen/subscribe to an async stream of EventNotifications, such as:

  • An account was added
  • An account was updated
  • Current Gateway changed
  • Imported Profile
  • Profile Saved
    etc...

This have been used with great success in the iOS Example App to build TCA Shared State, with which I've built UI which automagically updates without any listener setup boilerplate needed at all.

So as you understand, SargonOS "emits" those EventNotifcations for each mutating method respectively.

Bios ^

The BIOS is a small struct looking like this:

#[derive(Debug, uniffi::Object)]
pub struct Bios {
    pub(crate) drivers: Arc<Drivers>,
}

#[uniffi::export]
impl Bios {
    #[uniffi::constructor]
    pub fn new(drivers: Arc<Drivers>) -> Arc<Self> {
        install_logger(drivers.logging.clone());
        Arc::new(Bios { drivers })
    }
}

Having one single purpose, which is to initialize subsystems (install_logger), for more information see Subsystems section below, otherwise essentially a wrapper around Drivers collection.

Drivers ^

A Driver is a trait, which is UniFFI exported, meaning it will be a Swift Protocol / Kotlin Interface, which host clients can conform to. Host clients pass instances of their conforming types to the BIOS which is passed to the SargonOS upon booting.

Drivers is a simple type holding onto each Driver, that the host clients need to conform to and initialize, it looks like this:

#[derive(Debug, uniffi::Object)]
pub struct Drivers {
    pub networking: Arc<dyn NetworkingDriver>,
    pub secure_storage: Arc<dyn SecureStorageDriver>,
    pub entropy_provider: Arc<dyn EntropyProviderDriver>,
    pub host_info: Arc<dyn HostInfoDriver>,
    pub logging: Arc<dyn LoggingDriver>,
    pub event_bus: Arc<dyn EventBusDriver>,
    pub file_system: Arc<dyn FileSystemDriver>,
    pub unsafe_storage: Arc<dyn UnsafeStorageDriver>,
}

Rust uses the Drivers to implement higher level abstractions called Clients, to perform specific tasks, e.g. File I/O or CRUD operations on secure (Swift: "Keychain") or unsecure storage (Swift: "UserDefaults").

Note

UniFFI has a limitation on the traits, all Swift Protocols are marked protocol Fooable: AnyObject, meaning we must use reference types to implement these.

Quite recently (a couple of months back) UniFFI added support for async trait methods, which makes it very easy to build async functionality such as File I/O and Networking!

Warning

Due to how UniFFI works, we are UNABLE to get the Driver class instance from SargonOS once booted. See explanation below:

  • iOS app creates instance eb of (swift) class EventBus (conforming to EventBusDriver)
  • iOS app pass eb it to bios b and boot SargonOS with said bios b
  • Even if SargonOS would #[uniffi::export] a method that returns the EventBus driver,
  • That instance will NOT be eb!
  • In fact the returned instance will not even be of type EventBus, instead it will be some UniFFI generated EventDriverImpl class.

The implications of this is that the EventBus instance SHOULD be a static let shared in Swift (and similar in Kotlin) so that we can subscribe to the events.

If UniFFI would have supported some kind of AsyncSequence on the Rust side (no such type exist natively in Rust, but maybe doable with a dependency, maybe.), we could have exported that async sequence to the FFI side. But since not doable, we SHOULD build that async sequence in our implementing Driver. So this is what I've done, and it works well. See the coming to sections for details.

Driver Rust Side ^

Here is the EventBusDriver on the Rust side:

/// doc omitted
#[uniffi::export(with_foreign)]
#[async_trait::async_trait]
pub trait EventBusDriver: Send + Sync + Debug {
    /// doc omitted
    async fn handle_event_notification(
        &self,
        event_notification: EventNotification,
    );
}

SargonOS will then install an instance of this driver (Swift / Kotlin provided instance), into a (currently trivial type) EventBusClient and use that client to emit relevant event notification, looking like this:

/// doc omitted
#[derive(Debug, Clone, PartialEq, Eq, Hash, uniffi::Record)]
pub struct EventNotification {
    pub id: Uuid,
    pub event: Event,
    pub timestamp: Timestamp,
}

Where Event is a nested enum with some small(!!) associated values:

/// doc omitted
#[derive(Debug, Clone, PartialEq, Eq, Hash, uniffi::Enum)]
pub enum Event {
    Booted,
    GatewayChangedCurrent { to: Gateway, is_new: bool },
    ProfileSaved,
    ImportedProfile { id: ProfileID },
    ModifiedProfile { change: EventProfileModified },
    ProfileLastUsedOnOtherDevice(DeviceInfo),
}

From an event we can query its discriminant, of type EventKind, which has no associated values, useful for filtering on EventNotifications on the FFI side.

Here is the implementation of save_profile in SargonOS:

impl SargonOS {
    async fn save_profile(&self, profile: &Profile) -> Result<()> {
        self.validate_is_allowed_to_active_profile().await?;
    
        self.secure_storage
            .save(
                SecureStorageKey::ProfileSnapshot {
                    profile_id: profile.header.id,
                },
                profile,
            )
            .await?;
    
        self.event_bus
            .emit(EventNotification::new(Event::ProfileSaved))
            .await;
    
        Ok(())
    }
}

After having successfully persisted profile using secure_storage.save the OS emits Event::ProfileSaved on the event_bus resulting in an EventNotifaction the host clients will receive and once handled, the function save_profile on the Rust side will return.

Driver FFI Side ^

Here is the EventBusDriver Swift implementation:

import SargonUniFFI

// For multicast / broadcast / share we must rely on third party dep:
// https://github.com/sideeffect-io/AsyncExtensions
// Since not yet supported by Swift Standard Library (nor Apple async-extensions)
import AsyncExtensions 

public final actor EventBus {
    private let stream = AsyncThrowingPassthroughSubject<Element, any Error>()
    private let subject: Subject
    public init() {
        subject = .init()
    }
}

extension EventBus {
    
    public typealias Element = EventNotification
    public typealias Subject = AsyncPassthroughSubject<Element>
    public typealias AsyncSequenceOfEvents = AsyncMulticastSequence<EventBus.Subject, AsyncThrowingPassthroughSubject<EventBus.Element, any Error>>
    
    /// ⚠️ MUST be able to access later, see PR description section
    /// ## `Drivers` above!
    public static let shared = EventBus()
    
    /// A multicasted async sequence of events we can subscribe to.
    public func notifications() -> AsyncSequenceOfEvents {
        subject.multicast(stream).autoconnect()
    }
}

extension EventBus: EventBusDriver {
    public func handleEventNotification(eventNotification: EventNotification) async {
        log.debug("Handle event: \(String(describing: eventNotification.event))")
        subject.send(eventNotification)
    }
}

When booting the SargonOS with the BIOS we will use EventBus.shared as EventBusDriver, now used by Rust and by iOS App (using Swift Sargon).

Clients ^

Clients are higher level abstractions built on top of drivers, translating some needed-to-be-UniFFI-exported types into Rust types. Here is HostInfoClient:

pub struct HostInfoClient {
    driver: Arc<dyn HostInfoDriver>,
}

impl HostInfoClient {
    pub(crate) fn new(driver: Arc<dyn HostInfoDriver>) -> Self {
        Self { driver }
    }
}

All clients have the same "shape" in the sense that they have a single field, called driver being Arc<dyn HostInfoDriver> (which is akin any HostInfoDriver in Swift), and a constructor called new.

The HostInfoClient has a method:

impl HostInfoClient {
    pub async fn create_device_info(&self) -> DeviceInfo {
        let host_name = self.driver.host_device_name().await;
        let host_model = self.driver.host_device_model().await;
        let host_os_version = self.driver.host_device_system_version().await;
        let host_app_version = self.driver.host_app_version().await;
        let host_vendor = self.driver.host_device_vendor().await;

        let maybe_device_id = self.driver.host_device_id().await;

        DeviceInfo::with_details(
            maybe_device_id,
            host_name,
            host_model,
            host_os_version,
            host_app_version,
            host_vendor,
        )
    }
}

Which using the driver constructs a DeviceInfo instance, used for fresh-app-install users to create the DeviceInfo, representing their host device (phone).

The EventBusClient "invert" the notifier-notifiee relationship, because from a SargonOS perspective it makes sense to tell its event_client to "emit" a notification, rather than asking the host client to "handle a notification".

#[derive(Debug)]
pub struct EventBusClient {
    driver: Arc<dyn EventBusDriver>,
}

impl EventBusClient {
    pub(crate) fn new(driver: Arc<dyn EventBusDriver>) -> Self {
        Self { driver }
    }
}

impl EventBusClient {
    pub async fn emit(&self, event_notification: EventNotification) {
        self.driver
            .handle_event_notification(event_notification)
            .await
    }
}

Subsystems ^

Some drivers might not map into a client at all, because they might need "their own life time". Those drivers are use to init "sub systems". At the time of writing only one such driver / "sub system" pair exists - LoggingDriver and LogSystem. The LogginDriver is used by host clients to be able to receive "forwarding of log calls" from Rust into FFI, while still being able to use Rust's neat info! and debug! and warn! macros. This makes it possible to see logs from rust, inside of Xcode with pretty colors and metadata, without loosing any convenience Rust side, we can use the de facto industry standard log crate with the above mentioned logging macros. For more details about how to use logging see the Logging section.

The LoggingDriver looks like this:

#[uniffi::export(with_foreign)]
pub trait LoggingDriver: Send + Sync + Debug {
    fn log(&self, level: LogLevel, msg: String);
}

And the LogSystem looks like this:

#[derive(Debug)]
struct LogSystem(RwLock<Option<Arc<dyn LoggingDriver>>>);

We impl log::Log for LogSystem, so that an instance of LogSystem can be used as Rust logger, and this instance will call log on the driver, forwarding the logging to the host client (FFI side).

impl Log for LogSystem {
    fn log(&self, r: &log::Record<'_>) {
        let driver = &*self.0.read().unwrap() else { return };
        driver.log(LogLevel::from(r.level()), r.args().to_string())
    }
}

Declare LOG as a LogSystem "singleton", starting without any value, and will be set once "initialized".

static LOG: LogSystem = LogSystem(RwLock::new(None));

Declare private init method - for those new to Rust, a fn is always private if not annotated pub.

fn init() {
    static ONCE: Once = Once::new();
    ONCE.call_once(|| {
        set_logger(&LOG).unwrap();
        set_max_level(logLevelFilter::Trace); // can be changed from FFI client
    });
}

Declare public - visible for Rust side - install_logger method which takes a logging_driver and installs it into the LOG singleton log "sub system", calling init which hooks this LOG to the log crates macros debug!, warn!, info! etc...

pub fn install_logger(logging_driver: Arc<dyn LoggingDriver>) {
    init();
    *LOG.0.write().unwrap() = Some(logging_driver);
}

Finally ensure to call install_logger when creating the bios (done before the SargonOS is booted)!.

#[uniffi::export]
impl Bios {
    #[uniffi::constructor]
    pub fn new(drivers: Arc<Drivers>) -> Arc<Self> {
        install_logger(drivers.logging.clone());
        Arc::new(Bios { drivers })
    }
}

We also UniFFI export install_logger useful for unit tests in Swift / Kotlin

/// Do not call this when you are using the SargonOS, it will have installed
/// a logger already. This is useful in tests which are NOT SargonOS tests,
/// or BIOS tests.
#[uniffi::export]
pub fn rust_logger_init() {
    install_logger(RustLoggingDriver::new())
}

Changes ^

Note

This is only a best effort summary of the most important changes,

LOC ^

Using cloc to count lines of code changes:

$ cloc --git-diff-rel dda3a506882c30311a020d313ea785261634a86b c124c5a0110bbe5e9883f7b0031691460e6a8c1b
     254 text files.
     120 text files.
      12 files ignored.

github.com/AlDanial/cloc v 1.98  T=0.96 s (283.6 files/s, 28546.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Rust
 same                            0              0            477          10554
 modified                       44              0             17            214
 added                          19            279            160           2155
 removed                        96            905            575           5320
Swift
 same                            0              0             72           1158
 modified                       27              0              2            141
 added                           6             43             17            465
 removed                        54            494            223           2435
Kotlin
 same                            0              0             13            649
 modified                       12              0              0             38
 added                           2             23              1            174
 removed                         3             12             11             54
-------------------------------------------------------------------------------
SUM:
 same                            0              0            612          13002
 modified                       89              0             19            400
 added                          27            345            179           2797
 removed                       157           1419            810           7894
-------------------------------------------------------------------------------

Profile Format ^

This PR changes Profile Format - still backwards compatible as always, meaning old Profiles will be autoupgraded into the new format, but Profiles created with this version of Sargon is not compatible with old wallets (as always).

The following change has been made into DeviceInfo:

+pub struct DeviceInfoDescription {
+    pub name: String,
+    pub model: String,
+}

pub struct DeviceInfo {
-   pub id: Uuid,
+   pub id: DeviceID,
    pub date: Timestamp
-   pub description: String,
+   pub description: DeviceInfoDescription,
+   pub system_version: Option<String>,
+   pub host_app_version: Option<String>,
+   pub host_vendor: Option<String>,
}

The following changes have been made into DeviceFactorSourceHint:

pub struct DeviceFactorSourceHint {
    pub name: String,
    pub model: String,
    pub mnemonic_word_count: BIP39WordCount,
+   pub system_version: Option<String>
+   pub host_app_version: Option<String>,
+   pub host_vendor: Option<String>,
}

This is to provide more information about the Host Device creating and using the Profile and also show more information about a Mnemonic.

Rust ^

  1. Remove Wallet, replaced by...
  2. Add ProfileHolder, which just holds a mutable reference to the Profile (RwLock).
  3. Add SargonOS which as an UF exported async throwing constructor async fn boot(bios: Bios) -> Result<()>
  4. Add Bios, which holds onto a collection of Drivers, which host client declare and pass to Bios before boot.
  5. Add Drivers collection, holding onto each Driver trait that wallet
  6. Wrap each Driver in a Client, besides LoggingDeriver which is used by the `LogSystem
  7. Add Clients collection, holding onto each Client.
  8. Implement supported features

Swift ^

For iOS Example app see below, this section describes changes in Swift Sargon.

  1. Implement all Drivers, they might not be fully production ready, especially for SecureStorageClient (Keychain), we need to make a decision if it is something we want to put in Swift Sargon or if it should be in the iOS wallet (iOS Example App does however impl an POC variant).
  2. Swiftify SargonOS and BIOS, especially declaring static var shared allowing us to have singletons for those but please note those are ASYNC set so might be nil and cause crash if access before SargonOS has booted. As long as our app does not try to eagerly access the the object we are fine.
  3. Swiftify and add Sample Value for various models.
  4. Implement a simple TestOS class which is effectively a builder, wrapping SargonOS, where each (async throwing) method returns Self (wrapper around SargonOS), so that we can chain method calls, like so os.createAccount().createAccount() creating two accounts (which is a silly example since you can also os.batchCreateAccounts(count: 2, namePrefix: "Unnamed")).

Kotlin ^

I have not implemented any Drivers other than updating the NetworkAntenna -> NetworkingDriver to make the Kotlin project compile and tests pass.

Future Work ^

Once this PR gets merged after having applied useful feedback, Android devs ought to implement the Drivers in Kotlin and perhaps update the Android demo app to evaluate "how it feels" (typically took me 2 days).

Important

It is important Android team familize themselves with the iOS example app and the SargonOS and evaluate if the "API" works. We need this feedback and make adjustment BEFORE iOS team starts migrating over to SargonOS.

After the Android team has given 👍🏽 on SargonOS, or made adjustment, then iOS team should be able to start migration in Radix iOS Wallet to use SargonOS and re-implement (harden, make final production ready) my near-production-ready Shared State (@SharedReader) implementations. The iOS Example App is so mature enought to act as inspiration. If implementation in that example app is not GREAT, then make adjustments - synced with Android team - and make it GREAT, then copy paste over the same solution to the iOS app. I think it is already pretty great, so we might be able to do it directly.

SargonOS should already be almost a 1:1 replacement of ProfileStore and SecureStorageClient on iOS.

Btw, we really should move out all logic on the Rust side from the v100 module (folder) containing the models of Profile, into the "sibling" folder logic. So that v100 only contains models, and all methods and functions be placed in logic, split into perhaps separate modules (files) with good name, like profile::logic::account::create_account and profile::logic::account::query_accounts.

Gradual adoption ^

Thanks to the save_profile UniFFI exported method, it is possible, albeit, discouraged to changes to profile on the FFI side and tell SargonOS to save it. In the land of Swift it would look like this:

func expensiveChangeHeaderLastUpdatedAt() async throws {
    let os = SargonOS.shared
    var profile = os.profile() // Cost `1R`
    profile.header.lastUpdatedAt = Date()
    try await os.saveProfile(profile) // Cost `2R`
}

Where the "cost" 1R means roundtripping the WHOLE profile accross the UniFFI boundary, which is an expensive operation (takes time), and total cost is actually 3R, since when we save the profile we send it back to UniFFI accross the UniFFI boundary, and then secure_storage will save it, which ones again sends it back from UniFFI to FFI as data. But this is acually not true, the actual cost is much much greater, because how Kotlin and Swift will want to react to events in the EventBusDriver impl... Let me explain by looking at change_current_gateway:

#[uniffi::export]
impl SargonOS {
    pub async fn change_current_gateway(
        &self,
        to: Gateway,
    ) -> Result<ChangeGatewayOutcome> {
        let did_change = self.update_profile_with(|mut p| {
            // some code omitted
            p.app_preferences.gateways.change_current(to.clone());
        })
        .await?;

        if did_change {
            self.event_bus
                .emit(EventNotification::new(
                    Event::GatewayChangedCurrent { to },
                ))
            .await;
        };

        Ok(outcome)
    }
}

Apart from the big win of having logic for changing current gateway in one place, we also see that we emit a notification if the gateway changed. In Host clients we can use the value to emitted directly and update UI based on that, no more UniFFI roundtrop needed, especially can we AVOID the S * R, where S denotes the number of subscriptions to the change of gateway event, and R once again denotes the cost of roundtripping the whole of Profile.

Lets now say we defer the migration of saving a Persona into Profile, so we use the "loop hole":

func savePersonaToProfile(persona: Persona) async throws {
    let os = SargonOS.shared
    var profile = os.profile() // Cost `1R`
    let networkID = persona.networkID
    var network = profile.networks.first({ where: $0.id == networkID }) ?? ProfileNetwork(id: networkID)
    network.personas.append(persona)
    profile.networks[id: networkID] = networkID // PSEUDOCODE (`networks` is Array, so requires index...)
    try await os.saveProfile(profile) // Cost `2R`
}

Then if we wanna write a subscriber for personas we cannot listen for a personaAdded event using our EventBus because no such event exists!. Instead we can listen to the event profileSaved, but how do we read out the personas? Well we MUST read the whole of Profile, i.e. a cost of S * R (1R per subscriber). But I've actually POCed that it is possible to have one single subscriber per type, i.e. can be one single Personas shared state subscriber, so only costing 1R for Personas.

Note

But by adding just a few lines of code in Rust, the save_persona_to_profile method and a new EventKind addedPersona and a simple pub fn personas() -> Personas which would take me cirka 20 minutes with test, we suddenly remove the (3 + S) * P cost! And we get a constant tiny cost.

But nevertheless, the saveProfile method allows for gradual adoption! We should aim to migrate a few methods from Swift/Kotlin into Rust per week, then after a few months we will have it all inside of Rust Sargon! 🎉

Logging ^

Logging is such a crucial part of development and debugging that I wanted to dedicate an entire section about it, because logging in SargonOS / Sargon and in host clients is amazing to use thanks to this PR.

image

As you can see in the screenshot above, Xcode treats those logged messages really well, and we can chose which metadata we wanna include, timestamp, category (Rust or Swift), and we can filter on those metadata too! All messages logged from Rust show up with Rust and those logged from inside of Swift show up with Swift.

Swift Sargon SHOULD make use of the public global logger named log - which the iOS wallet also can start using - instead of loggerGlobal.

iOS Example App ^

The iOS app example app has been upgraded to use TCA Shared State, demonstrating example usage of the EventBusDriver.

Note

The ONLY thing you need to do to get access to autoupdating list of accounts (non hidden, on current network) is declare @SharedReader(.accountsForDisplay) var accountsForDisplay in TCA State. Yes, that is it 🎉

This is the AccountsFeature's State:

    
    @ObservableState
    public struct State: Equatable {
        @SharedReader(.network) var network
        @SharedReader(.accountsForDisplay) var accountsForDisplay
    }

An usage in the view:

    if store.state.accountsForDisplay.isEmpty {
        Text("You dont have any accounts on \(store.state.network.description)")
    } else {
        ScrollView {
            ForEach(store.state.accountsForDisplay) { accountForDisplay in
                VStack {
                    AccountCardView(accountForDisplay: accountForDisplay) {
                        send(.accountCardTapped(accountForDisplay))
                    }
                }
            }
        }
    }

And the SharedReader is very easy to implement:

extension PersistenceReaderKey where Self == PersistenceKeyDefault<SargonKey<AccountsForDisplay>> {
    public static var accountsForDisplay: Self {
        Self.sharedAccountsForDisplay
    }
}

extension PersistenceKeyDefault<SargonKey<AccountsForDisplay>> {
    public static let sharedAccountsForDisplay = Self(
        SargonKey(
            on: .currentAccounts,
            accessing: \.accountsForDisplayOnCurrentNetworkIdentified
        ),
        .default
    )
}

Footnotes

  1. Shared STate is a way to extremely easily listen to async streams of updates in UI, recently (May 2024) introduced in TCA 1.10.0

@Sajjon
Copy link
Contributor

Sajjon commented May 26, 2024

Why do we need SharedReader? Couldn't we just use Shared, and make a special persistence strategy?

Because Shared's conformance to protocol PersistenceKey required method save is not async, it is: func save(_ value: Value). But we can use SharedReader sync. This is the 95% perfect WIN. All the boilerplate we rid of in Reducers were related to reading, so using SharedReader instead of setting up async stream listeners in a task in Reducers is itself a huge win.

Writing need be async sync writing to secure storage must be async in Android. It's probably best to not try something like:

struct SargonKey: PersistenceKey {
  ...
  func save(_ value: Value) {
      Task {
          try await SargonOS.shared.save(value)
      }
  }
}

Because I think this might get out of sync, what if saving failed? I think the Shared state in Reducer might be out of sync with SargonOS (and Keychain).

But you are most welcome to try it yourself and prove me wrong :) I'm would love to be wrong about that!

@kugel3 also note that the SharedReader is not In Swift Sargon - it is in the small example app, but should be made production ready by iOS team and maybe moved into Swift Sargon, behind a #canImport(ComposableArchitecture) perhaps? So if you or anyone else in iOS team make it production ready, part of that work we ought to revisit if we can make it a Writer as well as a Reader. But if we cannot, as I said above, we already have a huge win :)

@Sajjon
Copy link
Contributor

Sajjon commented May 26, 2024

@matiasbzurovski I think I have addressed all PR comments and applied all relevant suggestions. Thanks for through review, have a look and if it is to your satisfaction - and if you dont have any suggestions on alternative analogy to Bios, Drivers and OS - approve? 😊

public static let shared = Keychain(service: "works.rdx.planbok")

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and that's exactly what we are doing actually 🙂 We don't need the shared Keychain instance on iOS side, if you delete it it would still compile as it isn't used.

Copy link
Contributor

@matiasbzurovski matiasbzurovski left a comment

Choose a reason for hiding this comment

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

Thanks for tackling/answer all my comments. Added some minor things and unresolved some comments that I don't think have been resolved. Approving anyway as they are all minor, great work 👏

src/system/sargon_os/sargon_os_accounts.rs Outdated Show resolved Hide resolved
src/profile/logic/profile_header.rs Show resolved Hide resolved
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.

Tremendous work 🥇 💯 , left some comments that I would like to clarify.


#[uniffi::export(with_foreign)]
#[async_trait::async_trait]
pub trait HostInfoDriver: Send + Sync + std::fmt::Debug {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why there are multiple functions instead of one that provides the full model?

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 granularity is good I think! But sure could have been get_device_info() single function. Hmm I dont feel strongly about it either way :)

Comment on lines +68 to +78
let (profile, bdfs) =
Self::create_new_profile_with_bdfs(&clients, bdfs_mnemonic)
.await?;

secure_storage.save_private_hd_factor_source(&bdfs).await?;

secure_storage
.save_profile_and_active_profile_id(&profile)
.await?;

info!("Saved new Profile and BDFS, finish booting SargonOS");
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it should be part of ProfileHolder.
Then, in general, IMO, I would expect SargonOS to not have much logic in its functions, it should ideally just forward requests to the respective subcomponents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which part specifically do you think should move to ProfileHolder? ProfileHolder does NOT have the clients, nor do I think it should, it should only "hold the Profile". But yes we might wanna introduce one more layer though!

CURRENT (current state of this PR):
SargonOS(HAS ALL clients) -> ProfileHolder(NO clients) -> Profile

ONE MORE LAYER:
SargonOS(HAS ALL clients) -> ProfileManager(HAS SOME clients) -> ProfileHolder(NO clients) -> Profile

But maybe something that can be done later?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also asking this in the context of what would be the standard, should only SargonOS operate with clients, or subcomponents can have clients?

I would add RadixConnectMobile which would need some clients.

As for having one more layer, ProfileHolder by itself doesn't seem quite useful, is there a reason I am missing as to why not have just ProfileManager that operates on the Profile?

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 has to do with Rust and UniFFI :) SargonOS cannot UniFFI export *mut self methods. So we cannot use mut. Thus SargonOS MUST have "interior mutability". But UniFFI cannot use RefCell (or Cell) so we MUST use RwLock. And working with that is .... a bit cumbersome, so that code I dont want in SargonOS, but it has to go somewhere, that is the purpose of ProfileHolder! To be able to AT ALL mutate a Profile and still work with UniFFI :)

RadixConnectMobile is probably going to be a client! It might be a client which uses other clients - which would be a first. So maybe we should introduce one new "classifications"? Maybe "component"? So we call it RCMComponent and we say: "'Components' are allowed to have 'Clients'" - and then we call ProfileManager ProfileManagerComponent?

I think that would work quite well?

Copy link
Contributor

Choose a reason for hiding this comment

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

RadixConnectMobile is probably going to be a client

Isn't client reserved to drivers? I don't expect Wallets to have to init a RadixConnectMobile client, so with your terminology it would be more of a component, similar to ProfileManager.

As Sargon would grow, we will have more and more components for the sake of encapsulation.

Probably Module would be a better term in the context of OS, but might be confusing in the context of Cargo :).

Copy link
Contributor

@GhenadieVP GhenadieVP Jun 5, 2024

Choose a reason for hiding this comment

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

but wouldn't that be too much to put a holder around whole SargonOS? does it mean that a potential RadixConnectMobile request might be blocked by some unrelated Profile change?

Profile is quite likely to be accessed concurrently with MFA(also potentially with cloud backups done in background), so we should start thinking about it. An option seems to be using actor model in Sargon directly with actix, although it is likely to make the read/writes async.

Additionally, we are already using actor model for ProfileStore in iOS codebase, any reason that we don't need the actor functionality anymore with SargonOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, Im not sure what we wanna do. if we can find truely distinct components, where RadixConnectMobile does not use Profile at all then it is easily built. We need to give it some thought. And while giving it thought, we need to eval "current state" and "after having moved to Workspace and only One crate uses UniFFI" scenarios. The former prevents us from using actix actors, since they are not UniFFI compatible (and probably very hard to be made UniFFI compatible).

Copy link
Contributor

@GhenadieVP GhenadieVP Jun 5, 2024

Choose a reason for hiding this comment

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

What is the reason for actors not working with UniFFI when we want it to be fully private to SargonOS? You say that this:

#[derive(Debug, uniffi::Object)]
struct SargonOS {
   profile_manager: Actor
}

cannot be exposed as an object?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong! We can! Works with uniffi::Object indeed! - But does not work with uniffi::Record... I'm too used to thinking about records...

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing then! We can pursue using Actors, hopefully it works smoothly 🤞

Copy link
Contributor

@sergiupuhalschi-rdx sergiupuhalschi-rdx left a comment

Choose a reason for hiding this comment

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

Impressive work! 👏

Copy link

@giannis-rdx giannis-rdx left a comment

Choose a reason for hiding this comment

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

This will be a huge win for the wallets when we migrate to Sargon! 💯

I still keep an eye on your discussion with Ghenadie.

@CyonAlexRDX CyonAlexRDX changed the base branch from main to sargon_os June 28, 2024 11:39
@CyonAlexRDX CyonAlexRDX merged commit 967ebbc into sargon_os Jun 28, 2024
9 of 10 checks passed
@CyonAlexRDX CyonAlexRDX deleted the ac/system_with_drivers branch June 28, 2024 13:17
@CyonAlexRDX
Copy link
Contributor Author

Merged into temporary branch sargon_os into which we will merge #150 also, and then ensure not Profile format change happens, aka revert the DeviceInfoDescription field in DeviceInfo introduced by this PR.

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>
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
Coverage OK ☂️ ✅ False Negatives in Code Coverage by Tarpaulin 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.

7 participants