Token Transfer Processor - Derive classic events from operation and operation results #5609
Conversation
karthikiyer56
force-pushed
the
karthik/ttp-classic-operations
branch
from
159c84a to
36946d4
Compare
karthikiyer56
force-pushed
the
karthik/ttp-classic-operations
branch
from
36946d4 to
571679c
Compare
…action_envelope and the test fixture. It is not used anyway
… work, regardless of order
ingest/processors/token_transfer/token_transfer_processor_test.go
Outdated
Show resolved
Hide resolved
ingest/processors/token_transfer/token_transfer_processor_test.go
Outdated
Show resolved
Hide resolved
…ad of a muxed account. This helps simplify the logic at call site for the operations when called with LPhash or CBid
| return changes | ||
| } | ||
|
|
||
| func mapsEqual(map1, map2 map[balanceKey]int64) bool { |
There was a problem hiding this comment.
you can use https://pkg.go.dev/maps#Equal
There was a problem hiding this comment.
Yeah, I want to retain this, since I will use this to print a delta, assuming we find that verifcation fails for some ledgers
|
|
||
| eventType := event.GetEventType() | ||
| switch eventType { | ||
| case "Fee": |
There was a problem hiding this comment.
we should have constant variables for the event type strings so they can be referenced here and other places which reference event types
There was a problem hiding this comment.
Thanks for the reminder here.
I had meant to change it to switch on types instead.
I will make the change
| "github.com/stellar/go/ingest" | ||
| addressProto "github.com/stellar/go/ingest/address" | ||
| assetProto "github.com/stellar/go/ingest/asset" | ||
| "github.com/stellar/go/support/errors" |
There was a problem hiding this comment.
we should avoid using this package as the go standard library now has support for wrapping / unwrapping errors.
instead of errors.New() from github.com/stellar/go/support/errors we should prefer https://pkg.go.dev/errors#New and instead of errors.Wrap() from github.com/stellar/go/support/errors we should be wrapping errors using fmt.Errorf in the way described here, https://pkg.go.dev/errors#pkg-overview
There was a problem hiding this comment.
Actually, the "github.com/pkg/errors" itself has errors.New and errors.WrapF . I am just using that and removing "github.com/stellar/go/support/errors"
There was a problem hiding this comment.
github.com/pkg/errors is not part of the standard library. github.com/pkg/errors Is currently in maintenance mode. the errors package in the standard library is preferred over github.com/pkg/errors
There was a problem hiding this comment.
TIL
WIll make the change
There was a problem hiding this comment.
dpone
reworked all errors in this package to use the golang errors package or fmt.Errorf
| isSuccess := mapsEqual(eventsMap, changesMap) | ||
| if !isSuccess { | ||
| fmt.Println("----ChangeMap-----") | ||
| printMap(changesMap) | ||
| fmt.Println("------") | ||
| fmt.Println("----EventsMap-----") | ||
| printMap(eventsMap) | ||
| fmt.Println("------") | ||
| } | ||
| return isSuccess |
There was a problem hiding this comment.
consider using this library to get a diff which can then be printed by the caller of this function
…Id everywhere for classic assets
| // Calculate the difference | ||
| var diff int64 | ||
| if existsInChanges && existsInEvents { | ||
| // Both maps have entries for this account/asset | ||
| diff = changesBalance - eventsBalance | ||
| } else if existsInChanges { | ||
| // Only in changes map, not in events map | ||
| diff = changesBalance | ||
| } else if existsInEvents { | ||
| // Only in events map, not in changes map | ||
| diff = -eventsBalance | ||
| } else { | ||
| // Not in either map, no difference | ||
| return events | ||
| } |
There was a problem hiding this comment.
you don't need the existsInChanges and existsInEvents variables because if a key doesn't exist in the map the zero value of the type is returned:
It is generally considered good style to "make the zero value useful"
So your calculation can be simplified to:
| // Calculate the difference | |
| var diff int64 | |
| if existsInChanges && existsInEvents { | |
| // Both maps have entries for this account/asset | |
| diff = changesBalance - eventsBalance | |
| } else if existsInChanges { | |
| // Only in changes map, not in events map | |
| diff = changesBalance | |
| } else if existsInEvents { | |
| // Only in events map, not in changes map | |
| diff = -eventsBalance | |
| } else { | |
| // Not in either map, no difference | |
| return events | |
| } | |
| diff := changesBalance - eventsBalance |
There was a problem hiding this comment.
Yeah, I had a few debug breakpoints there, to see why my code wasnt working.
i wil clean it up.
| changesBalance, existsInChanges := changesMap[key] | ||
| eventsBalance, existsInEvents := eventsMap[key] |
There was a problem hiding this comment.
| changesBalance, existsInChanges := changesMap[key] | |
| eventsBalance, existsInEvents := eventsMap[key] | |
| changesBalance := changesMap[key] | |
| eventsBalance := eventsMap[key] |
see comment below where I explain why the exists check is not necessary
There was a problem hiding this comment.
done.
reworked this to remove checks
| // DO not run this reconciliation check for ledgers > 8 or if operation is Inflation | ||
| if tx.Ledger.ProtocolVersion() > 8 { |
There was a problem hiding this comment.
In https://github.com/stellar/stellar-protocol/blob/master/core/cap-0067.md#retroactively-emitting-events it states:
Another relevant detail for replaying events is that prior to protocol 8, there was a bug that could result in the minting/burning of XLM.
| // DO not run this reconciliation check for ledgers > 8 or if operation is Inflation | |
| if tx.Ledger.ProtocolVersion() > 8 { | |
| // DO not run this reconciliation check for ledgers with protocol version >= 8 or if operation is Inflation | |
| if tx.Ledger.ProtocolVersion() >= 8 { |
There was a problem hiding this comment.
the if statement should also check that op.Body.Type is not xdr.OperationTypeInflation to be consistent with your comment
There was a problem hiding this comment.
nice catch.
will make the change
There was a problem hiding this comment.
done.
removed the check altogether from this function.
The check is not at callsite, and at the callsite (EventsFromOperation()), reconciliation will be skipped for tx.Ledger.ProtocolVersion() >= 8
| func protoAddressFromLpHash(lpId xdr.PoolId) string { | ||
| return xdr.Hash(lpId).HexString() | ||
| } | ||
|
|
||
| func protoAddressFromClaimableBalanceId(cb xdr.ClaimableBalanceId) string { | ||
| return cb.MustV0().HexString() | ||
| } | ||
|
|
||
| // TODO convert to strkey for LpId | ||
| func lpIdToStrkey(lpId xdr.PoolId) string { | ||
| return xdr.Hash(lpId).HexString() | ||
| } | ||
|
|
||
| // TODO convert to strkey for CbId | ||
| func cbIdToStrkey(cbId xdr.ClaimableBalanceId) string { | ||
| return cbId.MustV0().HexString() | ||
| } |
There was a problem hiding this comment.
it seems like these functions are duplicates of each other so we should probably be deleting lpIdToStrkey() and cbIdToStrkey() and replacing their uses with protoAddressFromLpHash() and protoAddressFromClaimableBalanceId()
There was a problem hiding this comment.
🤦 yup, will make the change
…mint and burn to appear correctly in the final list of elements
This PR deals with deriving TokenTransferEvents from operation and operation results as described in #5580
This PR is strictly dealing with classic and classic operations, i.e the PR implements the ask from #5592