Enable custom account in deploying L1-L2 messaging contract #758
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change introduces support for specifying a custom deployer account private key when configuring or deploying the L1-L2 messaging contract in Starknet Devnet. The messaging configuration flow now accepts an optional private key parameter, which is passed through the API, internal configuration methods, and down to the Ethereum messaging core. The system validates that any provided messaging contract address actually contains contract code on-chain. Test infrastructure is updated to allow custom mnemonic and private key configuration for Ethereum providers. Integration tests are added to cover scenarios involving custom private keys, contract deployment, and address validation. Documentation and API schemas are updated to reflect the new parameter and renamed fields. Assessment against linked issues
Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
FabijanC
changed the title
* [skip ci] * feat: add private key parameter in loading l1 contract * replace dummy private key * update docs for postman load contract * add incorrect private key tests * formatting changes * Improve predeclaration/predeployment startup log (#734) * Abstract public+private key as KeyPair: reduce number of fn args * fix tests revert anvil changes * update incorrect test assert code --------- Co-authored-by: FabijanC <[email protected]>
|
@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
🧹 Nitpick comments (5)
tests/integration/common/constants.rs (1)
105-108: Consider adding documentation for test constants.These test constants look good. Consider adding doc comments to clarify that these are standard test values and should never be used in production environments.
+/// Standard test mnemonic phrase for Anvil - DO NOT USE IN PRODUCTION pub const DEFAULT_ANVIL_MNEMONIC_PHRASE: &str = "test test test test test test test test test test test junk"; +/// Test private key corresponding to the first account from DEFAULT_ANVIL_MNEMONIC_PHRASE - DO NOT USE IN PRODUCTION pub const DEFAULT_ETH_ACCOUNT_PRIVATE_KEY: &str = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";website/static/devnet_api.json (1)
133-141: Consider adding format validation for the private key parameter.The parameter addition is correct, but consider adding a pattern constraint to validate the private key format (e.g., 64 hex characters with 0x prefix).
{ "name": "deployer_account_private_key", "description": "The private key of the account that shall be used for deploying the messaging contract. Not applicable if `messaging_contract_address` is specified.", "required": false, "schema": { "title": "Deployer account private key", - "type": "string" + "type": "string", + "pattern": "^0x[a-fA-F0-9]{64}$" } }crates/starknet-devnet-core/src/messaging/ethereum.rs (1)
68-85: Simplify the empty code validation logic.The validation correctly checks for contract existence, but the condition can be simplified.
- if &messaging_contract_code.to_string() == "0x" || messaging_contract_code.len() <= 2 { + if messaging_contract_code.is_empty() { return Err(Error::MessagingError(MessagingError::EthersError(format!( "The specified address ({address:#x}) contains no contract" )))); }Note:
Bytes::is_empty()properly handles the case where the code is just "0x".tests/integration/test_messaging.rs (2)
378-402: Consider adding explicit assertions after loading the contract.While the test implicitly verifies success through the
unwrap()call, it would be more robust to add explicit assertions to verify the loaded contract's state or address.Consider adding an assertion after line 401 to verify the loaded contract:
second_devnet .send_custom_rpc( "devnet_postmanLoad", json!({ "network_url": anvil.url, "address": MESSAGING_L1_ADDRESS }), ) .await .unwrap(); + +// Verify the contract was loaded successfully +let load_result = second_devnet + .send_custom_rpc( + "devnet_postmanLoad", + json!({ "network_url": anvil.url, "address": MESSAGING_L1_ADDRESS }), + ) + .await + .unwrap(); +assert_eq!( + load_result.get("messaging_contract_address").unwrap().as_str().unwrap(), + MESSAGING_L1_ADDRESS +);
440-467: Consider making the expected contract address more maintainable.The test hardcodes the expected contract address
"0xca945eebf408d4a73e5d330fc8f6b55cd1e419ff". While this is valid for deterministic deployments, consider extracting it as a constant for better maintainability.Add a constant for the expected address:
+const EXPECTED_CONTRACT_ADDRESS_WITH_SEED_42: &str = "0xca945eebf408d4a73e5d330fc8f6b55cd1e419ff"; + #[tokio::test] async fn deploy_l1_messaging_contract_with_custom_key() {Then use it in the assertion:
assert_eq!( body.get("messaging_contract_address").unwrap().as_str().unwrap(), - "0xca945eebf408d4a73e5d330fc8f6b55cd1e419ff" + EXPECTED_CONTRACT_ADDRESS_WITH_SEED_42 );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/starknet-devnet-core/src/messaging/ethereum.rs(4 hunks)crates/starknet-devnet-core/src/messaging/mod.rs(1 hunks)crates/starknet-devnet-server/src/api/http/endpoints/postman.rs(1 hunks)crates/starknet-devnet-server/src/api/http/models.rs(1 hunks)tests/integration/common/background_anvil.rs(5 hunks)tests/integration/common/background_devnet.rs(1 hunks)tests/integration/common/constants.rs(1 hunks)tests/integration/test_messaging.rs(2 hunks)website/docs/postman.md(4 hunks)website/static/devnet_api.json(2 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/postman.md
[uncategorized] ~44-~44: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...a new one, which is used for exchanging message between L1 and L2. ### L1 network The...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[style] ~61-~61: To strengthen your wording, consider replacing the phrasal verb “leave out”.
Context: ...simplify wish to deploy a new instance, leave out the messaging_contract_address proper...
(OMIT_EXCLUDE)
[style] ~62-~62: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ssaging_contract_address` property. - If your L1 network is a local testnet (e.g...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (19)
tests/integration/common/background_devnet.rs (1)
136-136: Intentional debugging change looks good.The commented stdout piping allows developers to see devnet output during test runs, which is helpful for debugging. The inline comment clearly explains the purpose.
crates/starknet-devnet-server/src/api/http/endpoints/postman.rs (1)
23-29: Implementation correctly adds custom deployer support.The addition of
data.deployer_account_private_key.as_deref()as the third parameter toconfigure_messagingproperly implements the PR's objective of enabling custom account deployment for L1-L2 messaging contracts. Usingas_deref()ensures the parameter is optional, maintaining backward compatibility.crates/starknet-devnet-server/src/api/http/models.rs (1)
32-37: LGTM! Good backward compatibility approach.The renaming of
addresstomessaging_contract_addresswith the#[serde(alias = "address")]attribute ensures backward compatibility while making the field name more descriptive. The addition of the optionaldeployer_account_private_keyfield properly supports custom account deployment as intended.website/docs/postman.md (1)
56-63: Clear and helpful deployment instructions!The documentation clearly explains the different scenarios for messaging contract deployment and when each parameter should be used. This will help users understand how to properly configure their setup based on their L1 network type.
🧰 Tools
🪛 LanguageTool
[style] ~61-~61: To strengthen your wording, consider replacing the phrasal verb “leave out”.
Context: ...simplify wish to deploy a new instance, leave out themessaging_contract_addressproper...(OMIT_EXCLUDE)
[style] ~62-~62: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ssaging_contract_address` property. - If your L1 network is a local testnet (e.g...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
crates/starknet-devnet-core/src/messaging/mod.rs (1)
100-115:✅ Verification successful
LGTM! The new parameter is properly integrated.
The addition of the optional
deployer_account_private_keyparameter is well-documented and correctly passed through to theEthereumMessagingconstructor. This enables custom account deployment as intended by the PR.Let me verify that the
EthereumMessaging::newmethod properly handles the new parameter:
🏁 Script executed:
#!/bin/bash # Description: Verify that EthereumMessaging::new accepts and uses the deployer_account_private_key parameter # Check the EthereumMessaging::new method signature and implementation ast-grep --pattern $'impl EthereumMessaging { $$$ pub async fn new($$$) -> $_ { $$$ } $$$ }' # Also check for any usage of the private key parameter in the ethereum module rg -A 10 "deployer_account_private_key" crates/starknet-devnet-core/src/messaging/Length of output: 26354
✅ Verified:
deployer_account_private_keyis correctly handledVerification of
EthereumMessaging::newshows:
- In crates/starknet-devnet-core/src/messaging/ethereum.rs (lines 121–124), the
deployer_account_private_keyparameter is matched and used to initialize the signer’s private key, falling back toETH_ACCOUNT_DEFAULT.private_keywhenNone.No further changes required.
tests/integration/common/background_anvil.rs (5)
12-12: LGTM!The import of
DEFAULT_ANVIL_MNEMONIC_PHRASEis correctly added alongside other constants.
33-35: LGTM!The delegation pattern maintains backward compatibility while simplifying the code.
37-76: LGTM!The new method correctly implements custom signer support by:
- Passing the mnemonic phrase to Anvil via command-line arguments
- Forwarding the private key to
setup_ethereum_providerfor wallet creation- Maintaining the existing error handling and retry logic
78-87: LGTM!The refactored method correctly delegates to the new implementation with default constants, maintaining backward compatibility. The documentation clearly states the assumptions about the CLI arguments.
182-203: LGTM!The function signature update correctly parameterizes the private key, making the provider setup more flexible. The wallet creation properly uses the provided private key parameter.
website/static/devnet_api.json (2)
126-132: LGTM!The parameter rename from
addresstomessaging_contract_addressimproves API clarity. The description clearly indicates the parameter's purpose and behavior.
152-152: LGTM!The response schema correctly uses the
ETH_ADDRESSreference, maintaining consistency with the parameter schema.crates/starknet-devnet-core/src/messaging/ethereum.rs (4)
106-111: LGTM!The updated method signature correctly adds support for custom deployer accounts while maintaining backward compatibility with the optional parameter.
121-124: LGTM!The private key selection logic correctly implements the fallback pattern, maintaining backward compatibility when no custom key is provided.
132-132: LGTM!Wrapping the provider in
Arcis consistent with theprovider_signerfield and enables safe sharing across threads.
145-146: LGTM!The contract validation ensures that the provided address actually contains deployed contract code, preventing runtime errors when attempting to interact with a non-existent contract.
tests/integration/test_messaging.rs (3)
404-407: LGTM! Test constants are well-named and appropriate.The mnemonic and private key constants are clearly named to indicate they're derived from seed 42, making their purpose obvious for testing custom signer functionality.
350-376: LGTM! Test correctly validates the contract existence check.This test properly verifies that the system rejects loading a messaging contract from an address where no contract is deployed, which aligns with the PR's validation improvements.
410-438:❓ Verification inconclusive
Test name doesn't fully describe the test scenarios.
The test name
setup_anvil_incorrect_eth_private_keyis misleading as it tests two scenarios:
- Loading without providing a private key (lines 421-426)
- Loading with an incorrect private key (lines 427-438)
Additionally, the error message
CallGasCostMoreThanGasLimitseems unexpected for a private key mismatch issue.Consider renaming the test to better reflect both scenarios:
-async fn setup_anvil_incorrect_eth_private_key() { +async fn test_loading_messaging_contract_with_invalid_credentials() {Also, please verify if
CallGasCostMoreThanGasLimitis the expected error for authentication failures:
🌐 Web query:
What error is typically thrown when using an incorrect private key for Ethereum contract deployment?💡 Result:
When using an incorrect private key for Ethereum contract deployment, several specific errors can occur depending on the exact issue with the private key. These errors typically relate to validation problems, authentication failures, or deployment issues.
Common Private Key Errors During Deployment
Invalid Length or Format Errors
One of the most common errors is related to the private key's format or length. Ethereum private keys must be 256 bits (32 bytes) in length and typically start with "0x"[2]. When the private key doesn't meet these requirements, you'll encounter errors like:
- "Invalid private key: invalid length, need 256 bits"[2]
- "Make sure the private key string begins with 0x"[7]
Contract Deployment Failures
Using an incorrect private key often leads to deployment failures, which can manifest as:
- "Error deploying contract: invalid private key"[2]
- "Deployment Failed" errors in various deployment tools like Truffle[5]
- "Contract Creation Code Storage Out of Gas" - which can occur when the transaction signing fails due to private key issues[4]
Authentication Issues
When a private key is missing or not properly set in environment variables, you might see:
- "Failed to connect to the Ethereum client"[6]
- "No contract code at given address" - this often happens when contract deployment silently fails due to private key issues[6]
Transaction Verification Failures
If the private key doesn't match the intended deployer address:
- "Transaction was not mined within expected time"[5]
- "Execution reverted" errors during contract initialization[3]
Troubleshooting Tips
To resolve private key related issues:
- Verify that your private key is correctly formatted with 32 bytes and begins with "0x"[7]
- Ensure the private key environment variable is properly set and accessible to your deployment script[2][8]
- Check that the private key corresponds to an address with sufficient funds to cover deployment gas costs[4]
- Confirm you're not running deployment commands with different permissions (like sudo) that might prevent access to environment variables[2]
- Validate that the nonce is correct for the address associated with your private key[8]
If deployment silently fails, you can use transaction inspection tools like
eth.getTransaction("hash")on the geth console to determine the exact cause of failure[6].Citations:
- 1: Syncing deployment DisputeGameFactory: contract DisputeGameFactory.0.8.15. script failed: Error parsing version ethereum-optimism/optimism#10114
- 2: Error deploying contract: invalid private key: invalid length, need 256 bits gochain/web3#220
- 3: https://forum.openzeppelin.com/t/couldnt-deploy-my-smart-contract/37353
- 4: https://docs.tatum.io/docs/evm-error-contract-creation-code-storage-out-of-gas
- 5: https://community.linea.build/t/i-need-help-deployment-failed/197
- 6: 'No contract code at given address' happens when calling the auto generated DeployToken function. ethereum/go-ethereum#15930
- 7: https://book.getfoundry.sh/forge/deploying
- 8: https://www.axelar.network/blog/same-address-cross-chain-tutorial
- 9: https://docs.etherscan.io/contract-verification/common-verification-errors
- 10: https://docs.ethers.org/v5/troubleshooting/errors/
Rename test for clarity and confirm expected error for invalid credentials
The name
setup_anvil_incorrect_eth_private_keysuggests only an incorrect key, but the test actually covers both a missing key and a wrong key. Consider renaming for accuracy:-async fn setup_anvil_incorrect_eth_private_key() { +async fn test_loading_messaging_contract_with_invalid_credentials() {Also verify whether
CallGasCostMoreThanGasLimitis truly the expected error when credentials are missing or incorrect. If the failure should indicate a key‐validation or authentication error, update the assertion to match:- assert_contains(&body.message, "CallGasCostMoreThanGasLimit"); + assert_contains(&body.message, "Invalid private key");Locations to update:
- tests/integration/test_messaging.rs → lines 410–438
Usage related changes
messaging_contract_address(addressstill acceptable for backwards compatibility but deprecated).Development related changes
BackgroundAnvilmethods for starting Anvil with custom signer.Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
New Features
API Changes
Bug Fixes
Documentation
Tests