fix(node): propagate ContractNotPresent to RPC layer#1475
Merged
Conversation
d18d85c to
7ecf43c
Compare
LGLO
approved these changes
May 7, 2026
e0b6905 to
3af23ba
Compare
m2ux
approved these changes
May 8, 2026
m2ux
left a comment
Contributor
There was a problem hiding this comment.
Approving review (work-package review-mode pass).
The PR completes the audit fix correctly: it surfaces LedgerApiError::ContractNotPresent through the midnight_contractState RPC as a dedicated StateRpcError::ContractNotPresent variant. Layering is preserved (translation lives at the boundary; LedgerApiError is re-exported rather than introducing a new dep), the change is additive (strict superset of prior outcomes), and the e2e suite enforces all three audit invariants end-to-end (well-formed-but-undeployed, malformed-vs-not-present distinctness, historical-vs-current-block).
The two scope-adjacent changes (DEPLOY_SERIAL mutex and E2E_SKIP_DEPLOY_GATE env-var) are defensible — the new third deploy test surfaces the deploy-phase race they prevent, and bundling them avoids a forced ordering between this PR and a separate concurrency PR.
No blocking concerns. Three optional comments inline (C1–C3) for quality-of-life only; they should not delay merge.
PR #916 added LedgerApiError::ContractNotPresent in the runtime, but the midnight_contractState RPC still collapsed every LedgerApiError into the generic UnableToGetContractState — so callers could not distinguish "empty state" from "no such contract", which is why audit Issue AD was re-opened as #1166. Add StateRpcError::ContractNotPresent and route the matching ledger error to it. All other variants still fall through to UnableToGetContractState, so this is a strict superset of prior behaviour. Re-export LedgerApiError from pallet-midnight so the RPC crate can match on it without a new dependency. Lock in the audit invariants with three e2e tests in tests/e2e: - ContractNotPresent for an undeployed (but well-formed) address - BadContractAddress and ContractNotPresent stay distinct paths - same address yields ContractNotPresent at block 1 and the deployed state at the post-deploy best block The third test generates DEPLOY_TX dynamically via the toolkit's in-process prover against the live chain rather than using the static .mn fixture: that fixture's intent_ttl is baked in at generation time and expires once chain time advances past genesis.tblock + global_ttl (14 days by default), so static fixtures can't carry the test on long-lived or live envs. Serialize deploy tests behind a Mutex so concurrent submissions of the same DEPLOY_TX don't race in the txpool, and add an E2E_SKIP_DEPLOY_GATE escape hatch so the test can be run standalone via `cargo test <name>` without the gate semaphore deadlocking on unrun pre-deploy tests. Bump NUM_PRE_DEPLOY_TESTS / NUM_DEPLOY_TESTS 2→3 and the Earthfile test-threads 4→6 so the new deploy gate doesn't deadlock. Closes: #1166 Signed-off-by: Radosław Sporny <404@rspo.dev>
3af23ba to
878bb73
Compare
scottbuckel
pushed a commit
that referenced
this pull request
May 8, 2026
PR #916 added LedgerApiError::ContractNotPresent in the runtime, but the midnight_contractState RPC still collapsed every LedgerApiError into the generic UnableToGetContractState — so callers could not distinguish "empty state" from "no such contract", which is why audit Issue AD was re-opened as #1166. Add StateRpcError::ContractNotPresent and route the matching ledger error to it. All other variants still fall through to UnableToGetContractState, so this is a strict superset of prior behaviour. Re-export LedgerApiError from pallet-midnight so the RPC crate can match on it without a new dependency. Lock in the audit invariants with three e2e tests in tests/e2e: - ContractNotPresent for an undeployed (but well-formed) address - BadContractAddress and ContractNotPresent stay distinct paths - same address yields ContractNotPresent at block 1 and the deployed state at the post-deploy best block The third test generates DEPLOY_TX dynamically via the toolkit's in-process prover against the live chain rather than using the static .mn fixture: that fixture's intent_ttl is baked in at generation time and expires once chain time advances past genesis.tblock + global_ttl (14 days by default), so static fixtures can't carry the test on long-lived or live envs. Serialize deploy tests behind a Mutex so concurrent submissions of the same DEPLOY_TX don't race in the txpool, and add an E2E_SKIP_DEPLOY_GATE escape hatch so the test can be run standalone via `cargo test <name>` without the gate semaphore deadlocking on unrun pre-deploy tests. Bump NUM_PRE_DEPLOY_TESTS / NUM_DEPLOY_TESTS 2→3 and the Earthfile test-threads 4→6 so the new deploy gate doesn't deadlock. Closes: #1166 Signed-off-by: Radosław Sporny <404@rspo.dev> Signed-off-by: Scott Buckel <scott.buckel@shielded.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
PR #916 added LedgerApiError::ContractNotPresent in the runtime, but the
midnight_contractState RPC still collapsed every LedgerApiError into
the
generic UnableToGetContractState — so callers could not distinguish
"empty state" from "no such contract", which is why audit Issue AD was
re-opened as #1166.
Add StateRpcError::ContractNotPresent and route the matching ledger
error to it. All other variants still fall through to
UnableToGetContractState, so this is a strict superset of prior
behaviour. Re-export LedgerApiError from pallet-midnight so the RPC
crate can match on it without a new dependency.
Lock in the audit invariants with three e2e tests in tests/e2e:
Bump NUM_PRE_DEPLOY_TESTS / NUM_DEPLOY_TESTS 2→3 and the Earthfile
test-threads 4→6 so the new deploy gate doesn't deadlock.
Closes: #1166
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links