-
Notifications
You must be signed in to change notification settings - Fork 55
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 leading zero bug #255
fix leading zero bug #255
Conversation
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.
LGTM, but probably good if andrew also takes a look
src/services/verifier.rs
Outdated
let encoded_info = &attr_info.encoded; | ||
|
||
let reveal_attr_encoded = if leading_zero_regex.captures(encoded_info).is_some() { | ||
encoded_info.trim_start_matches('0') |
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 could not use the captured group sadly as it captures the entire number, seems like an insignificant performance decrease to me.
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 could create a capture group for this I think?
Not sure if it's better for performance, or if it's better at all. But this would work I think:
^(0+)[1-9]([0-9]+)?$
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.
Actually the regex probably should use $ to avoid stripping leading zeros on "01.0" for instance
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.
Moved from the regex to String::parse<i32>
as it changes -0100
to -100
.
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'll need to add a similar fix to credx. TBH I wasn't sure why it was trimming zeroes there when I came across it.
Related: hyperledger/indy-shared-rs#43
11a3f0c
to
556fa9f
Compare
assert_eq!(normalize_encoded_attr("01.0"), "01.0"); | ||
assert_eq!(normalize_encoded_attr("0abc"), "0abc"); | ||
assert_eq!(normalize_encoded_attr("-100"), "-100"); | ||
assert_eq!(normalize_encoded_attr("-0100"), "-100"); |
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 you expect this to have the leading 0 trimmed?
We need to make sure the spec aligns with this behavior
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.
The spec mentions convert any string integer (e.g. "1234") to be a 32-bit integer (e.g. 1234)
. Since integers can be negative, I thought this was the correct way. Maybe @swcurran can offer some clarification on this?
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.
That makes sense yes 👍
Signed-off-by: Berend Sliedrecht <blu3beri@proton.me>
556fa9f
to
0f84ead
Compare
closes #252
Minor bug where a value contains a "0" without having any digits behind it.
Updated the regex to check for leading zeroes which must be followed by another digit. If this is the case, we trim the zeroes and return the leftover. If it is something like
0011212abab
it does not strip the zeroes.https://github.com/hyperledger/aries-rfcs/tree/main/features/0592-indy-attachments#notes-on-encoding-claims