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

Use cosmos plugin to generate proof proto files for compatibility with SDK #33

Merged
merged 3 commits into from
Aug 4, 2020
Merged

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Jul 30, 2020

While working on client-migration I found out the the proofs.pb.go is not compatible with the plugin used on the SDK, this can be replicated by checking out the lastest commit of this pr and trying to build. The error will be:

# github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types
x/ibc/07-tendermint/types/tendermint.pb.go:512:37: m.ProofSpecs[iNdEx].MarshalToSizedBuffer undefined (type *ics23.ProofSpec has no field or method MarshalToSizedBuffer)
x/ibc/07-tendermint/types/tendermint.pb.go:696:37: m.ProofSpecs[iNdEx].MarshalToSizedBuffer undefined (type *ics23.ProofSpec has no field or method MarshalToSizedBuffer)

**UNRELATED ERRORS**
x/ibc/07-tendermint/types/test_utils.go:52:11: undefined: header 
x/ibc/07-tendermint/types/test_utils.go:58:3: cannot use valSet (type *"github.com/tendermint/tendermint/types".ValidatorSet) as type *"github.com/tendermint/tendermint/proto/tendermint/types".ValidatorSet in field value 

this occurs because the plugin used by the SDK to generate the ClientState which contains the ics23.ProofSpecs uses functions not defined by the generated proto files that create these proof specs. There are two possible fixes for this:

  • copy the ProofSpecs scheme into the SDK and generate the proto files with the SDK's setup
  • change the setup for the proto files are generated

This pr goes with the second approach since the golang ProofSpecs were implemented for SDK usage

I have updated the Makefile to generate the proofs file in the same way the SDK does. I tested that this works locally and this can be seen looking at the build of my branch with this fix, The above error is no longer occurs (the build is broken for many other unrelated reasons).

I based the Makefile changes off how the SDK does this, here is the reason for the install section and here is for the protoc changes

cc/ @ethanfrey @fedekunze

@colin-axner
Copy link
Contributor Author

colin-axner commented Jul 30, 2020

@aaronc could you please review these changes to ensure they are correct 🙏

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No changes to the .proto file?

Please check if this works for you or if you need more changes. The go codec is tuned for the cosmos sdk. I just want to ensure the other languages still work with the proto file, which this did not modify

protoc --gogofaster_out=. $(PROTOC_FLAGS) ../proofs.proto
protoc --gocosmos_out=plugins=interfacetype+grpc,Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types:. $(PROTOC_FLAGS) ../proofs.proto

install-proto-dep:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to add that

@colin-axner
Copy link
Contributor Author

No changes to the .proto file?

yea so this issue #32 can't be addressed without some package name changes. Protocol Buffers specifies that:

The go_package option defines the import path of the package which will contain all the generated code for this file. The Go package name will be the last path component of the import path.

In the SDK we need to use ProofSpecs as a field in a proto generated type. We pull the proto files from here and put them in our third_party directory, and point our import in our proto file to that path (third_party/proto/confio). When we generate the proto files for that type, if the proofs.proto does not have option go_package = "github.com/confio/ics23/go" the go src for the package is assumed to be go/src/confio which does not exist.

If I add that same option to the proofs file here directly and generate the files, the build won't work because the package name will be go instead of ics23 like the rest of the files.

Basically to solve this either

  • third parties insert in the go_package option when importing the proofs.proto file
  • the go dir is renamed to something other than go so all the files can use the same package name as the directory name. I guess I could also just embed another directory one layer down if we wanted to maintain the language distinction for directory names.

Either solution is fine with me, but I figured I'd start by addressing the issue that would be blocking our development (the proofs.pb.go file needs to be generated using the same plugin as the SDK).

@colin-axner
Copy link
Contributor Author

colin-axner commented Aug 3, 2020

The current changes work and are all we really need at the moment. What I mentioned above could be trivially addressed later

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from what I can see although the gogo proto usage is only necessary if you are using gRPC or certain extensions (just castrepeated I believe). It doesn't hurt to use it though.

@colin-axner
Copy link
Contributor Author

proof of build

@ethanfrey
Copy link
Contributor

@colin-axner I'm trying to figure out the import comment, and do understand this is a major pain for you.

Basically to solve this either

  • third parties insert in the go_package option when importing the proofs.proto file
  • the go dir is renamed to something other than go so all the files can use the same package name as the directory name. I guess I could also just embed another directory one layer down if we wanted to maintain the language distinction for directory names.

It is a bit confusing trying to keep 3 languages in one repo, but I see it as the only way to make sure they all stay in sync and maintained. I do not want to have one folder with 3 different languages mixed. If needed for the go imports, I guess we could generate the *.pb.go file in the same top-level dir as *.proto and then just have all the logic in the go directory. Would that resolve the import issue?

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, hope this helps your integration

@ethanfrey ethanfrey merged commit a9fbc74 into cosmos:master Aug 4, 2020
@colin-axner
Copy link
Contributor Author

I guess we could generate the *.pb.go file in the same top-level dir as *.proto and then just have all the logic in the go directory. Would that resolve the import issue?

I don't think so. I think the *.pb.go files need to be in the same directory (with the same package name) as the .go logic files. I think renaming the the dir and package names to golang would work.

For now on the SDK I've just added a sed command to insert in the go_package option. I think this is an ok solution for now and if it causes trouble we can revisit this issue

@ethanfrey
Copy link
Contributor

Ah, golang is fine but go is a reserved word. Now I get it. Happy to change that in the next release.

I just released v0.6.2 with this PR merged, please try it out.

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.

4 participants