-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat!: add support for VerifiableURI and deprecate JSONURL / AssetURL #365
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #365 +/- ##
===========================================
- Coverage 83.29% 83.18% -0.11%
===========================================
Files 19 19
Lines 1221 1231 +10
Branches 276 282 +6
===========================================
+ Hits 1017 1024 +7
Misses 110 110
- Partials 94 97 +3 ☔ View full report in Codecov by Sentry. |
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.
added review comments
a074259
to
4df2377
Compare
4df2377
to
8a02062
Compare
Let's get @Hugoo review as well 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.
LGTM except that I think the type should be rather named VerifiableURI and not VerifiableURL.
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.
src/lib/encoder.ts
Outdated
if (verificationMethod !== undefined) { | ||
const encodedLength = `0x${value.slice(14, 18)}`; // Rest of data string after function hash | ||
const dataLength = hexToNumber(encodedLength, false) as number; | ||
const dataHash = `0x${value.slice(18, 18 + dataLength * 2)}`; // Get jsonHash 32 bytes | ||
const dataSource = hexToUtf8('0x' + value.slice(18 + dataLength * 2)); // Get remainder as URI | ||
return { | ||
verification: { | ||
method: verificationMethod.name, | ||
data: dataHash, | ||
}, | ||
url: dataSource, | ||
}; | ||
} |
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.
what happens if there is no verification method?
Id rather do it the other way with early return
if (verificationMethod === undefined) {
return null or undefined or stmg;
}
// and the current code here
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.
A null verification method should return the data but no verification method or data. So you're right we should not exit this way. Fixing.
src/lib/encoder.ts
Outdated
const dataSource = hexToUtf8('0x' + value.slice(18 + dataLength * 2)); // Get remainder as URI | ||
return { | ||
verification: { | ||
method: verificationMethod.name, |
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.
method: verificationMethod.name, | |
method: verificationMethod?.name || UNKNOWN_VERIFICATION_METHOD, |
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.
Can name not be defined on verificationMethod ?
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 verificationMethod could be undefined and therefore the url is not validated
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 was more thinking of a situation where : it is defined but doesnt have a name property
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.
But we kind of want it to be undefined if not defined
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 default should be the signature hash so that reading a VerificationURI can return a yet unsupported custom verification method.
src/lib/encoder.ts
Outdated
const dataSource = hexToUtf8('0x' + value.slice(18 + dataLength * 2)); // Get remainder as URI | ||
return { | ||
verification: { | ||
method: verificationMethod.name, |
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 was more thinking of a situation where : it is defined but doesnt have a name property
…se an error within decoder.
2e22522
to
b3a273a
Compare
fmi, will we keep the backward compatibility forever or is it temporary? If so we can already create a ticket that we keep in the backlog to remove the deprecated code |
@magalimorin18 it will be for some time until devs can update their code but ultimately
|
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.
Creating github issue for unknown method.
stripHexPrefix(verification ? verification.data : padLeft(0, 64)) + | ||
stripHexPrefix(utf8ToHex(dataSource)) | ||
); | ||
return [ |
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.
This currently cannot return data containing an unsupported verification method. I think it needs to copy the verification function signature and data verbatim every time. I messed this up.
|
||
return { | ||
verification: { | ||
method: verificationMethod?.name || UNKNOWN_VERIFICATION_METHOD, |
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 default should be verificationMethodSig and not UNKNOWN_VERIFICATION_METHOD.
What kind of change does this PR introduce (bug fix, feature, docs update, ...)?
VerifiableURI
support and make it default on encode.JSONURL
What is the current behaviour (you can also link to an open issue here)?
The current software does not support the
VerifiableURI
encoding.What is the new behaviour (if this is a feature change)?
Support
VerifiableURI
andJSONURL
on read and always output VerifiableURL on encode.Other information: