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

Isolate UniFFI #194

Open
CyonAlexRDX opened this issue Aug 5, 2024 · 0 comments
Open

Isolate UniFFI #194

CyonAlexRDX opened this issue Aug 5, 2024 · 0 comments

Comments

@CyonAlexRDX
Copy link
Contributor

CyonAlexRDX commented Aug 5, 2024

Abstract

We plan to isolate the usage of UniFFI to a single wrapping crate, which hosts (iOS/Android wallet app) will use, which will allow us to convert Sargon (this repo) into using Cargo Workspace - which as seen in #143 leads to a massive speedup in compilation and build time.

Rationale

#143 is not mergeable since we hit issues with the complexities of UniFFI tag conversion between different crates. Even if we were to solve the issues preventing us from using the "naive" approach in #143, where many crates use UniFFI (e.g. by not using uniffi::custom_newtype!(NonPrimitive, NewType) but rather always use UniFFI primitive types such as String, i8 etc), we still want to limit our dependency on UniFFI, if we ever want to use another bindgen tool some day.

Another benefit of limiting the usage of UniFFI is that the restrictions put on the Rust code by UniFFI is lifted, such as lack of support of generics! Suddenly we can translate lots of our macro heavy code to use generics instead.

Vision

The vision, how we want it in the future, when we have migrated over to Cargo Workspace is many small crates, allowing us to get parallellisation and caching of builds, thanks to cargo and sccache.

===== SARGON PRIVATE =====
📦 SargonOS Crate ("system")
📦 Profile Crate
📦 Radix Connect Crate
📦 .... more crates
===== UNIFFI  =====
📦 SargonFFI Crate
  🗂️ SargonOS Module | wraps 📦 'sargon-os'
  🗂️ Profile Module (?) | wraps 📦 'profile'
  🗂️ Radix Connect Module (?) | wraps 📦 'radix-connect'
  🗂️ ... more modules
===== SARGON PUBLIC =====
🔮 Swift Sargon
🔮 Kotlin Sargon
===== APPS =====
📱 iOS App
📱 Android App

Important

This issue is about completing the isolation of UniFFI into a single crate, which is the prerequisites of a proper splitting of code into crates. We dont have to create more than two crates part of this PR, if it is not deemed helpful.

Work

Unfortunately we note that there is a little bit of a chicken or the egg situation between isolating-UniFFI-into-single-crate work and refactor-logic-from-hosts-into-Sargon-and-use-SargonOS work, since if we had moved all the logic from hosts into Sargon, we could remove many of the models, since they need no longer be UniFFI exported. E.g. Account would not need to be UniFFI exported at all, instead a small "ViewModel" AccountForDisplay would be enough for hosts. An end-goal is to not have to have Profile UniFFI exported at all, but just use drivers to load Profile JSON data from hosts and let Profile be fully internal and wrapped by Sargon. However, the work of moving logic from hosts into Sargon will greatly benefit from fast code completion, compilation and build times.

Important

Since moving from hosts into Sargon spreads across 3 repos, compared to isolating UniFFI which means working in a single repo, we will do the work of isolating UniFFI into a single crate first.

Strategies

Strategy 1a

  1. Merge-ASAP commit moving src -> crates/private (except for CLI src/bindgen perhaps). Since this touches all files we wanna do it directly as to mitigate merge conflicts with future changes to Sargon (and update the path of [lib] in Cargo.toml).
  2. Create new folder crates/sargon-uniffi which is NOT a module/crate or known to Rust yet, only file level.
  3. Duplicate files with types with #[derive(uniffi::Record)] and remove that derive from struct Foo in crates/private/.../foo.rs, and having a new struct Foo in crates/sargon-uniffi/../foo.rs and also implement From<T> for U and From<U> for T in crates/sargon-uniffi/../foo.rs. crates/sargon-uniffi/../foo.rs folder is still not compiled. We would typically want to migrate some types being defined by macros -> generics here.
  4. Remove all UniFFI exported functions from all sources files in crates/private, by moving all #[uniffi::export] functions to the crates/sargon-uniffi folder (still not compiled).
  5. Migrate Cargo.toml to declare a workspace and let all code in crates/private be one single crate.
  6. Let Cargo.toml declare an actual package for sargon-uniffi, with the sargon-private crate as a dependency
  7. Update CI/CD for Android and Swift.

Strategy 1b

Just like 1a but instead of creating one single crate in step 5, we declare many small ones and reference used dependencies from sargon-uniffi crate

Strategy 2 #

Like Strategy 1 but more aggressive rewrite, instead of gradual translation of UniFFI in step 3 and step 4, we simply bring the axe and remove UniFFI completely. Steps:

  1. Merge-ASAP commit moving src -> crates/private
  2. Complete removal of UniFFI (disabling CI/CD for Kotlin Sargon and Swift Sargon).
  3. Split project into Workspace (I might in fact be able to reuse the source branch of WIP: Convert to workspace (multi crate) #143 !!! )
  4. De-UniFFI / Re-Rustify the remaining Rust code - i.e. convert macros to generics, make more references/lifetimes if advantageous (was impossible/hard with UniFFI)
  5. Optimize DX - decrease code completion and compilation time. Measure, analyze, re-iterate and improve. We can tweak lots of stuff, linker, build config, parallelism, sccache, but primarily it will be dictated by our structure of crates, if we manage to create a broad and shallow dependency tree or not.
  6. Reintroduction of UniFFI as a single crate, wrap all types as needed, impl From in both directions, export functions. Fix CI/CD and Kotlin/Swift Sargon and make host apps work.

Strategy 2: Unknowns / Undecided:

  • Step 2: Should we perhaps retain (move to a new folder, but not compiled) all global freestanding functions that are #[uniffi::export] annotated ? Or should we delete those? :thinking_face: Those contain no logic, maybe just noise, and will be quite easy - albeit cumbersome - to reintroduce.
  • Step 3: How much work should we put in to do this in a clever and optimised way, vs how much work should we put into Step 5 on structuring the modules in crates?
  • Step 4: We might be able to drastically simplify the implementation of all addresses in the private/internal crates, perhaps by using generics instead of the 300 LOC macro we use today, the challenge later on is when we re-introduce UniFFI. Hopefully I/we can come up with a simpler version than what we have today, but perhaps not. Then worst case we just need to resurrect that macro from git history (or main branch) and modify it to our needs.
  • Step 5: Can be deferred, but I think a couple of days spent here might yield pretty nice speeds up compared to the more naive approach taken in step 3.
  • Step 6: It is an interesting thought to try to try to limit the "API surface" of UniFFI, this relates to the chicken or egg scenario explained above, it might be hard to limit the surface part of this work. I'm leaning towards initially doing a 1-1 mapping with existing surface, and then start work moving logic from hosts to Sargon and once that is complete start aggressively limiting the UniFFI surface! However, both those next big work chunks would probably have to be done by someone who is not me (so I can continue pre-vacation work).

Unknowns / Uncertainties / Risks

I have a hard time seeing exactly how Radix-Engine-Toolkit fits in here. Specifically ret::Decimal, which today is wrapped by Sargon as Decimal192 which is UniFFI exported, which has quite a bit of complexity. We have different tactics here:

Decimal Tactic 1

  1. Continue to never use ret::Decimal anywhere in code, by wrapping it in Decimal192 in crates/sargon-private (probably crates/private/decimal) which ofc does NOT know anything about UniFFI.
  2. Crate a new sargon_uniffi::Decimal192 which wraps sargon_private::Decimal192 and UniFFI exports it.

Decimal Tactic 2

  1. Use ret::Decimal directly everywhere in crates/private/*
  2. Use Decimal192 almost as-is in crates/sargon-uniffi crate

If we use Tactic 2 then how do we handle errors??? Hmm.... anywhere in crates/private when we use any throwing method on ret::Decimal we would need to convert that error into CommonError which seems like a less elegant option than wrapping? However, Tactic 1 with its 3 type results in two layers of From<T> for U and From<U> for T which seems very costly if we also wanna declare all methods on both layers. There are a LOT of methods.

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

No branches or pull requests

1 participant