-
Notifications
You must be signed in to change notification settings - Fork 3
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
DIP-263 Use User Data for Public Keys #276
Conversation
pages/DSNP/UserData.md
Outdated
@@ -190,12 +191,12 @@ The following example illustrates the output of a Get User Data Operation invoca | |||
{ | |||
"data": base64_string, | |||
"etag": string, | |||
"keyId": integer | |||
"keyIndex": integer |
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 this be a change to the https://github.com/LibertyDSNP/graph-sdk/ that needs to happen?
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 don't think this "example" JSON serialization is used directly in graph-sdk, but @aramikm or Joe might know better. I'll get some more eyes on it. If there's prior art out there using keyId
I'd be fine with keeping it that way for simplicity's sake, but documenting that keyId
is the index.
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.
Per Joe this won't impact graph-sdk, so resolving this thread.
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.
After thinking about this, I think this should just keep keyId
. It is an identifier even if it applies an index algo.
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.
Agreed
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.
All looks correct to me!
4391c5d
to
5e03123
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.
Just updating my approval for the small additional updates.
Problem
See #263
Solution