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.

Sign up for GitHub

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

Jump to bottom

Adapt wallet to the changes on HDPathComponent #1225

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Adapt wallet to the changes on HDPathComponent #1225

merged 4 commits into from
Oct 30, 2024

Conversation

micbakos-rdx
Copy link
Contributor

@micbakos-rdx micbakos-rdx commented Oct 29, 2024 •
edited
Loading

Description

For more information about sargon's changes take a look at this PR.

In general a simple HDPathValue is no more. Values have been scoped so we can differentiate between them.

0                             2^31         2^31+2^30         2^32

|--------- Unhardened----------|---------- Hardened-----------|

|-------------------Unsecurified-------------|---Securified---|

Key areas that are affected:
(1) Calculating the next derivation path of a new entity when creating it.
(2) Rederiving public keys when doing the account recovery scan
(3) Importing from legacy wallet
(4) Creating auth-signing derivation paths from transaction signing.

How to test

  1. Rederive accounts using seed phrases and ledger. Click on next too. When done so, try rederiving again with the same seed phrase. You should not see the same derived accounts appearing in the list.
  2. Import from legacy wallet

@micbakos-rdx micbakos-rdx self-assigned this Oct 29, 2024
micbakos-rdx
micbakos-rdx commented Oct 29, 2024
View reviewed changes
...m/babylon/wallet/android/presentation/accessfactorsources/AccessFactorSourceProxyContract.kt
@@ -98,19 +99,19 @@ sealed interface AccessFactorSourcesInput {

val factorSource: FactorSource
val isForLegacyOlympia: Boolean
val nextDerivationPathOffset: UInt // is used as pointer when user clicks "scan the next 50"
val nextDerivationPathIndex: HdPathComponent // is used as pointer when user clicks "scan the next 50"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped the value with HdPathComponent. A simple UInt has no meaning with no context. We need to know if this index is calculated in the local key space or in the global.

micbakos-rdx
micbakos-rdx commented Oct 29, 2024
View reviewed changes
...on/wallet/android/presentation/accessfactorsources/deriveaccounts/DeriveAccountsViewModel.kt Outdated
@@ -180,7 +186,7 @@ class DeriveAccountsViewModel @Inject constructor(
accessFactorSourcesIOHandler.setOutput(
output = AccessFactorSourcesOutput.DerivedAccountsWithNextDerivationPath(
derivedAccounts = derivedAccounts,
nextDerivationPathOffset = indicesToScan.last() + 1u
nextDerivationPathIndex = HdPathComponent.init(indicesToScan.last().indexInGlobalKeySpace + 1u)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next value is derived from the previous one + 1.

micbakos-rdx
micbakos-rdx commented Oct 29, 2024
View reviewed changes
core/src/main/java/rdx/works/core/sargon/PersonasExtensions.kt
val Persona.derivationPathEntityIndex: HDPathValue
get() = securityState.transactionSigningFactorInstance.publicKey.derivationPath.nonHardenedIndex
val Persona.derivationPathEntityIndex: HdPathComponent
get() = securityState.transactionSigningFactorInstance.publicKey.derivationPath.path.components.last()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

last() should never throw since we don't allow empty derivation path component array.

micbakos-rdx
micbakos-rdx commented Oct 29, 2024
View reviewed changes
core/src/main/java/rdx/works/core/sargon/ProfileExtensions.kt
} else {
accountsControlledByFactorSource.maxOf { it.derivationPathEntityIndex } + 1u
val global = accountsControlledByFactorSource.maxOf { it.derivationPathEntityIndex.indexInGlobalKeySpace } + 1u
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 next is the max of all the keys in global key space for simplicity.

Copy link

@Sajjon Sajjon Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we note that this code is obsolete very soon, made obsolete by FactorInstancesProvider in Sargon (soon to be migrated into Sargon from Sargon-MFA. radixdlt/sargon-mfa#18 )

micbakos-rdx
micbakos-rdx commented Oct 29, 2024
View reviewed changes
...on/wallet/android/presentation/accessfactorsources/deriveaccounts/DeriveAccountsViewModel.kt
}
?.toSet().orEmpty()

val indicesToScan = mutableSetOf<HDPathValue>()
val startIndex = nextDerivationPathOffset
val indicesToScan = LinkedHashSet<HdPathComponent>()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Later in code we use the last() method. Sets don't keep the insertion order, but LinkedHashSets do. Better to keep the new indices in a set that retains the insertion order.

micbakos-rdx
micbakos-rdx commented Oct 29, 2024
View reviewed changes
...on/wallet/android/presentation/accessfactorsources/deriveaccounts/DeriveAccountsViewModel.kt
@@ -180,7 +186,7 @@ class DeriveAccountsViewModel @Inject constructor(
accessFactorSourcesIOHandler.setOutput(
output = AccessFactorSourcesOutput.DerivedAccountsWithNextDerivationPath(
derivedAccounts = derivedAccounts,
nextDerivationPathOffset = indicesToScan.last() + 1u
nextDerivationPathIndex = HdPathComponent.init(globalKeySpace = indicesToScan.last().indexInGlobalKeySpace + 1u)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here and...

micbakos-rdx
micbakos-rdx commented Oct 29, 2024
View reviewed changes
...on/wallet/android/presentation/accessfactorsources/deriveaccounts/DeriveAccountsViewModel.kt
@@ -206,15 +212,15 @@ class DeriveAccountsViewModel @Inject constructor(
accessFactorSourcesIOHandler.setOutput(
output = AccessFactorSourcesOutput.DerivedAccountsWithNextDerivationPath(
derivedAccounts = derivedAccounts,
nextDerivationPathOffset = indicesToScan.last() + 1u
nextDerivationPathIndex = HdPathComponent.init(globalKeySpace = indicesToScan.last().indexInGlobalKeySpace + 1u)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... also here last() was used. That is why I think LinkedHashSet should be used.

CyonAlexRDX
CyonAlexRDX approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

@micbakos-rdx micbakos-rdx merged commit bbf993a into main Oct 30, 2024
8 of 9 checks passed
@micbakos-rdx micbakos-rdx deleted the micbakos/hd-path-component branch October 30, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sajjon Sajjon Sajjon left review comments

@CyonAlexRDX CyonAlexRDX CyonAlexRDX approved these changes

@giannis-rdx giannis-rdx Awaiting requested review from giannis-rdx

@sergiupuhalschi-rdx sergiupuhalschi-rdx Awaiting requested review from sergiupuhalschi-rdx

Assignees

@micbakos-rdx micbakos-rdx

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@micbakos-rdx @Sajjon @CyonAlexRDX