-
Notifications
You must be signed in to change notification settings - Fork 156
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
initial MMP protocol draft #899
base: main
Are you sure you want to change the base?
Conversation
7a712bb
to
b257a2d
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.
Initial comments; reviewed with @str4d, @daira, @arya2 and @conradoplg.
zips/zip-tbd1.md
Outdated
|
||
## ABNF definition of message URI syntax | ||
mmp-uri = mmp ":" version ":" cid query-params | ||
mmp-uri = /1*512VCHAR; Limit to 512 ASCII VCHARs |
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.
Is VCHAR the correct choice here? Should this be a smaller character set? Perhaps pchar / "?"
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.
To avoid parsing issues with URI libraries, it does make sense to keep this URI safe. However, because this needs maximum extensibility such that a version has the versatility it might need with various reserve characters I'm thinking it makes sense to make this unreserved
/ reserved
which I think would be correct here in saying that any URI safe character can be used as long as the total length is less than 512 characters.
Does that sound good to you?
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.
The important thing is that parsing ambiguities aren't introduced here. Also, since this isn't valid ABNF (to have two productions with different constraints) there should be some other mechanism used to express this constraint.
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.
The important thing is that parsing ambiguities aren't introduced here.
Agreed, will make sure that this is complying with the URI spec.
Also, since this isn't valid ABNF (to have two productions with different constraints) there should be some other mechanism used to express this constraint.
I believe this would be valid ABNF under: https://datatracker.ietf.org/doc/html/rfc5234#section-3.3
The reason I used this method was to make it clear the maximum length of the entire URI is meant to be 512 characters. If there's a way to do that within a single rule, I'm happy to change it but with the variability of length for the message-location
and query-params
sections it wasn't clear to me how to do it other than this way.
zips/zip-tbd1.md
Outdated
In all versions of the Media Memo Protocol that require passing a secret key between sender and recipients the key parameter MUST be used to. The key parameter MUST be the first query parameter if used so it should follow directly after a ? symbol. Reuse of this query parameter for many protocols will ensure the greatest possible interoperability. | ||
|
||
### ABNF for Encryption Key Query Parameter | ||
encrypt-key-param = "key=" 1*64VCHAR |
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 doesn't define the ttl
field that you show in your example; under the current ABNF, it would need to be part of the encrypt-key-param
production, which is required to appear first. Instead of defining an explicit initial encrypt-key-param
production that must appear before other query parameter pairs, permit an arbitrary order and then just require the key
and potentially ttl
(if intended) keys be present in prose.
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.
Also: the key should appear in a fragment, not as a query parameter, as it should NOT be sent to the server (if processed by a naive parser.)
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.
The various options I considered here were:
- Add an additional
:
delimited section after version, but before the message location that is the key, but this becomes weird if someone opts to not have a key all together in a version. This could be solved with an empty section that produces::
if we want and saves a few bytes. - Reserve use of fragments for passing the key only and prevent usage of other fragments from being used
- query parameter specific so that it can be dropped or added as needed.
The reason I opted to go with the query parameter option is that I expected that the parsers would handle this fine, but as soon as someone passed this to a URL resolver it would fail in which case it wouldn't make a network call at all. The one edge case that I can think of here is the registration of custom scheme handlers in operating systems and some browsers which may end up passing the URI around locally on device, but would likely still fail to resolve them in which case I couldn't see a reason that the query parameter didn't make sense.
However, in looking into the behavior of fragments a bit more for different media types, it does look possible to define multiple such as the example for PDFs.
In this case, I'm convinced that using fragments is the better option here but will keep the syntax matching what's described (well with some modifications to the characters that can be used as the questions above around VCHAR apply here too).
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.
Also, I opted to just drop the ttl example from this spec. Instead it seems more relevant that this is defined in the actual v1 spec.
This is great feedback @nuttycom - I'll come back to this next week and address each of the comments. Thanks to you and the others for reviewing this! |
Some good feedback provided around ABNF irregularities which led to a simpler ABNF. Additionally updated some spec text to align with the new ABNF definitions. Co-authored-by: Kris Nuttycombe <[email protected]> Co-authored-by: Jack Grigg <[email protected]> Co-authored-by: Daira-Emma Hopwood <[email protected]> Co-authored-by: Arya <[email protected]> Co-authored-by: Conrado Gouvea <[email protected]>
Ok, I believe this should address all the great feedback provided so far. This actually seemed to simplify the ABNF definitions once I thought through it a bit more too. |
This is the initial protocol specification for sending messages larger than 512 characters between two parties using ZIP-302 via IPFS. It's been intentionally designed to be extensible in order to allow for additional storage, cryptography, or other more comprehensive protocol behavior while still keeping it simple and interoperable to start.
I still need to do another editing pass through it before I'd think it's read to be merged, but I wanted to get this published out there so we can start getting early review from the community on it.