-
Notifications
You must be signed in to change notification settings - Fork 17
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
Address TXT records > 255 characters; make language more consistent; separate section for root records #162
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
==========================================
+ Coverage 55.33% 55.63% +0.29%
==========================================
Files 29 29
Lines 2230 2245 +15
==========================================
+ Hits 1234 1249 +15
Misses 879 879
Partials 117 117 ☔ View full report in Codecov by Sentry. |
| _k0.did. | TXT | 7200 | id=0;t=0;k=sTyTLYw-n1NI9X-84NaCuis1wZjAA8lku6f6Et5201g | | ||
| _k1.did. | TXT | 7200 | id=WVy5IWMa36AoyAXZDvPd5j9zxt2t-GjifDEV-DwgIdQ;t=3;k=3POE0_i2mGeZ2qiQCA3KcLfi1fZo0311CXFSIwt1nB4;a=ECDH-ES+A128KW | | ||
| _s0.did. | TXT | 7200 | id=service-1;t=TestLongService;se=https://test-lllllllllllllllllllllllllllllllllllooooooooooooooooooooonnnnnnnnnnnnnnnnnnngggggggggggggggggggggggggggggggggggggsssssssssssssssssssssssssseeeeeeeeeeeeeeeeeeerrrrrrrrrrrrrrrvvvvvvvvvvvvvvvvvvvviiiiiiiiiiiiiiii iiiiiiiiiiiiiiiccccccccccccccccccccccccccccccceeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee.com/1 | |
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 a test vector for a chunked record
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.
Sweet, great work! One suggestion on handling other charsets.
impl/internal/did/did.go
Outdated
for len(record) > 255 { | ||
chunks = append(chunks, record[:255]) | ||
record = record[255:] | ||
} |
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.
TIL (thanks chad!) that len(str)
returns the number of bytes. This may not be equivalent to the number of chars. For instance, len("£")
returns 2
.
Given the above, if you want to actually do this into 255 chars, then see the function below.
// SplitString255 splits a string into substrings of up to 255 characters.
func SplitString255(input string) []string {
var chunks []string
runeArray := []rune(input) // Convert to rune slice to properly handle multi-byte characters
for len(runeArray) > 0 {
if len(runeArray) <= 255 {
chunks = append(chunks, string(runeArray))
break
}
chunks = append(chunks, string(runeArray[:255]))
runeArray = runeArray[255:]
}
return chunks
}
But the real question is whether the library should allow setting a value in a DID Document which contains values outside the ASCII range. I think the library should. And thus, I believe the value should be base64url encoded before doing chunking, so there are no issues with existing DNS systems. If you agree, then my suggestion doesn't matter, as all ASCII chars are 1-byte when encoded in UTF-8.
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 is a great catch about bytes vs character sizes - thank you
I am opposed to b64 encoding because of size increases
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 am going to merge, but please feel free to open an issue to support non-ASCII characters.
Fix #94