Sample protos build #5582

karthikiyer56 · 2025-01-29T00:05:09Z

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
This PR's title starts with name of package that is most changed in the PR, ex.
services/friendbot, or all or doc if the changes are broad or impact many
packages.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.
I've updated any docs (developer docs, .md
files, etc... affected by this change). Take a look in the docs folder for a given service,
like this one.

Release planning

I've reviewed the changes in this PR and if I consider them worthwhile for being mentioned on release notes then I have updated the relevant CHANGELOG.md within the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.
I've decided if this PR requires a new major/minor version according to
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.

What

[TODO: Short statement about what is changing.]

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

… of the main proto file

leighmcculloch

Couple thoughts inline.

leighmcculloch · 2025-02-04T05:34:01Z

protos/address/address_helpers.go

+// String returns a human-readable representation of the Address.
+func (a *Address) Strings() string {


Typo? Comment says String, function says Strings. Typically the interface to implement is "String".

Suggested change

// String returns a human-readable representation of the Address.

func (a *Address) Strings() string {

func (a *Address) String() string {

this was just to show the presence of helper/receiver functions for the generated golang code
I cant have a function called String() on that data type, since protoc genertes a String() function for you, so I lazily, for demonstration purposes, called it Strings()

leighmcculloch · 2025-02-04T05:35:33Z

protos/address/address.proto

+message Address {
+  oneof address_type {
+    string smartContractAddress = 1; // Smart Contract address
+    string accountAddress = 2;       // Account address
+    string liquidityPoolHash = 3;    // Liquidity Pool hash
+    string claimableBalanceId = 4;   // Claimable Balance ID


Could we make these a strkey instead? Or is there a reason to go the structured type? (Thoughts @chowbao?)

Liquidity Pools and Claimable Balances are likely to get strkeys as part of the work on CAP-67 and be rendered as strkeys in the XDR-JSON, so you could use the same format here.

We can move along strkey definitions for them to unblock any work that might be dependent on them.

Hmm is there a reasonable way to calculate the strkey for lp and claimable balances? I would say if it's added to the strkey lib then yes I would agree with using them.

One thing to note is that would be net new and would need existing systems to know to use the strkey lp instead of hash going forward (not blocking but would "break" things like stellar-etl)

I am not even sure what you guys mean when you say strkey.

The reason the Address is structured as a oneof is to align with the enum SCAddressType - the one that will be extended to add Liquiditypools and CBs as a part of CAP-67

https://github.com/stellar/go/tree/master/strkey

It's basically the way to get the G*/C* address
https://github.com/stellar/go/blob/master/strkey/main.go#L20-L44

Leigh is saying they are gonna give liquidityPools and claimableBalances these strkeys (unless I'm also incorrect 😅)

Edit: "way to get" isn't the correct term. It's just the definition of the G/C/T etc...

so, basically, you are saying that the Address can just be a proto string then, for this token_transfer_event.proto`?

Strkeys is the format of Stellar Addresses and other Stellar entities, they're defined in SEP-23:

https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0023.md

leighmcculloch · 2025-02-04T05:37:03Z

protos/address/address.proto

+// Address message with oneof for different address types
+message Address {
+  oneof address_type {
+    string smartContractAddress = 1; // Smart Contract address


Suggestion, the longer form is unnecessary. Elsewhere in code we simply refer to them as contract addresses:

Suggested change

string smartContractAddress = 1; // Smart Contract address

string contractAddress = 1; // Contract address

Noted.
I want to point out that I want this PR to be used to decide on structure of the code, more than what goes inside the eventual proto data model will look like
That will be worked upon in a separte PR. review comments from here will be addressed there.

leighmcculloch · 2025-02-04T05:39:09Z

protos/address/address.pb.go

It's undesirable to group all types into a functional package like "proto" rather than a domain focused package which will be the location where they'll be used.

For example, if as a dev using the ingest SDK I'm browsing the ingest package docs, and am looking for related and interesting types, I won't see any there because they're all defined outside the ingest package in the 'protos' package. When I look at the protos package I'll see an assortment of types that relate to the ingest SDK, and potentially others that don't assuming it becomes a general place to place any protos for anything in the monorepo.

Is it possible to generate the protos within each application or package where they'll be used? So that the proto types for a specific processor are grouped with that processor?

I am not entirely sure I understand the concern here, but Ima try to provide my reasoning

Its not uncommon for all the proto defintions for a particular project to be nested under one directory called protos.
This is especially needed when protos need to import another protos that are defined in a separate directory structure.

Re:

When I look at the protos package I'll see an assortment of types that relate to the ingest SDK, and potentially others that don't assuming it becomes a general place to place any protos for anything in the monorepo....

The protos are not exported as a one big package per se. As a matter of fact there is no package protos at all.
The option go_package field that I specify in the .proto files already separates the resulting golang code logically, if that is what your concern was.
the proto definitions are just co-housed together, is all
The packages generated (address/asset/token_transfer), in a sense, do follow the go idomatic way

I could very well move this entire protos inside the ingest/ directory
That way it is all under ingest, if thats what you were suggesting?

oh wait, I think I mistook what you were asking @leighmcculloch
did you mean that we still have the top level protos directory have the .proto files, as is the case in this pr, , but generate the golang bindings in a place that is closer to the token_transfer_processor implementation?

The option go_package field that I specify in the .proto files already separates the resulting golang code logically, if that is what your concern was.

Yes this was my concern because I saw .pb.go files ending up in a protos/ directory. Ideally the go types live with their respective SDKs / directories.

Ideally .proto files would live there too, but I understand that's not a desirable tradeoff.

@leighmcculloch - I made it so that the data model for tokenTransferEvent is generated under where the code will potentially reside.

And I am hoping that all the new code for the upcoming proccessors is under /ingest/processors

Now, assuming that the new place for these processors in under ingest/processors/<processor_name>/,
the as long as the corresponding .proto files follow the same directory structure under /protos

i.e

/protos/address/address.proto --> /ingest/address/address.pb.go (Additionally, added a helper file with receiver functions in /ingest/address)

/protos/asset/asset.proto --> /ingest/asset/asset.pb.go (Additionally, added a helper file with receiver functions in /ingest/asset/)

The main one
/protos/processors/token_transfer/token_transfer_event.proto --> /ingest/processors/token_transfer/token_transfer_event.pb.go

There are a few important points to note here:

We should actually not be using the option go_path in the .proto files themselves. I dont know why I thought it was a good idea when i used it the first time around, or even when I wrote this comment. I have since revised my stance

Instead, I got creative with the Makefile target, and achieved the same sentiment using -go_opts and the -M flag. Checkout the commit message, where I explain why I did that
cc @tamirms

…d re-order code The real ingenuity is in the Makefile target - "generate-proto" Specifying the option go_package in the .proto files would be a bad idea, especially if and when we plan to pull the protos into their own repo If someone wants to generate their own go-bindings from the proto files, assuming the proto files are in their own repo, then they wont have been able to do it properly. They would have to contend with the fact that the option go_package points to "github.com/stellar/go/ingest/...." and they would not be able to rename the pakcage/location for the generated go code. So, it is best to be clever with your Makefile target to achieve the same thing, with the use of the -go_opt and -M flags, instead of using option go_package

karthikiyer56 · 2025-02-05T03:20:04Z

protos/processors/token_transfer/token_transfer_event.proto

@@ -1,7 +1,7 @@
 syntax = "proto3";

 package token_transfer;
-option go_package = "github.com/stellar/go/protos/processors/token_transfer;token_transfer";
+//option go_package = "github.com/stellar/go/protos/processors/token_transfer;token_transfer";


This is a big no-no
I have left it commented out, instead of deleting, for the sake of clarity, in this PoC PR

karthikiyer56 · 2025-02-05T03:32:52Z

Makefile

+    	echo "  $(PROTO_FILES)"; \
+    	protoc -I=$(PROTO_DIR) \
+    	       --go_out=ingest --go_opt=paths=source_relative \
+    	       --go_opt=$$MAP_OPTS \


Use -go_opt with the M flag to indicate what the package location/path inside the generated proto should look like

…usion

Sample protos build

716847e

karthikiyer56 mentioned this pull request Jan 31, 2025

TokenTransferProcessor and events - Data modeling compliant with CAP-67 unified events #5580

Closed

karthikiyer56 added 2 commits February 3, 2025 11:42

extract ledger from xdr string

8a16299

Structure the code to break out asset and Address definitions outside…

73ed301

… of the main proto file

leighmcculloch reviewed Feb 4, 2025

View reviewed changes

karthikiyer56 added 3 commits February 4, 2025 09:26

remove unnecessary code

13e069d

delete all generated proto code

000482d

karthikiyer56 commented Feb 5, 2025

View reviewed changes

karthikiyer56 added 2 commits February 4, 2025 19:45

Makefile cleanup

5cec37d

Merge remote-tracking branch 'origin/master' into karthik/protos-incl…

37a8a7f

…usion

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sample protos build #5582

Sample protos build #5582

karthikiyer56 commented Jan 29, 2025

leighmcculloch left a comment

leighmcculloch Feb 4, 2025

karthikiyer56 Feb 4, 2025 •

edited

Loading

leighmcculloch Feb 4, 2025 •

edited

Loading

chowbao Feb 4, 2025

karthikiyer56 Feb 4, 2025

chowbao Feb 4, 2025 •

edited

Loading

karthikiyer56 Feb 4, 2025 •

edited

Loading

leighmcculloch Feb 4, 2025 •

edited

Loading

leighmcculloch Feb 4, 2025

karthikiyer56 Feb 4, 2025

leighmcculloch Feb 4, 2025

karthikiyer56 Feb 4, 2025 •

edited

Loading

karthikiyer56 Feb 4, 2025

karthikiyer56 Feb 4, 2025 •

edited

Loading

leighmcculloch Feb 4, 2025

karthikiyer56 Feb 5, 2025 •

edited

Loading

karthikiyer56 Feb 5, 2025

karthikiyer56 Feb 5, 2025

		// String returns a human-readable representation of the Address.
		func (a *Address) Strings() string {

	// String returns a human-readable representation of the Address.
	func (a *Address) Strings() string {
	func (a *Address) String() string {

	string smartContractAddress = 1; // Smart Contract address
	string contractAddress = 1; // Contract address

Sample protos build #5582

Are you sure you want to change the base?

Sample protos build #5582

Conversation

karthikiyer56 commented Jan 29, 2025

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

leighmcculloch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikiyer56 Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

leighmcculloch Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chowbao Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

karthikiyer56 Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

leighmcculloch Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikiyer56 Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikiyer56 Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikiyer56 Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikiyer56 Feb 4, 2025 •

edited

Loading

leighmcculloch Feb 4, 2025 •

edited

Loading

chowbao Feb 4, 2025 •

edited

Loading

karthikiyer56 Feb 4, 2025 •

edited

Loading

leighmcculloch Feb 4, 2025 •

edited

Loading

karthikiyer56 Feb 4, 2025 •

edited

Loading

karthikiyer56 Feb 4, 2025 •

edited

Loading

karthikiyer56 Feb 5, 2025 •

edited

Loading