Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
services/horizon: Add
account_credited
/debited
effects for SAC events. #4806services/horizon: Add
account_credited
/debited
effects for SAC events. #4806Changes from 9 commits
b20cdcb
38fb603
f8ed770
9a8b8c9
5400ac0
0cdc618
3f61df6
12ccba0
a5c5408
ec9baaa
bcd0050
cd1e883
5a1a75e
854251e
50d2ce5
5b36a76
61b281d
9328689
93018e0
891d3bc
6839d2c
7a6e45d
e7749c6
434f398
8ff1e11
e175930
f38376e
1cd57f3
5a1d104
72334f0
f452ad0
f0fe652
d6e13c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code does not check if the sender / recipient of the assets are accounts as opposed to contracts. since the name of the effects imply that accounts were credited / debited I think we should only emit these effects if the object of the effect is an account. Also, if we were want to do these effects for contracts we'd have to keep in mind that contracts can hold up to 128 bits of an asset. Only accounts have the 64 bit restriction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising these subtleties!
The code should handle accounts and contracts transparently by nature of
contractevents
usingstrkey
appropriately to fill out the e.g.From
fields, and because these are SAC events, they can't have more than 64 bits of an asset because of the limitation from Stellar Classic (if I understand it correctly). (The newamount.String128
helper should handle this sanely, too, but we'd need more work across the board to understand non-standard events anyway.)However, you make a compelling point that the name of the effect itself implies that it should only have
G...
(orM...
, but that doesn't apply for contracts) values in the fields. I guess, in my mind, contracts are a form of accounts, but you're right that it may not be intuitive. If we drop events withC...
values, though, there are two issues I see:transfer-to-contract
event, and this is the first thing many will doThere are a few ways to resolve this, I think:
contract_debited
andcontract_credited
effects: This gives a fuller picture and can handle, for example, intra-contract transfers. The downside is that downstream users now need toaccount_debited
andaccount_credited
include contracts: aka do nothing different in this PR. This has the issue of being confusing in name (as you mentioned) if there are purely contract-to-contract events captured here.debited
andcredited
effects and drop theaccount_*
versions: this is a breaking change and would probably be a bunch of work, but it would give everyone a unified view into what's happening to an asset(This isn't an exhaustive list by any means!)
Thoughts? I can also cross-post to Slack so we can get more design input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
through the integration tests I wrote for asset stats I discovered that contracts can hold more than 64 bits worth of an asset. only the accounts are restricted in having at most 64 bits.
yes, you're absolutely right. I think out of the solutions you proposed I prefer introducing contract_debited and contract_credited effects.
However, horizon support for contracts is already incomplete because we do not have an endpoint to query the contract balance whereas we do have an accounts endpoint that can tell us what assets an account holds. Given that we have limited support for contract visibility in Horizon, I think it would be ok to not have contract debited / credited effects until we decide we want more comprehensive support contract balances in horizon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh woah, okay, yeah that makes sense, e.g. if two accounts hold the max amount and both deposit into a contract then it should work. Noted, great catch 👏
Good point regarding contracts. I do think that the debit/credit of an asset into/out of a contract lies directly on the bridge between Classic and Soroban, but contract-to-contract can be out of scope. We can cross that bridge if we get to it. It's unfortunate that we still have to store/propogate/parse the whole event only to find it as being irrelevant at the final stage, but 🤷♂️
I'll amend this PR to include
account_debited
when the funds come from an account (regardless of destination, incl. contracts) and includeaccount_credited
when the funds come to an account (again, regardless of source).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good discovery, I think this aspect of dropping sac events which are strictly contract-2-contract(have no classic account reference) as they are not applicable to current horizon data model, applies when deriving payments as well, will take same approach on the upcoming pr for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an idiomatic subtlety for the no-op default clause, it seems self-describing w/o, it's worth extra code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess just showing that it's intentional rather than forgetting some other event types 🤷