Skip to content

[ANCHOR-1242]: Contract sub invocation transfers are skipped#1971

Open
amandagonsalves wants to merge 11 commits into
developfrom
fix/anchor-1242
Open

[ANCHOR-1242]: Contract sub invocation transfers are skipped#1971
amandagonsalves wants to merge 11 commits into
developfrom
fix/anchor-1242

Conversation

@amandagonsalves

@amandagonsalves amandagonsalves commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Description

Before this change, StellarRpcPaymentObserver.processTransferEvent selected the credited operation with:

LedgerOperation op = txn.getOperations().get(result.event.getOperationIndex().intValue());

event.getOperationIndex() is a 0-based index into the full on-chain operation array as returned by the Soroban-RPC getEvents API. txn.getOperations() is not that full array — it is the compacted output of LedgerClientHelper.getLedgerOperations(), which silently drops every operation for which convert() returns null (MANAGE_DATA, CHANGE_TRUST, CREATE_ACCOUNT, BUMP_SEQUENCE, SET_OPTIONS, any INVOKE_HOST_FUNCTION whose direct function name is not "transfer", etc.) with no placeholder. Using the full-list index against the shorter compacted list produced two failure modes:

  • Silent drop (Variant A): any multi-operation transaction containing a filtered op at or before the payment → IndexOutOfBoundsException → caught by the generic handler → event permanently dropped, SEP-6/24/31 transaction frozen, user funds received but never credited.
  • Sub-invocation skip (Variant B): a top-level INVOKE_HOST_FUNCTION whose direct function name is not "transfer" (contract/SEP-45 sub-invocation) emits a transfer event at the full-list index of that op, which convert() drops → same out-of-bounds or wrong-element result.

The fix stops trusting positional alignment. convert() stores the TOID — derived from (sequenceNumber, applicationOrder, 1-based opIndex) — as each operation's id. The event's operationIndex (0-based) is converted to the same 1-based coordinate, the TOID for the target operation is derived, and the compacted list is stream-filtered by identity match.

Crediting Variant B correctly required more than a first pass covered, surfaced by review:

  1. Asset spoofing. The first-pass fix credited Variant B by trusting the transfer event's own claimed asset topic. But buildEventRequest's Soroban event filters match on topic shape only, not on which contract emitted the event — any Soroban contract can emit a transfer-shaped event naming the anchor's distribution account and an arbitrary SEP-11 asset string, with no real token movement. Trusting the event's own asset claim (and pre-populating LedgerInvokeHostFunctionOperation.asset, which also caused processOperation's SAC-verification guard to skip itself) let a forged event synthesize a credited payment for any asset/amount.
  2. i128 truncation. The event's amount is a 128-bit signed integer; the original decode narrowed it through Scv.fromInt128(...).longValue() before the value was ever used, silently wrapping any amount outside Long range.
  3. Payment details never reached the persisted transaction. DefaultPaymentListener only forwards (transactionId, stellarTxnHash, amount) to notifyOnchainFundsReceived. NotifyOnchainFundsReceivedHandler independently re-fetches the transaction to record payment details (PaymentHelper.addStellarTransaction) and, for SEP-31, to set fromAccount. A synthetic operation that only existed inside the observer's local, in-memory LedgerTransaction object never survived that re-fetch — the re-fetch reconstructed the operations list from scratch via the exact same filtering that dropped the sub-invocation operation the first time, so it came back empty again. The credit (status/amount) happened, but the payment record (operationId/from/to/asset) was silently lost, and SEP-31's fromAccount was never set.

Fix for all three: moved sub-invocation reconstruction out of the observer and into the shared LedgerClientHelper.getLedgerOperations(), the same compaction step used both by the observer's initial fetch and by NotifyOnchainFundsReceivedHandler's later re-fetch (GetTransactionResponse.getEvents().parseContractEventsXdr() already exposes the transaction's own emitted contract events per top-level operation, from the same fetch — no second RPC round-trip needed). For each operation convert() drops, LedgerClientHelper now scans that operation's contract events for a CAP-67 transfer event, resolves the emitting contract through a sacResolver function, and only synthesizes an operation if the contract resolves to a real SAC and its canonical asset matches the event's claimed asset — otherwise the operation stays dropped. The amount is carried as BigInteger throughout, never narrowed to long. Because this now happens inside the shared compaction step, both the observer and NotifyOnchainFundsReceivedHandler's re-fetch see the same complete, verified operations list — StellarRpcPaymentObserver's own synthesis logic became dead code once this landed and was removed, and NotifyOnchainFundsReceivedHandler/PaymentHelper needed no changes at all.

StellarRpc (the only LedgerClient implementation with Soroban event access) exposes the resolver via a mutable field and a setSacResolver(...) setter rather than a constructor parameter, so its existing constructors and UtilityBeans' wiring are untouched; the setter is called once, in PaymentObserverBeans, which already has both stellarRpc and sacToAssetMapper in scope for constructing the observer.

Changes

  • LedgerClientHelper.parseTransferEvent: new — decodes a CAP-67 transfer ContractEvent (topics + i128/map data, including the muxed-memo destination case) into a TransferEventData record, mirroring the decode logic the observer's shouldProcess() already used for its own event-index-sourced events, now operating on the transaction's embedded ContractEvent XDR instead.
  • LedgerClientHelper.synthesizeSubInvocationTransfer: new — given a dropped operation's contract events, finds a transfer event, resolves the emitting contract via the supplied sacResolver, requires the canonical asset to match the event's claimed asset, and only then builds a LedgerInvokeHostFunctionOperation with the correct TOID-derived id.
  • LedgerClientHelper.getLedgerOperations: new overload accepting List<List<ContractEvent>> perOperationContractEvents and a Function<String, Asset> sacResolver; for each dropped operation, attempts synthesis via the above. Original 4-arg signature preserved as a delegating overload (null, null) — Horizon.java and existing callers unchanged.
  • StellarRpc: added a mutable sacResolver field (default id -> null) and setSacResolver(...); getTransaction/fromGetTransactionResponse now parse GetTransactionResponse.getEvents().parseContractEventsXdr() and pass it plus the resolver into getLedgerOperations. No constructor or existing method signature changed.
  • PaymentObserverBeans.stellarPaymentObserver: one line — stellarRpc.setSacResolver(sacToAssetMapper::getAssetFromSac) before constructing StellarRpcPaymentObserver, using the SacToAssetMapper bean already injected there.
  • StellarRpcPaymentObserver: removed buildSubInvocationOperation and the sub-invocation branch in processTransferEvent (now dead — txn.getOperations() already contains the synthesized, verified operation by the time the TOID lookup runs); ShouldProcessResult.amount changed from Long to BigInteger.
  • LedgerClientHelperTest: new tests — synthesizes a sub-invocation transfer from a verified SAC; refuses when the contract isn't a resolvable SAC; refuses when the event's claimed asset doesn't match the contract's canonical asset; preserves an i128 amount above Long.MAX_VALUE without wrapping.
  • StellarRpcPaymentObserverTest / StellarRpcObserverIndexDomainTest: removed the sub-invocation-crediting tests that mocked stellarRpc.getTransaction directly (that mocking bypasses the real synthesis, which now lives below StellarRpc); the generic "no matching operation → skip" case is still covered at the observer level.
  • DefaultPaymentListenerTest: added WITHDRAWAL_EXCHANGE/DEPOSIT_EXCHANGE coverage for the sibling SEP-6 asset-mismatch fix (report #3856257), verifying the exchange kinds resolve enforceAssetMatch the same way their non-exchange counterparts do.

Acceptance Criteria

  • A two-operation transaction [MANAGE_DATA, PAYMENT→dist] with transfer event at operationIndex=1 is credited with the correct from/to/amount/operationId.
  • A contract sub-invocation transfer from a genuine, resolvable SAC whose canonical asset matches the event's claimed asset is credited, with the correct from/to/amount/operationId, and its payment details persist on the transaction record (including SEP-31 fromAccount) after NotifyOnchainFundsReceivedHandler's own re-fetch.
  • A transfer event from a contract that is not a resolvable SAC, or whose canonical asset doesn't match the event's claimed asset, is not credited.
  • An i128 amount above Long.MAX_VALUE is credited with its exact value, not a wrapped one.
  • Observer status remains RUNNING after any of the above scenarios.
  • Horizon mode (HorizonPaymentObserver) is unaffected — it never calls the new getLedgerOperations overload.

Context

HackerOne #3791580

Testing

  • Unit: ./gradlew :core:test --tests "org.stellar.anchor.ledger.LedgerClientHelperTest"
  • Unit: ./gradlew :platform:test --tests "org.stellar.anchor.platform.observer.stellar.StellarRpcPaymentObserverTest"
  • Integration: ./gradlew :platform:test --tests "org.stellar.anchor.platform.observer.stellar.StellarRpcObserverIndexDomainTest"
  • Full suite: ./gradlew :core:test :platform:test
  • Manual: re-verified against this branch's current HEAD that Variant B is credited end-to-end, not merely made safe — an earlier review comment, written against an intermediate commit, correctly noted Variant B was "logged and skipped" at that point; that predates this PR's follow-up.

Documentation

N/A

Known limitations

N/A

* add `buildsubinvocationoperation` to create synthetic operations for contract sub-invocation transfers.

* update `processtransferevent` to credit sub-invocation transfers that lack a direct top-level operation.

* update `buildpaymenttransferevent` to conditionally map sac to asset.

* add tests to verify the crediting of sub-invocation transfers.

* add tests to verify skipping sub-invocation transfers with malformed asset data.
@amandagonsalves amandagonsalves changed the title fix(stellar-rpc-observer): credit sub-invocation transfers [ANCHOR-1242]: Contract sub invocation transfers are skipped Jul 7, 2026
@amandagonsalves
amandagonsalves marked this pull request as ready for review July 14, 2026 21:24
Copilot AI review requested due to automatic review settings July 14, 2026 21:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds support for crediting Soroban contract sub-invocation transfers missing from the compacted operation list.

Changes:

  • Matches operations by TOID and synthesizes missing invoke-host-function operations.
  • Preserves event-derived assets during processing.
  • Updates unit and scheduler tests for crediting and malformed assets.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
StellarRpcPaymentObserver.java Adds TOID lookup and synthetic transfer processing.
StellarRpcPaymentObserverTest.kt Tests synthetic crediting and malformed assets.
StellarRpcObserverIndexDomainTest.kt Updates scheduler and mixed-batch expectations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* delete test for sub-invocation transfer events

* update mixed batch test to remove sub-invocation event assertions
*   refactor amount field type from long to biginteger

*   update amount initialization to biginteger

*   refactor amount parsing from scv_i128 to biginteger
* delete `buildsubinvocationoperation` method

* update logic to remove synthetic sub-invocation operation creation

* refactor contract event handling when no direct operation is found
* delete test case for processing contract sub-invocation transfers without top-level operations

* update test name to more accurately reflect skipping behavior when no operation matches
* add a configurable sac resolver to stellar rpc

* add contract event parsing to transaction responses
* add ability to parse stellar smart contract transfer events.

* update ledger operation generation to include synthesized operations from contract events.

* add helper methods for contract event processing and transfer data extraction.
* add logic to synthesize invoke host function operations from sac transfer events

* update stellar rpc client to resolve sac contract ids to assets

* add tests for soroban contract event processing and validation

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comment thread core/src/main/java/org/stellar/anchor/ledger/LedgerClientHelper.java Outdated
* refactor ledger client helper to synthesize all verified transfers from a single operation's contract events, not just the first one

* update ledger client helper to prioritize contract event based transfers, replacing generic invoke host function operations when verified transfers are found

* fix stellar rpc payment observer to match contract event operations by content when multiple candidates with the same operation index exist
* add test for sub-invocation transfer from a verified sac

* add test for disambiguating multiple operations with same toid by event content

* add test for skipping events when no operation matches content for same toid

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment on lines +505 to +518
SCVal amountVal = entries[0].getVal();
SCVal memoVal = entries[1].getVal();
if (amountVal.getDiscriminant() != SCValType.SCV_I128) {
return null;
}
amount = Scv.fromInt128(amountVal);
eventMemo =
switch (memoVal.getDiscriminant()) {
case SCV_STRING -> memoVal.getStr().getSCString().toString();
case SCV_U64 -> memoVal.getU64().toString();
case SCV_BYTES ->
new String(Base64.getEncoder().encode(memoVal.getBytes().getSCBytes()));
default -> null;
};
Comment on lines 289 to 293
SCVal amountVal = entries[0].getVal();
SCVal memoVal = entries[1].getVal();
if (amountVal.getDiscriminant() != SCValType.SCV_I128) {
return builder.build();
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants