-
Notifications
You must be signed in to change notification settings - Fork 0
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
FactorInstancesProvider #18
Conversation
This reverts commit 7c633ee.
…e add_n on HDPathComponent to be throwing, cascading to NextEntityIndexAssigner and FactorInstanceProvider to also be throwing.
…tityIndexWithEphemeralOffsets
f5d61c3
to
8136341
Compare
if instances.first().unwrap().derivation_entity_base_index() | ||
!= last.derivation_entity_base_index() + 1 | ||
{ | ||
warn!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically want all indices to be contiguous - as in no "gaps" - but in case of legacy Profile that will not be true, so we can typically not enforce it. Perhaps this warn!
should be changed to info!
....
/// **OR LESS**, never more, and if less, it means we MUST derive more, and if we | ||
/// must derive more, this function returns the quantities to derive for each factor source, | ||
/// for each derivation preset, not only the originally requested one. | ||
pub fn get_poly_factor_with_quantities( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be cool if anyone of you can clean this up, quite a fun challenge :) probably we can use the same code for all DerivationPreset::all()
to produce some intermediary result, and then conditionally reduce into CachedInstancesWithQuantitiesOutcome
...
… of it, and changing the FactorInstancesProvider to have fields and use methods rather than functions.
…ethod split out special functions on FactorInstancesProvider to be adopters…
…d important missing unit tests asserting that the Cache remains unchanged if the KeysCollector fails deriving keys.
assert_eq!( | ||
os.cache_snapshot(), | ||
cache_before_fail, | ||
"Cache should not have changed when failing." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
important behaviour! This unit tests proves that we only mutate the cache in case of success!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 👏
Added some comments/questions, will revisit FactorInstancesProvider
again later
src/factor_instances_provider/agnostic_paths/derivation_preset.rs
Outdated
Show resolved
Hide resolved
src/factor_instances_provider/next_index_assigner/next_derivation_entity_index_assigner.rs
Outdated
Show resolved
Hide resolved
...tances_provider/next_index_assigner/next_derivation_entity_index_cache_analyzing_assigner.rs
Outdated
Show resolved
Hide resolved
...r_instances_provider/provider/provider_adopters/securify_entity_factor_instances_provider.rs
Show resolved
Hide resolved
...r_instances_provider/provider/provider_adopters/virtual_entity_creating_instance_provider.rs
Show resolved
Hide resolved
Co-authored-by: matiasbzurovski <[email protected]>
Co-authored-by: matiasbzurovski <[email protected]>
Co-authored-by: matiasbzurovski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good overall, and code quality is more than acceptable :). Amazing job!
pub fn key_kind(&self) -> CAP26KeyKind { | ||
match self { | ||
Self::AccountVeci | Self::IdentityVeci | Self::AccountMfa | Self::IdentityMfa => { | ||
CAP26KeyKind::TransactionSigning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this possibly be expanded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes if we would add ROLA keys. And then I want this match to stop compiling :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work!
...next_index_assigner/next_derivation_entity_index_with_ephemeral_offsets_for_factor_source.rs
Outdated
Show resolved
Hide resolved
…_source.rs Co-authored-by: matiasbzurovski <[email protected]>
CodeCoverage is higher than reported, many false negatives due to
.await
.Important
This implementation does NOT use the index of the last FactorInstance to be taken from the Cache, in order to derive instances AFTER that index. This means that if the
FactorInstancesProvider
is used without a Profile, then0'
/0^
(^
is new notation meaning "hardened and in KeySpace::Securified) will be usedsee https://github.com/radixdlt/sargon-mfa/blob/factor_instances_provider/src/factor_instances_provider/provider/provider_adopters/virtual_entity_creating_instance_provider.rs#L491
This feels OK, since in most operations we use Profile.
It is important we remember this when we build Account Recovery Scan, because it would start over at
0'
when all instances from cache have been used. A simple solution to this would be to allow keeping some local offset counter outside ofFactorInstancesProvider
and allow injecting it / passing it as argument to theFactorInstancesProvider
orFactorInstancesCache
, typically to be set onNextDerivationEntityIndexWithEphemeralOffsets
.KeySpace
DerivationPreset
These are the supported "presets" used by the
FactorInstancesProvider
. If we ever need to derive more factor instances, we derive for all these presets for ALL references FactorSources. However, we only derive is the cache is not "full" for that factor source for that preset. Definition of "full" is if there areCACHE_FILLING_QUANTITY
(30
) many FactorInstances.We use
CACHE_FILLING_QUANTITY
for ALL FactorSourceKinds for ALL Presets. To keep things simple. Meaning per FactorSource there will beCACHE_FILLING_QUANTITY
many FactorInstances in Unsecurified KeySpace andCACHE_FILLING_QUANTITY
FactorInstances in Securified KeySpace to be used by both Accounts and Identities respectively, in total 4 *CACHE_FILLING_QUANTITY
=120
FactorInstances. We can tweak this in the future if we want.A
DerivationPreset
is essentially a triple(CAP26EntityKind, CAP26KeyKind, KeySpace)
.IndexAgnosticPath
From the triple
(CAP26EntityKind, CAP26KeyKind, KeySpace)
we get fromDerivationPreset
we can give it aNetworkID
to get anIndexAgnosticPath
Cache
The cache needs to work independent of entity index, meaning we cannot use
DerivationPath
s as keys, which is quite self-explanatory if you think about this for a couple of seconds... we wanna go to the cache and say, "give me the 5 next free FactorInstances for this DerivationPreset on this network for this FactorSource". Meaning we should not have to think about which indices those next 5 instances have. So instead the cache usesIndexAgnosticPath
as Cache Keys. So we haveHashMap<IndexAgnosticPath, IndexSet<HierarchicalDeterministicFactorInstance>>
.But I've created a
struct FactorInstances(IndexSet<HierarchicalDeterministicFactorInstance>)
new type, so we cache:HashMap<IndexAgnosticPath, FactorInstances>
But we support multiple FactorSources... so the cache is:
FactorInstancesProvider
evolutionInit
Naive
That would not be so helpful an implementation, since what would happen if the cache
contains less than
number_of_accounts
of FactorInstances for any of the FactorSources?So the solution must be more sophisticated.
"Partial" Load from cache / Derive More
Important
We are getting somewhere, but we have omitted fundamental logic, which
is which Derivation Entity Indices to use? Remember our DerivationPath
"m/44'/1022'/<NETWORK>'/<ENTITY_KIND>'/<KEY_KIND>'/<ENTITY_INDEX>'"
What to use for
ENTITY_INDEX>'
?The type is
EntityIndex
and it is au32
with either+ 2^30
ifit is in the securified keyspace or
+ 0
, if in unsecurified keyspace.Important
But it is more complex than that, subsequent calls to
next_index_assigner.next
will return the same index! So the
NextIndexAssigner
must keep some "local"counters, lets call them, "ephemeral offsets" per factor source, so that subsequent
calls to
next
returns differnet ids!Important
But it is not as simple as that, if we are gonna prompt user to derive more keys
using the
KeysCollector
, we might as well take the opportunity to fill the cachefor other
DerivationPreset
s as well. In the example above we are providingFactorInstances for
DerivationPreset::AccountMfa
but what if the cache does notcontain
CACHE_FILLING_SIZE
many factors forDerivationPreset::IdentityVeci
?We should derive for that to, and fill the cache! Just like we fill the cache
for
DerivationPreset::all()
when we are adding a new FactorSource!Alas...
Important
It is more complex than this, because we must
Adoption in Sargon and Future work
The overly simplified
SargonOS
in this PR contains many not-too-far-from-being-production-ready implementations of usages of theFactorInstancesProvider
and the adopter typesVirtualEntityCreatingInstanceProvider
andSecurifyEntityFactorInstancesProvider
which both are essentially wrappers around theFactorInstancesProvider
but with the appropriate set of parameters for the two operations: create VECI and securify entity respectively. We should probably not copy paste them over, but we can use them as inspiration, and I expect only very small changes to the parameter and usage.Derive instances for new
FactorSource
https://github.com/radixdlt/sargon-mfa/blob/factor_instances_provider/src/factor_instances_provider/provider/test_sargon_os.rs#L254-L293
New (Unsecurified) Virtual Entity
Create new virtual entities using the
FactorInstancesProvider
:https://github.com/radixdlt/sargon-mfa/blob/factor_instances_provider/src/factor_instances_provider/provider/test_sargon_os.rs#L96-L138
Securifying Entities
Securying Entities using the
FactorInstancesProvider
:https://github.com/radixdlt/sargon-mfa/blob/factor_instances_provider/src/factor_instances_provider/provider/test_sargon_os.rs#L166-L243