rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Add go package names to GRPC protobuf definitions

Open donovanhide opened this issue 11 months ago • 3 comments

High Level Overview of Change

Add Go package names to GRPC protobuf definitions, as per:

https://protobuf.dev/reference/go/faq/#fix-namespace-conflict

Context of Change

Make the GRPC API available to Go developers without having to manually alter protobuf definition files.

Type of Change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Refactor (non-breaking change that only restructures code)
  • [ ] Performance (increase or change in throughput and/or latency)
  • [ ] Tests (you added tests for code that already exists, or your new feature included in this PR)
  • [ ] Documentation update
  • [ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • [ ] Release

API Impact

  • [ x] Public API: New feature (new methods and/or new fields)
  • [ ] Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • [ ] libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • [ ] Peer protocol change (must be backward compatible or bump the peer protocol version)

donovanhide avatar Feb 11 '25 07:02 donovanhide

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.2%. Comparing base (fa5a854) to head (cb67de8). Report is 140 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5281   +/-   ##
=======================================
  Coverage     78.2%   78.2%           
=======================================
  Files          790     790           
  Lines        67638   67638           
  Branches      8164    8161    -3     
=======================================
+ Hits         52860   52869    +9     
+ Misses       14778   14769    -9     

see 2 files with indirect coverage changes

Impacted file tree graph

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 12 '25 22:02 codecov[bot]

@donovanhide would you be able to sign your commit?

  • See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits for the official documentation. Although if you use something like 1Password, you can generate an SSH key there and configured that one to sign the commits.
  • To amend an earlier unsigned commit and sign it, this set of instructions is useful: https://dev.to/za-h-ra/git-fixing-unsigned-gpg-commits-phn.

bthomee avatar Feb 12 '25 22:02 bthomee

Signed!

donovanhide avatar Feb 13 '25 07:02 donovanhide

@donovanhide I took a closer look and my initial assessment is still valid.

To make this work, I believe at the minimum you need to:

  • Add the necessary statements for the proto files to be compiled into .pb.go files.
  • Add the generated files into the include/proto/org/xrpl/rpc/v1 directory (or elsewhere, but that path seems reasonable).
  • Add a go.mod to the include/proto/org/xrpl/rpc/v1 directory that has as module xrpl.org/rpc/v1 and then specifies a Go version. Although 1.24.4 is the latest, I'd just specify 1.22 to give some flexibility for people who haven't updated their Go version in over a year.

To top it off:

  • In the precommit hook add the commands to compile the proto files into .pb.go, and then use something like DIFF=$(git status --porcelain); if [ -n "${DIFF}" ]; then echo "${DIFF}"; echo 'Please compile the protos and commit the changed files.'; exit 1; fi to ensure when someone modifies the protos that the generated protos are included in the codebase.
  • Add an additional CI pipeline in .github/workflows to run alongside clang-format that does the same.

Do you think you'll be able to make these changes?

bthomee avatar Jun 24 '25 17:06 bthomee