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

[ABW-4135] Export method to derive public keys #350

Merged
merged 19 commits into from
Jan 23, 2025

Conversation

matiasbzurovski
Copy link
Contributor

@matiasbzurovski matiasbzurovski commented Jan 22, 2025

JIRA ticket: ABW-4135

Changelog

  • Exports API to allow deriving public keys from a given source. Such method will be used hosts to perform Account Recovery scan.
  • Adds some extensions on Swift Sargon to allow integration HostInteractor.
  • Updates references of bdfs to main_bdfs.

Notes

A lot of the file diff comes from the latest change mentioned, so you may want to review that commit independently. You can review the 16 previous commits here.

@matiasbzurovski matiasbzurovski force-pushed the ABW-4135-derive-public-keys branch from 1a8e46d to e9f57e4 Compare January 22, 2025 12:12
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 64.10256% with 42 lines in your changes missing coverage. Please review.

Project coverage is 92.57%. Comparing base (5766453) to head (b462f12).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ppToWalletInteractionMetadata+Wrap+Functions.swift 0.00% 10 Missing ⚠️
...es/Sargon/SargonOS/SargonOS+SargonOSProtocol.swift 0.00% 10 Missing ⚠️
apple/Sources/Sargon/SargonOS/TestOS.swift 0.00% 4 Missing ⚠️
crates/system/os/os/src/sargon_os_personas.rs 81.81% 4 Missing ⚠️
...Methods/RET/CompiledSubintent+Wrap+Functions.swift 0.00% 3 Missing ⚠️
...RET/CompiledTransactionIntent+Wrap+Functions.swift 0.00% 3 Missing ⚠️
...ing/Authentication/AuthIntent+Wrap+Functions.swift 0.00% 3 Missing ⚠️
crates/system/os/os/src/os_testing_support.rs 25.00% 3 Missing ⚠️
...urces/Sargon/SargonOS/SargonOS+Static+Shared.swift 66.66% 1 Missing ⚠️
...ple/Sources/Sargon/SargonOS/SargonOSProtocol.swift 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #350      +/-   ##
==========================================
- Coverage   92.68%   92.57%   -0.11%     
==========================================
  Files        1166     1169       +3     
  Lines       26084    26143      +59     
  Branches       81       81              
==========================================
+ Hits        24175    24202      +27     
- Misses       1898     1930      +32     
  Partials       11       11              
Flag Coverage Δ
kotlin 97.73% <ø> (ø)
rust 92.17% <90.54%> (-0.01%) ⬇️
swift 93.02% <18.60%> (-0.75%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matiasbzurovski matiasbzurovski force-pushed the ABW-4135-derive-public-keys branch from e9f57e4 to 5b9a9b1 Compare January 22, 2025 13:57
///
/// # Emits Event
/// Emits `Event::ProfileModified { change: EventProfileModified::PersonaAdded }`
pub async fn create_and_save_new_persona_with_bdfs(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename it to _main_bdfs

///
/// # Emits Event
/// Emits `Event::ProfileModified { change: EventProfileModified::AccountAdded }`
pub async fn create_and_save_new_account_with_bdfs(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename it to _main_bdfs

@matiasbzurovski matiasbzurovski changed the title Abw 4135 derive public keys [ABW-4135] Export method to derive public keys Jan 23, 2025
@matiasbzurovski matiasbzurovski force-pushed the ABW-4135-derive-public-keys branch from 5877c75 to 2b47db8 Compare January 23, 2025 10:09
Comment on lines -103 to -106
/// Checks if it is Main Babylon Device Factor Source (BDFS).
pub fn is_main_bdfs(&self) -> bool {
self.supports_babylon() && self.is_main()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on latest design session, this check shouldn't be part of FactorSourceCommon, since it only matters for DeviceFactorSource. The logic in such file has been updated to check what we were previously delegating here.

@matiasbzurovski matiasbzurovski marked this pull request as ready for review January 23, 2025 10:13
Copy link
Contributor

@micbakos-rdx micbakos-rdx left a comment

Choose a reason for hiding this comment

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

Interesting what you did in tests using double _ to "group" them.

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.

LGTM, nice work!

@matiasbzurovski matiasbzurovski force-pushed the ABW-4135-derive-public-keys branch from 6bfb375 to e5e23f8 Compare January 23, 2025 13:01
@matiasbzurovski matiasbzurovski merged commit 4653813 into main Jan 23, 2025
11 of 13 checks passed
@matiasbzurovski matiasbzurovski deleted the ABW-4135-derive-public-keys branch January 23, 2025 13:51
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

Successfully merging this pull request may close these issues.

3 participants