Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix byte length of publickKeyMultibase. #67

Merged
merged 2 commits into from
Dec 27, 2023
Merged

Fix byte length of publickKeyMultibase. #67

merged 2 commits into from
Dec 27, 2023

Conversation

JSAssassin
Copy link
Contributor

@JSAssassin JSAssassin commented Nov 3, 2023 •
edited by pr-preview bot

Addresses #66

Preview | Diff

@JSAssassin JSAssassin mentioned this pull request Nov 3, 2023
Merged
@w3cbot
Copy link

w3cbot commented Nov 3, 2023

iherman marked as non substantive for IPR from ash-nazg.

TallTed
TallTed reviewed Nov 9, 2023
View reviewed changes
index.html Outdated Show resolved Hide resolved
TallTed
TallTed reviewed Nov 9, 2023
View reviewed changes
index.html Outdated Show resolved Hide resolved
@msporny msporny added the normative This item is a substantive or normative change. label Dec 10, 2023
Wind4Greg
Wind4Greg reviewed Dec 11, 2023
View reviewed changes
Copy link
Contributor

@Wind4Greg Wind4Greg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it seems that the description is also bit problematic. The publicKeyMultibase value is a string that contains the base-58-btc encoding of the 2 byte prefix plus the 32 byte key. So the length in bytes of what is being encoded is 34 bytes but the string size is whatever. Shouldn't we re-write this a little to be clearer.

@msporny msporny added editorial This item is editorial in nature. and removed normative This item is a substantive or normative change. labels Dec 27, 2023
@msporny
Copy link
Member

msporny commented Dec 27, 2023 •
edited

Hmm, it seems that the description is also bit problematic. The publicKeyMultibase value is a string that contains the base-58-btc encoding of the 2 byte prefix plus the 32 byte key. So the length in bytes of what is being encoded is 34 bytes but the string size is whatever. Shouldn't we re-write this a little to be clearer.

Yes, I'll do some editorial fix-ups to make this more clear after the merge. I can't edit the PR, so I have to do the fixes in a separate commit.

UPDATE: This is now complete in commit 215cdba

@msporny msporny added normative This item is a substantive or normative change. and removed editorial This item is editorial in nature. labels Dec 27, 2023
@msporny
Copy link
Member

msporny commented Dec 27, 2023

Arguably normative, multiple reviews, changes requested (and will be made in a separate commit), no objections, merging.

@msporny msporny merged commit b3f6739 into w3c:main Dec 27, 2023
1 check passed
@msporny msporny mentioned this pull request Dec 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wind4Greg Wind4Greg Wind4Greg left review comments

@msporny msporny msporny approved these changes

@dmitrizagidulin dmitrizagidulin Awaiting requested review from dmitrizagidulin dmitrizagidulin is a code owner

@TallTed TallTed Awaiting requested review from TallTed

Assignees
No one assigned
Labels
normative This item is a substantive or normative change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JSAssassin @w3cbot @msporny @Wind4Greg @TallTed