Remove support for adding new v1 declaration transactions (RPC 0.8.0)#716
Remove support for adding new v1 declaration transactions (RPC 0.8.0)#716FabijanC merged 11 commits intostarknet-0.13.4from
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request removes legacy support for Cairo 0 contracts and version 1 declare transaction logic across the codebase. References to outdated dummy contract classes and transaction variants are replaced with their newer counterparts (Cairo 1 and declare_v3). Changes span core modules, JSON-RPC endpoints, types, and integration tests, consolidating transaction processing and test validations to use updated signatures, parameters, and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Account
participant TransactionHandler
Client->>Account: Initiate declare transaction (v3)
Account->>TransactionHandler: Process declare_v3(contract_artifact, casm_hash)
TransactionHandler-->>Account: Return transaction confirmation
Account-->>Client: Respond with transaction hash
Suggested Reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/integration/test_restart.rs (2)
120-121: Consider handling errors more gracefully.
Currently,.unwrap()will panic if spawning the devnet fails. Encouraging an explicit error handling or custom message could improve maintainability and debugging.- let devnet = BackgroundDevnet::spawn_with_additional_args(&devnet_args).await.unwrap(); + let devnet = BackgroundDevnet::spawn_with_additional_args(&devnet_args) + .await + .expect("Failed to spawn devnet with additional args");
138-142: Avoid panics by handling the fee estimation error.
Unwrapping here will panic at runtime if fee estimation fails. Consider returning a result or using a descriptive.expect(...)..estimate_fee() .await - .unwrap(); + .expect("Fee estimation failed");tests/integration/test_estimate_fee.rs (1)
258-258: Updated calldata toFelt::from(100_u128), Felt::ONE.
You might consider making constants for these values if reused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
crates/starknet-devnet-core/src/account.rs(2 hunks)crates/starknet-devnet-core/src/starknet/add_declare_transaction.rs(6 hunks)crates/starknet-devnet-core/src/starknet/events.rs(2 hunks)crates/starknet-devnet-core/src/starknet/mod.rs(4 hunks)crates/starknet-devnet-core/src/state/mod.rs(7 hunks)crates/starknet-devnet-core/src/state/state_diff.rs(1 hunks)crates/starknet-devnet-core/src/transactions.rs(3 hunks)crates/starknet-devnet-core/src/utils.rs(2 hunks)crates/starknet-devnet-server/src/api/json_rpc/mod.rs(1 hunks)crates/starknet-devnet-server/src/api/json_rpc/models.rs(1 hunks)crates/starknet-devnet-server/src/api/json_rpc/write_endpoints.rs(1 hunks)crates/starknet-devnet-server/src/test_utils.rs(0 hunks)crates/starknet-devnet-server/test_data/rpc/declare_v1.json(0 hunks)crates/starknet-devnet-types/src/constants.rs(0 hunks)crates/starknet-devnet-types/src/rpc/transactions.rs(2 hunks)crates/starknet-devnet-types/src/rpc/transactions/broadcasted_declare_transaction_v1.rs(0 hunks)crates/starknet-devnet-types/src/rpc/transactions/broadcasted_declare_transaction_v3.rs(2 hunks)crates/starknet-devnet-types/src/rpc/transactions/declare_transaction_v0v1.rs(0 hunks)tests/integration/get_transaction_by_hash.rs(0 hunks)tests/integration/get_transaction_receipt_by_hash.rs(1 hunks)tests/integration/test_account_selection.rs(1 hunks)tests/integration/test_estimate_fee.rs(7 hunks)tests/integration/test_estimate_message_fee.rs(3 hunks)tests/integration/test_fork.rs(3 hunks)tests/integration/test_get_class.rs(0 hunks)tests/integration/test_restart.rs(3 hunks)tests/integration/test_simulate_transactions.rs(3 hunks)tests/integration/test_transaction_handling.rs(1 hunks)
💤 Files with no reviewable changes (7)
- crates/starknet-devnet-server/src/test_utils.rs
- crates/starknet-devnet-server/test_data/rpc/declare_v1.json
- crates/starknet-devnet-types/src/constants.rs
- crates/starknet-devnet-types/src/rpc/transactions/declare_transaction_v0v1.rs
- tests/integration/get_transaction_by_hash.rs
- tests/integration/test_get_class.rs
- crates/starknet-devnet-types/src/rpc/transactions/broadcasted_declare_transaction_v1.rs
🔇 Additional comments (70)
crates/starknet-devnet-core/src/starknet/events.rs (2)
144-144: Updated test utility import to v3The import has been updated to use
dummy_declare_transaction_v3instead ofdummy_declare_transaction_v1, which aligns with the PR objective of removing v1 transaction support.
401-401: Updated test function to use v3 transactionTest setup now uses
dummy_declare_transaction_v3()instead of the v1 version, maintaining consistency with the import change and overall PR objective.crates/starknet-devnet-core/src/account.rs (2)
226-228: Updated test utility imports to use Cairo 1The imports now reference
dummy_cairo_1_contract_classinstead of the Cairo 0 version, aligning with the PR objective to replace Cairo 0 contracts with Cairo 1.
320-320: Updated test function to use Cairo 1 contract classThe setup function now uses
dummy_cairo_1_contract_class()instead of the Cairo 0 version, ensuring consistency with the import changes and PR objectives.crates/starknet-devnet-types/src/rpc/transactions/broadcasted_declare_transaction_v3.rs (2)
5-5: Added import for BroadcastedDeclareTransactionAdded the required import to support the new From implementation that converts v3 transactions to the general transaction type.
22-26: Added conversion from v3 to general declare transaction typeImplemented
From<BroadcastedDeclareTransactionV3>forBroadcastedDeclareTransaction, which simplifies working with v3 transactions by allowing automatic conversion to the generic transaction type. This is crucial for the transition away from v1 transactions.crates/starknet-devnet-core/src/transactions.rs (2)
296-296: Updated test utility import to v3The import has been updated to use
dummy_declare_transaction_v3instead of the v1 version, which aligns with the PR objective of removing v1 transaction support.
312-312: Updated test functions to use v3 transactionsTest cases now use
dummy_declare_transaction_v3()instead of the v1 version, ensuring consistency with the import changes and supporting the PR goal of removing v1 transaction support.Also applies to: 337-337
crates/starknet-devnet-server/src/api/json_rpc/models.rs (2)
389-389: Update to test assertions after V1 transaction removalThe test has been updated to assert a length of 3 instead of 4, which aligns with the removal of V1 transaction support from the request array as part of the JSON-RPC v0.8.0 migration.
394-395: Updated array index assertions for transaction type checksThe assertions have been correctly adjusted to check the transaction types at their new positions in the array after removing the V1 declare transaction.
crates/starknet-devnet-core/src/state/state_diff.rs (1)
224-224: Using constant directly for compiled class hashThe code now uses the
DUMMY_CAIRO_1_COMPILED_CLASS_HASHconstant directly, which is consistent with the PR objective of focusing on Cairo 1 contracts and removing Cairo 0 support.tests/integration/test_account_selection.rs (1)
8-8: Simplified imports after removing Cairo 0 supportThe import statement has been updated to remove unused imports after removing the Cairo 0 account testing function, which aligns with the PR's goal of phasing out support for v1 transactions and Cairo 0 contracts.
crates/starknet-devnet-server/src/api/json_rpc/write_endpoints.rs (2)
194-195: Removed imports related to V1 transaction testingThese imports are no longer needed after removing the test for V1 declare transactions, which is consistent with the PR's goal of removing support for V1 transactions.
198-200: Simplified test function to focus on deploy account transaction onlyThe test has been streamlined to only test the deploy account transaction functionality, removing the V1 declare transaction test as part of phasing out V1 transaction support.
crates/starknet-devnet-core/src/state/mod.rs (6)
434-437: Imports updated to use Cairo 1 contract classes in testsThe imports have been correctly updated to use Cairo 1 contract classes and related utilities, aligning with the PR objective of removing support for Cairo 0 contracts.
450-451: Cairo 0 contract class replaced with Cairo 1Contract class instantiation updated to use Cairo 1 contract, consistent with the overall migration away from Cairo 0.
488-488: Test function name updated to reflect Cairo 1The function name now accurately reflects that it tests declaring Cairo 1 contract classes.
509-512: Updated assertion for Cairo 1 contract classThe assertion logic has been properly updated to verify the Cairo 1 contract class, maintaining test coverage while transitioning away from Cairo 0.
525-525: Test function name updated for Cairo 1 contract deploymentFunction name now correctly indicates it's testing the deployment of Cairo 1 contracts.
590-593: Updated setup function to use Cairo 1Setup function now uses Cairo 1 contract class and compiled class hash, maintaining consistency with other changes in the file and the broader codebase transition.
tests/integration/test_fork.rs (3)
10-11: Updated imports to include necessary transaction typesAdded
DeclareTransactionand other necessary types to handle transaction assertions in the new test case.
147-158: Simplified test for fetching Cairo 0 class from forked networkThe test has been refactored to directly use a hardcoded class hash instead of reading and declaring a legacy contract class, which aligns with removing support for adding new v1 transactions while still testing the ability to fetch existing ones.
686-702: Added test to verify backward compatibility with v1 transactionsThis new test ensures that while adding new v1 transactions is no longer supported, the system can still fetch existing v1 transactions from the origin network. This distinction is important for backward compatibility.
tests/integration/test_transaction_handling.rs (2)
34-36: Updated contract artifact acquisitionNow using the function
get_simple_contract_in_sierra_and_compiled_class_hash()to obtain both the contract artifact and its CASM hash, which supports the transition to Cairo 1 contracts.
39-39: Updated declaration method from legacy to v3Changed from
declare_legacytodeclare_v3with updated parameters, now properly including both the contract artifact and CASM hash. Also switched from usingmax_feetogasparameter, reflecting the updated transaction version.tests/integration/get_transaction_receipt_by_hash.rs (1)
7-7: Removed BroadcastedDeclareTransactionV1 from importsImport simplified to remove v1 transaction type, consistent with removing support for adding new v1 transactions.
tests/integration/test_restart.rs (3)
16-18: Imports look good.
No issues found with these added imports; they align well with usage patterns in the file.
148-148: Same note on unwrapping.
This declaration call also uses.unwrap(). Please apply similar error handling to avoid panics at runtime.
133-135:Details
✅ Verification successful
Verify correctness of the retrieved artifact and CASM hash.
Ensure thatget_simple_contract_in_sierra_and_compiled_class_hash()returns accurate data objects. If usage is widespread, confirm all call sites are updated accordingly.
🏁 Script executed:
#!/bin/bash # Verify that get_simple_contract_in_sierra_and_compiled_class_hash is used consistently # and returns valid (contract_artifact, casm_hash). This script will look for references # to the function and check subsequent usage. rg 'get_simple_contract_in_sierra_and_compiled_class_hash' -A 5Length of output: 16004
All call sites correctly process the retrieved artifact and CASM hash
After verifying usage across the test suite,
get_simple_contract_in_sierra_and_compiled_class_hash()is consistently invoked and its return values are handled as expected (e.g., wrapping the artifact in anArc::newwhen required). No discrepancies or outdated usages were found, so no further updates are needed at this time.crates/starknet-devnet-types/src/rpc/transactions.rs (2)
584-584: Marking Declare V2 as deprecated is appropriate.
This change correctly aligns with the removal or phasing out of older transaction logic.
533-533:Details
✅ Verification successful
Confirm removal of negation in fee validation.
Previously, code may have negated the invoke transaction’sis_max_fee_validresult. Now it is used directly. Verify this aligns with the intended fee validation logic.
🏁 Script executed:
#!/bin/bash # Check references to is_max_fee_valid to confirm consistent usage across the codebase rg 'is_max_fee_valid' -A 3Length of output: 4471
Fee Validation Logic for Invoke Transactions Confirmed
The usage ofbroadcasted_invoke_transaction.is_max_fee_valid()without an extra negation is consistent with our intended logic. In the types module, the invoke variant for V1 already returns!v1.common.is_max_fee_zero_value(), while V3 directly checksv3.common.are_gas_bounds_valid(). The core modules appropriately wrap the call with a negation when validating (e.g.,if !broadcasted_invoke_transaction.is_max_fee_valid() { … }), ensuring that the fee validation behaves as expected across all transaction types.crates/starknet-devnet-server/src/api/json_rpc/mod.rs (1)
1380-1381: Test condition for Declare v3 error message.
Replacing "0x2" with "0x3" here ensures the test checks the correct invalid version scenario for declare v3 transactions.tests/integration/test_estimate_message_fee.rs (4)
11-11: Updated dependency import to align with newer contract handling.The change from a legacy utility to
get_messaging_contract_in_sierra_and_compiled_class_hashreflects the transition to Cairo 1 contracts as mentioned in the PR objectives.
27-30: Contract artifact handling properly migrated to v3 format.The code now correctly retrieves both the contract artifact and its CASM hash, which is required for v3 declaration transactions.
33-37: Successfully migrated from declare_legacy to declare_v3.The code correctly implements the v3 declaration method, which requires the additional CASM hash parameter. Also, the gas parameter is now used with a fixed value instead of max_fee, which aligns with newer transaction handling.
51-57: Successfully migrated from deploy_v1 to deploy_v3.The deployment method has been updated to use deploy_v3 with appropriate parameters, completing the transition from legacy v1 deployment.
tests/integration/test_simulate_transactions.rs (5)
33-37: Updated utility imports to support newer transaction versions.The import list has been properly updated to include necessary types for v3 transactions.
232-238: Contract creation and declaration properly migrated to v3.The code now correctly obtains both the contract artifact and CASM hash, then uses declare_v3 instead of declare_legacy, which aligns with the PR objective of removing v1 transaction support.
244-257: Deploy method updated from v1 to v3.The contract deployment has been successfully migrated to use deploy_v3 with appropriate parameters, completing the transition from legacy deployment methods.
262-263: Updated calldata format for newer contract version.The calldata format has been updated to match the expected structure for Cairo 1 contracts.
281-285: Simplified parameter passing in transaction simulation.The parameters are now passed directly rather than being converted to hexadecimal format, which simplifies the code and improves readability.
crates/starknet-devnet-core/src/starknet/mod.rs (3)
1460-1490: Removed V1 transaction support from validation logic.The
should_transaction_skip_validation_if_sender_is_impersonatedmethod has been updated to handle only V2 and V3 transactions, removing V1 support as intended by the PR. The code correctly matches sender addresses from the supported transaction types.
1546-1547: Updated test utility from v1 to v3.Test utility function reference has been updated from
dummy_declare_transaction_v1todummy_declare_transaction_v3, ensuring consistency across the codebase.
1561-1563: Added L2 gas price parameters to StarknetConfig.The configuration has been updated to include
l2_gas_price_weiandl2_gas_price_fri, which is consistent with the updated transaction handling across the PR.crates/starknet-devnet-core/src/utils.rs (4)
81-83: Updated imports to support newer transaction types.The imports have been modified to include
DataAvailabilityModeandTipand adjust theFeeimport, which reflects the changes needed for v3 transaction support.
112-114: Improved type safety for compiled class hash constant.Changed
DUMMY_CAIRO_1_COMPILED_CLASS_HASHfrom a string to aFelttype, improving type safety and eliminating the need for runtime conversion.
121-144: Added support function for v3 transaction testing.The new
dummy_broadcasted_declare_tx_v3function properly creates test instances of the v3 transaction type, which is needed to replace the removed v1 functionality.
146-164: Successfully migrated dummy transaction creation to v3.The
dummy_declare_transaction_v3function has been updated to use the new v3 transaction structure, correctly calculating transaction hashes and properly utilizing the added helper function.crates/starknet-devnet-core/src/starknet/add_declare_transaction.rs (10)
117-117: No issues with the new importTransactionFeeError.
The usage is appropriate for error handling.
126-126: No issues with addingResourceBoundsWrapper.
It's adequately utilized to handle resource bounds logic.
136-138: Imported test utility functions.
All these references (e.g.,dummy_broadcasted_declare_tx_v3) appear to be used in the test suite. No further issues.
160-163: Test logic updated to expectonly-query transactionserror.
This is consistent with dropping v1 transaction support.
185-190: Test properly checksInsufficientResourcesForValidateerror.
No concerns as it aligns with updated resource bounds logic.
227-228: Test expectsInsufficientAccountBalanceerror.
No concerns, the logic is correct.
244-245: Refactored usage ofdeclare_txn.clone().into().
This approach is succinct and uses type inference properly.
333-349: Test ensuring error on insufficient max fee for declare v3 transactions.
The resource bounds usage is consistent with the new approach.
353-369: Test checksResourcesBoundsExceedBalanceunder v3.
Logic is consistent with the new code path.
372-389: Test verifying successful storage change for v3 declare.
Implementation suits the new version.tests/integration/test_estimate_fee.rs (12)
10-13: Additional imports for v3, resources, and error data
No immediate concerns, these are relevant for the updated test coverage.
31-31: Importingget_simple_contract_in_sierra_and_compiled_class_hash.
Ensure this function returns correct artifacts for consistent test coverage.
223-225: Using a simpler contract artifact retrieving function.
This approach is more straightforward than using legacy or advanced artifacts. Code is concise and clear.
229-231: Usingdeclare_v3with.gas(...).
This aligns with updated transaction flow. Ensure the chosen gas value is appropriate in test contexts.
240-240: Constructor calldata changed to a single zero.
Behavior looks correct for a minimal constructor invocation.
302-302: Test now checks final balance is 101.
This increment logic is consistent with updated invoke.
476-478: Refactored to retrieve simpler contract class.
This is consistent with the pattern in other tests.
481-483: Settingl1_gasandl1_gas_priceto 0.
Double-check whether these zero values are intentional for test coverage or if non-zero could reveal potential issues.
484-487: Chaining method calls with.declare_v3(...),.gas(...), and.gas_price(...).
The structure is readable and consistent with typical builder patterns.
492-493: Signature creation forprepared_declaration.
No issues. This approach is correct for cryptographic signing in the test environment.
500-501: Switching toBroadcastedDeclareTransaction::V3.
This is consistent with the deprecation of older transaction versions.
531-533: Adjusted constructor arguments.
Ensure that this updated logic accurately models the contract's expected constructor if it’s not intentionally a no-op.
Usage related changes
Development related changes
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
New Features
Refactor
Tests