Skip to content

rpcv2: getEventsV2 integration tests - #1027

Merged
urvisavla merged 13 commits into
feature/full-historyfrom
getevents-v2-integration-tests
Sep 18, 2026
Merged

urvisavla merged 13 commits into
feature/full-historyfrom
getevents-v2-integration-tests

Conversation

@urvisavla

@urvisavla urvisavla commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

What

Fixes #1021

getEventsV2 integration tests: paging in both directions, filters, parity with getEvents, paging while the tip moves. Plus a tampered-cursor test on the JSON-RPC boundary.

Also fixes two harness setup flakes: waitForRPC waits for one consensus ledger after catch-up, because Core has no Soroban transaction queue until then and rejected the limits upgrade with txNOT_SUPPORTED; and the upgrade key is set only once the Core container has closed the ledger that holds it.

Known limitations

JSON topic filters are rejected; one subtest pins that.

🤖 Generated with Claude Code

urvisavla and others added 3 commits September 16, 2026 08:32
Five tests against a real Core in rpcv2/integrationtest: ascending drain
to the tip, closed range in both orders, filters over the wire, parity
with getEvents on the same node, and paging while the tip moves. The
package's TestMain defaults the daemon selector to rpcv2.

The tampered-cursor cases run in process in jsonrpc_test.go; the cursor
decoder itself is already fuzzed in the query package.

Needs the SDK client's GetEventsV2 method; the SDK pin bumps separately.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Points at stellar/go-stellar-sdk#6005's head until it merges; the pin then
moves to the merge commit.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ferral

#940 shipped the converter and is closed; the handler stays unwired on
purpose and json remains a spec option. Stop citing the issue as pending.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 17, 2026 02:06
@socket-security

socket-security Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatedgolang/​github.com/​stellar/​go-stellar-sdk@​v0.7.4-0.20260903222904-d8c8acf5f56c ⏵ v0.7.4-0.20260918190431-7092f4b101ef75 +1100100100100

View full report

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The empty-cursor subtest expects an error even though the API treats an empty cursor as an omitted cursor.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds end-to-end getEventsV2 coverage for pagination, filtering, v1 parity, moving tips, and malformed cursors.

Changes:

  • Adds rpcv2 integration-test setup and getEventsV2 scenarios.
  • Adds JSON-RPC cursor-tampering coverage.
  • Updates the SDK dependency and JSON-input limitation wording.
File summaries
File Description
go.mod Updates the Stellar SDK dependency.
go.sum Updates SDK checksums.
cmd/stellar-rpc/internal/rpcv2/jsonrpc_test.go Tests malformed cursors at the JSON-RPC boundary.
cmd/stellar-rpc/internal/rpcv2/integrationtest/main_test.go Forces the rpcv2 integration-test daemon.
cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go Adds comprehensive getEventsV2 integration tests.
cmd/stellar-rpc/internal/rpcv2/eventsapi/get_events_v2.go Clarifies unsupported JSON topic input.
cmd/stellar-rpc/internal/rpcv2/eventsapi/conversions_test.go Updates the JSON-topic rejection test name.
Review details
  • Files reviewed: 6/7 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cmd/stellar-rpc/internal/rpcv2/jsonrpc_test.go
…es up

Core creates its Soroban transaction queue only when a ledger closes
through consensus (HerderImpl::lastClosedLedgerIncreased), never during
catch-up. A captive core that has just caught up answers every Soroban
transaction with txNOT_SUPPORTED until its first consensus ledger closes.
The harness submitted the limits upgrade inside that window, about one
second wide, which is the setup flake seen as ERROR/txNOT_SUPPORTED at
test.go:1071. Waiting for one more committed ledger after the daemon is
caught up closes the window.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 16:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The cursor test incorrectly documents unsigned cursors as node-bound and tamper-protected.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/stellar-rpc/internal/rpcv2/jsonrpc_test.go:140

  • The node-provenance premise is incorrect: getEventsV2 cursors are unsigned, self-contained state, and decoding only checks well-formedness (query/event_cursor.go:3-7; eventsapi/get_events_v2.go:156-158). A structurally valid cursor built by a client or another node can be accepted, so this test covers malformed encodings rather than tamper detection. Please rename it and narrow the comment to avoid promising authentication that the API does not provide.
  • Files reviewed: 7/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Cursors are unsigned, self-contained state; decoding checks
well-formedness only. Rename the test and its comment to say so, and drop
the empty-cursor case, which the handler reads as an omitted cursor and
rejects for the missing minLedger, not as a malformed token.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 17:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new unconditional ledger wait causes finite synthetic-ingestion runs to time out after their final ledger.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 7/8 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread cmd/stellar-rpc/internal/integrationtest/infrastructure/test.go Outdated
The synthetic load test has no Core and a finite stream that may already
be done when health first passes; the delayed-daemon mode is behind on
purpose. Both would wait out the deadline for a ledger that never comes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 18:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The test coverage and harness stabilization are coherent, scoped, and have no identified correctness issues.

Review details
  • Files reviewed: 7/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@tamirms tamirms 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.

The test set covers what the unit tests cannot reach, and I like that the paged cases assert against the unpaged read rather than against a hand-written expectation. That is the shape that catches a real pager bug.

A few things I checked before reading: go vet is clean on every changed package, go mod tidy produces no diff (so the prometheus/client_model flip to // indirect is right, nothing imports it), and the branch still merges cleanly on the base now that #978 has landed there.

I also worked through the txNOT_SUPPORTED diagnosis and it holds up. daemon_rpcv2.go points ingestion.core_url at the daemon's own captive core, so rpcv2 submits there rather than to the Core container the way rpcv1 does, and because caughtUpWithCore compares against the container's tip read after the health reading, one more ingested ledger does imply the captive core externalized a ledger through consensus. Good find.

Five comments below. The first is the only one I would not leave as it is, since it carries a real per-run flake that a green run does not rule out. The rest are smaller.

Comment thread cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go Outdated
Comment thread cmd/stellar-rpc/internal/rpcv2/jsonrpc_test.go Outdated
Comment thread cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go Outdated
Comment thread cmd/stellar-rpc/internal/integrationtest/infrastructure/test.go Outdated
Comment thread cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go Outdated
- MatchesV1 reads the tip twice across a moving Core, so compare the two
  latestLedger readings by order, not equality.
- Fold the extra-ledger wait into the waitForRPC loop so it keeps the
  daemon exit watch and the per-poll log.
- The malformed-cursor cases all decode through DecodeEventCursor, so pin
  cursor_malformed exactly.
- Add Test.CreateEventsContract beside the other create helpers and use it
  instead of a copied deploy block.
- require, not assert, on the idle tip cursor so a failure stops at its
  cause instead of the next round.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 18, 2026 18:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new event assertions misclassify fee events, and the paging helper can panic on valid empty pages.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:77

  • HAS_MORE pages are allowed to contain no events when the scan-ledger budget is exhausted, so after an empty first page the next iteration indexes all[-1] and panics. Compare against the accumulated tail only when it exists, and append each event as it is checked; this also validates ordering within a multi-event page.
    cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:100
  • CAP-67 marks the fee-charged and fee-refund events as system; only the invoked contract's emitted event is contract. This helper therefore fails on two of the three expected events per invocation, preventing most new integration tests from reaching their paging assertions. Assert contract only when e.ContractID == fx.contractID and system otherwise. The same incorrect assumption appears in the OR-filter subtest at line 276: change its second clause to EventTypeSystem so the two clauses cover the intended union.
  • Files reviewed: 7/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 18, 2026 18:38
… a page

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The SDK pin is stale, and older protocol configurations need a guard before deploying the test contract.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

cmd/stellar-rpc/internal/integrationtest/infrastructure/test.go:603

  • needOneMore is also true when ApplyLimits is SkipLimitsUpgrade, even though upgradeLimits then returns without submitting the Soroban setup transaction. This adds an unnecessary consensus-ledger delay to tests that intentionally skip that setup. Gate the extra wait on the configured limit file being non-empty.
    cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:39
  • This helper deploys with CreateContractV2, which is unavailable when the integration harness runs against a Core release whose maximum protocol is below 23. The existing TestContractEvents integration test skips that configuration; add the same guard here so this entire new package skips instead of failing during fixture setup.
    go.mod:29
  • The PR description says this dependency tracks stellar/go-stellar-sdk#6005's head until merge, but this pseudo-version still points to the PR's first commit (c033eaa). The current head is 6c2b520, and the intervening 2f2e7b1 commit fixes explicit empty Filters slices being omitted and serialized as unfiltered requests. Repin go.mod and go.sum to the current head (or to the merged release).
  • Files reviewed: 7/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 18, 2026 18:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Event-type expectations currently break the new tests, and the SDK pin is behind the stated upstream head.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:276

  • Both arms of this OR filter match only the fixture contract events: the first by contract ID and the second by EventTypeContract. The fee and refund records are system events from a different contract, so the result is wantIDs, not the full unfiltered page. The current expectation therefore fails once the fixture reaches this subtest.
    go.mod:29
  • The PR description says this dependency is pinned to the head of stellar/go-stellar-sdk#6005, but c033eaa5e2ff is only that PR's first commit. Its current head is 6c2b52080b31, which includes the filters,omitzero fix that preserves an explicit empty filter list; without it, the SDK omits Filters: [] and silently turns that invalid request into an unfiltered query. Update both go.mod and go.sum to the intended PR head before merging.
  • Files reviewed: 7/8 changed files
  • Comments generated: 1
  • Review effort level: Balanced

…current head

Picks up the Filters omitzero tag; moves again to the merge commit when
the PR lands.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 18, 2026 19:03
…ar-sdk#6005 merged)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Integration assertions and protocol compatibility need correction, while the harness adds avoidable startup delays.

Review details

Suppressed comments (6)

Previously missed (3) — in code that hasn't changed since the last review.

cmd/stellar-rpc/internal/integrationtest/infrastructure/test.go:603

  • needOneMore is enabled for every live-Core test, including tests that set ApplyLimits: SkipLimitsUpgrade(). Those tests never call upgradeLimitsWithFile, so the queue-readiness flake cannot affect them and this adds an unnecessary consensus-ledger delay to many non-Soroban tests. Gate the extra wait on the non-empty limit file used by upgradeLimits.
    cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:40
  • This fixture assumes protocol 23's unified transaction events: each invocation must produce the fee-charged and fee-refund events used by eventsPerInvoke and the paging assertions. The shared contract-events integration test skips below protocol 23 (cmd/stellar-rpc/internal/integrationtest/transaction_test.go:303-305), but these tests do not, so a protocol-22 run fails rather than skips. Add the same guard before creating the fixture.
    cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:274
  • The contract-ID filter and type=contract filter both match only the fixture contract's events. They exclude the fee-charged and fee-refund system events, so the result cannot equal unfiltered.Events and the test fails instead of verifying OR semantics. Use type=system for the second, disjoint filter.

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:103

  • The fee-charged and fee-refund events emitted by Core are system events, while only the contract's own event is contract. This assertion therefore fails for every fixture page before the new integration tests can exercise paging; assert the contract type only for fx.contractID and expect EventTypeSystem for the other two events.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)
		if e.ContractID == fx.contractID {
			fromContract++
		}

cmd/stellar-rpc/internal/rpcv2/eventsapi/get_events_v2.go:356

  • xdr2json is the response-side XDR-to-JSON converter used by eventInfoV2; it cannot produce the canonical XDR bytes required for a JSON input topic. Name a JSON-to-XDR converter instead so this comment does not misdirect the eventual implementation.
// bytes; xdr2json can produce them, but the handler is not wired to it.

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:99

  • This fixture explicitly includes fee and refund events, which are system events, so asserting that every returned event is contract makes every caller of requireFixtureEvents fail. Assert contract only for the fixture contract ID and system for the other events.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
  • Files reviewed: 7/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 18, 2026 19:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Protocol compatibility, filter assertions, and RPC-to-Core catch-up handling have unresolved correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

cmd/stellar-rpc/internal/integrationtest/infrastructure/test.go:615

  • After caughtUp is recorded, this branch no longer checks RPC against Core. If Core advances by multiple ledgers while RPC ingests only one, LatestLedger > caughtUp can be true while RPC is still behind Core, reintroducing the stale captive-Core state that caughtUpWithCore prevents. Require both the extra ledger and current catch-up before returning.
    cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:125
  • Add the protocol-23 guard before creating the harness. These tests deploy with CreateContractV2, and existing event integration tests skip older Core versions before calling infrastructure.NewTest (cmd/stellar-rpc/internal/integrationtest/transaction_test.go:303-308). Without an early guard, pre-protocol-23 runs fail during setup instead of skipping. Factor the guard into a shared constructor and use it at all five test entry points.

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:99

  • The fee debit and refund events in this three-event fixture are system events, not contract events; transaction-level event fixtures in cmd/stellar-rpc/internal/rpcv1/sqlitedb/event_test.go:32-43 use ContractEventTypeSystem. Asserting contract for every event makes every unfiltered fixture check fail before the paging and filter assertions run. Check the type based on whether the event belongs to fx.contractID and expect system for the other two.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)

cmd/stellar-rpc/internal/integrationtest/infrastructure/test.go:603

  • This condition is true for every normal Core-backed test, including configurations that explicitly use SkipLimitsUpgrade(). Those tests are documented to avoid Soroban transactions and upgradeLimits() returns immediately for an empty limit file, so this adds an unnecessary consensus-ledger wait to all of them; include the limit-file setting in needOneMore.
	needOneMore := i.coreClient != nil && i.delayDaemonForLedgerN == 0

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:103

  • The three events per invocation are not all contract events: the fee-charged and refund transaction-level events are indexed as EventTypeSystem, while only the contract's emitted event is contract. This assertion therefore fails for every fixture invocation; restrict the contract assertion to fx.contractID and assert system for the other two events.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)
		if e.ContractID == fx.contractID {
			fromContract++
		}
  • Files reviewed: 7/8 changed files
  • Comments generated: 1
  • Review effort level: Balanced

…re setting the key

The daemon reports inclusion from its captive core. The upgrade key is
then resolved by the Core container against its own last closed ledger,
which can trail the captive core under CI load, and Core answers "Error
setting configUpgradeSet". Seen twice on the P27 rpcv2 leg.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 18, 2026 20:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The tests need a pre-harness protocol-23 guard and corrections to their event-shape assertions.

Review details

Suppressed comments (10)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:125

  • Every test in this file creates and invokes the protocol-23 events contract, but none skips older Core versions before starting the harness. In environments where GetCoreMaxSupportedProtocol() < 23, setup proceeds into unsupported Soroban operations instead of skipping, unlike TestContractEvents in cmd/stellar-rpc/internal/integrationtest/transaction_test.go:303-306. Add that guard before infrastructure.NewTest in each test, or use a shared setup helper that performs the guard first.

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:275

  • The contract-ID filter already selects the sole contract event from each invocation, so OR-ing it with EventTypeContract does not add the fee/refund events and cannot equal the unfiltered result. Use the system event type for the second clause to exercise the intended union.
			{ContractID: fx.contractID},
			{EventType: protocol.EventTypeContract},
		}

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:103

  • The fee-charged and fee-refund records are transaction-level system events, so this assertion fails for two of every three events that the helper requires. Assert contract only for the fixture contract's event and system for the other two.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)
		if e.ContractID == fx.contractID {
			fromContract++
		}

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:100

  • These assertions make every fixture query fail: the CAP-67 fee and refund entries are system events with no ContractID; only the operation event is EventTypeContract and carries fx.contractID (the existing transaction integration test expects one contract event plus two transaction events). Classify the event inside the fx.contractID branch and assert EventTypeSystem/empty ContractID for the other two.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:274

  • The fixture's fee/refund events are system events, so OR-ing the fixture contract ID with EventTypeContract returns only the contract event(s), not unfiltered.Events. Use EventTypeSystem for the second clause (or compare with the contract-only result) so this actually tests an OR that covers all three events.
			{EventType: protocol.EventTypeContract},

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:103

  • The two fee-related events in this fixture are system events with no ContractId, while only the contract's emitted event is type contract. Requiring every event to have a contract ID and contract type makes every test using this helper fail on the first fee/refund event; classify the non-fixture events as system events instead.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)
		if e.ContractID == fx.contractID {
			fromContract++
		}

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:274

  • Both filters here select only the contract event: the contract-ID filter matches the emitted event, and the contract type filter excludes the fee/refund system events. Therefore this result cannot equal unfiltered.Events, which contains all three events per invocation; use a system-type filter as the second OR branch (or compare against the contract-only set).
			{EventType: protocol.EventTypeContract},

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:100

  • Each invocation yields two transaction-level fee/refund events of type system plus one contract event; the existing integration test asserts this 1+2 shape. Requiring every event to be contract with a non-empty ContractID makes every fixture validation fail before the paging and filter assertions. Check the ID only for contract events and require an empty ID for system events.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:274

  • The second OR clause is EventTypeContract, so both clauses select only the fixture's contract event; it cannot equal unfiltered.Events, which also contains the fee/refund system events. Use EventTypeSystem here (or compare with contractOnly(unfiltered)) so the union covers all three events per invocation.
			{EventType: protocol.EventTypeContract},

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:88

  • This comment misclassifies the fee/refund events as belonging to a native asset contract. They are transaction-level system events with no ContractID; only the fixture's emitted event is a contract event. Update the comment so it matches the response shape asserted below.
// The fee and refund events belong to the native asset contract, so only the
// contract id tells the fixture's own event apart.
  • Files reviewed: 7/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Same guard the shared contract-events test uses, applied before the
harness starts a Core.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 18, 2026 20:23
@urvisavla

Copy link
Copy Markdown
Contributor Author

All five addressed in 9007058.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new assertions misclassify fee and refund system events, causing multiple integration tests to fail.

Review details

Suppressed comments (7)

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:112

  • The fee and refund records are transaction-level system events; only the event emitted by the fixture is a contract event. This unconditional assertion therefore fails for two of the three events from every invocation. Branch on the fixture contract ID and assert system for the native-asset fee events.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)
		if e.ContractID == fx.contractID {
			fromContract++
		}

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:285

  • Both OR clauses match only the fixture's contract event: the first by contract ID and the second by contract type. The fee/refund events are system events with a different contract ID, so expecting the entire unfiltered page makes this subtest fail; the expected IDs should be wantIDs.
		assert.Equal(t, eventIDs(unfiltered.Events), eventIDs(call(t, req).Events))

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:109

  • These assertions do not match the CAP-67 events emitted by this fixture: the fee and refund transaction events are system events with no ContractID; only the operation event is contract and carries fx.contractID. As written, every invocation fails this helper at these assertions. Branch on ContractID/event type so only the fixture event is counted as the contract event.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:283

  • With the actual event types, both of these filters select only the contract event: the contract-ID clause matches that event, and EventTypeContract excludes the system fee/refund events. The assertion therefore cannot test OR semantics and returns only one event per invocation instead of unfiltered.Events. Use a system-event filter for the second clause so the two clauses select disjoint sets.
			{EventType: protocol.EventTypeContract},

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:112

  • The fee and refund rows are system events; only the contract's emitted row is a contract event. Since this assertion runs for all three events, every fixture call fails before the paging/filter/parity checks; assert the type based on whether the event belongs to fx.contractID instead of requiring every row to be a contract event.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)
		if e.ContractID == fx.contractID {
			fromContract++
		}

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:112

  • This helper treats every event as a contract event, but each invocation also produces the fee and refund events, which are indexed as system events (the existing integration test expects one contract event and two transaction-level events). As written, every test calling requireFixtureEvents will fail on those two events; classify the fixture event by ContractID and assert EventTypeSystem for the remaining events instead.
		assert.Equal(t, protocol.EventTypeContract, e.EventType)
		assert.NotEmpty(t, e.ContractID)
		if e.ContractID == fx.contractID {
			fromContract++
		}

cmd/stellar-rpc/internal/rpcv2/integrationtest/get_events_v2_test.go:285

  • The OR query uses a contract-ID clause and EventTypeContract, so it can only return the contract's emitted events; the fee and refund events are EventTypeSystem and are excluded. Therefore this assertion against the unfiltered three-events-per-invocation result will fail. Use EventTypeSystem for the second clause (or assert wantIDs if this subtest is intended to cover contract-event union semantics).
	t.Run("filters are OR-ed", func(t *testing.T) {
		req := rng
		req.Filters = []protocol.EventFilterV2{
			{ContractID: fx.contractID},
			{EventType: protocol.EventTypeContract},
		}
		assert.Equal(t, eventIDs(unfiltered.Events), eventIDs(call(t, req).Events))
  • Files reviewed: 7/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@urvisavla
urvisavla merged commit 7aaed5d into feature/full-history Sep 18, 2026
17 checks passed
@urvisavla
urvisavla deleted the getevents-v2-integration-tests branch September 18, 2026 20:32
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.

3 participants