-
Notifications
You must be signed in to change notification settings - Fork 200
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: persist leading zero #1836
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ const isString = (value: unknown): value is string => typeof value === 'string' | |
const isNumber = (value: unknown): value is number => typeof value === 'number' | ||
const isBoolean = (value: unknown): value is boolean => typeof value === 'boolean' | ||
const isNumeric = (value: string) => /^-?\d+$/.test(value) | ||
const isDecimal = (value: string): boolean => /\d+\.\d+/.test(value) | ||
const hasLeadingZero = (value: string) => value.length > 1 && value.startsWith('0') | ||
|
||
const isInt32 = (number: number) => { | ||
const minI32 = -2147483648 | ||
|
@@ -49,8 +51,20 @@ export function encodeCredentialValue(value: unknown) { | |
return value.toString() | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be removed now right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay If I remove that then the number There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TimoGlastra This PR fix leading zero bug in AnonCreds has some fixes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
That would mean that integer strings with leading zeroes can not be used for predicates as the encoded value will be used for that.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay makes sense, so what should change in Credo then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 ? |
||
// If value is an int32 number string return as number string | ||
if (isString(value) && !isEmpty(value) && !isNaN(Number(value)) && isNumeric(value) && isInt32(Number(value))) { | ||
if ( | ||
isString(value) && | ||
!isEmpty(value) && | ||
!isNaN(Number(value)) && | ||
isNumeric(value) && | ||
isInt32(Number(value)) && | ||
!hasLeadingZero(value) | ||
) { | ||
return Number(value).toString() | ||
} | ||
|
||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding
!hasLeadingZero(value)
in the next if statement resolves it.