refactor(rpc/v10): rework AddTransaction, decouple EstimateMessageFee, tighten validation

[thiagodeev]

TODO: apply the changes for other rpc pkgs as well

Validation

• Enabled struct-level validation via validator.WithRequiredStructEnabled. Lets us drop pointer indirection on ResourceBoundsMap / ResourceBounds (value types now) and rely on validate tags directly on the fields.
• Removed redundant nil/shape checks in the adapters — JSON-schema + struct validation already cover them.

EstimateMessageFee decoupled from EstimateFee

• Previously, an L1 handler transaction was wrapped inside a BroadcastedTransaction so it could reuse the EstimateFee plumbing. That workaround is gone — EstimateMessageFee now has its own direct path with no BroadcastedTransaction round-trip.

AddTransaction refactor

• Multiple speed improvements (benchmarks below)
• Split the monolithic AdaptBroadcastedTransaction into smaller, single-purpose functions.
• Cleaner and faster feeder gateway payload generation.
• Introduced a dedicated ContractClass type for the contract_class field instead of json.RawMessage. Two wins:
  • the field is now actually validated (previously it wasn't);
  • downstream code works with a typed value instead of raw JSON.
• New direct adapter BroadcastedTransaction → core.Transaction, removing the intermediate hop through starknet.Transaction.

Bug fix (v9; v10 unaffected)

• For DECLARE transactions, pushToFeederGateway was mutating the original tx.ContractClass. By the time the tx reached receivedTransactionFeed, the mutated state caused the send to fail and the tx was never published.
• Fixed in v9 to unblock benchmarks. v8 still carries the bug — left as-is intentionally: the full v10 changes will be ported to v8/v9 once the team approves the changes.

Benchmark result:

Config: count=5, benchtime=3s, Intel i5-11400H @ 2.70GHz, Go 1.26.

Benchmark	old sec/op	new sec/op	Speedup	old B/op	new B/op	old allocs/op	new allocs/op
Mempool/declare (stub)	59.7 ms	22.7 ms	2.63x	2.16 MiB	235.6 KiB	33,505	5,508
Mempool/invoke	187.0 us	92.6 us	2.02x	2.59 KiB	1.06 KiB	51	21
Mempool/deploy_account	401.7 us	241.5 us	1.66x	2.62 KiB	1.08 KiB	50	21
Gateway/declare (stub)	43.8 ms	28.4 ms	1.54x	3.95 MiB	1.22 MiB	28,142	11,029
Gateway/invoke	102.1 us	103.1 us	~1x	3.99 KiB	3.72 KiB	73	65
Gateway/deploy_account	170.3 us	166.1 us	~1x	3.75 KiB	3.43 KiB	69	60
Declare_RealCompiler_Mempool	235.1 ms	110.4 ms	2.13x	8.20 MiB	3.17 MiB	70,935	24,223
Declare_RealCompiler_Gateway	134.6 ms	113.4 ms	1.19x	6.98 MiB	4.70 MiB	46,857	29,757

• Declare on the mempool path is 2.1-2.6x faster: new compiles Sierra once instead of twice and reuses the class hash for the feed broadcast.
• Gateway/declare gains come from the streaming ContractClassToGatewayPayload (pooled gzip.Writer, gzip.BestSpeed, streamed json -> gzip -> base64 pipeline).
• Non-declare gateway cases are unchanged: no ContractClass work involved.
• Declare_RealCompiler_Gateway gain is modest because FFI Sierra->CASM compile dominates wall-clock time.
• All new sub-benchmarks are equal or better than old. No regressions.

[codecov]

Codecov Report

❌ Patch coverage is 68.43854% with 95 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.92%. Comparing base (1f66f48) to head (e8beb33).

Files with missing lines	Patch %	Lines
rpc/v10/transaction.go	51.68%	36 Missing and 7 partials ⚠️
rpc/v10/adapt_transaction.go	81.11%	24 Missing and 3 partials ⚠️
rpc/v10/simulation.go	60.00%	11 Missing and 1 partial ⚠️
rpc/v10/validator.go	42.85%	3 Missing and 5 partials ⚠️
rpc/v10/transaction_types.go	0.00%	2 Missing and 1 partial ⚠️
rpc/v10/estimate_fee.go	89.47%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3582      +/-   ##
==========================================
- Coverage   75.01%   74.92%   -0.09%     
==========================================
  Files         433      433              
  Lines       38606    38815     +209     
==========================================
+ Hits        28960    29082     +122     
- Misses       7643     7728      +85     
- Partials     2003     2005       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Claude finished @thiagodeev's task in 5m 15s —— View job

---

Claude Code Review

I reviewed the v10 AddTransaction rework, EstimateMessageFee decoupling, and validation changes against origin/main. Tasks:

• Read PR diff and changed files
• Review adapt_transaction.go (new adapters, ContractClass type)
• Review transaction.go (AddTransaction refactor, feeder gateway)
• Review estimate_fee.go / simulation.go (EstimateMessageFee decoupling)
• Review validator.go / transaction_types.go (validation changes)
• Post inline comments + summary

---

Summary

The refactor is well-structured and the performance wins are real. I found one blocker (a typed-nil interface bug that panics the builder), plus a few minor notes.

🔴 Blocker

• rpc/v10/transaction.go:155 — typed-nil DeclaredClass panics block building. adaptAndCompileBroadcastedTxToCore returns userClass as a concrete *core.SierraClass, which is nil for INVOKE / DEPLOY_ACCOUNT. Assigning it straight into the DeclaredClass core.ClassDefinition interface yields a non-nil interface wrapping a nil pointer. In builder/executor.go:73 the txn.DeclaredClass != nil check then passes for every non-declare tx, and vm/class.go marshalClassInfo matches case *core.SierraClass: and dereferences c.Compiled → nil pointer panic when sequencing a block. The estimate/simulate path (simulation.go:338) already guards this with if declaredClass != nil; the mempool path needs the same guard. Suggested fix posted inline. (Fix this →)

🟡 Minor / nits

• rpc/v10/transaction.go ContractClassToGatewayPayload — json.Encoder.Encode appends a trailing \n inside the gzipped sierra_program, whereas the old json.Marshal did not. Harmless for the gateway's JSON parse, but worth being aware of since the produced bytes differ from the previous implementation.
• rpc/v10/simulation.go:293 checkTxHasResourceBounds — now redundant with the new validate:"required" on Transaction.ResourceBounds (jsonrpc validates the param before the handler runs). Same for the SenderAddress check. Not harmful, just dead-ish belt-and-suspenders.
• rpc/v10/estimate_fee.go EstimateMessageFee — opens state once for the ContractClassHash check and simulateTransactions opens it again. Both are closed correctly (no leak); pre-existing pattern, just slightly wasteful.

✅ Looks good

• The pointer-deref-heavy adaptBroadcasted*ToCore adapters are safe: every dereferenced field has a matching required / required_if validate tag, and jsonrpc validates params before the handler (jsonrpc/server.go:671).
• validateFeltMaxBits correctly receives the felt's string form via the existing RegisterCustomTypeFunc, and big.Int.SetString(s, 0) handles the 0x prefix — the =64 / =128 bounds line up with the later .Uint64() truncation in adaptResourceBoundsToCore.
• The v9 pushToFeederGateway copy correctly stops mutating the caller's ContractClass (dropping Proof is fine — v9's AddTxGatewayPayload doesn't carry it).
• The CodeQL "useless assignment to paidFeesOnL1" finding in simulation.go appears already resolved — paidFeesOnL1 no longer exists in that file.
  · branch thiagodeev/refactor-add-txn

[github-actions]

Claude finished @thiagodeev's task in 4m 54s —— View job

---

Claude Code Review

Re-reviewed after the latest pushes (typed-nil fix, EstimateMessageFee state reuse, validation rework) against origin/main. Package builds clean.

• Read PR diff against origin/main
• Verify the previously-reported typed-nil DeclaredClass blocker is resolved
• Review adapt_transaction.go / transaction.go
• Review estimate_fee.go / simulation.go
• Review validator.go / transaction_types.go
• Summary

---

Summary

The earlier blocker is fixed ✅ and I found no new blockers. Remaining items are minor/nits.

✅ Blocker resolved — typed-nil DeclaredClass

adaptAndCompileBroadcastedTxToCore now declares var sierraClass core.ClassDefinition (interface) and returns it as core.ClassDefinition instead of a concrete *core.SierraClass (commit 6d029dd9). For INVOKE / DEPLOY_ACCOUNT it stays a true nil interface, so addToMempool's DeclaredClass: userClass (transaction.go:155) is genuinely nil and the builder/executor.go / vm/class.go *core.SierraClass path is no longer reached. Confirmed the prepareTransactions path (simulation.go:341) guards the same way with if declaredClass != nil. Good.

✅ Spot-checks that passed

• EstimateMessageFee decoupling — opens state once and threads it into simulateTransactions (no double state open; commit 40279206). The L1-handler tx is built directly as core.L1HandlerTransaction, which is correct since AdaptBroadcastedTransactionToCore intentionally returns invalid transaction type for L1_HANDLER — and no other caller routes L1-handler through prepareTransactions.
• Pointer-deref adapters (adaptBroadcasted{Invoke,Declare,DeployAccount}ToCore) — every dereferenced field (*Signature, *PaymasterData, *AccountDeploymentData, *NonceDAMode, *FeeDAMode, *ConstructorCallData, *CallData) has a matching required / required_if tag, and jsonrpc validates params before the handler runs. CompiledClassHash is assigned as a pointer (nil-safe).
• validateFeltMaxBits + nil felts — verified the go-playground validator returns at the reflect.Ptr nil branch before invoking the registered custom-type func, so a missing max_amount ({"l1_gas": {}}) yields a required validation error, not a nil-deref panic from (*felt.Felt).String(). SetString(s, 0) handles the 0x prefix; the =64/=128 bounds line up with the later .Uint64() truncation in adaptResourceBoundsToCore.
• The CodeQL "useless assignment to paidFeesOnL1" finding is resolved — the symbol no longer exists in simulation.go.

🟡 Minor / nits

• rpc/v10/simulation.go:296 checkTxHasResourceBounds / checkTxHasSenderAddress — now largely redundant with Transaction.ResourceBounds carrying validate:"required" and SenderAddress carrying required_if, both validated by jsonrpc before the handler. Harmless belt-and-suspenders.
• Tip truncation — adaptResourceBoundsToCore got felt_max_bits=64 guards, but tx.Tip.Uint64() (e.g. adapt_transaction.go invoke/declare/deploy adapters) has no equivalent bound, so a > 2^64 tip silently truncates. Matches the pre-existing core.Tip uint64 semantics, so not a regression — just an inconsistency with the new resource-bounds validation.
• ContractClassToGatewayPayload — json.Encoder.Encode appends a trailing \n inside the gzipped sierra_program that the old json.Marshal didn't. Harmless for the gateway's parse, just a byte-for-byte difference from before.
• genesis/genesis.go:347 — the added // bug: TxnDeployAccount does not contain SenderAddress field, so this is nil comment documents a latent issue (ContractAddress set from a nil SenderAddress) without fixing it. Fine as a TODO marker, but worth a tracking issue so it isn't lost.
• Gateway feed re-adapt — in AddTransaction's gateway branch, the received-feed path re-runs adaptAndCompileBroadcastedTxToCore, which recompiles the Sierra class for DECLARE after the gateway already accepted the tx. Unavoidable to produce a core.Transaction for the feed, but it adds FFI-compile latency to the submission call; consider doing it off the hot path if it ever shows up in profiles.

Nice work — the refactor reads cleanly and the perf wins are well-supported.
· branch thiagodeev/refactor-add-txn

[github-actions]

Claude finished @thiagodeev's task in 1m 38s —— View job

---

Claude Code Review

Re-reviewed after the latest push (4d90e8ded — simplify L1DataGas resource bounds init) on top of the merge of main. go build ./rpc/v10/... passes clean.

• Read PR diff against origin/main
• Review the new L1DataGas init in adapt_transaction.go
• Re-check transaction.go / validator.go / transaction_types.go
• Confirm previously-reported blocker stays resolved

---

Summary

No new blockers. The new commit is a clean, behavior-preserving refactor and the previously-fixed typed-nil DeclaredClass blocker is still resolved. Remaining items are the same minor nits flagged before.

✅ New change is correct — adaptCoreResourceBounds (adapt_transaction.go:73-99)

Hoisting the {&felt.Zero, &felt.Zero} default out of the if/else is equivalent to the prior logic: when core.ResourceL1DataGas is absent the zero default stands, otherwise it's overwritten. Matches the suggestion from the earlier review thread. The double map index rb[core.ResourceL1DataGas] is harmless (just a re-lookup, not a hot path here).

✅ Still-good from prior reviews

• Typed-nil DeclaredClass: adaptAndCompileBroadcastedTxToCore returns core.ClassDefinition (interface), genuinely nil for INVOKE / DEPLOY_ACCOUNT, so addToMempool (transaction.go:157-160) passes a true-nil DeclaredClass — no panic in builder/executor.go / vm/class.go.
• EstimateMessageFee decoupling threads a single opened state into simulateTransactions (no double-open).
• Pointer-deref adapters (adaptBroadcasted{Invoke,Declare,DeployAccount}ToCore) each have a matching required / required_if tag, validated by jsonrpc before the handler runs.
• validateFeltMaxBits + nil felt: validator short-circuits at the nil-pointer branch before the custom-type func, so a missing max_amount yields a required error, not a deref panic. SetString(s, 0) handles the 0x prefix; =64/=128 line up with the later .Uint64() truncation.

🟡 Standing nits (unchanged, non-blocking)

• Tip truncation — resource bounds now get felt_max_bits guards, but tx.Tip.Uint64() in the adapters has no equivalent bound, so a > 2^64 tip silently truncates. Matches pre-existing core.Tip uint64 semantics — just an inconsistency with the new validation.
• ResourceBoundsMap.MarshalJSON (transaction_types.go:243) — pointer-receiver MarshalJSON mutates r.L1DataGas to fill defaults during marshalling. Pre-existing (just ported from pointer to value type), but mutating-on-marshal is a smell and not concurrency-safe if a ResourceBoundsMap is ever marshalled while shared. Consider filling the default on a local copy.
• ContractClassToGatewayPayload — json.Encoder.Encode appends a trailing \n inside the gzipped sierra_program that the old json.Marshal didn't. Harmless for the gateway parse, just a byte-for-byte difference.
• checkTxHasResourceBounds / checkTxHasSenderAddress (simulation.go) — now redundant with the new validate:"required" / required_if tags. Harmless belt-and-suspenders.
• Gateway feed re-adapt — the gateway branch re-runs adaptAndCompileBroadcastedTxToCore, recompiling the Sierra class for DECLARE after the gateway already accepted the tx, adding FFI-compile latency to the submit call. Consider moving off the hot path if it shows up in profiles.

The genesis.go:347 and adapt_transaction.go:431 items raised earlier are already tracked in #3741 / #3742. Nice work — the refactor reads cleanly and the perf wins are well-supported.
· branch thiagodeev/refactor-add-txn

[thiagodeev]

@claude you're wrong. The gateway branch only calls adaptAndCompileBroadcastedTxToCore once, which is after sending the tx to the feeder. Before that, the code doesn't compile anything.
Read the code and check it

[github-actions]

Claude finished @thiagodeev's task in 37s —— View job

---

Re: gateway branch compile path

You're right, and my earlier wording was wrong. I went back and read the code:

• AddTransaction gateway branch (h.memPool == nil, transaction.go:87-104):
  • pushToFeederGateway → ContractClassToGatewayPayload (transaction.go:237) only gzip+base64-compresses the Sierra program. No Sierra→CASM compile.
  • AdaptBroadcastedTransactionToFeeder (adapt_transaction.go:101) is a pure field-mapping adapter. No compile.
  • adaptAndCompileBroadcastedTxToCore (transaction.go:113) is then called exactly once, in the receivedTransactionFeed != nil branch after the gateway accepted the tx — and only that call compiles (for DECLARE).

So there is no double / re-compilation in the gateway path. My "re-runs … recompiling" phrasing implied redundant duplicate work, which is incorrect — it's a single compile, and nothing before it compiles. I withdraw that nit. Apologies for the confusion.

The only substantive (and much weaker) point that remains is that this single compile sits on the submit call's latency for DECLARE when a received-feed is wired up — but that's inherent to producing a core.Transaction for the feed, not redundant work. Not a concern.
· branch thiagodeev/refactor-add-txn

[github-actions]

Claude finished @thiagodeev's task in 3m 2s —— View job

---

Claude Code Review

Re-reviewed the latest pushes on top of the main merge. go build / go vet ./rpc/v10/... pass clean.

• Read diff for recent commits (%s error fmt, removed determinism test, bench rename)
• Re-check transaction.go / adapt_transaction.go state
• Confirm prior blocker stays resolved
• Post summary

---

Summary

No blockers. The three follow-up commits since the last review are clean and each addresses a reviewer comment correctly:

• e8beb33 (%v → %s) — correct. For the data field a plain string is right, and since the arg is an error, %s/%v produce identical output (both call Error()); %s better signals "string, not wrapped error." ✅
• e997a74 (remove determinism test) — fine to drop; it exercised two pure calls of AdaptBroadcastedTransactionToCore and asserted equal hashes, so it couldn't really fail unless hashing became non-deterministic. No coverage of unique behavior is lost. The assert import is still used elsewhere in the file, so no unused-import breakage (build confirms). ✅
• 9de8c33 (bench file rename) — rename only, matches the requested addtxn_bench_test.go. ✅

The previously-flagged typed-nil DeclaredClass blocker remains resolved (adaptAndCompileBroadcastedTxToCore returns a true-nil core.ClassDefinition interface for INVOKE / DEPLOY_ACCOUNT), and the genesis.go:347 / adapt_transaction.go:431 items are tracked in #3741 / #3742.

🟡 Nit (non-blocking)

• rpc/v10/transaction.go:202 & :224 — fmt.Sprintf("marshal transaction: %v", err) and "unmarshal gateway response: %v", err are the same data-field-string pattern you just switched at line 188. If the %s change was about consistency for data-field strings, these two are still %v. Purely cosmetic (identical output for an error), just flagging for uniformity.

Nothing else changed materially. The refactor still reads cleanly and the perf wins are well-supported — LGTM.
· branch thiagodeev/refactor-add-txn