refactor(e2e): observe on-chain outcomes past the RPC wait window - #4108
refactor(e2e): observe on-chain outcomes past the RPC wait window#4108kdeforth-bot wants to merge 3 commits into
Conversation
A `sign` is a yield-resume promise that stays unresolved until the nodes answer or the protocol times the yield out ~200 blocks later. That is far longer than nearcore's RPC polling window (10s, across near-kit's retries), so a slow or unanswerable request failed at the transport layer and its on-chain outcome was never observed — reported as `contract call failed`, indistinguishable from a broken test. `NearKitCaller` now takes an optional timeout. Without one it behaves exactly as before. With one it submits, waits for inclusion so the transaction is known to the RPC, then polls `tx_status` to `ExecutedOptimistic` until the outcome exists or the deadline passes, retrying only while near-kit reports a retryable error. The transaction is never re-broadcast. `CallError::Deadline` names the pending transaction instead of claiming the call failed, so the next author is told to widen the window rather than left guessing at a flake. Both tests that worked around this now assert the on-chain timeout directly: the metric race in `timeout_metric` becomes an outcome assertion followed by the metric check, and `distinct_reconstruction_thresholds` drops its parallel submit-and-poll ladder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CallError::Deadline` carried `tx: Option<CryptoHash>` while its doc claimed the transaction was on chain. Only one of the two paths could promise that: the timed path waits for inclusion before polling, so it holds a real hash, while the untimed path never confirms inclusion and never learns one. The `Option` was carrying that distinction silently, and the two cases want different remedies — widen an existing deadline, or set one at all. They are now separate variants, so `Deadline` is unconditionally true to its name and `RpcGaveUp` keeps the underlying error as a `#[source]` instead of discarding it. `call_contract` becomes a dispatcher over `send` and `send_and_observe`, which puts each waiting strategy in one place rather than interleaving them behind a `let ... else`. Comments audited against docs/engineering-standards.md: dropped the ones restating a signature or naming a caller, and corrected the `NearKitCaller` doc, which still pointed at `Deadline` for a path that now yields `RpcGaveUp`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`NearKitCaller` kept its own copy of the account id purely to pass a sender to `tx_status`. `near_kit::Near` already exposes the signer's account, so the copy was duplicated state that nothing kept in sync — a caller built with a different signer would have polled under the wrong account. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| .await | ||
| .expect("sign request did not reach an on-chain outcome"); | ||
| assert!( | ||
| outcome.is_failure(), |
There was a problem hiding this comment.
So, this, IMO, is the biggest improvement of the approach in this and PR #4051 - we can now actually wait for the sign request to fail.
|
@SimonRastikian The main difference to #4051 is that here, we allow any contract method to be called with a custom timeout and still rely on the contract interface trait. |
| .await | ||
| { | ||
| Ok(outcome) => return Ok(outcome), | ||
| Err(e) if !is_retryable(&e) => return Err(CallError::Rpc(e)), |
There was a problem hiding this comment.
I took me a couple of minutes to understand this line having the definition of is_retriable. I was wondering mosly whether it's not possible to simplify the logic.
| } | ||
|
|
||
| /// Submitted once: retrying `send_tx` would re-broadcast, so only the polling repeats. | ||
| async fn send_and_observe( |
There was a problem hiding this comment.
I expect from the function to send something just like the previous one calling internally call.send().
I did not see where it sends. Could you guide me through what you are doing here please?
There was a problem hiding this comment.
This function submits the signed transaction to the chain and then waits for the transaction outcome.
|
@claude review this PR |
Pull request overviewThis replaces four ad-hoc workarounds for nearcore's RPC polling window with one knob on the e2e transport. Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
3 participants
Stacked on #4051 — the base branch is
3164-assert-onchain-timeout-in-recon-test, so thisdiff is just the one commit on top of Simon's work. Opened from a fork for access reasons;
the commit is authored by @kevindeforth.
Problem
A
signis a yield-resume promise that stays unresolved until the nodes answer or theprotocol times the yield out ~200 blocks later. That is far longer than nearcore's RPC
polling window (10s,
chain/jsonrpc/src/lib.rs:134, across near-kit's 3 retries — roughly45s in total).
So a slow or unanswerable request fails at the transport layer and its on-chain outcome is
never observed. The author sees
contract call failed: <rpc timeout>, indistinguishablefrom a broken test, and the real result (
RequestError::Timeout) cannot be asserted. Fourseparate workarounds for this had accumulated: the
select!metric races intimeout_metricanddistinct_reconstruction_thresholds, the submit-and-poll ladder addedby #4051, and the devnet loadtest's own poll loop.
Change
NearKitCallertakes an optional timeout.None— today's.send()path, untouched. near-kit's timeout is mapped toCallError::Deadlineso the diagnostic points at the pending request instead of claimingthe call failed.
Some(d)— submit, wait forIncludedso the transaction is known to the RPC, thenpoll
tx_statustoExecutedOptimisticuntil the outcome exists or the deadline passes,retrying only while near-kit reports a retryable error.
The transaction is never re-broadcast: retrying
send_txwould re-submit it, so inclusionis confirmed once and only observation is repeated.
OutputstaysFinalExecutionOutcome, so no call site signature moves. The knob reachesthe typed handle through one new generic method on the shared crate:
Effect on #4051
This removes
wait_tx_final,call_from_with_deposit_includedandsend_sign_request_included. The assertion added by #4051 survives unchanged — only themechanism beneath it does, and it now goes through the typed handle rather than around it.
timeout_metricloses itsselect!race too, and keeps its metric assertion, now sequencedafter the outcome rather than racing it.
Verification
cargo clippy -p e2e-tests -p near-mpc-contract-interface --all-targets --locked -- -D warningsand
cargo fmt --checkare clean. The rest of the e2e suite has not been run on this branch.🤖 Generated with Claude Code