ingest: Adds basic event parsing for standard Stellar Asset Contract events #4798
Conversation
ingest/event.go
Outdated
| } | ||
|
|
||
| assetBytes, ok := assetContainer.GetBin() | ||
| fmt.Println("here", len(assetBytes), assetBytes) |
There was a problem hiding this comment.
good debug logging, just a little more descriptive, fmt.Printf('SAC asset event received for %v', string(assetBytes)) ?
There was a problem hiding this comment.
lol I left this in while I was debugging tests 🤦 maybe we shouldn't print stuff in this package? Horizon itself should probably do that, though
Shaptic
force-pushed
the
sac-event-parsing
branch
from
5b99544 to
b76d202
Compare
Shaptic
force-pushed
the
sac-event-parsing
branch
from
adb9970 to
7cb137c
Compare
ingest/event.go
Outdated
| ErrNotTransferEvent = errors.New("event is an invalid 'transfer' event") | ||
| ) | ||
|
|
||
| type StellarAssetContractEvent struct { |
There was a problem hiding this comment.
This event is meant to model a sub-group of similar functional events from SAC related to balance changes mostly correct? Maybe can indicate that with name like StellarAssetContractBalanceChangedEvent to infer that meaning a bit more?
[edit, not suggesting change now, just for consideration]
Do you think this eventually scales to point of being 1-1, a separate event struct specific to each SAC method, i.e. StellarAssetContract<sac_method>Event, each supporting common interface:
type StellarAssetContractEvent interface {
getType() int
}
NewStellarAssetContractEvent() becomes more polymorphic, returning instance of event specific struct that matched contract event, caller uses getType() to cast to real struct type.
There was a problem hiding this comment.
This event is meant to model a sub-group of similar functional events from SAC related to balance changes mostly correct?
I thought, rather, that this would be done for all SAC events, but we'd start with balance-related ones since those have the immediate need. I do like the idea of making it polymorphic (kinda like txnbuild.Operation is, see here), but maybe it's too soon? For now, it's easier to use the struct and just know which fields are set for which types.
There was a problem hiding this comment.
yep, we don't want to make it too fancy yet, as it may change, but thinking of how callers will use the event struct, they're gonna end up hardcoding what fields in the uber-struct(StellarAssetContractEvent ) are valid for each type of event(xfer, mint, clawback, etc), whereas if we bite the bullet now and capture the fields-per-contract-event in separate structs, then callers can handle different events in more deterministic type-safe way
There was a problem hiding this comment.
yeah I 100% agree, was just being lazy tbh - added in latest push (a7ebfd6)! is that what you had in mind?
| switch evt.Type { | ||
| case EventTypeTransfer: | ||
| return evt, evt.parseTransferEvent(topics, value) | ||
| case EventTypeMint: |
There was a problem hiding this comment.
these seem to return an un-usable event, as only Type has been populated, maybe just throw unsupported also to avoid callers trying to do something, or do you think we can squeeze mint/clawback into the current StellarAssetContractEvent?
I was wondering if mint is qualified for including in payments, I see payments includes CreateAccount operation, which is somewhat similar to mint?
There was a problem hiding this comment.
Yeah you're right - this is captured in the docstring: It will return a best-effort parsing even in error cases.. I could see callers actually finding this desirable: it helps debugging where parsing failed (i.e. you can see which fields got parsed correctly and which didn't), and it can also make this a useful parsing mechanism even for custom events. For example, if I have a "SAC-like" event in the form transfer/blah/blah/<asset>, then I can use this helper to extract <asset> without actually conforming as a proper SAC event.
There was a problem hiding this comment.
As for payments, I think it should count, because in the Classic world, a "mint" of your custom asset only really happens after you make a PaymentOp to someone. Same thing with burns.
| switch address.Type { | ||
| case xdr.ScAddressTypeScAddressTypeAccount: | ||
| pubkey := address.MustAccountId().Ed25519 | ||
| fmt.Println("pubkey:", address.MustAccountId()) |
There was a problem hiding this comment.
another print statement we should remove
There was a problem hiding this comment.
good catches, thanks! should be gone in the other PR
What
Adds an abstraction layer for standardized contract events that come from the Stellar Asset Contract.
Why
Event parsing is necessary to generate effects and record payments as part of #4722 (and #4725).
Known limitations
This only covers the
transferevents generated by callingxferandxfer_from. It could also use more test cases.Ideally, @sreuland and I would build the parsing/testing on top of this PR for the events we need as they pertain to our issues.