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.

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

Merged
merged 9 commits into from
Apr 3, 2024

Conversation

decentralgabe
Copy link
Member

Fix #94

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 83.72093% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 55.63%. Comparing base (98c10ef) to head (6ae2a1e).

❗ Current head 6ae2a1e differs from pull request most recent head 5db77ba. Consider uploading reports for the commit 5db77ba to get more accurate results

Files Patch % Lines
impl/internal/did/did.go 83.72% 4 Missing and 3 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

@decentralgabe decentralgabe marked this pull request as ready for review April 3, 2024 19:15
| _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 |
Copy link
Member Author

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

Copy link
Contributor

@andresuribe87 andresuribe87 left a 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.

Comment on lines 804 to 807
for len(record) > 255 {
chunks = append(chunks, record[:255])
record = record[255:]
}
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider designing with DNS TXT record limits in mind
3 participants