chore: improvements#51
Conversation
Autoparallel
left a comment
Autoparallel
left a comment
There was a problem hiding this comment.
Looking good so far!
| let mut new_factors = vec![]; | ||
|
|
||
| for (id, factor) in factors.values_mut().enumerate() { | ||
| for (id, (_, mut factor)) in factors.into_iter().enumerate() { |
There was a problem hiding this comment.
this seems a little awkward, why not iter_mut and drop the enumerate?
There was a problem hiding this comment.
that's because we need ownwership of factor to prevent any unnecessary clones.
There was a problem hiding this comment.
I see, just to be fair cloning a ref is just cloning the pointer, right?
| fn include_params(&mut self, params: Self::Params) -> MFKDF2Result<()> { | ||
| // Store the policy parameters for derive phase | ||
| self.params = serde_json::to_string(¶ms).unwrap(); | ||
| self.params = params.clone(); |
There was a problem hiding this comment.
seems like a redundant clone based on line 23 with HOTPParams below?
There was a problem hiding this comment.
How so? self.params and from_value takes the ownership of the underlying Value, so the clone is needed. In next refactoring, the aim is to get rid of the Value and work with factor specific params, because Value is only needed for FFI boundaries and serialization.
| shares_bytes.push(Some(material.data())); | ||
| } else { | ||
| material.factor_type.include_params(serde_json::from_str(&factor.params).unwrap())?; | ||
| material.factor_type.include_params(factor.params.clone())?; |
There was a problem hiding this comment.
if you swapped the order of some of this around you could probably rid of this clone.
There was a problem hiding this comment.
Tried that, but not possible :)
will have to modify include_params to take in a shared reference, but that would mean we'll have to clone the params inside the method anyway to add it in the factor.
There was a problem hiding this comment.
Yeah, but cloning a ref?
lonerapier
force-pushed
the
fix/todos
branch
from
d7864b5 to
73be1d0
Compare
|
Updates:
|
Autoparallel
left a comment
Autoparallel
left a comment
There was a problem hiding this comment.
Good improvements! See latest feedback.
|
|
||
| #[cfg_attr(feature = "bindings", derive(uniffi::Record))] | ||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct HOTPConfig { |
There was a problem hiding this comment.
Having this and HOTPOptions feels redundant
There was a problem hiding this comment.
I wanted to remove any unwraps from the params and output path in Final key derivation. So, had to create a similar config struct. What's a better solution?
|
|
||
| #[cfg_attr(feature = "bindings", derive(uniffi::Record))] | ||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct HOTPParams { |
There was a problem hiding this comment.
... And HOTPParams. Unneeded?
| pub struct ByteArray<const N: usize>(pub [u8; N]); | ||
|
|
||
| /// 32 byte key | ||
| pub type Key = ByteArray<32>; | ||
|
|
||
| /// 32 byte salt | ||
| pub type Salt = ByteArray<32>; |
There was a problem hiding this comment.
added a generic ByteArray and wrapped Key and Salt.
|
all review comments handled. Merging the PR!! |
2 participants
factor.outputargsRelated PR: multifactor/MFKDF#28