-
Notifications
You must be signed in to change notification settings - Fork 110
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
add WebTransport multiaddr components #176
Conversation
a07348a
to
2903efa
Compare
transcoders.go
Outdated
} | ||
|
||
func certHashBtS(b []byte) (string, error) { | ||
return multibase.Encode(multibase.Base58BTC, b) |
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 choice is totally arbitrary. We could use any other multibase that's widely supported.
Is there any best practice for choosing one over the other?
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.
cc @lidel, but the general guidelines I know of are:
- Do you want compact -> choose base64URL (
u
), although base58btc is probably fine too - Do you want lowercase -> choose base32 (
b
)
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.
Switched to Base64url
.
var TranscoderCertHash = NewTranscoderFromFunctions(certHashStB, certHashBtS, nil) | ||
|
||
func certHashStB(s string) ([]byte, error) { | ||
_, data, err := multibase.Decode(s) |
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.
Are there any other requirements here? e.g. is it a multihash, is it just 32 bytes, etc.
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.
There are. First of all, it needs to be a multihash. Do you want to check that here, just to make sure we're not encoding garbage?
Currently browsers only support SHA256, but I'd say that the WebTransport implementation would be the better place to handle 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.
I don't live in this repo much, I was mostly copying from what we do with the p2p multiaddr
Line 314 in f5adc3b
func p2pStB(s string) ([]byte, error) { |
This seems to indicate that if adding some basic checking is relatively cheap then we might as well put it here. If web transport isn't specifically 32 bytes (plus the extra couple multihash bytes) then leaving that part defined in the transport seems reasonable.
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 added the validation on encoding.
2903efa
to
db19e64
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.
🚀
@@ -15,6 +15,8 @@ const ( | |||
P_IP6ZONE = 0x002A | |||
P_IPCIDR = 0x002B | |||
P_QUIC = 0x01CC | |||
P_WEBTRANSPORT = 0x01D1 | |||
P_CERTHASH = 0x01D2 |
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 you claim the code on https://github.com/multiformats/multiaddr/blob/master/protocols.csv?
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.
Based on libp2p/specs#404.