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

Proof that SargonOS does not introduce non-trivial breaking changes #1190

Merged
merged 14 commits into from
Jul 30, 2024

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented Jun 28, 2024

Important

Cannot be merged until SargonOS PR radixdlt/sargon#176 has been merged.
Merge SargonOS, then update this, which uses LOCAL Sargon, and use new version.

It fixes the breaking changes introduce by it - non are hard, easy changes.

The biggest change is really that DeviceFactorSource creation method takes a DeviceInfo to populate the DeviceFactorSourceHint. This is not important, and I dont thin we even display this to the user.

This PR could/should be clean up! And can be cleaned up in several ways, but I have no time to do it.

One idea is to create a new temporary method on SecureStorageClient loadDeviceInfoOrFallback or similar, which is a non-throwing function which returns a non-optional DeviceInfo with description "iPhone"/.

Once again, this info is not at all crucial. Feel free to use whatever workaround suits you!

Or a bit more elegant, is to change declare an initialiser in Swift Sargon which takes an optional DeviceInfo...

I gave it a quick spin and it seems to work fine! (I got some iCloud error, but it is most likely completely unrelated)

RPReplay_Final1719589001.MP4

…t however you like. IT IS NOT IMPORTANT data passed to creation of DeviceFactorSources.
@@ -181,6 +181,7 @@ extension DebugInspectFactorSourceView {
DeviceFactorSouceView(deviceFactorSource: deviceFactorSource)
case let .ledger(ledgerFactorSource):
LedgerFactorSourceView(ledgerFactorSource: ledgerFactorSource)
default: fatalError("DISCREPANCY: Found non .device | .ledger factor source. A real world user cannot possible have this.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is safe for our users, it is not possible for our users to create any other factor source than DeviceFactorSource or LedgerFactorSource.

@@ -89,6 +89,8 @@ public struct DerivePublicKeys: Sendable, FeatureReducer {
.device
case let .ledger(ledger):
.ledger(ledger)
default:
fatalError("DISCREPANCY: Found non .device | .ledger factor source. A real world user cannot possible have this.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is safe for our users, it is not possible for our users to create any other factor source than DeviceFactorSource or LedgerFactorSource.

@@ -210,6 +212,7 @@ public struct DerivePublicKeys: Sendable, FeatureReducer {
)
case let .ledger(ledgerFactorSource):
return deriveWith(ledgerFactorSource: ledgerFactorSource, state)
default: fatalError("DISCREPANCY: Found non .device | .ledger factor source. A real world user cannot possible have this.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is safe for our users, it is not possible for our users to create any other factor source than DeviceFactorSource or LedgerFactorSource.

@@ -195,8 +195,7 @@ public struct Signing: Sendable, FeatureReducer {
signingFactors: nextFactors,
signingPurposeWithPayload: signingPurposeWithPayload
)
case .offDeviceMnemonic, .securityQuestions, .trustedContact:
fatalError("Implement me")
default: fatalError("DISCREPANCY: Found non .device | .ledger factor source. A real world user cannot possible have this.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is safe for our users, it is not possible for our users to create any other factor source than DeviceFactorSource or LedgerFactorSource.

// MUST NOT be called twice (panics...)
// MUST NOT be enabled in PROD (not ensured that secrets are omitted)
// This is VERY VERY verbose, so mosly useful for debugging.
enableLoggingFromRust()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use SargonOS loggin systems instead. So I removed that freestanding enable log function in Sargon.

@GhenadieVP GhenadieVP marked this pull request as ready for review July 11, 2024 08:13
@GhenadieVP GhenadieVP merged commit f706e5e into main Jul 30, 2024
6 checks passed
@GhenadieVP GhenadieVP deleted the sargon_os_ensure_no_non_trivial_breaking_change branch July 30, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment