[MERGE ME] Split Sargon into many crates #318

CyonAlexRDX · 2024-12-21T08:49:53Z

Note

Mergable even if check-version and code-coverage checks fail.

Important

the large git diff comes from many Cargo locks files from all new crates primary

Important

Sargon-uniffi crate is now NOT a "default member" of the workspace, meaning cargo checks will not include it, for faster checks. You can cargo check --workspace to include it. Or cargo test --workspace to unit test its tests. The command cargo run nextest --all includes it, all other checks in CI/precommit will include it (I've verified this). The cargo build -p sargon-uniffi which we use in CD/build scripts will of course include it...

Important

see README for how we bump / control version in all Cargo.toml files using cargo ws crate

sargon-core-utils
sargon-core-error
sargon-core-assert-json
identified-vec-of
sargon-core
sargon-hierarchical-deterministic
sargon-factors
sargon-factors-supporting-types
sargon-addresses
sargon-keys-collector
sargon-transaction-models
sargon-manifests
sargon-profile-supporting-types
sargon-drivers
http-client
sargon-clients
sargon-profile-gateway
sargon-profile
sargon-profile-app-preferences
sargon-profile-security-structures
sargon-profile-logic
sargon-factor-instances-provider
next-derivation-index-ephemeral
gateway-models
gateway-client-and-api

codecov · 2024-12-21T09:02:59Z

Codecov Report

Attention: Patch coverage is 82.56684% with 163 lines in your changes missing coverage. Please review.

Project coverage is 92.2%. Comparing base (e18bd66) to head (16162d6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../app/signing/src/collector/signatures_collector.rs	82.8%	42 Missing ⚠️
...wiftified/Profile/Factor/FactorSource+Extras.swift	0.0%	27 Missing ⚠️
...file/Factor/FactorSourceIntegrity+Swiftified.swift	0.0%	24 Missing ⚠️
...es/core/has-sample-values/src/has_sample_values.rs	33.3%	16 Missing ⚠️
crates/app/radix-connect/src/mobile/client.rs	80.2%	14 Missing ⚠️
...dix-connect/src/mobile/deep_link_parsing/parser.rs	74.0%	14 Missing ⚠️
crates/common/bytes/src/secret_bytes.rs	76.9%	6 Missing ⚠️
...ctor_of_instances_required_to_sign_transactions.rs	70.5%	5 Missing ⚠️
...o_dapp/success_response/transaction/transaction.rs	80.9%	4 Missing ⚠️
...wiftified/Profile/Account/Account+Swiftified.swift	0.0%	3 Missing ⚠️
... and 5 more

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #318     +/-   ##
=======================================
- Coverage   93.3%   92.2%   -1.1%     
=======================================
  Files       1111    1143     +32     
  Lines      24248   25773   +1525     
  Branches      79      79             
=======================================
+ Hits       22637   23779   +1142     
- Misses      1596    1979    +383     
  Partials      15      15

Flag	Coverage Δ
kotlin	`97.1% <ø> (+<0.1%)`	⬆️
swift	`93.4% <20.5%> (-1.5%)`	⬇️

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

Files with missing lines	Coverage Δ
...thods/Profile/CAP26EntityKind+Wrap+Functions.swift	`100.0% <100.0%> (ø)`
...ied/Crypto/Derivation/AccountPath+Swiftified.swift	`100.0% <100.0%> (ø)`
...ed/Crypto/Derivation/IdentityPath+Swiftified.swift	`100.0% <100.0%> (ø)`
...wiftified/Profile/CAP26EntityKind+Swiftified.swift	`100.0% <100.0%> (ø)`
...rc/deferred_deep_link/deferred_deep_link_method.rs	`100.0% <ø> (ø)`
...c/deferred_deep_link/onboarding_deep_link_value.rs	`100.0% <ø> (ø)`
crates/app/home-cards/src/home_card.rs	`100.0% <ø> (ø)`
crates/app/home-cards/src/home_cards.rs	`100.0% <100.0%> (ø)`
crates/app/home-cards/src/manager.rs	`81.8% <ø> (ø)`
...p/radix-connect-models/src/auth_challenge_nonce.rs	`100.0% <ø> (ø)`
... and 131 more

... and 1158 files with indirect coverage changes

Plan

Split out HD crate

Split out module `Factors` and others into separate crates Split out module `Addresses` & `KeysCollector` into new crates

…and TX models (#326)

CyonAlexRDX · 2025-01-02T12:30:29Z

Merge at will @GhenadieVP

Sajjon · 2025-01-02T13:25:40Z

I downgraded the bump to 1.2 so that that CI check passes

…e for faster default developer experience.

Sajjon · 2025-01-02T15:32:38Z

Here are some measurements:

BEFORE this PR (main branch):
Opening ONLY sargon crate
change lib.rs && cargo check => 4.50s

AFTER/WITH this PR:
change in sargon/src/lib.rs && cargo check => 1.70s

That is a 63% reduction!

CyonAlexRDX · 2025-01-02T17:10:37Z

@micbakos-rdx i hope you and @GhenadieVP can coordinate to merge this PR before your changes in:

micbakos/ABW-4027-update-io

Branch - or any other branch.

Merge hell otherwise 🤪

Sajjon · 2025-01-03T07:11:08Z

If anyone sporadically gets test failures with this (or similar) message:

called Result::unwrap() on an Err value: FailedToDeserializeJSONToValue { json_byte_count: 0, type_name: "FactorInstancesCacheSnapshot", serde_message: "EOF while parsing a value at line 1 column 0" }

it is because crates in the same workspace do NOT share cfg(test) compilation, meaning that a non FactorInstancesCacheClient::in_memory() instance has been used. i.e. a real on file FactorInstancesCache is used, and two tests are running in parallell most likely causing the tests to fail.

I think I've fixed it in: d81f018

but there might be some few more places left.

Sajjon · 2025-01-03T07:16:28Z

Other changes:
I've created a type ShortString which is Copy 🥳, used by DisplayName and Email making them Copy too.

* Remove many unused dependencies * fix more FactorInstancesCache not using in-memory scenarios causing unit tests to fail. --------- Co-authored-by: Alexander Cyon <alex.cyon@gmail.com>

Sajjon · 2025-01-03T12:51:26Z

@GhenadieVP I merge some last minute improvements: https://github.com/radixdlt/sargon/pull/327/files

I went through all Cargo.toml and removed all unused/unneeded dependencies. So now each crate only declares exactlty what it actually uses/needs.

An I forgot to mention in PR description:
I've removed the Physical variant of FactorInstanceBadge: https://github.com/radixdlt/sargon/blob/ac/split/crates/sargon-factors/src/factor_instance/factor_instance_badge.rs#L15-L17

We are not using it and I think we won't use it. Removing it allowed me to not depend on sargon-addresses crate in sargon-factors crate (as seen here https://github.com/radixdlt/sargon/blob/ac/split/crates/sargon-factors/Cargo.toml#L7-L11) and every elision of crates as dependency matters, as it builds up. Just an FYI.

OK so now I think this PR is ready to be merge from my perspective. As I wrote above merge at will :)

Important

I've not built Sargon locally and tried it in Android or iOS host.
Maybe you can do that or ask Umair to do one soundness check before mergeing?

GhenadieVP · 2025-01-03T14:42:47Z

@CyonAlexRDX There is a breaking change:

pub enum AccountOrAddressOf {
    ProfileAccount { value: Account },
    AddressOfExternalAccount { value: AccountAddress },
}

is changed to:

pub enum OwnedOrThirdPartyAccountAddress {
    OwnedAccount { value: AccountAddress },
    ThirdPartyAccount { value: AccountAddress },
}

the issue is that the OwnedAccount is not an Account but just an address. I don't think that it should be the actual Account, having the AccountForDisplay form should be enough. The Host uses this information to display the info of the account, just having an address would require doing an async lookup on the profile.

micbakos-rdx · 2025-01-03T14:49:54Z

@micbakos-rdx i hope you and @GhenadieVP can coordinate to merge this PR before your changes in:

micbakos/ABW-4027-update-io

Branch - or any other branch.

Merge hell otherwise 🤪

@CyonAlexRDX No worries I have updated my changes on top of this PR.

micbakos-rdx · 2025-01-03T15:07:23Z

If anyone sporadically gets test failures with this (or similar) message:

called Result::unwrap() on an Err value: FailedToDeserializeJSONToValue { json_byte_count: 0, type_name: "FactorInstancesCacheSnapshot", serde_message: "EOF while parsing a value at line 1 column 0" }

it is because crates in the same workspace do NOT share cfg(test) compilation, meaning that a non FactorInstancesCacheClient::in_memory() instance has been used. i.e. a real on file FactorInstancesCache is used, and two tests are running in parallell most likely causing the tests to fail.

I think I've fixed it in: d81f018

but there might be some few more places left.

A similar test that was sometimes failing in my branch was add_structure_emits_event.

Sajjon · 2025-01-03T16:18:31Z

@CyonAlexRDX There is a breaking change:
pub enum AccountOrAddressOf {
    ProfileAccount { value: Account },
    AddressOfExternalAccount { value: AccountAddress },
}
is changed to:
pub enum OwnedOrThirdPartyAccountAddress {
    OwnedAccount { value: AccountAddress },
    ThirdPartyAccount { value: AccountAddress },
}
the issue is that the OwnedAccount is not an Account but just an address. I don't think that it should be the actual Account, having the AccountForDisplay form should be enough. The Host uses this information to display the info of the account, just having an address would require doing an async lookup on the profile.

@GhenadieVP ah right ofc, we should use AccountForDisplay, I will fix over the weekend!

Sajjon · 2025-01-03T16:19:38Z

A similar test that was sometimes failing in my branch was add_structure_emits_event.

@micbakos-rdx did your branch contain this commit: 2d21cc2

CyonAlexRDX · 2025-01-04T14:20:19Z

@micbakos-rdx sorry you need to rebase again, since I merged: #329

@GhenadieVP i fixed the breaking change with AccountOrAddressOf and in this host PR I've fixed host: radixdlt/babylon-wallet-ios#1429

@micbakos-rdx you probably need minor adjustments in Android host too, see radixdlt/babylon-wallet-ios#1429

…center`, `signing` & account, persona (#330)

CyonAlexRDX · 2025-01-05T20:40:10Z

I progress ever further with #330 merged

CyonAlexRDX · 2025-01-06T21:34:58Z

@GhenadieVP and even further!

See #332

On that branch Speedups are near 10x that of main branch for common operations

Let's sync tomorrow! I think we should merge it into this, and the this :)

Sajjon · 2025-01-06T21:36:45Z

Also we should switch to only declare paths to our own crates once:

rust-lang/cargo#14355 (comment)

Add strategy for SPLIT

197faea

Sajjon mentioned this pull request Dec 21, 2024

Split Sargon into many crates #317

Closed

CyonAlexRDX force-pushed the ac/split branch from bb76f34 to 197faea Compare December 23, 2024 09:16

CyonAlexRDX and others added 5 commits December 23, 2024 10:17

Ac/split plan (#320)

fb8a1d7

Plan

Merge branch 'main' into ac/split

a3a1cea

Split out module core into separate crate (#321)

1f33e00

Split out Hierarchical Deterministic module into separate crate (#323)

42f3b01

Split out HD crate

Split Factors module into separate crate (and some more) (#324)

81aef4b

Split out module `Factors` and others into separate crates Split out module `Addresses` & `KeysCollector` into new crates

CyonAlexRDX marked this pull request as ready for review December 30, 2024 08:55

CyonAlexRDX changed the title ~~[WIP - EPIC] Split Sargon into many crates~~ [WIP] Split Sargon into many crates Dec 30, 2024

radixdlt deleted a comment from Sajjon Dec 30, 2024

CyonAlexRDX requested review from GhenadieVP and micbakos-rdx December 30, 2024 09:01

CyonAlexRDX mentioned this pull request Jan 1, 2025

Split out Profile, Gateway, FIP, Drivers, Clients, Manifest building and TX models #326

Merged

Split out Profile, Gateway, FIP, Drivers, Clients, Manifest building …

1b27e20

…and TX models (#326)

CyonAlexRDX changed the title ~~[WIP] Split Sargon into many crates~~ [MERGE ME] Split Sargon into many crates Jan 2, 2025

Use default-members in workspace and excluding sargon-uniffi crat…

e461656

…e for faster default developer experience.

ensure FactorInstancesCacheClient uses In-Memory cache for more tests

d81f018

GhenadieVP approved these changes Jan 3, 2025

View reviewed changes

micbakos-rdx approved these changes Jan 3, 2025

View reviewed changes

Ac/split udeps (#327)

2d21cc2

* Remove many unused dependencies * fix more FactorInstancesCache not using in-memory scenarios causing unit tests to fail. --------- Co-authored-by: Alexander Cyon <alex.cyon@gmail.com>

CyonAlexRDX mentioned this pull request Jan 4, 2025

Fix for AccountOrAddressOf breaking change radixdlt/babylon-wallet-ios#1429

Merged

Split core, group crates, rename crates (#329)

4d6c7d8

fix ci

f1c14a0

CyonAlexRDX force-pushed the ac/split branch from 3d84558 to f1c14a0 Compare January 4, 2025 14:53

Split sargon crate => app/home-cards, radix-connect, `security-…

ed1ea7e

…center`, `signing` & account, persona (#330)

Split Sargon-OS crate (#332)

16162d6

CyonAlexRDX merged commit 2e9c1e8 into main Jan 8, 2025
11 of 13 checks passed

CyonAlexRDX deleted the ac/split branch January 8, 2025 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MERGE ME] Split Sargon into many crates #318

[MERGE ME] Split Sargon into many crates #318

CyonAlexRDX commented Dec 21, 2024 •

edited

Loading

codecov bot commented Dec 21, 2024 •

edited

Loading

CyonAlexRDX commented Jan 2, 2025

Sajjon commented Jan 2, 2025

Sajjon commented Jan 2, 2025

CyonAlexRDX commented Jan 2, 2025

Sajjon commented Jan 3, 2025

Sajjon commented Jan 3, 2025

Sajjon commented Jan 3, 2025

GhenadieVP commented Jan 3, 2025

micbakos-rdx commented Jan 3, 2025

micbakos-rdx commented Jan 3, 2025

Sajjon commented Jan 3, 2025

Sajjon commented Jan 3, 2025

CyonAlexRDX commented Jan 4, 2025

CyonAlexRDX commented Jan 5, 2025

CyonAlexRDX commented Jan 6, 2025

Sajjon commented Jan 6, 2025

[MERGE ME] Split Sargon into many crates #318

[MERGE ME] Split Sargon into many crates #318

Conversation

CyonAlexRDX commented Dec 21, 2024 • edited Loading

codecov bot commented Dec 21, 2024 • edited Loading

Codecov Report

CyonAlexRDX commented Jan 2, 2025

Sajjon commented Jan 2, 2025

Sajjon commented Jan 2, 2025

CyonAlexRDX commented Jan 2, 2025

Sajjon commented Jan 3, 2025

Sajjon commented Jan 3, 2025

Sajjon commented Jan 3, 2025

GhenadieVP commented Jan 3, 2025

micbakos-rdx commented Jan 3, 2025

micbakos-rdx commented Jan 3, 2025

Sajjon commented Jan 3, 2025

Sajjon commented Jan 3, 2025

CyonAlexRDX commented Jan 4, 2025

CyonAlexRDX commented Jan 5, 2025

CyonAlexRDX commented Jan 6, 2025

Sajjon commented Jan 6, 2025

CyonAlexRDX commented Dec 21, 2024 •

edited

Loading

codecov bot commented Dec 21, 2024 •

edited

Loading