Skip to content
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. #4806

Merged
merged 33 commits into from
Mar 21, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
b20cdcb
Add initial effect propogation
Shaptic Mar 10, 2023
38fb603
Harden amount parser
Shaptic Mar 10, 2023
f8ed770
Add event generator to ease testing
Shaptic Mar 14, 2023
9a8b8c9
Propogate network passphrase and add tests
Shaptic Mar 14, 2023
5400ac0
Fix amount handling, tests pass! :pray:
Shaptic Mar 14, 2023
0cdc618
Add other event test cases
Shaptic Mar 14, 2023
3f61df6
WIP of getting 'real' event parsing working
Shaptic Mar 14, 2023
12ccba0
Fix event asset topic parsing
Shaptic Mar 14, 2023
a5c5408
Integration tests check effect values, passing :pray:
Shaptic Mar 15, 2023
ec9baaa
Swap debit <-> credit :facepalm:
Shaptic Mar 15, 2023
bcd0050
Flip credit/debit in tests, check empties where appropriate
Shaptic Mar 15, 2023
cd1e883
Hide my shameful print debugging code
Shaptic Mar 15, 2023
5a1a75e
Merge branch 'soroban-xdr-next' into add-sac-effects
Shaptic Mar 15, 2023
854251e
Somehow screwed this up, passes now \o/
Shaptic Mar 16, 2023
50d2ce5
Add remaining test cases
Shaptic Mar 16, 2023
5b36a76
Add proper validation for contract addresses
Shaptic Mar 16, 2023
61b281d
Test for account, not contract :facepalm:
Shaptic Mar 16, 2023
9328689
Prefer direct allocation over throwing away ctor
Shaptic Mar 16, 2023
93018e0
Move stringification to xdr package
Shaptic Mar 16, 2023
891d3bc
Bugfix: Add strkey directly; before, contract IDs blew up
Shaptic Mar 16, 2023
6839d2c
Fixup, add, and explain add'l assertions
Shaptic Mar 16, 2023
7a6e45d
Add comment about amount precision limitations
Shaptic Mar 16, 2023
e7749c6
Fix burn amount in assertion (copy-paste bug)
Shaptic Mar 16, 2023
434f398
Merge branch 'soroban-xdr-next' into add-sac-effects
Shaptic Mar 16, 2023
8ff1e11
Fixups
Shaptic Mar 16, 2023
e175930
Fixup for a bad merge
Shaptic Mar 16, 2023
f38376e
Fix the credit/debit misuse in the other tests
Shaptic Mar 16, 2023
1cd57f3
Fix operation type being unrecognized w/o passphrase
Shaptic Mar 16, 2023
5a1d104
Separate to/from addresses in contract events
Shaptic Mar 20, 2023
72334f0
Update tests to conform to new xfer effect behavior
Shaptic Mar 20, 2023
f452ad0
PR feedback: simplify code path to network passphrase
Shaptic Mar 20, 2023
f0fe652
Add test cases around account <-> contract events
Shaptic Mar 20, 2023
d6e13c6
Assert <= 2 effects for any SAC tx
Shaptic Mar 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions amount/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,22 @@ func String(v xdr.Int64) string {
return StringFromInt64(int64(v))
}

// String128 converts a signed 128-bit integer into a string, boldly assuming
// 7-decimal precision.
sreuland marked this conversation as resolved.
Show resolved Hide resolved
//
// TODO: This should be adapted to variable precision when appopriate, but 7
// decimals is the correct default for Stellar Classic amounts.
func String128(v xdr.Int128Parts) string {
// the upper half of the i128 always indicates its sign regardless of its
// value, just like a native signed type
val := big.NewInt(int64(v.Hi))
val.Lsh(val, 64).Add(val, new(big.Int).SetUint64(uint64(v.Lo)))

rat := new(big.Rat).SetInt(val)
rat.Quo(rat, bigOne)
return rat.FloatString(7)
}

// StringFromInt64 returns an "amount string" from the provided raw int64 value `v`.
func StringFromInt64(v int64) string {
r := big.NewRat(v, 1)
Expand Down
15 changes: 12 additions & 3 deletions services/horizon/internal/db2/history/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,20 @@ const (
// EffectAccountRemoved effects occur when one account is merged into another
EffectAccountRemoved EffectType = 1 // from merge_account

// EffectAccountCredited effects occur when an account receives some currency
EffectAccountCredited EffectType = 2 // from create_account, payment, path_payment, merge_account
// EffectAccountCredited effects occur when an account receives some
// currency
//
// from create_account, payment, path_payment, merge_account, and SAC events
// involving transfers, mints, and burns.
EffectAccountCredited EffectType = 2

// EffectAccountDebited effects occur when an account sends some currency
EffectAccountDebited EffectType = 3 // from create_account, payment, path_payment, create_account
//
// from create_account, payment, path_payment, create_account, and SAC
// involving transfers, mints, and burns.
//
// https://github.com/stellar/rs-soroban-env/blob/5695440da452837555d8f7f259cc33341fdf07b0/soroban-env-host/src/native_contract/token/contract.rs#L51-L63
EffectAccountDebited EffectType = 3

// EffectAccountThresholdsUpdated effects occur when an account changes its
// multisig thresholds.
Expand Down
2 changes: 1 addition & 1 deletion services/horizon/internal/ingest/processor_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func (s *ProcessorRunner) buildTransactionProcessor(
sequence := uint32(ledger.Header.LedgerSeq)
return newGroupTransactionProcessors([]horizonTransactionProcessor{
statsLedgerTransactionProcessor,
processors.NewEffectProcessor(s.historyQ, sequence),
processors.NewEffectProcessor(s.historyQ, sequence, s.config.NetworkPassphrase),
processors.NewLedgerProcessor(s.historyQ, ledger, CurrentVersion),
processors.NewOperationProcessor(s.historyQ, sequence),
tradeProcessor,
Expand Down
143 changes: 130 additions & 13 deletions services/horizon/internal/ingest/processors/effects_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import (
"github.com/stellar/go/keypair"
"github.com/stellar/go/protocols/horizon/base"
"github.com/stellar/go/services/horizon/internal/db2/history"
"github.com/stellar/go/strkey"
"github.com/stellar/go/support/contractevents"
"github.com/stellar/go/support/errors"
"github.com/stellar/go/xdr"
)
Expand All @@ -24,12 +26,14 @@ type EffectProcessor struct {
effects []effect
effectsQ history.QEffects
sequence uint32
network string
}

func NewEffectProcessor(effectsQ history.QEffects, sequence uint32) *EffectProcessor {
func NewEffectProcessor(effectsQ history.QEffects, sequence uint32, networkPassphrase string) *EffectProcessor {
return &EffectProcessor{
effectsQ: effectsQ,
sequence: sequence,
network: networkPassphrase,
}
}

Expand All @@ -56,7 +60,10 @@ func (p *EffectProcessor) loadAccountIDs(ctx context.Context, accountSet map[str
return nil
}

func operationsEffects(transaction ingest.LedgerTransaction, sequence uint32) ([]effect, error) {
func operationsEffects(
transaction ingest.LedgerTransaction,
sequence uint32,
networkPassphrase string) ([]effect, error) {
effects := []effect{}

for opi, op := range transaction.Envelope.Operations() {
Expand All @@ -65,6 +72,7 @@ func operationsEffects(transaction ingest.LedgerTransaction, sequence uint32) ([
transaction: transaction,
operation: op,
ledgerSequence: sequence,
network: networkPassphrase,
}

p, err := operation.effects()
Expand Down Expand Up @@ -119,7 +127,7 @@ func (p *EffectProcessor) ProcessTransaction(ctx context.Context, transaction in
}

var effectsForTx []effect
effectsForTx, err = operationsEffects(transaction, p.sequence)
effectsForTx, err = operationsEffects(transaction, p.sequence, p.network)
if err != nil {
return err
}
Expand Down Expand Up @@ -173,8 +181,9 @@ func (operation *transactionOperationWrapper) effects() ([]effect, error) {
}

wrapper := &effectsWrapper{
effects: []effect{},
operation: operation,
effects: []effect{},
operation: operation,
passphrase: operation.network,
Shaptic marked this conversation as resolved.
Show resolved Hide resolved
}

switch operation.OperationType() {
Expand Down Expand Up @@ -210,8 +219,11 @@ func (operation *transactionOperationWrapper) effects() ([]effect, error) {
err = wrapper.addCreateClaimableBalanceEffects(changes)
case xdr.OperationTypeClaimClaimableBalance:
err = wrapper.addClaimClaimableBalanceEffects(changes)
case xdr.OperationTypeBeginSponsoringFutureReserves, xdr.OperationTypeEndSponsoringFutureReserves, xdr.OperationTypeRevokeSponsorship:
// The effects of these operations are obtained indirectly from the ledger entries
case xdr.OperationTypeBeginSponsoringFutureReserves,
xdr.OperationTypeEndSponsoringFutureReserves,
xdr.OperationTypeRevokeSponsorship:
// The effects of these operations are obtained indirectly from the
// ledger entries
case xdr.OperationTypeClawback:
err = wrapper.addClawbackEffects()
case xdr.OperationTypeClawbackClaimableBalance:
Expand All @@ -223,10 +235,19 @@ func (operation *transactionOperationWrapper) effects() ([]effect, error) {
case xdr.OperationTypeLiquidityPoolWithdraw:
err = wrapper.addLiquidityPoolWithdrawEffect()
case xdr.OperationTypeInvokeHostFunction:
// TODO: https://github.com/stellar/go/issues/4585
return nil, nil
// If there's an invokeHostFunction operation, there's definitely V3
// meta in the transaction, which means this error is real.
events, innerErr := operation.transaction.GetOperationEvents(operation.index)
if innerErr != nil {
return nil, innerErr
}

// For now, the only effects are related to the events themselves.
// Possible add'l work: https://github.com/stellar/go/issues/4585
err = wrapper.addInvokeHostFunctionEffects(events)

default:
return nil, fmt.Errorf("Unknown operation type: %s", op.Body.Type)
return nil, fmt.Errorf("unknown operation type: %s", op.Body.Type)
}
if err != nil {
return nil, err
Expand All @@ -246,16 +267,18 @@ func (operation *transactionOperationWrapper) effects() ([]effect, error) {

// Liquidity pools
for _, change := range changes {
// Effects caused by ChangeTrust (creation), AllowTrust and SetTrustlineFlags (removal through revocation)
// Effects caused by ChangeTrust (creation), AllowTrust and
// SetTrustlineFlags (removal through revocation)
wrapper.addLedgerEntryLiquidityPoolEffects(change)
}

return wrapper.effects, nil
}

type effectsWrapper struct {
effects []effect
operation *transactionOperationWrapper
effects []effect
operation *transactionOperationWrapper
passphrase string
}

func (e *effectsWrapper) add(address string, addressMuxed null.String, effectType history.EffectType, details map[string]interface{}) {
Expand Down Expand Up @@ -1386,3 +1409,97 @@ func (e *effectsWrapper) addLiquidityPoolWithdrawEffect() error {
e.addMuxed(e.operation.SourceAccount(), history.EffectLiquidityPoolWithdrew, details)
return nil
}

// addInvokeHostFunctionEffects iterates through the events and generates
// account_credited and account_debited effects when it sees events related to
// the Stellar Asset Contract corresponding to those effects.
func (e *effectsWrapper) addInvokeHostFunctionEffects(events []contractevents.Event) error {
Copy link
Contributor

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

Copy link
Contributor Author

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 using strkey 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 new amount.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... (or M..., 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 with C... values, though, there are two issues I see:

  • many events will have at least one field be a contract (by nature of the event coming from a contract), e.g. anyone letting a contract hold a balance on their behalf will cause a transfer-to-contract event, and this is the first thing many will do
  • excluding said events gives an incomplete picture of the credit/debit effects pertaining to an asset

There are a few ways to resolve this, I think:

  1. introduce contract_debited and contract_credited effects: This gives a fuller picture and can handle, for example, intra-contract transfers. The downside is that downstream users now need to
  2. let account_debited and account_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.
  3. introduce generic debited and credited effects and drop the account_* 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because these are SAC events, they can't have more than 64 bits of an asset because of the limitation from Stellar Classic

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.

excluding said events gives an incomplete picture of the credit/debit effects pertaining to an asset

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.

Copy link
Contributor Author

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.

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 include account_credited when the funds come to an account (again, regardless of source).

Copy link
Contributor

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

if e.operation.network == "" {
return errors.New("invokeHostFunction effects cannot be determined unless network passphrase is set")
}

for _, event := range events {
evt, err := contractevents.NewStellarAssetContractEvent(&event, e.operation.network)
if err != nil {
continue // irrelevant or unsupported event
}

details := make(map[string]interface{}, 4)
addAssetDetails(details, evt.GetAsset(), "")

//
// Note: We ignore effects that involve contracts (until the day we have
// contract_debited/credited effects, may it never come :pray:)
//

switch evt.GetType() {
// Transfer events generate an `account_debited` effect for the `from`
// (sender) and an `account_credited` effect for the `to` (recipient).
case contractevents.EventTypeTransfer:
xferEvent := evt.(*contractevents.TransferEvent)
details["amount"] = amount.String128(xferEvent.Amount)
if strkey.IsValidEd25519PublicKey(xferEvent.From) {
e.add(
Shaptic marked this conversation as resolved.
Show resolved Hide resolved
xferEvent.From,
null.String{},
history.EffectAccountDebited,
details,
)
}
if strkey.IsValidEd25519PublicKey(xferEvent.To) {
e.add(
xferEvent.To,
null.String{},
history.EffectAccountCredited,
details,
)
}

// Mint events imply a non-native asset, and it results in a credit to
// the `to` recipient.
case contractevents.EventTypeMint:
mintEvent := evt.(*contractevents.MintEvent)
details["amount"] = amount.String128(mintEvent.Amount)
if strkey.IsValidEd25519PublicKey(mintEvent.To) {
e.add(
mintEvent.To,
null.String{},
history.EffectAccountCredited,
details,
)
}

// Clawback events result in a debit to the `from` address, but acts
// like a burn to the recipient, so these are functionally equivalent
case contractevents.EventTypeClawback:
cbEvent := evt.(*contractevents.ClawbackEvent)
details["amount"] = amount.String128(cbEvent.Amount)
if strkey.IsValidEd25519PublicKey(cbEvent.From) {
e.add(
cbEvent.From,
null.String{},
history.EffectAccountDebited,
details,
)
}

case contractevents.EventTypeBurn:
burnEvent := evt.(*contractevents.BurnEvent)
details["amount"] = amount.String128(burnEvent.Amount)
if strkey.IsValidEd25519PublicKey(burnEvent.From) {
e.add(
burnEvent.From,
null.String{},
history.EffectAccountDebited,
details,
)
}

// Other events are irrelevant to debit/credit effects
default:
Copy link
Contributor

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?

Copy link
Contributor Author

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 🤷

continue
}
}

return nil
}
Loading