feat: prove_transaction L2→L1 messages, ProofMode::None behavior change#915
feat: prove_transaction L2→L1 messages, ProofMode::None behavior change#915
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The pull request title clearly summarizes the main changes: adding L2→L1 messages to prove_transaction and modifying ProofMode::None behavior regarding proof_facts validation. |
| Description check | ✅ Passed | The pull request description includes both Usage related changes and Development related changes sections, covers all significant modifications, and includes a completed checklist confirming standard repository checks were performed. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/prove-transaction-l2-to-l1-messages
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
- Simulate transactions during proof generation to extract L2→L1 messages - Add l2_to_l1_messages field to ProveTransactionResponse - Add messages() and all_messages() accessors to FunctionInvocation - Change ProofMode::None to ignore proof but preserve and validate proof_facts - Fix flaky integration test: add block retention buffer and re-fetch gas prices - Add integration tests for L2→L1 message extraction and proof_facts validation - Update CLI help text and documentation
3alpha
force-pushed
the
feat/prove-transaction-l2-to-l1-messages
branch
from
eea7639 to
8379968
Compare
…ailure - Extract proof generation into separate `generate_proof` function - Add `compute_messages_hash` to bind L2→L1 messages into proof/proof_facts - Swap order in `prove_transaction`: execute tx first, then generate proof - Replace silent failure in `extract_l2_to_l1_messages` with `ProvingError::TransactionExecutionFailed` for all failure paths - Add `TransactionExecutionFailed` variant to `ProvingError` - Update proof_facts from 8 to 9 elements (includes messages_hash) - Update verify_proof to validate 9-element proof_facts - Add integration test for execution failure error propagation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
website/docs/proofs.md (2)
65-69:⚠️ Potential issue | 🟡 MinorRequest-shape section is outdated for
transactiontype.Line 68 still says only broadcasted
INVOKE v3, but this PR updatesstarknet_proveTransactionto accept a fullBroadcastedTransaction.📝 Proposed doc patch
-- `transaction` (a broadcasted `INVOKE v3` transaction payload) +- `transaction` (a full broadcasted transaction payload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/docs/proofs.md` around lines 65 - 69, The docs for starknet_proveTransaction are outdated: update the request-shape description for the `transaction` parameter to state it accepts a full `BroadcastedTransaction` (not just a broadcasted `INVOKE v3` payload). Edit the `starknet_proveTransaction` request-shape section (where `transaction` is documented) to reference the `BroadcastedTransaction` type and, if present, link or point to its definition so readers know the exact fields expected.
103-133:⚠️ Potential issue | 🟡 MinorUpdate proof-facts cardinality in docs to 9.
Line 131 still states 8 facts, and the example currently shows 8 entries, but current behavior expects 9.
📝 Proposed doc patch
"proof_facts": [ "0x...", "0x...", "0x...", "0x...", "0x...", "0x...", "0x...", - "0x..." + "0x...", + "0x..." ], @@ -`proof_facts` length is expected to be 8 in devnet mode. +`proof_facts` length is expected to be 9 in devnet mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/docs/proofs.md` around lines 103 - 133, Update the docs to reflect that `proof_facts` length is 9 in devnet mode: change the explanatory sentence that currently says "expected to be 8 in devnet mode" to "expected to be 9 in devnet mode" and update the example `proof_facts` array to contain nine entries (add one more "0x..." entry) so the sample JSON matches current behavior; ensure the surrounding text referencing `proof_facts` and the adjacent `l2_to_l1_messages` note remain consistent with this change.
🧹 Nitpick comments (3)
tests/integration/prove_transaction.rs (2)
677-679: Hardcoded nonce may cause test fragility.The deploy call uses a hardcoded
.nonce(Felt::ONE), assuming the declare transaction consumed nonce 0. If the account's initial nonce or declaration behavior changes, this test will fail.Consider fetching the current nonce dynamically:
♻️ Suggested improvement
+ let provider = account.provider(); + let nonce = provider + .get_nonce(BlockId::Tag(BlockTag::Latest), account.address()) + .await + .expect("Failed to get nonce"); contract_factory .deploy_v3(constructor_calldata, salt, false) - .nonce(Felt::ONE) + .nonce(nonce) .send() .await .expect("Failed to deploy messaging contract");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/prove_transaction.rs` around lines 677 - 679, The test is brittle because it hardcodes .nonce(Felt::ONE) for the deploy_v3 call; replace this with a dynamic read of the account's current nonce (e.g., call the account/client method that returns nonce or query the chain for the sender's nonce) and pass that value to .nonce(...) before .send(); locate the deploy invocation using deploy_v3(constructor_calldata, salt, false) and remove the Felt::ONE constant so the test computes and uses the up-to-date nonce at runtime.
871-876: Test removes proof fields but doesn't verify the specific error cause.The test manually removes
proofandproof_factsfrom the serialized transaction, then expects "Transaction execution failed". However, the error might be due to the invalid contract call (0xdeadbeef), not the missing proof fields. The test setup conflates two potential failure modes.Consider either:
- Using a valid contract call to isolate the "no proof" failure path, or
- Clarifying the test name/comment to indicate the primary failure is the invalid contract call
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/prove_transaction.rs` around lines 871 - 876, The test removes the "proof" and "proof_facts" fields from the serialized transaction (variable transaction derived from invoke_for_prove) but then may be asserting a generic "Transaction execution failed" which could instead be caused by the invalid contract call (0xdeadbeef); either make the contract call valid so the failure is isolated to the missing proof fields (adjust the invoke_for_prove setup to use a known-good contract/address) or change the test assertion and name/comment to explicitly expect the invalid-contract failure (or explicitly assert for a "missing proof" error string) so the test no longer conflates the two failure modes.crates/starknet-devnet-core/src/starknet/proofs.rs (1)
134-171: Consider handling reverted transactions more explicitly.The current implementation only returns messages for
ExecutionInvocation::Succeeded. For reverted transactions, it returns an error. This is likely the intended behavior since reverted transactions shouldn't produce valid L2→L1 messages, but consider whether the error message could be more informative.♻️ Optional: More informative error for reverted executions
if let ExecutionInvocation::Succeeded(func) = invoke_trace.execute_invocation { return Ok(func.all_messages()); } - return Err(ProvingError::TransactionExecutionFailed( - "invoke execution did not succeed".to_string(), - ) - .into()); + // Handle reverted case explicitly for better error messages + if let ExecutionInvocation::Reverted(reverted) = invoke_trace.execute_invocation { + return Err(ProvingError::TransactionExecutionFailed( + format!("invoke execution reverted: {}", reverted.revert_reason), + ) + .into()); + } + return Err(ProvingError::TransactionExecutionFailed( + "invoke execution did not succeed".to_string(), + ) + .into());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/starknet-devnet-core/src/starknet/proofs.rs` around lines 134 - 171, The function extract_l2_to_l1_messages currently treats any non-Succeeded invoke as a generic failure; update the handling of the invoke trace (the match on TransactionTrace::Invoke and the check of invoke_trace.execute_invocation) to explicitly match other ExecutionInvocation variants (e.g., Failed/Reverted) and return a more informative ProvingError that includes the execution status and any available failure reason or revert messages from invoke_trace (use invoke_trace.execute_invocation and any error/revert fields on invoke_trace or the ExecutionInvocation variant) so callers can see that the transaction reverted and why instead of a generic "did not succeed".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/prove_transaction.rs`:
- Around line 765-768: The assertion incorrectly expects the L2→L1 message
from_address to be the account; update the check to assert that
result.l2_to_l1_messages[0].from_address equals the messaging contract address
(contract_address) because send_message_to_l1 sets from_address to the calling
contract (the deployed messaging contract used by withdraw()). Replace the
assert comparing msg.from_address to account.address() with one comparing it to
contract_address and keep the other assertions unchanged.
---
Outside diff comments:
In `@website/docs/proofs.md`:
- Around line 65-69: The docs for starknet_proveTransaction are outdated: update
the request-shape description for the `transaction` parameter to state it
accepts a full `BroadcastedTransaction` (not just a broadcasted `INVOKE v3`
payload). Edit the `starknet_proveTransaction` request-shape section (where
`transaction` is documented) to reference the `BroadcastedTransaction` type and,
if present, link or point to its definition so readers know the exact fields
expected.
- Around line 103-133: Update the docs to reflect that `proof_facts` length is 9
in devnet mode: change the explanatory sentence that currently says "expected to
be 8 in devnet mode" to "expected to be 9 in devnet mode" and update the example
`proof_facts` array to contain nine entries (add one more "0x..." entry) so the
sample JSON matches current behavior; ensure the surrounding text referencing
`proof_facts` and the adjacent `l2_to_l1_messages` note remain consistent with
this change.
---
Nitpick comments:
In `@crates/starknet-devnet-core/src/starknet/proofs.rs`:
- Around line 134-171: The function extract_l2_to_l1_messages currently treats
any non-Succeeded invoke as a generic failure; update the handling of the invoke
trace (the match on TransactionTrace::Invoke and the check of
invoke_trace.execute_invocation) to explicitly match other ExecutionInvocation
variants (e.g., Failed/Reverted) and return a more informative ProvingError that
includes the execution status and any available failure reason or revert
messages from invoke_trace (use invoke_trace.execute_invocation and any
error/revert fields on invoke_trace or the ExecutionInvocation variant) so
callers can see that the transaction reverted and why instead of a generic "did
not succeed".
In `@tests/integration/prove_transaction.rs`:
- Around line 677-679: The test is brittle because it hardcodes
.nonce(Felt::ONE) for the deploy_v3 call; replace this with a dynamic read of
the account's current nonce (e.g., call the account/client method that returns
nonce or query the chain for the sender's nonce) and pass that value to
.nonce(...) before .send(); locate the deploy invocation using
deploy_v3(constructor_calldata, salt, false) and remove the Felt::ONE constant
so the test computes and uses the up-to-date nonce at runtime.
- Around line 871-876: The test removes the "proof" and "proof_facts" fields
from the serialized transaction (variable transaction derived from
invoke_for_prove) but then may be asserting a generic "Transaction execution
failed" which could instead be caused by the invalid contract call (0xdeadbeef);
either make the contract call valid so the failure is isolated to the missing
proof fields (adjust the invoke_for_prove setup to use a known-good
contract/address) or change the test assertion and name/comment to explicitly
expect the invalid-contract failure (or explicitly assert for a "missing proof"
error string) so the test no longer conflates the two failure modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2ad11f2-2589-44bb-bf93-c02c412c2eb3
📒 Files selected for processing (12)
crates/starknet-devnet-core/src/error.rscrates/starknet-devnet-core/src/starknet/add_invoke_transaction.rscrates/starknet-devnet-core/src/starknet/mod.rscrates/starknet-devnet-core/src/starknet/proofs.rscrates/starknet-devnet-server/src/api/endpoints.rscrates/starknet-devnet-server/src/api/models/mod.rscrates/starknet-devnet-types/src/rpc/transactions.rscrates/starknet-devnet/src/cli.rstests/integration/common/background_devnet.rstests/integration/get_transaction_by_hash.rstests/integration/prove_transaction.rswebsite/docs/proofs.md
1 participant
Usage related changes
prove_transactionresponse now includesl2_to_l1_messagesextracted via transaction simulationProofMode::Noneno longer stripsproof_facts— invalidproof_factswill causeValidationFailureprove_transactionendpoint now accepts fullBroadcastedTransaction(not just invoke)Development related changes
deploy_l2_msg_contracthelper and 3 new integration tests for L2→L1 message extraction and None mode proof_facts validationinvoke_in_proof_mode_nonetest (block retention buffer, stale gas prices, proof_facts now validated) now when it validatesproof_facts.Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
New Features
starknet_proveTransactionnow returns L2→L1 messages alongside proof and proof facts.Bug Fixes
Improvements
proof_mode=nonebehavior: validates proof facts while ignoring the proof field.