Skip to content

Conversation

@mkobetic
Copy link
Contributor

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 the index.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.

@mkobetic mkobetic requested a review from a team August 24, 2022 18:08
@neekolas
Copy link
Collaborator

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 @xmtp/proto to the exact version in xmtp-js, but somehow that got reverted (probably by me).


// The PublicKeyBundle of the user on the network.
// Used to derive shared secrets for encrypted messaging
message ContactBundleV1 {
Copy link
Collaborator

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?

Copy link
Contributor Author

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:.
Copy link
Contributor Author

@mkobetic mkobetic Aug 25, 2022

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

@mkobetic mkobetic requested a review from neekolas August 25, 2022 15:59

// Signature represents a generalized public key signature,
// defined as a union to support cryptographic algorithm agility.
message Signature {
Copy link

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

Copy link
Contributor Author

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.
@mkobetic mkobetic changed the title Split public_key out of xmtp_envelope Split up xmtp_envelope.proto Aug 25, 2022
@mkobetic mkobetic merged commit b5cd249 into main Aug 25, 2022
@mkobetic mkobetic deleted the refactor branch August 25, 2022 19:42
@github-actions
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants