Custom account in deploying L1-L2 messaging contract#740
Conversation
WalkthroughThe changes introduce an optional parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Starknet
participant EthereumMessaging
Client->>API: POST /postman/load_l1_messaging_contract\n(with funded_account_private_key)
API->>Starknet: configure_messaging(rpc_url, contract_address, funded_account_private_key)
Starknet->>EthereumMessaging: new(rpc_url, contract_address, funded_account_private_key)
alt funded_account_private_key provided
EthereumMessaging-->>Starknet: Use provided private key for signing
else
EthereumMessaging-->>Starknet: Use default key (ETH_ACCOUNT_DEFAULT)
end
Starknet-->>API: Return contract deployment info
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)tests/integration/test_messaging.rs (3)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 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 (
|
FabijanC
left a comment
There was a problem hiding this comment.
The documentation ought to be updated in website/docs/postman.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/docs/postman.md (2)
26-27: Clarify the New Parameter in the JSON Example:
The JSON snippet now includes thefunded_account_private_keyas an optional field. It would be beneficial to explicitly note that this field is provided solely for the customization of the signing account and that its usage requires careful security handling (e.g., ensuring that production private keys are never exposed). Also, since inline comments (using//) are non-standard in JSON, consider adding a note in the surrounding text clarifying that the comments are for illustrative purposes only.
44-44: Enhanced Explanation for the Custom Signer Account:
The updated descriptive text clearly explains that when thefunded_account_private_keyis provided, the associated account is used as the signer and must be pre-funded. For further clarity, you might consider including an example or a direct reference to where additional security or usage guidelines can be found. This could help users understand the implications of using a custom funded account key in various environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/test_messaging.rs (1)
356-370: Consider simplifying the should_panic test.While the test correctly verifies that using an incorrect private key fails, it contains multiple operations after the likely panic point. For clearer failure diagnosis, consider focusing the test only on the operation expected to panic.
#[tokio::test] #[should_panic] async fn setup_anvil_incorrect_eth_private_key() { // This operation alone should cause the panic let _anvil = BackgroundAnvil::spawn_with_additional_args(&[], Some(INCORRECT_ETH_ACCOUNT_PRIVATE_KEY)) .await .unwrap(); - - let (devnet, _, _) = setup_devnet(&["--account-class", "cairo1"]).await; - - let _body = devnet - .send_custom_rpc("devnet_postmanLoad", json!({ "network_url": anvil.url, "funded_account_private_key": INCORRECT_ETH_ACCOUNT_PRIVATE_KEY })) - .await - .expect("deploy l1 messaging contract failed"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/common/background_anvil.rs(4 hunks)tests/integration/common/constants.rs(1 hunks)tests/integration/test_messaging.rs(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
tests/integration/common/background_anvil.rs (2)
tests/integration/common/background_devnet.rs (1)
spawn_with_additional_args(132-165)crates/starknet-devnet-core/src/messaging/ethereum.rs (1)
private_key(107-107)
tests/integration/test_messaging.rs (2)
tests/integration/common/background_anvil.rs (2)
spawn(33-35)spawn_with_additional_args(38-73)tests/integration/common/background_devnet.rs (2)
spawn(91-93)spawn_with_additional_args(132-165)
🔇 Additional comments (9)
tests/integration/common/constants.rs (1)
104-108: LGTM: Added necessary constants for testing account functionality.The new constants
DEFAULT_ETH_ACCOUNT_ADDRESSandINCORRECT_ETH_ACCOUNT_PRIVATE_KEYare well-defined for testing the custom account functionality in the messaging contract deployment.tests/integration/common/background_anvil.rs (4)
34-35: LGTM: Updated spawn method to use the new parameter structure.The modification correctly updates the
spawnmethod to callspawn_with_additional_argswithNoneas the funded account private key parameter, maintaining backward compatibility.
38-38: LGTM: Enhanced method signature with optional funded account parameter.The updated signature for
spawn_with_additional_argsproperly accepts an optional funded account private key, aligning with the PR objective of allowing custom account specification.
61-64: LGTM: Implemented conditional key selection logic.The implementation correctly chooses between the provided custom key or falls back to the default key, enabling flexible account management during Ethereum provider setup.
170-170: LGTM: Updated provider setup function signature.The function signature for
setup_ethereum_providerhas been appropriately updated to accept the funded account private key parameter.tests/integration/test_messaging.rs (4)
350-354: LGTM: Test for default Ethereum private key setup.This test correctly verifies that spawning Anvil with the default setup uses the expected Ethereum account address.
372-391: LGTM: Test for correct Ethereum private key setup.This test effectively verifies that using a correct custom private key allows for successful spawning and contract deployment.
393-408: LGTM: Integration test for deployment with custom account.This test properly validates the end-to-end flow of deploying the L1 messaging contract using a custom funded account private key, addressing the core functionality of PR #727.
617-617: LGTM: Updated test to use new method signature.The existing test has been properly updated to use the new
spawn_with_additional_argssignature withNonefor the private key parameter.
|
I have added some more tests, please let me know if this is what is expected. I understand that the parameters for the RPC call should go from anvil env, but I am not able to obtain the secret key added during spawn. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/test_messaging.rs (1)
359-373: Consider adding more specific assertions to this testWhile the
#[should_panic]attribute correctly indicates that this test should fail, it would be more informative to add specific assertions about the expected failure mode. This would clarify what exactly is being tested and why it should fail.Additionally, based on the previous review comments, consider testing with a non-default set of prefunded accounts to more comprehensively test the failure case.
#[tokio::test] #[should_panic] async fn setup_anvil_incorrect_eth_private_key() { let anvil = BackgroundAnvil::spawn_with_additional_args(&[], Some(INCORRECT_ETH_ACCOUNT_PRIVATE_KEY)) .await .unwrap(); let (devnet, _, _) = setup_devnet(&["--account-class", "cairo1"]).await; - let _body = devnet + // This should fail because the private key isn't associated with a funded account + let result = devnet .send_custom_rpc("devnet_postmanLoad", json!({ "network_url": anvil.url, "funded_account_private_key": INCORRECT_ETH_ACCOUNT_PRIVATE_KEY })) - .await - .expect("deploy l1 messaging contract failed"); + .await; + + // If we reach this point, the call unexpectedly succeeded + panic!("Expected RPC call to fail, but it succeeded: {:?}", result); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/starknet-devnet-core/src/messaging/ethereum.rs(2 hunks)crates/starknet-devnet-core/src/messaging/mod.rs(1 hunks)crates/starknet-devnet-server/src/api/http/endpoints/postman.rs(1 hunks)tests/integration/common/background_anvil.rs(4 hunks)tests/integration/common/constants.rs(1 hunks)tests/integration/test_messaging.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/starknet-devnet-core/src/messaging/mod.rs
- tests/integration/common/constants.rs
- crates/starknet-devnet-server/src/api/http/endpoints/postman.rs
- tests/integration/common/background_anvil.rs
🧰 Additional context used
🧬 Code Definitions (1)
tests/integration/test_messaging.rs (2)
tests/integration/common/background_anvil.rs (2)
spawn(33-35)spawn_with_additional_args(38-80)tests/integration/common/background_devnet.rs (2)
spawn(91-93)spawn_with_additional_args(132-165)
🔇 Additional comments (8)
crates/starknet-devnet-core/src/messaging/ethereum.rs (3)
87-89: Documentation is appropriate for the new parameterThe docstring clearly describes the purpose of the
funded_account_private_keyparameter, explaining that it's the private key of a funded account on the L1 node that will be used as the signer.
91-93: Parameter addition is implemented correctlyThe new parameter
funded_account_private_keyis properly added as an optional string reference, which aligns with Rust patterns for optional parameters and matches the PR objectives to allow custom account keys.
102-105: Good implementation of fallback logicThe implementation correctly handles the presence or absence of a custom private key, falling back to the default when needed. This satisfies the PR objective of enabling custom funded account private keys for signing while maintaining backward compatibility.
tests/integration/test_messaging.rs (5)
33-35: Import statement updated appropriatelyThe import statement has been updated to include the required constants for testing the new functionality.
350-357: Test validates baseline behaviorThis test establishes a baseline by verifying that the default private key maps to the expected address, which provides good test coverage for the unchanged case.
375-397: Test validates explicit private key usageThis test correctly verifies that explicitly providing the default private key works as expected, ensuring that the system properly handles the new parameter.
399-414: Test directly addresses PR objectiveThis test fulfills the PR objective by specifically testing the deployment of a messaging contract using a custom funded account private key. It provides crucial coverage for the primary new functionality.
623-624: Updated to use new parameter APIThe test has been correctly updated to use the new API by passing
Nonefor thefunded_account_private_keyparameter, indicating that it should use the default behavior.
|
|
||
| /// Spawns an instance at random port. Assumes CLI args in `args` don't contain `--port`. | ||
| pub(crate) async fn spawn_with_additional_args(args: &[&str]) -> Result<Self, TestError> { | ||
| pub(crate) async fn spawn_with_additional_args( |
There was a problem hiding this comment.
I would expect this to be testable without changing the API of BackgroundAnvil. IMO we should be specifying a custom mnemonic or seed rather than relying on the default. Check out this documentation page. Look at how we use the --seed param with BackgroundDevnet in some of the integration tests.
tests/integration/test_messaging.rs
Outdated
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn setup_anvil_correct_eth_private_key() { |
There was a problem hiding this comment.
Tests setup_anvil_incorrect_eth_private_key and setup_anvil_correct_eth_private_key should be a single test.
website/docs/postman.md
Outdated
| "address": "0x123...def", // optional | ||
| "funded_account_private_key": "0x123...def" // optional |
There was a problem hiding this comment.
It is unfortunate that the address and the key seem to refer to the same account. It should be pointed out that they do not.
|
Got it, updating with all the revisions |
* Abstract public+private key as KeyPair: reduce number of fn args
…o sanchet/feat/private-key-l1-contract-deploy
…o sanchet/feat/private-key-l1-contract-deploy
|
I used the |
|
Can you please tell me if there are any updates needed here from my side? Thanks! |
|
@nagxsan Thanks for the work done so far! We will take it on from here to improve the testing and not to keep you waiting (sorry for the waiting). |
* [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 <fc@spaceshard.io>
Attempts to fix #727
Usage related changes
Allows users to add a custom funded account private key as the signer instead of relying on the default one.
Development related changes
Added an integration test
deploy_l1_messaging_contract_with_funded_account_private_keyintests/integration/test_messaging.rsto replicate flow.Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit