fix: persist leading zero #1836
Conversation
sairanjit
commented
Apr 20, 2024
sairanjit
commented
- This fixes the removal of the leading zeroes
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com>
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com>
| // If value is a number string with leading zero and not a decimal return as number string | ||
| if (isString(value) && !isNaN(Number(value)) && !isDecimal(value) && hasLeadingZero(value)) { | ||
| return value.toString() | ||
| } | ||
|
|
There was a problem hiding this comment.
If the value is not a number we still need to encode it as a number.
So i would expand the next if statement to also add && !hadLeadingZero.
Otherwise it will reach the end as be encoded as any other string (by hashing)
There was a problem hiding this comment.
You either need to encode it as a number and remove the leading zero, or you need to encode it as a normal string
There was a problem hiding this comment.
I think adding !hasLeadingZero(value) in the next if statement resolves it.
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com>
| // If value is a number string with leading zero and not a decimal return as number string | ||
| if (isString(value) && !isNaN(Number(value)) && !isDecimal(value) && hasLeadingZero(value)) { | ||
| return value.toString() | ||
| } | ||
|
|
There was a problem hiding this comment.
This can be removed now right?
There was a problem hiding this comment.
Okay If I remove that then the number 0678 is hashed instead it should return a number right ?
There was a problem hiding this comment.
A number with a leading zero is not a valid number i think for AnonCreds
@berendsliedrecht we had an issue with leading 0 before in AnonCreds. What's the behavior there so we can match it?
There was a problem hiding this comment.
@TimoGlastra This PR fix leading zero bug in AnonCreds has some fixes.
There was a problem hiding this comment.
Yeah so it trims the leading 0's. We'd have to fix it in anoncreds as well.
I think it's best to treat strings with leading 0's as a string. If you want it as a number, just remove the leading 0's manually.
There was a problem hiding this comment.
I think we also have to adjust in AnonCreds RS, as that's where the values are transformed for the credential.
We should keep behavior the same beteeen the two
There was a problem hiding this comment.
according to https://github.com/hyperledger/aries-rfcs/blob/50d148b812c45af3fc847c1e7033b084683dceb7/features/0592-indy-attachments/README.md#notes-on-encoding-claims we should keep the leading zeroes in the raw values and strip them in the encoded value.
I think it's best to treat strings with leading 0's as a string.
That would mean that integer strings with leading zeroes can not be used for predicates as the encoded value will be used for that.
The use of AnonCreds predicates, such as proving "older than 21" based on a date of birth claim without sharing the date of birth, is based on an expression involving the encoded value. Thus, only raw integers or string integers can be used in AnonCreds predicates.
I think we should follow the structure for the encoding claims (I think which already happens correctly in anoncreds-rs) and we should maybe open an issue if a claim requires leading zeroes if that can be enabled somehow.
There was a problem hiding this comment.
Okay makes sense, so what should change in Credo then?
There was a problem hiding this comment.
nothing, this PR contradicts the definition of the RFC. There is absolutely a valid use case for leading zero numbers in claims, but that should change in the RFC and then we can update accordingly. I would not want to deviate from the specs.
There was a problem hiding this comment.
@berendsliedrecht I agree with you if we need to update the logic we should change in RFC first. @TimoGlastra Can we close this PR as of now ?
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com>
