search

hyperledger-archives / sawtooth-sdk-go

https://wiki.hyperledger.org/display/sawtooth
Apache License 2.0
28 stars 40 forks source link

Refactor protos #41

Closed paulwizviz closed 4 years ago

paulwizviz commented 4 years ago
arsulegai commented 4 years ago

Please also update, how to build this for local test.

paulwizviz commented 4 years ago

We need a bigger conversation about the architecture of the code.

Firstly, in terms of protos, please review if it is ok to go with the direction I have presented so far.

Secondly, do we want to remove the c/c++ dependencies related to PEM file processing?

Thirdly, do we want to work towards adopting this layout https://github.com/golang-standards/project-layout.

vaporos commented 4 years ago

If we adopt the layout, generated code should go into /internal.

The c/ code was added as a security concern from Intel, basically avoiding the use of strncpy. See commit id 496adad1ecc22be28bffeaf0c64c2022d04abd09. I think there might be a similar commit in the C++ SDK repo that might have more explanation as well.

With respect to the proto files, instead of using 'protobuf' in the package name, we should use 'protos' similar to transact's rust sdk, with 'protocol' being the wrapper/builder code.

Is there a reason you are removing 'go_package' in favor of 'package' on some of the protos?

vaporos commented 4 years ago

We should move Batch/Transaction code to Transact's Go SDK. That hasn't been initialized yet, so there is maybe an opportunity to use that subset of functionality to perfect the repo organization/approach in that context. Similar work is going on to initialize Transact's Javascript SDK.

paulwizviz commented 4 years ago

@vaporos

Is there a reason you are removing 'go_package' in favor of 'package' on some of the protos?

Prior to Go1.11, you need to specify a GOPATH. The older version, as far as I can tell from experimentation if you specify gp_package, the protobuf generated code will append the GOPATH plus your package name. For example, if you specify package "/ it will automatically generate import github.com/hyperledger/sawtooth/<project to your top level>/<you package>. Which is problematic when you have protos importing from other protos.

With Go1.11 onward, there is no GOPATH so protobuf could not generate the properly import path. At least as far as I can tell from experimentation. I could be wrong but I have not being about to ficyre out what is available in protogen to do so.

But I am happy to change if there are other ways to deal with packaging.

arsulegai commented 4 years ago

@vaporos As @paulwizviz commented, there's currently an issue with the Go modules and the Go SDK. Possible options are either to go with the older version v0.1.2 or update v0.1.3 to make it work with modules and release as v0.1.4 . The other approach would make it look like workaround.

arsulegai commented 4 years ago

@paulwizviz would it be possible to take this PR forward? Before going to Transact SDK, this can be fixed if possible.

paulwizviz commented 4 years ago

@paulwizviz would it be possible to take this PR forward? Before going to Transact SDK, this can be fixed if possible.

@arsulegai Feel free to move forward. This is only intended to facilitate discussion.

paulwizviz commented 4 years ago

Folks, unfortunately, I am not able to push this forward. If there is no objection, I shall close this PR.

paulwizviz commented 4 years ago

No objection to closing.