-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix verkey expansion #104
fix verkey expansion #104
Conversation
77c6814
to
007c3ff
Compare
failing step in GHA should be fixed by #105 |
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.
I am not too familiar with the specific implementation so my review is mainly about the code.
let expanded_verkey = expand_verkey_indy(id, verkey); | ||
if expanded_verkey.is_err() { | ||
return verkey.to_string(); | ||
} | ||
return expanded_verkey.unwrap(); |
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.
I think a match would be cleaner here:
let expanded_verkey = expand_verkey_indy(id, verkey); | |
if expanded_verkey.is_err() { | |
return verkey.to_string(); | |
} | |
return expanded_verkey.unwrap(); | |
match expand_verkey_indy(id, verkey) { | |
Ok(key) => key, | |
Err(_) => verkey.to_string() | |
} |
returning with a &str
would make it even terser though.
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.
Thanks! I fully agree - looking at your code feels like looking at rust code - mine feels like Go :D
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.
expand_verkey_indy(id, verkey).unwrap_or(verkey.to_string())
Would probably work as well now that I think about it.
let (key, key_type) = if verkey.contains(":") { | ||
let vec: Vec<&str> = verkey.split(":").collect(); | ||
match vec.len() { | ||
0 | 1 => { |
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.
You check here for a length of 0 or 1, but you never check the total element count. In the match case below vec[vec.len() - 1]
is called which might be the 99th element. I think a length check here would be a good addition.
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.
I will change that whole key_type part to expect ecactly 2 outputs and error on anything else - the indy-sdk implementation kinda expects the same.
Signed-off-by: Christian Bormann <ChristianCarl.Bormann@de.bosch.com>
007c3ff
to
f2483cf
Compare
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.
There are still a few odd code patterns for Rust but I think the behaviour is correct, and it can be cleaned up a bit later.
Fixes a bug that was taken over from the initial resolver prototype, see IDunion/indy-did-resolver#26 for more details.