xdrill functions for ledger entry changes #3

chowbao · 2025-01-29T23:05:04Z

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

stellar#5552

xdrill functions for ingest.Changes

Why

Create low level helper functions for ledger entry changes parsing

Known limitations

Refactor of the processor/transforms to be done in separate ticket/pr

chowbao · 2025-01-29T23:06:41Z

ingest/change.go

+	return ledgerKey.MarshalBinaryBase64()
+}
+
+func (c Change) EntryDetails(passphrase string) (map[string]interface{}, error) {


I restructured the ledger entry changes helper function to behave like the LedgerOperations helper function where the output from a given entry type will populate a details[] map.

How does this look? Are there any other preferred alternatives?

I'll write tests when we agree on a format

It could be an opportunity to have these be strictly typed instead of an opaque interface{} to interpret. Maybe not worth it? It'd be a looooot of work, so probably not if people are used to consuming/seeing this variant.

Yeah I would like to make it strictly typed but it definitely isn't worth it at this point. Could be a good new eng task though 🤔

Hmm actually how would strict typing work in this case?

I'm assuming I would need to create a struct for each entryDetail or would it be like a giant struct for all entryDetails?

Updated in 691bfd8

So instead of the generic interface{} each ledgerentry will now return a relevant struct. This seemed like the best way to strictly type the results but I'm open to other options if there are any

Shaptic

just drivin' by 🏎️

ingest/change.go

Shaptic · 2025-01-29T23:22:31Z

ingest/change.go

+	default:
+		return xdr.LedgerEntry{}, changeType, false, fmt.Errorf("unable to extract ledger entry type from change")


There's also LedgerEntryChangeTypeLedgerEntryState which should probably be handled here instead of bailing out, no? Then no error case at all.

Oh that's good to know. I think stellar-etl code has been skipping LedgerEntryChangeTypeLedgerEntryState on purpose (or accidentally 😨)

What is the bool for?

The ok bools are usually used to signify if the returned value is actually valid or not. And it being not valid is not an error.

For example think of soroban fees. If you want to get soroban fees for a classic transaction you'd return 0 for the fee value but also false for the bool because there is no real value for the fee

I see. Ideally it should be null but also go weirdly handles null values. So this is the way of handling it. 👍

case xdr.LedgerEntryChangeTypeLedgerEntryCreated, xdr.LedgerEntryChangeTypeLedgerEntryUpdated:
return *c.Post, changeType, false, nil

What's the reason of setting false bool in this case? Isn't it returning valid value?

Oh sorry in this case the bool is to signify if the entry was deleted or not

Shaptic · 2025-01-29T23:23:33Z

ingest/change.go

+	return ledgerKey.MarshalBinaryBase64()
+}
+
+func (c Change) EntryDetails(passphrase string) (map[string]interface{}, error) {


It could be an opportunity to have these be strictly typed instead of an opaque interface{} to interpret. Maybe not worth it? It'd be a looooot of work, so probably not if people are used to consuming/seeing this variant.

Shaptic · 2025-01-29T23:24:13Z

ingest/change.go

+		if err != nil {
+			return details, err
+		}


You can skip all of these checks by just returning details, err at the end since err will be set accordingly

Yeah makes sense. Is that good coding practice though? I thought it's generally good to return/err when it happens rather than at the end of a function. BUT these are also really small functions anyways so it's probably not a big deal

Depends who you ask 😆 imo omitting it can show "all this function does is switch/case + parse + return"

I also hate Go's pedantry for error verbosity so I like to shorten it when I can

Yeah just golang things 😢

Shaptic · 2025-01-29T23:27:21Z

ingest/change.go

+	Sponsor string
+}
+
+func AccountDetails(accountEntry *xdr.AccountEntry) (map[string]interface{}, error) {


These parsers feel like they belong in a separate file

Yeah I thought about doing that. Example for account

In the example I split it out into separate files (also gave each data element it's own function). It does seem to clutter the directory though. Maybe that's not a big deal?

Also I wonder is it worth doing the same split for each operation as well? 🤔 Both are pretty large files

If it's too cluttery you could have a parsers/ or equivalent subdirectory. At the very least I'd dump everything below this line into a parsedetails.go (or similar) file 🤷

Yeah I'll split it into separate files. Nicer on the eyes

Updated in 691bfd8

Shaptic · 2025-01-29T23:34:18Z

xdr/claimable_balance_id.go

+// MarshalBinaryBase64 marshals XDR into a binary form and then encodes it
+// using base64.
+func (c ClaimableBalanceId) MarshalBinaryBase64() (string, error) {
+	b, err := c.MarshalBinary()


This seems redundant given you can do:

var id xdr.ClaimableBalanceId str, err := xdr.MarshalBase64(&id)

At the very least this method should use that instead of repeating a xdr -> bytes -> base64 code path.

Oh I didn't realize there was an xdr.MarshalBase64() function. I split it out into xdr.* specific marshal base64 because that's what I saw existing for some other xdr structs (probably old and was just never refactored). I'll update

Yeah those are outdated I bet. They're convenient because of intellisense suggesting it on the object itself, but even then, they'd belong in xdrgen rather than handwritten.

Updated in 08a42b7

chowbao · 2025-01-30T16:55:17Z

just drivin' by 🏎️

Why does this make me think of Fast and Furious lol

Co-authored-by: George <[email protected]>

ingest/change.go

sydneynotthecity

This PR contains two of the material differences in schema between Horizon and Hubble:

Offers containing a buying and selling asset (vs base and counter),
Liquidity pools flattening the Reserves map into AssetA and AssetB

It may be worthwhile to ask some opinions of the platform team during team meeting to make sure there is consensus on this schema choice.

sydneynotthecity · 2025-02-04T04:16:07Z

ingest/change.go

+		if err != nil {
+			return details, err
+		}
+	default:


nit: missing ConfigSetting ledger entry type, but dunno if you're intentionally leaving off

I did leave that off intentionally. But you're right I should just add it and make it do nothing instead of leaving it completely out

sydneynotthecity · 2025-02-04T04:23:13Z

ingest/ledgerentry/claimable_balance.go

+	AssetCode   string     `json:"asset_code"`
+	AssetIssuer string     `json:"asset_issuer"`
+	AssetType   string     `json:"asset_type"`
+	AssetID     int64      `json:"asset_id"`


I'm not seeing where you parse AssetId in ClaimableBalanceDetails. Can we just drop?

Yup this is a mistake. I will drop AssetID

sydneynotthecity · 2025-02-04T04:26:23Z

ingest/ledgerentry/contract_data.go

+	AssetIssuer     string      `json:"asset_issuer"`
+	AssetType       string      `json:"asset_type"`
+	BalanceHolder   string      `json:"balance_holder"`
+	Balance         string      `json:"balance"`


dumb question: why is this balance a string and not any other balances? Are we not worried about int overflow for the other amounts?

Yeah this is a string because the Balance is a big.Int (not sure why just this balance is big.Int though might just be because how we defined smart contracts)

ah, ok. TIL that only soroban contract balance is a big.Int

sydneynotthecity · 2025-02-04T04:28:08Z

ingest/ledgerentry/data.go

perhaps one day we'll materialize this in hubble

sydneynotthecity · 2025-02-04T04:30:02Z

ingest/ledgerentry/liquidity_pool.go

+	AssetAType      string `json:"asset_a_type"`
+	AssetACode      string `json:"asset_a_code"`
+	AssetAIssuer    string `json:"asset_a_issuer"`
+	AssetAReserve   int64  `json:"asset_a_amount"`
+	AssetBType      string `json:"asset_b_type"`
+	AssetBCode      string `json:"asset_b_code"`
+	AssetBIssuer    string `json:"asset_b_issuer"`
+	AssetBReserve   int64  `json:"asset_b_amount"`


I'm curious if @Shaptic or others have thoughts on this proposed schema as it's a departure on how Horizon parses this ledger entry. Like? Dislike?

Seems fine, but amounts need to be strings and I feel like the structure could be a little less flat - both the JSON and the struct are a little gross to read. It'd be nice if assets had a generic structure we re-use everywhere, e.g. { type, code, issuer }

amounts need to be strings

Ah cause they are all big ints (or whatever the larger int is called)?

both the JSON and the struct are a little gross to read.

Oh from the OLAP realm I actually prefer all flattened lol. I can see JSOn being more flexible for other use cases though. We can discuss with the team if they have opinions

No because JS sucks and can't represent 64 bit ints as, well, ints.

lmao yes: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER, sry only 53 bits for u

Oh from the OLAP realm I actually prefer all flattened

+1 we would still unpack in BQ just because flattened data structure is nicer for normies. But i do think @Shaptic is right that others prefer a consistent non-flattened object for asset details

sydneynotthecity · 2025-02-04T04:33:27Z

ingest/ledgerentry/ttl.go

+import "github.com/stellar/go/xdr"
+
+type Ttl struct {
+	LiveUntilLedgerSeq uint32


missing the key hash, no?

It was intentionally left off (same with contract_data and contract_code). This is because the ledger_key_hash actually comes from the Change instead of the LedgerEntry and hence you can get LedgerKeyHash for anything (not only smart contract related entries) with LedgerKeyHash()

BUT if this seems clunky I don't mind adding the LedgerKeyHash() to these structs

Leave it as it is, the struct just seemed weirdly empty

xdrill functions for ledger entry changes

c8c4caf

chowbao commented Jan 29, 2025

View reviewed changes

Shaptic reviewed Jan 29, 2025

View reviewed changes

Update ingest/change.go

17a9868

Co-authored-by: George <[email protected]>

amishas157 reviewed Jan 30, 2025

View reviewed changes

ingest/change.go Show resolved Hide resolved

amishas157 reviewed Jan 30, 2025

View reviewed changes

ingest/change.go Show resolved Hide resolved

amishas157 reviewed Jan 30, 2025

View reviewed changes

ingest/change.go Show resolved Hide resolved

chowbao added 2 commits February 3, 2025 12:23

reformat changes

691bfd8

use generic xdr marshalbase64

08a42b7

chowbao mentioned this pull request Feb 3, 2025

xdrill for operations #2

Open

7 tasks

sydneynotthecity reviewed Feb 4, 2025

View reviewed changes

		default:
		return xdr.LedgerEntry{}, changeType, false, fmt.Errorf("unable to extract ledger entry type from change")

xdrill functions for ledger entry changes #3

Are you sure you want to change the base?

xdrill functions for ledger entry changes #3

Conversation

chowbao commented Jan 29, 2025

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

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

Choose a reason for hiding this comment

Shaptic left a comment

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

Choose a reason for hiding this comment

amishas157 Jan 31, 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

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

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

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

chowbao commented Jan 30, 2025

sydneynotthecity left a comment

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amishas157 Jan 31, 2025 •

edited

Loading