Include operation information in the Change struct #5536

karthikiyer56 · 2024-11-20T08:39:23Z

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

This PR deals with this github issue - #5535

Why

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

Known limitations

[TODO or N/A]

tamirms · 2024-11-20T10:21:15Z

ingest/change.go

+	Type              xdr.LedgerEntryType
+	Pre               *xdr.LedgerEntry
+	Post              *xdr.LedgerEntry
+	isOperationChange bool


It looks like there are several sources for a change within a Ledger:

fees which are executed at the transaction level

upgrades which are executed at the ledger level

evictions which are executed at the ledger level

transaction level changes

operation level changes

I think it would be could to have a separate type which represents the source of the change. The source of the change can be similar to an xdr union in that it is belonging to one of the categories listed above.

…Transaction.GetChanges()

tamirms · 2024-11-21T13:39:10Z

ingest/change.go

+	Pre             *xdr.LedgerEntry
+	Post            *xdr.LedgerEntry
+	reason          LedgerEntryChangeReason
+	TransactionData *TransactionEnvelopeAndResult


I think it's better to include a pointer to LedgerTransaction which already contains the envelope and the result. It can also be used to find the operation via the GetOperation() function. so we can also get rid of OperationInfo and store the operation index in Change. The operation index will only be valid if the change reason is equal to Operation

We should also add another field which is a pointer to the LedgerCloseMeta which produced this change

tamirms · 2024-11-21T13:39:43Z

ingest/change.go

+	Type            xdr.LedgerEntryType
+	Pre             *xdr.LedgerEntry
+	Post            *xdr.LedgerEntry
+	reason          LedgerEntryChangeReason


reason should be exported so that it is accessible outside the ingest package

tamirms · 2024-11-21T13:41:05Z

ingest/change.go

+	Operation
+	Transaction
+	FeeChange
+	ProtocolUpgrade


I'm not sure if the upgrades field in the LCM corresponds to protocol upgrades, I would confirm this with the stellar core team.

No need to confirm.
A deeper code inspection indicates that UpgradeChanges can mean much more than just ProtocolUpgrade related changes.
Have modified the Chagne struct to include LedgerUpgradeChange

tamirms · 2024-11-21T13:41:44Z

ingest/change.go

+	operationInfo   *OperationInfo
+}
+
+type LedgerEntryChangeReason uint16


could you add a doc comment for this type and the enum values listed below this code

Yes, I redid the comments on this Change struct nauseam 🙈

tamirms · 2024-11-21T14:16:34Z

@karthikiyer56 TestLiquidityPoolDepositDetails in master/services/horizon/internal/ingest/processors/transaction_operation_wrapper_test.go is one of the unit tests that is failing on your branch.

I looked into it and the reason it is failing is because the ingest.LedgerTransaction instance in the test is not complete. It is missing the transaction envelope.

The test exercises a code path where GetOperationChanges() is called on the ingest.LedgerTransaction instance. It fails there because you have modified the code to include operation details into the change. The operation info is pulled from the transaction envelope (which is the right thing to do). However, because the test case does not populate the ingest.LedgerTransaction instance with a transaction envelop, the operation is not found and that causes the test not to pass.

karthikiyer56 · 2024-11-21T14:26:21Z

@karthikiyer56 TestLiquidityPoolDepositDetails in master/services/horizon/internal/ingest/processors/transaction_operation_wrapper_test.go is one of the unit tests that is failing on your branch.

I looked into it and the reason it is failing is because the ingest.LedgerTransaction instance in the test is not complete. It is missing the transaction envelope.

The test exercises a code path where GetOperationChanges() is called on the ingest.LedgerTransaction instance. It fails there because you have modified the code to include operation details into the change. The operation info is pulled from the transaction envelope (which is the right thing to do). However, because the test case does not populate the ingest.LedgerTransaction instance with a transaction envelop, the operation is not found and that causes the test not to pass.

Yup.
Earlier in the day, when the code was fetching operational results, more tests were failing because the ledger operations were missing in more tests.
I tried fixing them, and they still were failing, even when i did add operations to a few test fixtures

So, i decided to simplify the data returned in the change structure 😓
Just as well, since we can derive more details from just envelope and tx result, at the call site itself, when we use the code in trades processing.

At any rate,i thought the latest changes should not cause any unit test failures anymore. I'll look into the one that is failing now.

Good call-out.
Ima just pass a pointer to the ingest.edgerTransaction itself in change,instead of TX envelope and result pair

tamirms · 2024-11-22T07:58:09Z

ingest/ledger_transaction.go

@@ -17,6 +17,8 @@ type LedgerTransaction struct {
 	FeeChanges    xdr.LedgerEntryChanges
 	UnsafeMeta    xdr.TransactionMeta
 	LedgerVersion uint32
+	Lcm           *xdr.LedgerCloseMeta // This is read-only and not to be modified by downstream functions
+	Hash          *xdr.Hash


Hash and Lcm should be values instead of pointers since we expect them to be non-nil

tamirms · 2024-11-22T07:59:49Z

ingest/ledger_transaction.go

@@ -17,6 +17,8 @@ type LedgerTransaction struct {
 	FeeChanges    xdr.LedgerEntryChanges
 	UnsafeMeta    xdr.TransactionMeta
 	LedgerVersion uint32
+	Lcm           *xdr.LedgerCloseMeta // This is read-only and not to be modified by downstream functions


I think this should be a private variable since it only is present here for populating changes.

Hash can remain public because it is a property of the transaction

Also, I think Lcm should be renamed to Ledger to be consistent with how the other fields in this struct are named (note that none of the other fields are abbreviated)

tamirms · 2024-11-22T08:00:54Z

ingest/ledger_transaction.go

+	changes := GetChangesFromLedgerEntryChanges(t.FeeChanges)
+	for _, change := range changes {
+		change.Reason = FeeChange
+		change.Tx = t


I think we should also populate the ledger field in the change

tamirms · 2024-11-22T08:01:11Z

ingest/ledger_transaction.go

+	for _, change := range changes {
+		change.Reason = Operation
+		change.Tx = t
+		change.OperationIdx = index


I think we should also populate the ledger field in the change

tamirms · 2024-11-22T08:09:46Z

xdr/transaction_envelope.go

+	// This is not expected to happen.
+	if (e == TransactionEnvelope{}) {
+		return []Operation{}
+	}


I don't think this is necessary

tamirms · 2024-11-22T08:10:54Z

services/horizon/internal/ingest/processors/operations_processor.go

@@ -324,7 +324,7 @@ func addAccountAndMuxedAccountDetails(result map[string]interface{}, a xdr.Muxed
 		// _muxed_id fields should had ideally been stored in the DB as a string instead of uint64
 		// due to Javascript not being able to handle them, see https://github.com/stellar/go/issues/3714
 		// However, we released this code in the wild before correcting it. Thus, what we do is
-		// work around it (by preprocessing it into a string) in Operation.UnmarshalDetails()
+		// work around it (by preprocessing it into a string) in OperationChange.UnmarshalDetails()


I don't think we need to update the comment here

tamirms · 2024-11-22T08:11:17Z

ingest/ledger_transaction_test.go

@@ -336,7 +336,7 @@ func TestFeeMetaAndOperationsChangesSeparate(t *testing.T) {
 	assert.Equal(t, operationChanges[0].Pre.Data.MustAccount().Balance, xdr.Int64(300))
 	assert.Equal(t, operationChanges[0].Post.Data.MustAccount().Balance, xdr.Int64(400))

-	// Ignore operation meta if tx result is txInternalError
+	// Ignore operation meta if Tx result is txInternalError


I don't think we should be changing this comment

tamirms · 2024-11-22T08:15:03Z

ingest/change.go

+	OperationIdx  uint32
+	Tx            *LedgerTransaction
+	Lcm           *xdr.LedgerCloseMeta


I think the fields should be consistent with how other public fields in this package are named. So, Tx should be renamed to Transaction, Lcm -> Ledger, OperationIdx -> OperationIndex

Include operation information in the Change struct

d158205

karthikiyer56 force-pushed the karthik/5535/Extend-change-struct-with-operationDetails branch from ba3ad68 to d158205 Compare November 20, 2024 08:40

Dont error out when operation index out of range

66abef5

karthikiyer56 requested a review from tamirms November 20, 2024 09:02

tamirms reviewed Nov 20, 2024

View reviewed changes

karthikiyer56 added 4 commits November 20, 2024 19:29

Fix failing unit test

fb94fdf

Reafactor ledger_transaction.GetChanges() to use internal functions

06139d3

reformat comments

070c552

Add all types of change reasons to the Change struct. Simplify Ledger…

26780fa

…Transaction.GetChanges()

tamirms reviewed Nov 21, 2024

View reviewed changes

karthikiyer56 added 5 commits November 21, 2024 08:29

Add LCM and transaction info altogether in change entry

b168415

Wrte help doc for Change Entry

29bc9bc

Add TxHash to LedgerTransaction struct

73e4494

reorg comments

c06e241

Comments cleanup

fe7cef8

tamirms reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include operation information in the Change struct #5536

Include operation information in the Change struct #5536

karthikiyer56 commented Nov 20, 2024 •

edited

Loading

tamirms Nov 20, 2024

karthikiyer56 Nov 21, 2024

tamirms Nov 21, 2024

tamirms Nov 21, 2024

karthikiyer56 Nov 21, 2024

tamirms Nov 21, 2024

karthikiyer56 Nov 21, 2024

tamirms Nov 21, 2024

karthikiyer56 Nov 21, 2024

tamirms Nov 21, 2024

karthikiyer56 Nov 21, 2024

tamirms commented Nov 21, 2024

karthikiyer56 commented Nov 21, 2024 •

edited

Loading

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

tamirms Nov 22, 2024

Include operation information in the Change struct #5536

Are you sure you want to change the base?

Include operation information in the Change struct #5536

Conversation

karthikiyer56 commented Nov 20, 2024 • edited Loading

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

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

tamirms commented Nov 21, 2024

karthikiyer56 commented Nov 21, 2024 • 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

karthikiyer56 commented Nov 20, 2024 •

edited

Loading

karthikiyer56 commented Nov 21, 2024 •

edited

Loading