feat: support 'legacy' v3 transaction resource bounds

[kariy]

In order to support database migration through re-execution of historical blocks, the transactions must be executed using the exact same parameters as when it was first executed. Currently, when the v6 versioned transaction (which uses 'legacy' v3 transactions resource bounds) is converted into blockifier's Transaction:

katana/crates/executor/src/implementation/blockifier/utils.rs

Lines 156 to 405 in 39f66a5

	pub fn to_executor_tx(mut tx: ExecutableTxWithHash, mut flags: ExecutionFlags) -> Transaction {
	use starknet_api::executable_transaction::AccountTransaction as ExecTx;
	let hash = tx.hash;
	// We only do this if we're running in fee enabled mode. If fee is already disabled, then
	// there's no need to do anything.
	if flags.fee() {
	// Disable fee charge if the total tx execution gas is zero.
	//
	// Max fee == 0 for legacy txs, or the total resource bounds == zero for V3 transactions.
	//
	// This is to support genesis transactions where the fee token is not yet deployed.
	flags = flags.with_fee(!skip_fee_on_zero_gas(&tx));
	}
	// In blockifier, if all the resource bounds are specified (ie., the
	// ValidResourceBounds::AllResources enum in blockifier types), then blockifier will use the
	// max l2 gas as the initial gas for this transaction's execution. So when we do fee estimates,
	// usually the resource bounds are all set to zero. so executing them as is will result in
	// an 'out of gas' error - because the initial gas will end up being zero.
	//
	// On fee disabled mode, we completely ignore any fee/resource bounds set by the transaction.
	// We always execute the transaction regardless whether the sender's have enough balance
	// (if the set max fee/resource bounds exceed the sender's balance), or if the transaction's
	// fee/resource bounds isn't actually enough to cover the entire transaction's execution.
	// So we artifically set the max initial gas so that blockifier will have enough initial sierra
	// gas to execute the transaction.
	//
	// Same case for when the transaction's fee/resource bounds are not set at all.
	//
	// See https://github.com/dojoengine/sequencer/blob/5d737b9c90a14bdf4483d759d1a1d4ce64aa9fd2/crates/blockifier/src/transaction/account_transaction.rs#L858
	if !flags.fee() {
	set_max_initial_sierra_gas(&mut tx);
	}
	match tx.transaction {
	ExecutableTx::Invoke(tx) => match tx {
	InvokeTx::V0(tx) => {
	let calldata = tx.calldata;
	let signature = tx.signature;
	let tx = InvokeTransaction {
	tx: ApiInvokeTransaction::V0(starknet_api::transaction::InvokeTransactionV0 {
	entry_point_selector: EntryPointSelector(tx.entry_point_selector),
	contract_address: to_blk_address(tx.contract_address),
	signature: TransactionSignature(signature),
	calldata: Calldata(Arc::new(calldata)),
	max_fee: Fee(tx.max_fee),
	}),
	tx_hash: TransactionHash(hash),
	};
	Transaction::Account(AccountTransaction {
	tx: ExecTx::Invoke(tx),
	execution_flags: flags.into(),
	})
	}
	InvokeTx::V1(tx) => {
	let calldata = tx.calldata;
	let signature = tx.signature;
	let tx = InvokeTransaction {
	tx: ApiInvokeTransaction::V1(starknet_api::transaction::InvokeTransactionV1 {
	max_fee: Fee(tx.max_fee),
	nonce: Nonce(tx.nonce),
	sender_address: to_blk_address(tx.sender_address),
	signature: TransactionSignature(signature),
	calldata: Calldata(Arc::new(calldata)),
	}),
	tx_hash: TransactionHash(hash),
	};
	Transaction::Account(AccountTransaction {
	tx: ExecTx::Invoke(tx),
	execution_flags: flags.into(),
	})
	}
	InvokeTx::V3(tx) => {
	let calldata = tx.calldata;
	let signature = tx.signature;
	let paymaster_data = tx.paymaster_data;
	let account_deploy_data = tx.account_deployment_data;
	let fee_data_availability_mode = to_api_da_mode(tx.fee_data_availability_mode);
	let nonce_data_availability_mode = to_api_da_mode(tx.nonce_data_availability_mode);
	let tx = InvokeTransaction {
	tx: ApiInvokeTransaction::V3(InvokeTransactionV3 {
	tip: Tip(tx.tip),
	nonce: Nonce(tx.nonce),
	sender_address: to_blk_address(tx.sender_address),
	signature: TransactionSignature(signature),
	calldata: Calldata(Arc::new(calldata)),
	paymaster_data: PaymasterData(paymaster_data),
	account_deployment_data: AccountDeploymentData(account_deploy_data),
	fee_data_availability_mode,
	nonce_data_availability_mode,
	resource_bounds: to_api_resource_bounds(tx.resource_bounds),
	}),
	tx_hash: TransactionHash(hash),
	};
	Transaction::Account(AccountTransaction {
	tx: ExecTx::Invoke(tx),
	execution_flags: flags.into(),
	})
	}
	},
	ExecutableTx::DeployAccount(tx) => match tx {
	DeployAccountTx::V1(tx) => {
	let calldata = tx.constructor_calldata;
	let signature = tx.signature;
	let salt = ContractAddressSalt(tx.contract_address_salt);
	let tx = DeployAccountTransaction {
	contract_address: to_blk_address(tx.contract_address),
	tx: ApiDeployAccountTransaction::V1(DeployAccountTransactionV1 {
	max_fee: Fee(tx.max_fee),
	nonce: Nonce(tx.nonce),
	signature: TransactionSignature(signature),
	class_hash: ClassHash(tx.class_hash),
	constructor_calldata: Calldata(Arc::new(calldata)),
	contract_address_salt: salt,
	}),
	tx_hash: TransactionHash(hash),
	};
	Transaction::Account(AccountTransaction {
	tx: ExecTx::DeployAccount(tx),
	execution_flags: flags.into(),
	})
	}
	DeployAccountTx::V3(tx) => {
	let calldata = tx.constructor_calldata;
	let signature = tx.signature;
	let salt = ContractAddressSalt(tx.contract_address_salt);
	let paymaster_data = tx.paymaster_data;
	let fee_data_availability_mode = to_api_da_mode(tx.fee_data_availability_mode);
	let nonce_data_availability_mode = to_api_da_mode(tx.nonce_data_availability_mode);
	let tx = DeployAccountTransaction {
	contract_address: to_blk_address(tx.contract_address),
	tx: ApiDeployAccountTransaction::V3(DeployAccountTransactionV3 {
	tip: Tip(tx.tip),
	nonce: Nonce(tx.nonce),
	signature: TransactionSignature(signature),
	class_hash: ClassHash(tx.class_hash),
	constructor_calldata: Calldata(Arc::new(calldata)),
	contract_address_salt: salt,
	paymaster_data: PaymasterData(paymaster_data),
	fee_data_availability_mode,
	nonce_data_availability_mode,
	resource_bounds: to_api_resource_bounds(tx.resource_bounds),
	}),
	tx_hash: TransactionHash(hash),
	};
	Transaction::Account(AccountTransaction {
	tx: ExecTx::DeployAccount(tx),
	execution_flags: flags.into(),
	})
	}
	},
	ExecutableTx::Declare(tx) => {
	let compiled = tx.class.as_ref().clone().compile().expect("failed to compile");
	let tx = match tx.transaction {
	DeclareTx::V0(tx) => ApiDeclareTransaction::V0(DeclareTransactionV0V1 {
	max_fee: Fee(tx.max_fee),
	nonce: Nonce::default(),
	sender_address: to_blk_address(tx.sender_address),
	signature: TransactionSignature(tx.signature),
	class_hash: ClassHash(tx.class_hash),
	}),
	DeclareTx::V1(tx) => ApiDeclareTransaction::V1(DeclareTransactionV0V1 {
	max_fee: Fee(tx.max_fee),
	nonce: Nonce(tx.nonce),
	sender_address: to_blk_address(tx.sender_address),
	signature: TransactionSignature(tx.signature),
	class_hash: ClassHash(tx.class_hash),
	}),
	DeclareTx::V2(tx) => {
	let signature = tx.signature;
	ApiDeclareTransaction::V2(DeclareTransactionV2 {
	max_fee: Fee(tx.max_fee),
	nonce: Nonce(tx.nonce),
	sender_address: to_blk_address(tx.sender_address),
	signature: TransactionSignature(signature),
	class_hash: ClassHash(tx.class_hash),
	compiled_class_hash: CompiledClassHash(tx.compiled_class_hash),
	})
	}
	DeclareTx::V3(tx) => {
	let signature = tx.signature;
	let paymaster_data = tx.paymaster_data;
	let fee_data_availability_mode = to_api_da_mode(tx.fee_data_availability_mode);
	let nonce_data_availability_mode =
	to_api_da_mode(tx.nonce_data_availability_mode);
	let account_deploy_data = tx.account_deployment_data;
	ApiDeclareTransaction::V3(DeclareTransactionV3 {
	tip: Tip(tx.tip),
	nonce: Nonce(tx.nonce),
	sender_address: to_blk_address(tx.sender_address),
	signature: TransactionSignature(signature),
	class_hash: ClassHash(tx.class_hash),
	account_deployment_data: AccountDeploymentData(account_deploy_data),
	compiled_class_hash: CompiledClassHash(tx.compiled_class_hash),
	paymaster_data: PaymasterData(paymaster_data),
	fee_data_availability_mode,
	nonce_data_availability_mode,
	resource_bounds: to_api_resource_bounds(tx.resource_bounds),
	})
	}
	};
	let tx_hash = TransactionHash(hash);
	let class_info = to_class_info(compiled).unwrap();
	Transaction::Account(AccountTransaction {
	tx: ExecTx::Declare(DeclareTransaction { class_info, tx_hash, tx }),
	execution_flags: flags.into(),
	})
	}
	ExecutableTx::L1Handler(tx) => Transaction::L1Handler(L1HandlerTransaction {
	paid_fee_on_l1: Fee(tx.paid_fee_on_l1),
	tx: starknet_api::transaction::L1HandlerTransaction {
	nonce: core::Nonce(tx.nonce),
	calldata: Calldata(Arc::new(tx.calldata)),
	version: TransactionVersion(1u128.into()),
	contract_address: to_blk_address(tx.contract_address),
	entry_point_selector: core::EntryPointSelector(tx.entry_point_selector),
	},
	tx_hash: TransactionHash(hash),
	}),
	}
	}

the resource bounds is always mapped to ValidResourceBounds::AllResources, this variant is for when all 3 bounds are specified by the transaction.

katana/crates/executor/src/implementation/blockifier/utils.rs

Lines 565 to 582 in 39f66a5

	fn to_api_resource_bounds(resource_bounds: fee::ResourceBoundsMapping) -> ValidResourceBounds {
	let l1_gas = ResourceBounds {
	max_amount: resource_bounds.l1_gas.max_amount.into(),
	max_price_per_unit: resource_bounds.l1_gas.max_price_per_unit.into(),
	};
	let l2_gas = ResourceBounds {
	max_amount: resource_bounds.l2_gas.max_amount.into(),
	max_price_per_unit: resource_bounds.l2_gas.max_price_per_unit.into(),
	};
	let l1_data_gas = ResourceBounds {
	max_amount: resource_bounds.l1_data_gas.max_amount.into(),
	max_price_per_unit: resource_bounds.l1_data_gas.max_price_per_unit.into(),
	};
	ValidResourceBounds::AllResources(AllResourceBounds { l1_gas, l2_gas, l1_data_gas })
	}

The resource bounds variant used matters here as it will determine how the transaction is being priced. Previous versions of katana were mapping to the blockifier's counterpart like so:

katana/crates/katana/executor/src/implementation/blockifier/utils.rs

Lines 490 to 504 in 783827e

	fn to_api_resource_bounds(
	resource_bounds: katana_primitives::fee::ResourceBoundsMapping,
	) -> ResourceBoundsMapping {
	let l1_gas = ResourceBounds {
	max_amount: resource_bounds.l1_gas.max_amount,
	max_price_per_unit: resource_bounds.l1_gas.max_price_per_unit,
	};
	let l2_gas = ResourceBounds {
	max_amount: resource_bounds.l2_gas.max_amount,
	max_price_per_unit: resource_bounds.l2_gas.max_price_per_unit,
	};
	ResourceBoundsMapping(BTreeMap::from([(Resource::L1Gas, l1_gas), (Resource::L2Gas, l2_gas)]))
	}

which now is the equivalent of mapping to ValidResourcesBounds::L1Gas:

katana/crates/executor/src/implementation/blockifier/utils.rs

Lines 592 to 595 in 10b51e8

	fee::ResourceBoundsMapping::L1Gas(bounds) => ValidResourceBounds::L1Gas(ResourceBounds {
	max_amount: bounds.max_amount.into(),
	max_price_per_unit: bounds.max_price_per_unit.into(),
	}),

The information about the correct resource bounds must be preserved in order to re-execute it exactly as it was for the first time.

[claude]

Claude finished @kariy's task —— View job

---

PR Review: Legacy V3 Transaction Resource Bounds

Review Tasks

• Analyze changes in executor/blockifier utils
• Review transaction fixture modifications
• Examine feeder-gateway transaction types
• Check primitives fee handling changes
• Review primitives transaction modifications
• Analyze RPC types transaction changes
• Check cartridge module updates
• Review storage database model changes
• Provide comprehensive feedback on code quality, bugs, performance, security, and test coverage

Overview

This PR implements support for "legacy" v3 transaction resource bounds, adding backward compatibility for v3 transactions created before L1 data gas resource bounds were introduced. The implementation extends the ResourceBoundsMapping enum to support both the newer format (with L1 gas, L2 gas, and L1 data gas) and the legacy format (L1 gas only).

Code Quality & Best Practices

✅ Strengths

1. Clean Architecture: The changes follow the existing codebase patterns well, with proper separation between storage versioning, primitives, and RPC layers.

2. Comprehensive Coverage: The implementation touches all necessary layers - from storage models to RPC conversions to transaction execution.

3. Type Safety: Good use of Rust's enum system to represent the two resource bounds variants clearly.

4. Documentation: The feeder-gateway changes include helpful comments explaining why l1_data_gas is optional.

⚠️ Areas for Improvement

1. Missing Documentation: crates/primitives/src/fee.rs:31-34 - The new ResourceBoundsMapping enum variants lack documentation explaining when to use each variant.

2. Code Duplication: In crates/primitives/src/transaction.rs:315-350, the hash calculation logic for ResourceBoundsMapping::All vs ResourceBoundsMapping::L1Gas is nearly identical. Consider extracting a helper function.

3. Inconsistent Naming: The term "legacy" appears in the PR title but not in the code. Consider adding doc comments explaining the legacy vs current distinction.

Potential Bugs & Issues

🔴 Critical Issues

1. Incomplete Zero Resource Bounds Check: crates/executor/src/implementation/blockifier/utils.rs:716-737 - The is_zero_resource_bounds function doesn't properly handle the new enum structure. The L1Gas variant calculation may not account for missing L2 and L1 data gas costs.

⚠️ Moderate Issues

1. RPC Conversion Accuracy: crates/rpc/rpc-types/src/transaction.rs:659-675 - The comment mentions that defaulting L2/L1 data gas to zero for legacy transactions "aren't technically accurate." This could lead to misleading fee estimates for legacy transactions.

2. Pattern Matching Completeness: Several files use pattern matching on ResourceBoundsMapping but the matches could be made more explicit about handling both variants.

Performance Considerations

✅ Good Performance Practices

1. Efficient Conversions: The conversion functions use pattern matching appropriately and avoid unnecessary allocations.

2. Zero-Copy Where Possible: The storage versioning uses appropriate conversion traits without unnecessary cloning.

💡 Minor Optimizations

1. Hash Calculation: Consider caching or pre-computing parts of the hash calculation that are common between All and L1Gas variants.

Security Concerns

✅ Security Positive

1. Input Validation: The feeder-gateway properly validates optional fields and provides appropriate error handling.

2. Type Safety: The enum-based approach prevents mixing incompatible resource bounds formats.

⚠️ Security Considerations

1. Fee Calculation Accuracy: The defaulting behavior in RPC conversions could potentially lead to incorrect fee calculations if not handled properly by clients.

Test Coverage

❌ Missing Test Coverage

1. Integration Tests: No tests verify the end-to-end behavior of legacy resource bounds through the transaction pipeline.

2. Edge Cases: Missing tests for:

   • Mixed legacy/modern transactions in the same block
   • Fee estimation with legacy resource bounds
   • RPC conversion edge cases
   • Zero resource bounds validation

3. Storage Migration: No tests verify backward compatibility when reading legacy transactions from storage.

💡 Test Recommendations

1. Add integration tests in crates/executor/tests/ that verify legacy transactions execute correctly.
2. Add property-based tests for resource bounds conversions.
3. Add tests for the storage versioning migration path.

Specific File Feedback

crates/primitives/src/fee.rs

• Line 28-34: Add documentation for the enum variants explaining the legacy vs modern distinction.

crates/executor/src/implementation/blockifier/utils.rs

• Line 571-597: The to_api_resource_bounds function handles both variants well.
• Line 716-737: The is_zero_resource_bounds logic needs review for correctness with L1Gas variant.

crates/feeder-gateway/src/types/transaction.rs

• Line 436-459: Excellent implementation of the backward compatibility conversion.
• Line 76: Good use of Option<ResourceBounds> for the optional L1 data gas field.

crates/storage/db/src/models/versioned/transaction/v6.rs

• Line 131-135: The conversion from v6 to current format correctly maps to L1Gas variant.
• Line 272: Good test coverage for the resource bounds conversion.

Recommendations

1. Add comprehensive documentation explaining the legacy vs modern resource bounds distinction.
2. Fix the zero resource bounds validation to properly handle both enum variants.
3. Add integration tests covering the legacy transaction execution path.
4. Consider adding migration utilities to help users understand the differences between formats.
5. Review fee calculation accuracy in RPC conversions to ensure no precision is lost.

Overall Assessment

This is a solid implementation of backward compatibility for legacy v3 transactions. The architecture is sound and follows good Rust practices. The main concerns are around test coverage and ensuring the fee calculation logic is correct for both variants. With the recommended fixes, this will be a valuable addition to support existing legacy transactions.

"Accept the things to which fate binds you, and love the people with whom fate brings you together."
— Marcus Aurelius, Meditations

[github-actions]

Binary size report 📊

Branch	Size
main	66.30MiB
legacy-resource-bound	66.35MiB ( +0.08% )

[claude]

Claude finished @kariy's task —— View job

---

PR Review: Legacy V3 Transaction Resource Bounds

Review Tasks

• Analyze changes in executor/blockifier utils
• Review transaction fixture modifications
• Examine feeder-gateway transaction types
• Check primitives fee handling changes
• Review primitives transaction modifications
• Analyze RPC types transaction changes
• Check cartridge module updates
• Review storage database model changes
• Provide comprehensive feedback on code quality, bugs, performance, security, and test coverage

Overview

This PR implements support for "legacy" v3 transaction resource bounds, adding backward compatibility for v3 transactions created before L1 data gas resource bounds were introduced. The implementation extends the ResourceBoundsMapping enum to support both the newer format (with L1 gas, L2 gas, and L1 data gas) and the legacy format (L1 gas only).

Code Quality & Best Practices

✅ Strengths

1. Clean Architecture: The changes follow the existing codebase patterns well, with proper separation between storage versioning, primitives, and RPC layers.

2. Comprehensive Coverage: The implementation touches all necessary layers - from storage models to RPC conversions to transaction execution.

3. Type Safety: Good use of Rust's enum system to represent the two resource bounds variants clearly.

4. Documentation: The feeder-gateway changes include helpful comments explaining why l1_data_gas is optional.

⚠️ Areas for Improvement

1. Missing Documentation: crates/primitives/src/fee.rs:31-34 - The new ResourceBoundsMapping enum variants lack documentation explaining when to use each variant.

2. Code Duplication: In crates/primitives/src/transaction.rs:315-350, the hash calculation logic for ResourceBoundsMapping::All vs ResourceBoundsMapping::L1Gas is nearly identical. Consider extracting a helper function.

3. Inconsistent Naming: The term "legacy" appears in the PR title but not in the code. Consider adding doc comments explaining the legacy vs current distinction.

Potential Bugs & Issues

🔴 Critical Issues

1. Incomplete Zero Resource Bounds Check: crates/executor/src/implementation/blockifier/utils.rs:716-737 - The is_zero_resource_bounds function doesn't properly handle the new enum structure. The L1Gas variant calculation may not account for missing L2 and L1 data gas costs.

⚠️ Moderate Issues

1. RPC Conversion Accuracy: crates/rpc/rpc-types/src/transaction.rs:659-675 - The comment mentions that defaulting L2/L1 data gas to zero for legacy transactions "aren't technically accurate." This could lead to misleading fee estimates for legacy transactions.

2. Pattern Matching Completeness: Several files use pattern matching on ResourceBoundsMapping but the matches could be made more explicit about handling both variants.

Performance Considerations

✅ Good Performance Practices

1. Efficient Conversions: The conversion functions use pattern matching appropriately and avoid unnecessary allocations.

2. Zero-Copy Where Possible: The storage versioning uses appropriate conversion traits without unnecessary cloning.

💡 Minor Optimizations

1. Hash Calculation: Consider caching or pre-computing parts of the hash calculation that are common between All and L1Gas variants.

Security Concerns

✅ Security Positive

1. Input Validation: The feeder-gateway properly validates optional fields and provides appropriate error handling.

2. Type Safety: The enum-based approach prevents mixing incompatible resource bounds formats.

⚠️ Security Considerations

1. Fee Calculation Accuracy: The defaulting behavior in RPC conversions could potentially lead to incorrect fee calculations if not handled properly by clients.

Test Coverage

❌ Missing Test Coverage

1. Integration Tests: No tests verify the end-to-end behavior of legacy resource bounds through the transaction pipeline.

2. Edge Cases: Missing tests for:

   • Mixed legacy/modern transactions in the same block
   • Fee estimation with legacy resource bounds
   • RPC conversion edge cases
   • Zero resource bounds validation

3. Storage Migration: No tests verify backward compatibility when reading legacy transactions from storage.

💡 Test Recommendations

1. Add integration tests in crates/executor/tests/ that verify legacy transactions execute correctly.
2. Add property-based tests for resource bounds conversions.
3. Add tests for the storage versioning migration path.

Specific File Feedback

crates/primitives/src/fee.rs

• Line 28-34: Add documentation for the enum variants explaining the legacy vs modern distinction.

crates/executor/src/implementation/blockifier/utils.rs

• Line 571-597: The to_api_resource_bounds function handles both variants well.
• Line 716-737: The is_zero_resource_bounds logic needs review for correctness with L1Gas variant.

crates/feeder-gateway/src/types/transaction.rs

• Line 436-459: Excellent implementation of the backward compatibility conversion.
• Line 76: Good use of Option<ResourceBounds> for the optional L1 data gas field.

crates/storage/db/src/models/versioned/transaction/v6.rs

• Line 131-135: The conversion from v6 to current format correctly maps to L1Gas variant.
• Line 272: Good test coverage for the resource bounds conversion.

Recommendations

1. Add comprehensive documentation explaining the legacy vs modern resource bounds distinction.
2. Fix the zero resource bounds validation to properly handle both enum variants.
3. Add integration tests covering the legacy transaction execution path.
4. Consider adding migration utilities to help users understand the differences between formats.
5. Review fee calculation accuracy in RPC conversions to ensure no precision is lost.

Overall Assessment

This is a solid implementation of backward compatibility for legacy v3 transactions. The architecture is sound and follows good Rust practices. The main concerns are around test coverage and ensuring the fee calculation logic is correct for both variants. With the recommended fixes, this will be a valuable addition to support existing legacy transactions.

"Accept the things to which fate binds you, and love the people with whom fate brings you together."
— Marcus Aurelius, Meditations

[codecov]

Codecov Report

Attention: Patch coverage is 71.14754% with 88 lines in your changes missing coverage. Please review.

Project coverage is 73.32%. Comparing base (5f86bfd) to head (10b51e8).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/primitives/src/transaction.rs	51.51%	48 Missing ⚠️
crates/rpc/rpc-types/src/transaction.rs	51.61%	15 Missing ⚠️
crates/feeder-gateway/src/types/transaction.rs	17.64%	14 Missing ⚠️
...es/executor/src/implementation/blockifier/utils.rs	86.95%	6 Missing ⚠️
crates/rpc/rpc/src/cartridge/mod.rs	0.00%	3 Missing ⚠️
.../storage/db/src/models/versioned/transaction/v6.rs	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
- Coverage   73.44%   73.32%   -0.12%     
==========================================
  Files         209      209              
  Lines       22944    23130     +186     
==========================================
+ Hits        16852    16961     +109     
- Misses       6092     6169      +77     

☔ View full report in Codecov by Sentry.
📢 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.

[claude]

Claude finished @kariy's task —— View job

---

PR Review: Legacy V3 Transaction Resource Bounds

Review Tasks

• Analyze changes in executor/blockifier utils
• Review transaction fixture modifications
• Examine feeder-gateway transaction types
• Check primitives fee handling changes
• Review primitives transaction modifications
• Analyze RPC types transaction changes
• Check cartridge module updates
• Review storage database model changes
• Provide comprehensive feedback on code quality, bugs, performance, security, and test coverage

Overview

This PR implements support for "legacy" v3 transaction resource bounds, enabling backward compatibility for v3 transactions created before L1 data gas resource bounds were introduced. The implementation extends the ResourceBoundsMapping from a struct to an enum with two variants:

• All(AllResourceBoundsMapping): Modern format with L1 gas, L2 gas, and L1 data gas
• L1Gas(ResourceBounds): Legacy format with only L1 gas bounds

Code Quality & Best Practices

✅ Strengths

1. Clean Architecture: The enum-based approach provides type safety and clear distinction between legacy and modern resource bounds.

2. Comprehensive Coverage: The implementation correctly touches all necessary layers - from storage versioning to RPC conversions to transaction execution.

3. Proper Hash Calculation: The hash_fee_fields function in crates/primitives/src/utils/transaction.rs:336-356 correctly handles both variants, including only relevant fields in the hash calculation.

4. Smart Storage Migration: The v6 to current conversion in crates/storage/db/src/models/versioned/transaction/v6.rs:131-135 correctly maps legacy transactions to the L1Gas variant.

5. Correct Blockifier Integration: The to_api_resource_bounds function in crates/executor/src/implementation/blockifier/utils.rs:571-597 properly converts both enum variants to blockifier types.

⚠️ Areas for Improvement

1. Missing Documentation: crates/primitives/src/fee.rs:31-34 - The ResourceBoundsMapping enum variants lack documentation explaining when to use each variant.

2. RPC Conversion Concerns: crates/rpc/rpc-types/src/transaction.rs:659-675 - The comment acknowledges that defaulting L2/L1 data gas to zero for legacy transactions "aren't technically accurate."

Potential Issues

🔴 Critical Issues

None identified - The implementation appears sound with proper handling of both variants.

⚠️ Moderate Issues

1. Fee Calculation Accuracy: The RPC conversion defaults missing gas bounds to zero, which could potentially confuse clients expecting accurate fee estimates for legacy transactions.

2. Incomplete Error Context: Some conversion functions could benefit from more descriptive error messages when dealing with malformed legacy transactions.

Performance Considerations

✅ Good Performance Practices

1. Efficient Pattern Matching: All conversion functions use appropriate pattern matching without unnecessary allocations.

2. Zero-Copy Conversions: The storage versioning uses efficient conversion traits.

3. Proper Hash Optimization: The hash_fee_fields function avoids including unnecessary fields in legacy transaction hashes.

Security Concerns

✅ Security Positive

1. Type Safety: The enum prevents mixing incompatible resource bounds formats.

2. Input Validation: The feeder-gateway properly validates optional fields.

3. Backward Compatibility: Legacy transactions maintain their original hash calculation, preventing replay attacks.

⚠️ Security Considerations

Fee Calculation Consistency: While not a security vulnerability, the RPC conversion behavior should be well-documented to prevent client-side fee estimation errors.

Test Coverage Analysis

❌ Missing Test Coverage (Based on Codecov report: 57.89% patch coverage)

The most concerning gaps are:

1. Transaction Hash Calculation: crates/primitives/src/transaction.rs (51.51% coverage) - Missing tests for both enum variants in hash calculation.

2. RPC Conversions: crates/rpc/rpc-types/src/transaction.rs (51.61% coverage) - Legacy transaction conversion paths need testing.

3. Feeder Gateway: crates/feeder-gateway/src/types/transaction.rs (17.64% coverage) - Critical conversion logic is undertested.

💡 Test Recommendations

1. Add unit tests for both ResourceBoundsMapping variants in transaction hash calculation
2. Add integration tests verifying end-to-end legacy transaction processing
3. Add property-based tests for resource bounds conversions
4. Test edge cases like zero resource bounds for both variants

Specific File Feedback

crates/primitives/src/fee.rs:28-34

pub enum ResourceBoundsMapping {
    L1Gas(ResourceBounds),           // Add: /// Legacy v3 transactions with only L1 gas bounds
    All(AllResourceBoundsMapping),   // Add: /// Modern transactions with L1, L2, and L1 data gas bounds
}

crates/executor/src/implementation/blockifier/utils.rs:716-737

The is_zero_resource_bounds function correctly handles both variants. The L1Gas variant appropriately only checks L1 gas bounds since that's all legacy transactions have.

crates/feeder-gateway/src/types/transaction.rs:436-459

Excellent implementation of backward compatibility. The conversion logic is clean and handles the optional l1_data_gas field properly.

crates/primitives/src/utils/transaction.rs:336-356

Perfect hash calculation implementation. The function correctly includes only relevant fields for each transaction type.

crates/rpc/rpc/src/cartridge/mod.rs:268

Correctly uses ResourceBoundsMapping::All for modern transactions in the cartridge module.

Overall Assessment

This is a well-implemented backward compatibility feature. The enum-based approach is clean and type-safe. The main concerns are around test coverage rather than the implementation itself.

Recommendations

1. Add comprehensive test coverage for the new enum variants
2. Document the enum variants with clear explanations
3. Consider adding integration tests for mixed legacy/modern transaction blocks
4. Document the RPC conversion behavior for client implementers

The implementation follows good Rust practices and maintains backward compatibility while preparing for future enhancements.

"Very little is needed to make a happy life; it is all within yourself, in your way of thinking."
— Marcus Aurelius, Meditations

[claude]

Claude finished @kariy's task —— View job

---

PR Review: Legacy V3 Transaction Resource Bounds

Review Tasks

• Analyze changes in executor/blockifier utils
• Review transaction fixture modifications
• Examine feeder-gateway transaction types
• Check primitives fee handling changes
• Review primitives transaction modifications
• Analyze RPC types transaction changes
• Check cartridge module updates
• Review storage database model changes
• Provide comprehensive feedback on code quality, bugs, performance, security, and test coverage

Overview

This PR implements support for "legacy" v3 transaction resource bounds, enabling backward compatibility for v3 transactions created before L1 data gas resource bounds were introduced. The implementation extends the ResourceBoundsMapping from a struct to an enum with two variants:

• All(AllResourceBoundsMapping): Modern format with L1 gas, L2 gas, and L1 data gas
• L1Gas(ResourceBounds): Legacy format with only L1 gas bounds

Code Quality & Best Practices

✅ Strengths

1. Clean Architecture: The enum-based approach provides type safety and clear distinction between legacy and modern resource bounds.

2. Comprehensive Coverage: The implementation correctly touches all necessary layers - from storage versioning to RPC conversions to transaction execution.

3. Proper Hash Calculation: The hash_fee_fields function in crates/primitives/src/utils/transaction.rs:336-356 correctly handles both variants, including only relevant fields in the hash calculation.

4. Smart Storage Migration: The v6 to current conversion in crates/storage/db/src/models/versioned/transaction/v6.rs:131-135 correctly maps legacy transactions to the L1Gas variant.

5. Correct Blockifier Integration: The to_api_resource_bounds function in crates/executor/src/implementation/blockifier/utils.rs:571-597 properly converts both enum variants to blockifier types.

⚠️ Areas for Improvement

1. Missing Documentation: crates/primitives/src/fee.rs:31-34 - The ResourceBoundsMapping enum variants lack documentation explaining when to use each variant.

2. RPC Conversion Concerns: crates/rpc/rpc-types/src/transaction.rs:659-675 - The comment acknowledges that defaulting L2/L1 data gas to zero for legacy transactions "aren't technically accurate."

Potential Issues

🔴 Critical Issues

None identified - The implementation appears sound with proper handling of both variants.

⚠️ Moderate Issues

1. Fee Calculation Accuracy: The RPC conversion defaults missing gas bounds to zero, which could potentially confuse clients expecting accurate fee estimates for legacy transactions.

2. Incomplete Error Context: Some conversion functions could benefit from more descriptive error messages when dealing with malformed legacy transactions.

Performance Considerations

✅ Good Performance Practices

1. Efficient Pattern Matching: All conversion functions use appropriate pattern matching without unnecessary allocations.

2. Zero-Copy Conversions: The storage versioning uses efficient conversion traits.

3. Proper Hash Optimization: The hash_fee_fields function avoids including unnecessary fields in legacy transaction hashes.

Security Concerns

✅ Security Positive

1. Type Safety: The enum prevents mixing incompatible resource bounds formats.

2. Input Validation: The feeder-gateway properly validates optional fields.

3. Backward Compatibility: Legacy transactions maintain their original hash calculation, preventing replay attacks.

⚠️ Security Considerations

Fee Calculation Consistency: While not a security vulnerability, the RPC conversion behavior should be well-documented to prevent client-side fee estimation errors.

Test Coverage Analysis

❌ Missing Test Coverage (Based on Codecov report: 57.89% patch coverage)

The most concerning gaps are:

1. Transaction Hash Calculation: crates/primitives/src/transaction.rs (51.51% coverage) - Missing tests for both enum variants in hash calculation.

2. RPC Conversions: crates/rpc/rpc-types/src/transaction.rs (51.61% coverage) - Legacy transaction conversion paths need testing.

3. Feeder Gateway: crates/feeder-gateway/src/types/transaction.rs (17.64% coverage) - Critical conversion logic is undertested.

💡 Test Recommendations

1. Add unit tests for both ResourceBoundsMapping variants in transaction hash calculation
2. Add integration tests verifying end-to-end legacy transaction processing
3. Add property-based tests for resource bounds conversions
4. Test edge cases like zero resource bounds for both variants

Specific File Feedback

crates/primitives/src/fee.rs:28-34

pub enum ResourceBoundsMapping {
    L1Gas(ResourceBounds),           // Add: /// Legacy v3 transactions with only L1 gas bounds
    All(AllResourceBoundsMapping),   // Add: /// Modern transactions with L1, L2, and L1 data gas bounds
}

crates/executor/src/implementation/blockifier/utils.rs:716-737

The is_zero_resource_bounds function correctly handles both variants. The L1Gas variant appropriately only checks L1 gas bounds since that's all legacy transactions have.

crates/feeder-gateway/src/types/transaction.rs:436-459

Excellent implementation of backward compatibility. The conversion logic is clean and handles the optional l1_data_gas field properly.

crates/primitives/src/utils/transaction.rs:336-356

Perfect hash calculation implementation. The function correctly includes only relevant fields for each transaction type.

crates/rpc/rpc/src/cartridge/mod.rs:268

Correctly uses ResourceBoundsMapping::All for modern transactions in the cartridge module.

Overall Assessment

This is a well-implemented backward compatibility feature. The enum-based approach is clean and type-safe. The main concerns are around test coverage rather than the implementation itself.

Recommendations

1. Add comprehensive test coverage for the new enum variants
2. Document the enum variants with clear explanations
3. Consider adding integration tests for mixed legacy/modern transaction blocks
4. Document the RPC conversion behavior for client implementers

The implementation follows good Rust practices and maintains backward compatibility while preparing for future enhancements.

"Very little is needed to make a happy life; it is all within yourself, in your way of thinking."
— Marcus Aurelius, Meditations