-
Notifications
You must be signed in to change notification settings - Fork 7
Split up xmtp_envelope.proto #13
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
Conversation
|
Given that this is a breaking change for importers of the library, I think we should include a semver commit comment that notes that and causes a major release. We got bit by this a couple weeks ago. Side note: I previously pinned |
|
|
||
| // The PublicKeyBundle of the user on the network. | ||
| // Used to derive shared secrets for encrypted messaging | ||
| message ContactBundleV1 { |
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.
Do we actually want to break ContactBundle into it's own file too while we are at it?
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'm game, do you mean just the ContactBundle or the PublicKeyBundle as well? If the latter then we should probably break out PrivateKeyBundle as well for consistency. I think I'd slightly prefer to keep it to just the ContactBundle.
|
|
||
| //go:generate go install google.golang.org/grpc/cmd/protoc-gen-go-grpc | ||
| //go:generate protoc --proto_path=../proto -I=. -I=../build/tmp/vendor --go_out=paths=source_relative:. --go-grpc_out=paths=source_relative:. --grpc-gateway_out=generate_unbound_methods=true,paths=source_relative:. ../proto/message_api/v1/message_api.proto ../proto/message_api/v1/authn.proto ../proto/message_contents/xmtp_envelope.proto | ||
| //go:generate -command compile protoc --proto_path=../proto -I=. -I=../build/tmp/vendor --go_out=paths=source_relative:. --go-grpc_out=paths=source_relative:. --grpc-gateway_out=generate_unbound_methods=true,paths=source_relative:. |
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.
Trying to split up this unmaintainable line (apparently we were already missing private key and composite here). This line defines an "alias" called "compile" that translates to the rest of this line (i.e. protoc with all the parameters). Then we can use the alias to compile the proto files individually, making the list of files clearer.
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.
We could also have it just run a script that is multi-line
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.
In that case it would probably make more sense to skip go:generate altogether. I'll leave as is, we can decide to switch in separate PR, for now I just wanted to make this more maintainable.
|
|
||
| // Signature represents a generalized public key signature, | ||
| // defined as a union to support cryptographic algorithm agility. | ||
| message Signature { |
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.
In the future this signature may be used outside of the publicKey message, to sign other objects. I don't think its really worth splitting it out, to figured I would point it out
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.
Hm, I think this might be worth splitting out as well, especially as the number of signature and key types grows. We should already have a second Signature type for wallet signatures. So unless there are objections, I'll split it out too.
BREAKING CHANGE: The signature, public key and contact bundle TS types moved from `xmtpEnvelope` namespace to `signature`, `publicKey` and `contact` namespaces respectively.
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Given that the public key stuff plays a role on its own in the contact topics and that the xmtp_envelope file is getting long and convoluted, I thought it might be a good idea to break it up in two.
This might require some updates in xmtp-js as some of the types will move into
publicKey.*in theindex.js. Other than that it should be pretty harmless. I ran the code generation steps for both go and ts, which seemed to have succeeded.