stellar / go

Stellar's public monorepo of go code
https://stellar.org/developers
Apache License 2.0
1.3k stars 502 forks source link

Ingestion Filtering: Claim operations are not matched against asset rules #5319

Open JakeUrban opened 5 months ago

JakeUrban commented 5 months ago

The function that inspects operations to determine whether it involves an asset included in the rules defined does not have a case for the claiming of a claimable balance, resulting in claim operations not being ingested.

JakeUrban commented 5 months ago

Note that this function also excludes contract invocations, which means SAC transfers will not match even if the asset transfered is defined in the ruleset.

JakeUrban commented 5 months ago

Generally speaking, it looks like we're only considering operations that reference the asset in the operation. If an asset is involved as a result of an operation that doesn't explicitly reference it, we miss it.

sisuresh commented 5 months ago

Generally speaking, it looks like we're only considering operations that reference the asset in the operation. If an asset is involved as a result of an operation that doesn't explicitly reference it, we miss it.

Yeah I'm not sure how this function is used, but the same issue exists for the liquidity pool operations.

sreuland commented 5 months ago

yes, this aspect was recognized during asset filter spike as depth or 'transitive hops' from an operation which has may contain reference id's that ultimately resolves to an asset or an account, which requires much more complexity to detect the fields that hold the id's and consequently more sql queries per op/tx to resolve the id's back to assets, which incurs bigger performance implications. At that time there were no firm requirements on filtering to reach further depth matching specific to any ops, so was intentional at that time to not introduce it yet.

we had created some feature tickets to track some other similar depth/transitive hops related to sponsorships and trustlines: https://github.com/stellar/go/labels/ingest-filtering

and I think this ticket could be converted to a feature for claiming balances.

JakeUrban commented 5 months ago

I'm not sure if taking an iterative approach in this case is the best path forward. Yes adding support for claiming balance would fix our partner's issue but I think users understandably assume Horizon is indexing transactions that involve the specified account or asset regardless of whether its explicitly referenced in the envelope. I think we want to encourage the use of these filters, but right now they only match a subset of cases, so we're recommending an incomplete solution.

Can we not iterate over the the ~transaction result~ ledger changes in addition to its associated envelope via LedgerTransaction.getChanges()?

sreuland commented 5 months ago

Can we not iterate over the the transaction result ledger changes in addition to its associated envelope via LedgerTransaction.getChanges()?

@JakeUrban, this is a good suggestion, maybe we can re-purpose this ticket scope to proof this out further as it sounds like potential to reach better coverage for all filter matching by resolving several of the 'first hop' transitive relationships from operations to their side effects on assets/accounts when no direct reference to any filterable value is present on the op.

There are a group of more advanced multi-level hops that were recognized during design spike, which we won't be able to solve with additional 'inspect ledger changes' approach, but maybe we can keep those cordoned off for new feature consideration later.

In parallel, we should probably maintain some details on filter docs for expected behavior based on latest limitations such as current exclusive list of operations to help manage expectations?

cc: @2opremio , what do you think about this approach of inspecting the LedgerEntry changes also as part of filter's search?

JakeUrban commented 5 months ago

Thanks for referencing the issues that require multiple hops! I agree that sounds harder to solve for. But the LedgerTransaction is already in memory and should have enough data to capture a larger set of cases.