xdrill for operations #2

chowbao · 2025-01-16T21:11:40Z

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

github issue

xdrill functions for LedgerOperations

Why

Create low level helper functions for operations parsing

Known limitations

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

chowbao · 2025-01-17T23:43:47Z

Similar problems in stellar#5568 and #1 exist in this PR as well. TODO: fix it

sydneynotthecity

High level, there are some quirks in stellar-etl. Now is the time for us to discuss whether it makes sense to incorporate back in to XDRill or not. Particularly:

int vs real representation for asset amounts - should this stay in raw integer form? Should we convert to real? Offer both?
Asset id - this is an optimization specific to Hubble. Does it make sense to create a more universal asset hash? If so, does FarmHash make the most sense?
buy vs sell side of offers. In Horizon, these are presented as base and counter assets, where base asset hash < counter asset hash so that the trade pairs are consistently on the same side. This may be an opportunity for us to reimagine how we want this schema to be defined

sydneynotthecity · 2025-01-22T22:06:00Z

ingest/ledger_operation.go

+	if ok {
+		operationResultTr, ok := operationResults[o.OperationIndex].GetTr()
+		if ok {
+			operationTraceCode, err := operationResultTr.MapOperationResultTr()


nit: = instead of :=

updated in c68cd9a

sydneynotthecity · 2025-01-22T22:22:29Z

ingest/ledger_operation.go

+	if err := o.addAccountAndMuxedAccountDetails(details, op.Destination, "to"); err != nil {
+		return details, err
+	}
+	details["amount"] = o.ConvertStroopValueToReal(op.Amount)


do we want to bring over this logic from stellar-etl? I think both RPC and horizon keep the raw stroop values to avoid floating point errors. wdyt of keeping amounts in stroops?

Hmm that's fair. Less railroading people into floating point if they don't want ot

updated in c68cd9a

sydneynotthecity · 2025-01-23T03:29:00Z

ingest/ledger_operation.go

+		if ledgerKey.TrustLine.Asset.Type == xdr.AssetTypeAssetTypePoolShare {
+			result["trustline_liquidity_pool_id"] = o.PoolIDToString(*ledgerKey.TrustLine.Asset.LiquidityPoolId)
+		} else {
+			result["trustline_asset"] = ledgerKey.TrustLine.Asset.ToAsset().StringCanonical()


nit: Should we be parsing asset consistent via addAssetDetails...?

Let me take a look at this again. Might have just been stellar-etl never updated this or there was an actual reason for this

Yeah so this is a type TrustLineAsset

type TrustLineAsset struct { Type AssetType AlphaNum4 *AlphaNum4 AlphaNum12 *AlphaNum12 LiquidityPoolId *PoolId }

Note the extra LiquidityPoolId *PoolId

Whereas addAssetDetails only handles normal Asset types. I'll try to consolidate as I think they effectively want the same "asset" info

sydneynotthecity · 2025-01-23T03:29:28Z

ingest/ledger_operation.go

+		return details, fmt.Errorf("could not access CreateClaimableBalance info for this operation (index %d)", o.OperationIndex)
+	}
+
+	details["asset"] = op.Asset.StringCanonical()


nit: asset representation is different from manage buy/sell offer

updated in c68cd9a

ingest/ledger_operation.go

sydneynotthecity · 2025-01-23T03:36:02Z

ingest/ledger_operation.go

+	details := map[string]interface{}{}
+	_, ok := o.Operation.Body.GetRestoreFootprintOp()
+	if !ok {
+		return details, fmt.Errorf("could not access InvokeHostFunction info for this operation (index %d)", o.OperationIndex)


nit: copy/paste error. wrong operation type

updated in c68cd9a

chowbao · 2025-01-23T16:59:10Z

High level, there are some quirks in stellar-etl. Now is the time for us to discuss whether it makes sense to incorporate back in to XDRill or not. Particularly:

int vs real representation for asset amounts - should this stay in raw integer form? Should we convert to real? Offer both?

Asset id - this is an optimization specific to Hubble. Does it make sense to create a more universal asset hash? If so, does FarmHash make the most sense?

buy vs sell side of offers. In Horizon, these are presented as base and counter assets, where base asset hash < counter asset hash so that the trade pairs are consistently on the same side. This may be an opportunity for us to reimagine how we want this schema to be defined

Now is the time for us to discuss whether it makes sense to incorporate back in to XDRill or not

Yeah I was debating this too. Thinking about it again I personally would lean towards addressing the quirks now. It means less refactoring in the future in favor of spending more time now.

But I think it might be the right decision to defer for now. It's much easier to test and compare similar logic rather than slightly different logic. Like for refactor/testing of stellar-etl/processors/xdrill it'll be as easy as writing the new code and compare the output of old stellar-etl == new stellar-etl. I think the fewer value fixes we make will make the diffing much easier. Basically sticking with the status quo makes it easier for myself or anyone else to pick up later.

int vs real representation for asset amounts - should this stay in raw integer form? Should we convert to real? Offer both?

I agree with keeping this in the raw form instead of converted float

Asset id - this is an optimization specific to Hubble. Does it make sense to create a more universal asset hash? If so, does FarmHash make the most sense?

iirc the issue with the pure FarmHash is that it didn't fit nicely into the BigQuery integer size (64/128 bytes?) which is why some asset_ids can be negative because the BQ column for asset_id is int64 (?) instead of a long long type (128+). This is hacky.

I'm not opposed to making this more standard and make more sense though

buy vs sell side of offers. In Horizon, these are presented as base and counter assets, where base asset hash < counter asset hash so that the trade pairs are consistently on the same side. This may be an opportunity for us to reimagine how we want this schema to be defined

👍

Edit: So I went ahead and did make the int versus real amount change

chowbao · 2025-02-03T22:57:05Z

ingest/account_merge_details.go

@@ -0,0 +1,47 @@
+package ingest


Similarly to #3 (comment)

OperationDetails has been updated to structs to preserve strict typing for data instead of the the previous map[string]string/interface{}

Oh also if anyone has better suggestions for organizing and what dir to put these files in I'm happy to update. One issue is that golang doesn't let you put these in a different directory and still have it as part of the same package (only files in the same dir can go in the same package) but if I separate these functions out to their own package I create a circular dependency with go/ingest

chowbao · 2025-02-04T16:16:29Z

ingest/invoke_host_function_details.go

+		}
+
+		// TODO: Parameters should be processed with xdr2json
+		invokeHostFunctionDetail.Parameters, invokeHostFunctionDetail.ParametersDecoded = o.serializeParameters(args)


How do we feel about making Parameters a straight JSON from xdr2json instead of our current ScVal parameter mess?

xdrill for operations

314b605

chowbao added 3 commits January 21, 2025 13:24

Merge branch '5550/xdrill-transactions' into 5551/xdrill-operations

e9d5511

update tests

3bc82f4

Merge branch '5550/xdrill-transactions' into 5551/xdrill-operations

3d7876c

sydneynotthecity reviewed Jan 23, 2025

View reviewed changes

chowbao added 3 commits January 23, 2025 13:37

updates from comments

c68cd9a

Add structs for each operation

86d5d78

Update marshalbase64

b7c0737

chowbao commented Feb 3, 2025

View reviewed changes

chowbao added 3 commits February 3, 2025 18:33

Fix typos

a3846f2

fix ci/cd

fc6b854

fix tests

875c995

chowbao commented Feb 4, 2025

View reviewed changes

Update hash.go

5c68c65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdrill for operations #2

xdrill for operations #2

chowbao commented Jan 16, 2025

chowbao commented Jan 17, 2025

sydneynotthecity left a comment

sydneynotthecity Jan 22, 2025

chowbao Jan 23, 2025

sydneynotthecity Jan 22, 2025

chowbao Jan 23, 2025

chowbao Jan 23, 2025

sydneynotthecity Jan 23, 2025

chowbao Jan 23, 2025

chowbao Jan 23, 2025

sydneynotthecity Jan 23, 2025

chowbao Jan 23, 2025

sydneynotthecity Jan 23, 2025

chowbao Jan 23, 2025

chowbao Jan 23, 2025

chowbao commented Jan 23, 2025 •

edited

Loading

chowbao Feb 3, 2025

chowbao Feb 4, 2025

chowbao Feb 4, 2025

xdrill for operations #2

Are you sure you want to change the base?

xdrill for operations #2

Conversation

chowbao commented Jan 16, 2025

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

chowbao commented Jan 17, 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

chowbao commented Jan 23, 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 commented Jan 23, 2025 •

edited

Loading