feat: add migration script to hopli #24
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughBumps package to 0.15.0; adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI User
participant CLI as SafeModuleSubcommands
participant Exec as execute_* helper
participant Methods as methods.rs helpers
participant Provider as Wallet/Provider
participant Safe as Safe Contract
User->>CLI: invoke Replace / NewModule / AddTarget
CLI->>Exec: dispatch to execute_safe_*
Exec->>Exec: resolve addresses & build node list
Exec->>Provider: load wallet & RPC provider
Exec->>Methods: call create_new_module_* or add_new_network_target or send_safe_transaction_with_threshold_one
Methods->>Methods: call build_default_target(channels_address)
Methods->>Safe: submit multisend (signed) transaction
Safe-->>Methods: tx confirmation
Methods-->>Exec: return result
Exec-->>CLI: propagate outcome
CLI-->>User: display result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds migration functionality to hopli for upgrading safes and modules to v4 compatibility. It introduces three new commands to facilitate the migration process: replace (replaces an old module with a new v4-compatible module), new-module (creates a new v4-compatible module without removing the old one), and add-target (adds new contract targets to existing modules).
- Adds three new subcommands to
SafeModuleSubcommands:Replace,NewModule, andAddTarget - Refactors duplicate code for building default target permissions into a shared utility function
- Updates version from 0.13.0 to 0.14.0
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/safe_module.rs | Adds new migration commands (Replace, NewModule, AddTarget) with execution logic and documentation |
| src/methods.rs | Implements core migration functions for creating modules and adding targets to existing modules |
| src/utils.rs | Adds shared utility function build_default_target for building default target permissions |
| src/payloads.rs | Refactors to use new shared build_default_target utility function |
| Cargo.toml | Updates version to 0.14.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/methods.rs (2)
1404-1404: Replace println! with debug! macro in test.Using
println!in tests creates unnecessary output noise. Thedebug!macro is more appropriate for test diagnostics.🔎 Suggested change
- println!("deployed hopr contracts {:?}", instances); + debug!("deployed hopr contracts {:?}", instances);
1831-1831: Replace println! with debug! macro in test.Using
println!in tests creates unnecessary output noise. Thedebug!macro is more appropriate for test diagnostics.🔎 Suggested change
- println!("deployed hopr contracts {:?}", instances); + debug!("deployed hopr contracts {:?}", instances);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/methods.rssrc/payloads.rssrc/safe_module.rssrc/utils.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/payloads.rs (1)
src/utils.rs (1)
build_default_target(998-1004)
src/safe_module.rs (3)
src/methods.rs (7)
add_new_network_target_to_module(1125-1163)create_new_module_and_include_nodes(1081-1120)create_new_module_include_nodes_and_remove_old_module(1037-1077)debug_node_safe_module_setup_main(1215-1285)include_nodes_to_module(920-957)addresses(442-455)from(114-116)src/utils.rs (3)
new(201-222)a2h(179-181)from(497-509)src/payloads.rs (1)
addresses(69-79)
src/methods.rs (2)
src/payloads.rs (1)
edge_node_deploy_safe_module_and_maybe_include_node(256-300)src/utils.rs (1)
build_default_target(998-1004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Docs / Rust
- GitHub Check: Pre-commit / Pre-commit
- GitHub Check: hopli / Docker
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: Unit Tests Nightly
- GitHub Check: Unit Tests
- GitHub Check: Integration Tests
🔇 Additional comments (18)
Cargo.toml (1)
3-3: LGTM - version bump reflects new features.The minor version increment (0.13.0 → 0.14.0) appropriately signals the addition of new public API functions for Safe module management while maintaining backward compatibility.
src/payloads.rs (3)
126-126: LGTM - refactored to use centralized default target builder.The inline string construction has been correctly replaced with the centralized
build_default_targethelper from utils, improving maintainability and consistency.
168-168: LGTM - consistent refactoring.Correctly uses the centralized
build_default_targethelper.
266-266: LGTM - consistent refactoring.Correctly uses the centralized
build_default_targethelper.src/safe_module.rs (8)
35-38: LGTM - clear documentation for new commands.The documentation concisely describes the three new commands: Replace (module migration with node inclusion), NewModule (v4 module creation), and AddTarget (adding contract targets).
359-400: LGTM - Replace subcommand structure is well-defined.The command includes all necessary parameters for module replacement. The deployment nonce uses appropriate range validation (0 to u64::MAX).
402-439: LGTM - NewModule subcommand structure is appropriate.The command correctly omits
old_module_addresssince it creates a new module without removing the existing one.
441-460: LGTM - AddTarget subcommand has minimal required parameters.The command appropriately includes only what's needed to add a new contract target to an existing module.
836-898: LGTM - execute function follows established patterns.The function correctly:
- Parses node addresses from both CLI args and identity files
- Validates address parsing
- Creates provider and safe instances
- Calls the appropriate underlying method (
create_new_module_include_nodes_and_remove_old_module)
900-956: LGTM - consistent execution pattern.The function follows the same pattern as
execute_safe_module_replace, appropriately callingcreate_new_module_and_include_nodeswithout the old module removal step.
958-990: LGTM - minimal execution for target addition.The function correctly handles the simpler case of adding a target without node operations, calling
add_new_network_target_to_module.
1085-1136: LGTM - match arms correctly wire new subcommands.All three new subcommands are properly dispatched to their corresponding execute functions with correct parameter extraction and type conversions (u64 → U256 for deployment_nonce).
src/methods.rs (6)
32-51: LGTM - import changes align with new functionality.The imports correctly add:
- New contract call types for module migration (
scopeTargetChannelsCall,deployNewV4ModuleCall,migrateSafeV141ToL2AndMigrateToUpgradeableModuleCall)- Centralized
build_default_targethelper- Remove now-unused
DEFAULT_CAPABILITY_PERMISSIONSconstant
700-700: LGTM - refactored to use centralized helper.Correctly replaced inline string construction with
build_default_target.
977-977: LGTM - consistent refactoring.Correctly uses
build_default_target.
1633-1633: LGTM - test code updated consistently.Test code correctly uses the new centralized helper.
1079-1120: Verify delegatecall target address is correct.Similar to
create_new_module_include_nodes_and_remove_old_module, this function usesto: *safe.address()with a delegatecall operation. ThedeployNewV4ModuleCallshould likely delegatecall to theHoprNodeSafeMigrationcontract, not the safe itself.The migration contract address should be passed as a parameter or accessed from contract instances, then used as:
to: migration_contract_address, tx_operation: SafeTxOperation::DelegateCall,
1122-1163: LGTM - function correctly adds target to existing module.The function appropriately:
- Uses
SafeTxOperation::Call(not DelegateCall) to interact with the module- Calls
scopeTargetChannelsCallto add the new channels contract as a target- Follows the established multisend safe transaction pattern
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/safe_module.rs (1)
846-866: Consider extracting node address parsing logic.The node address parsing block (reading from
node_addressstring andlocal_identityfiles) is duplicated across at least six functions in this file. Extracting this into a helper function would improve maintainability.💡 Example refactor
Create a helper function:
fn parse_node_addresses( node_address: Option<String>, local_identity: &IdentityFileArgs, ) -> Result<Vec<Address>, HelperErrors> { let mut node_eth_addresses: Vec<Address> = Vec::new(); if let Some(addresses) = node_address { node_eth_addresses.extend( addresses .split(',') .map(|addr| { Address::from_str(addr) .map_err(|e| HelperErrors::InvalidAddress(format!("Invalid node address: {e:?}"))) }) .collect::<Result<Vec<_>, _>>()?, ); } node_eth_addresses.extend( local_identity .to_addresses() .map_err(|e| HelperErrors::InvalidAddress(format!("Invalid node address: {e:?}")))? .into_iter() .map(a2h), ); Ok(node_eth_addresses) }Then replace the duplicated blocks with:
let node_eth_addresses = parse_node_addresses(node_address, &local_identity)?;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/safe_module.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/safe_module.rs (2)
src/methods.rs (7)
add_new_network_target_to_module(1125-1163)create_new_module_and_include_nodes(1081-1120)create_new_module_include_nodes_and_remove_old_module(1037-1077)debug_node_safe_module_setup_main(1215-1285)debug_node_safe_module_setup_on_balance_and_registries(1168-1206)addresses(442-455)from(114-116)src/payloads.rs (1)
addresses(69-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Integration Tests
- GitHub Check: Unit Tests
- GitHub Check: Build Candidate Binaries (hopli)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/safe_module.rs (1)
34-37: Minor: Consider grammar consistency in doc comment.Line 35 says "include nodes" but for parallel structure with "replaces" on line 34, it should be "includes nodes":
- Replaces an old module... and includes nodes...
However, this is a very minor stylistic issue and doesn't affect functionality.
🔎 Suggested grammar fix
-//! - [SafeModuleSubcommands::Replace] replaces an old module with a new module (v4 compatible) and include nodes in the +//! - [SafeModuleSubcommands::Replace] replaces an old module with a new module (v4 compatible) and includes nodes in the //! new one.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/methods.rssrc/safe_module.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/safe_module.rs (1)
src/methods.rs (8)
add_new_network_target_to_module(1125-1163)create_new_module_and_include_nodes(1081-1120)create_new_module_include_nodes_and_remove_old_module(1037-1077)debug_node_safe_module_setup_main(1215-1285)debug_node_safe_module_setup_on_balance_and_registries(1168-1206)deploy_safe_module_with_targets_and_nodes(669-828)addresses(442-455)from(114-116)
src/methods.rs (2)
src/payloads.rs (1)
edge_node_deploy_safe_module_and_maybe_include_node(256-300)src/utils.rs (1)
build_default_target(998-1004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: hopli / Docker
- GitHub Check: Unit Tests Nightly
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: Integration Tests
🔇 Additional comments (16)
src/methods.rs (7)
32-36: LGTM! Import additions support new module migration functionality.The new imports correctly support the three new async helper functions for module management (replace, create, add target).
46-52: LGTM! Centralized target construction improves maintainability.The removal of
DEFAULT_CAPABILITY_PERMISSIONSfrom the public constants and the addition ofbuild_default_targetto the imports correctly centralizes target construction logic.
700-701: LGTM! Cleaner target construction.The refactor to use
build_default_targetimproves code clarity and maintainability by centralizing the target construction logic.
1035-1077: LGTM! Migration function correctly implements delegatecall pattern.The function properly creates a multisend transaction that delegatecalls to the safe's own address with the
migrateSafeV141ToL2AndMigrateToUpgradeableModuleCallpayload. As confirmed in past review comments, this delegatecall-to-self pattern is intentional for this migration flow and allows the safe to execute migration logic in its own context.
1079-1120: LGTM! Consistent implementation of module creation.The function follows the same delegatecall-to-self pattern as the replace function, using
deployNewV4ModuleCallto create a new V4 module without removing the old one. Implementation is clean and consistent.
1122-1163: LGTM! Target addition correctly uses regular call.Unlike the migration functions, this function correctly uses a regular
Calloperation to interact with the module contract atmodule_address, which is the appropriate pattern for adding targets to an existing module.
1404-1404: LGTM! Test updates consistent with refactoring.The test changes correctly adopt
build_default_targetand include debug print statements that are helpful for test output.Also applies to: 1633-1634, 1670-1672, 1686-1686, 1830-1831
src/safe_module.rs (9)
94-133: LGTM! Comprehensive CLI examples.The three new command examples provide clear, complete demonstrations of the replace, new-module, and add-target subcommands with all required flags.
149-154: LGTM! Import updates align with new functionality.The added imports correctly support the three new execute functions for module management operations.
358-399: LGTM! Replace variant properly structured.The
Replaceenum variant includes all necessary parameters with appropriate validation (deployment_nonce range check) and clear help text.
401-438: LGTM! NewModule variant appropriately structured.The
NewModulevariant correctly omits theold_module_addressparameter since it creates a new module without replacing an existing one.
440-459: LGTM! AddTarget variant correctly simplified.The
AddTargetvariant appropriately excludes node-related parameters since adding a network target to a module doesn't require node information.
833-893: LGTM! Replace execution logic is sound.The function correctly:
- Aggregates node addresses from both CLI args and identity files
- Parses and validates addresses with clear error messages
- Delegates to the appropriate helper function
895-951: LGTM! NewModule execution logic is consistent.The function follows the same pattern as the Replace function but correctly omits old module handling since it creates a new module without replacing an existing one.
953-984: LGTM! AddTarget execution logic correctly simplified.The function appropriately excludes node address handling and focuses solely on parsing the safe and module addresses before adding the network target.
1079-1130: LGTM! Match arms correctly wire up new subcommands.All three new match arms properly:
- Destructure the enum variant fields
- Convert
deployment_noncefrom u64 to U256 where needed- Delegate to the corresponding execute functions
- Propagate errors with
.await?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/methods.rs (1)
1404-1404: Consider using proper logging instead of println! in tests.The test code uses
println!while the rest of the codebase uses the tracing infrastructure. Consider replacing these withdebug!orinfo!for consistency, especially sinceinit_tracing()is already available in the test helpers.🔎 Suggested change
- println!("deployed hopr contracts {:?}", instances); + debug!("deployed hopr contracts {:?}", instances);Apply the same change at both locations (lines 1404 and 1826).
Also applies to: 1826-1826
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/methods.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/methods.rs (2)
src/payloads.rs (2)
edge_node_deploy_safe_module_and_maybe_include_node(256-300)transfer_native_token_payload(51-86)src/utils.rs (1)
build_default_target(998-1004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: hopli / Docker
- GitHub Check: Integration Tests
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: Unit Tests
🔇 Additional comments (3)
src/methods.rs (3)
700-701: LGTM: Excellent refactoring to centralize default target construction.The replacement of manual string-based default target construction with
build_default_targetimproves code maintainability and reduces duplication. All call sites consistently use the utility and properly handle errors.Also applies to: 977-977, 1048-1048, 1092-1092, 1136-1136
1035-1077: LGTM: Well-structured migration function.The function follows consistent patterns with proper error handling and clear documentation. The delegatecall to the safe's own address with the migration payload is the expected pattern for this migration flow.
1079-1163: LGTM: Well-implemented migration helper functions.Both
create_new_module_and_include_nodesandadd_new_network_target_to_moduleare well-structured with:
- Clear documentation of their purpose
- Consistent error handling
- Proper use of the centralized
build_default_targetutility- Appropriate transaction operations (delegatecall for module deployment, regular call for target addition)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Cargo.toml (1)
50-57: Consider pinning git dependencies to specific commits.The hopr-* dependencies reference git branches (
main,master) without commit hashes, which can lead to non-reproducible builds and unexpected behavior when upstream changes. Consider pinning to specific commits or tags for better supply chain security and build reproducibility.Example:
hopr-bindings = { git = "https://github.com/hoprnet/contracts", rev = "abc123..." }Note: This is a pre-existing pattern, not introduced by this PR.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.gitignoreCargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Unit Tests
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: hopli / Docker
🔇 Additional comments (3)
.gitignore (1)
27-28: LGTM! Good security practice.Adding
.envto gitignore prevents accidental commits of secrets and environment variables, which is essential for a CLI tool that handles private keys and provider URLs.Cargo.toml (2)
3-3: LGTM! Appropriate version bump.The minor version bump to 0.14.0 correctly reflects the addition of new CLI features for Safe module migration.
25-25: No issues found. Thealloy-transport-httpdependency (version 1, resolving to 1.2.1) has no known security vulnerabilities and is safe to use.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/methods.rs (1)
202-210: Optional: Remove trailing whitespace on blank lines.Lines 204 and 207 appear to have trailing whitespace on otherwise blank lines. While this doesn't impact functionality, removing it would maintain consistent formatting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/methods.rssrc/safe_module.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/safe_module.rs (3)
src/methods.rs (7)
add_new_network_target_to_module(1209-1247)create_new_module_and_include_nodes(1164-1204)create_new_module_include_nodes_and_remove_old_module(1119-1160)deregister_nodes_from_node_safe_registry_and_remove_from_module(935-999)include_nodes_to_module(1002-1039)addresses(524-537)from(114-116)src/utils.rs (3)
new(201-222)a2h(179-181)from(497-509)src/payloads.rs (1)
addresses(69-79)
src/methods.rs (2)
src/payloads.rs (1)
edge_node_deploy_safe_module_and_maybe_include_node(256-300)src/utils.rs (3)
build_default_target(998-1004)get_create2_address(517-532)from(497-509)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint / Lint
- GitHub Check: hopli / Docker
🔇 Additional comments (9)
src/methods.rs (4)
32-52: LGTM!The updated imports correctly support the new Safe module migration and target management operations.
782-782: LGTM!The use of
build_default_targetcorrectly centralizes the default target construction logic, aligning with the architectural improvements in this PR.Also applies to: 1059-1059
1117-1247: LGTM!The three new public async helpers are well-structured and follow consistent patterns:
create_new_module_include_nodes_and_remove_old_module: Replaces old module with v4-compatible module via delegatecall to migration contractcreate_new_module_and_include_nodes: Creates v4-compatible module while keeping old moduleadd_new_network_target_to_module: Adds new channel target to existing moduleAll functions correctly use
build_default_targetfor target construction and properly orchestrate Safe multisend transactions with threshold-one signing.
1488-1488: LGTM!Test code updates appropriately use
println!for test output and correctly integrate with thebuild_default_targetutility function.Also applies to: 1717-1751, 1909-1910
src/safe_module.rs (5)
23-133: LGTM!The documentation updates clearly describe the three new Safe module commands (Replace, NewModule, AddTarget) and provide helpful example invocations with sample parameters.
150-154: LGTM!Import updates correctly include the new helper functions from
methods.rsneeded for the Replace, NewModule, and AddTarget commands.
358-459: LGTM!The three new enum variants (Replace, NewModule, AddTarget) are well-structured with appropriate CLI argument definitions using clap. The parameter types and validation (e.g.,
RangedU64ValueParserfordeployment_nonce) are correctly configured.
833-986: LGTM!The three new execution functions (
execute_safe_module_replace,execute_safe_create_new_module,execute_safe_create_add_new_target) are well-implemented with:
- Consistent patterns for address resolution from both CLI arguments and identity files
- Proper error handling with descriptive error messages
- Correct delegation to the corresponding helper functions in
methods.rs- Appropriate parameter validation and type conversions (e.g.,
U256::from(deployment_nonce))
1081-1132: LGTM!The match arms for the three new subcommands correctly destructure the enum variants and dispatch to their corresponding execution functions with proper parameter mapping and type conversions.
| /// Use safe to delegatecall to multisend contract | ||
| /// Note that when no additional signature is provided, the safe must have a threshold of one, | ||
| /// so that the transaction can be executed. | ||
| /// Note that the refund address is the caller (safe owner) wallet | ||
| pub async fn send_safe_transaction_with_threshold_one<P: WalletProvider + Provider>( | ||
| safe: SafeSingletonInstance<Arc<P>>, | ||
| signer_key: ChainKeypair, | ||
| multisend_contract: Address, | ||
| multisend_txns: Vec<MultisendTransaction>, | ||
| chain_id: U256, | ||
| nonce: U256, | ||
| ) -> Result<(), HelperErrors> { | ||
| // get signer | ||
| let signer = safe.provider().default_signer_address(); | ||
| // let signer = safe.client().default_sender().expect("client must have a sender"); | ||
| let wallet = PrivateKeySigner::from_slice(signer_key.secret().as_ref()).expect("failed to construct wallet"); | ||
|
|
||
| // prepare a safe transaction: | ||
| // 1. calculate total value | ||
| let total_value = multisend_txns | ||
| .clone() | ||
| .into_iter() | ||
| .fold(U256::ZERO, |acc, cur| acc.add(cur.value)); | ||
| // 2. prepare tx payload | ||
| let tx_payload = MultisendTransaction::build_multisend_tx(multisend_txns); | ||
| let multisend_payload = multiSendCall { | ||
| transactions: tx_payload.into(), | ||
| } | ||
| .abi_encode(); | ||
| // 3. get domain separator | ||
| let domain_separator = get_domain_separator(chain_id, *safe.address()); | ||
|
|
||
| debug!("multisend_payload {:?}", hex::encode(&multisend_payload)); | ||
|
|
||
| // get transaction hash | ||
| let transaction_hash = get_safe_transaction_hash( | ||
| multisend_contract, | ||
| total_value, | ||
| multisend_payload.clone(), | ||
| SafeTxOperation::DelegateCall, | ||
| signer, | ||
| nonce, | ||
| domain_separator, | ||
| ); | ||
|
|
||
| // sign the transaction | ||
| let signature = wallet | ||
| .sign_hash(&B256::from_slice(&transaction_hash)) | ||
| .await | ||
| .unwrap_or_else(|_| panic!("failed to sign a transaction hash")); | ||
| debug!("signature {:?}", hex::encode(signature.as_bytes())); | ||
|
|
||
| // execute the transaction | ||
| let tx_receipt = safe | ||
| .execTransaction( | ||
| multisend_contract, | ||
| total_value, | ||
| multisend_payload.into(), | ||
| SafeTxOperation::DelegateCall.into(), | ||
| U256::ZERO, | ||
| U256::ZERO, | ||
| U256::ZERO, | ||
| Address::ZERO, | ||
| signer, | ||
| Bytes::from(signature.as_bytes()), | ||
| ) | ||
| .send() | ||
| .await? | ||
| // .unwrap_or_else(|_| panic!("failed to exeute a pending transaction")) | ||
| .get_receipt() | ||
| .await?; | ||
|
|
||
| tx_receipt | ||
| .decoded_log::<SafeSingleton::ExecutionSuccess>() | ||
| .ok_or(HelperErrors::MultiSendError)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate function.
The function send_safe_transaction_with_threshold_one (lines 216-292) appears to be an exact duplicate of send_multisend_safe_transaction_with_threshold_one (lines 294-370). Both functions have identical signatures, logic, and behavior.
All three new helper functions in this file (create_new_module_include_nodes_and_remove_old_module, create_new_module_and_include_nodes, add_new_network_target_to_module) call send_multisend_safe_transaction_with_threshold_one, not the duplicate version.
Please remove send_safe_transaction_with_threshold_one (lines 216-292) to eliminate code duplication.
🔎 Verification: Confirm no external usages
#!/bin/bash
# Search for usages of the duplicate function to ensure it's safe to remove
rg -n "send_safe_transaction_with_threshold_one\(" --type rust -C2🤖 Prompt for AI Agents
In src/methods.rs around lines 216-292, the function
send_safe_transaction_with_threshold_one is a duplicate of
send_multisend_safe_transaction_with_threshold_one; delete the entire
send_safe_transaction_with_threshold_one function block (lines 216-292) so only
the intended send_multisend_safe_transaction_with_threshold_one remains, then
run a project-wide search to confirm no remaining references and cargo
build/test to ensure compilation succeeds.
There was a problem hiding this comment.
🧩 Analysis chain
@QYuQianchen, let me verify the differences between these two functions.
🏁 Script executed:
#!/bin/bash
# Extract and compare both functions to identify any differences
echo "=== Searching for usages of send_safe_transaction_with_threshold_one ==="
rg -n "send_safe_transaction_with_threshold_one\(" --type rust -C2
echo -e "\n=== Searching for usages of send_multisend_safe_transaction_with_threshold_one ==="
rg -n "send_multisend_safe_transaction_with_threshold_one\(" --type rust -C2
echo -e "\n=== Extracting send_safe_transaction_with_threshold_one function ==="
sed -n '216,292p' src/methods.rs > /tmp/func1.txt
cat /tmp/func1.txt
echo -e "\n=== Extracting send_multisend_safe_transaction_with_threshold_one function ==="
sed -n '294,370p' src/methods.rs > /tmp/func2.txt
cat /tmp/func2.txt
echo -e "\n=== Comparing the two functions ==="
diff -u /tmp/func1.txt /tmp/func2.txt || echo "Functions differ"
Length of output: 9721
@QYuQianchen, I've analyzed both functions, and the evidence shows they are exact duplicates. Here's what I found:
1. Function Comparison:
The diff output shows the functions are identical except for their names:
- Line 4:
send_safe_transaction_with_threshold_onevssend_multisend_safe_transaction_with_threshold_one
Every other line of code is identical.
2. Usage Analysis:
Searching the entire codebase reveals:
send_multisend_safe_transaction_with_threshold_oneis called 7 times (lines 986, 1028, 1104, 1149, 1193, 1236, 2041)send_safe_transaction_with_threshold_oneis called 0 times - it's unreferenced dead code
3. Recommendation:
Since send_safe_transaction_with_threshold_one is never called and is a duplicate, I recommend removing it (lines 216-292) to:
- Eliminate code duplication
- Reduce maintenance burden
- Prevent future confusion
If you intended these functions to behave differently, could you clarify what distinction you had in mind? Otherwise, removing the unused duplicate would improve code quality.
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Cargo.toml (1)
3-3: Version bump skips 0.14.0.The version jumps from 0.13.0 to 0.15.0, which is unconventional. While not incorrect, maintaining sequential minor versions helps with tracking release history and following semver conventions more clearly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Unit Tests Nightly
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: Unit Tests
- GitHub Check: Integration Tests
🔇 Additional comments (1)
Cargo.toml (1)
25-25: The version constraint"1"is appropriate and follows Rust semver best practices. In Cargo.toml,"1"is shorthand for"^1"(caret version), which constrains to>=1.0.0, <2.0.0—respecting the semver major version boundary. This constraint is standard across the Rust ecosystem and does not present the reproducibility or supply chain risks suggested. The crate is actively maintained (latest 1.3.0 released Jan 6, 2026) with no known security advisories. No action required.Likely an incorrect or invalid review comment.
Description
Add commands to help migrate safe and modules to be compatible with v4 smart contracts.
Added three commands: replace, new-module, and add-target.
Example:
hopli safe-module replace \ --network anvil-localhost \ --contracts-root "../ethereum/contracts" \ --identity-directory "./test" \ --password-path "./test/pwd" \ --node-address 0x47f2710069F01672D01095cA252018eBf08bF85e,0x0D07Eb66Deb54D48D004765E13DcC028cf56592b \ --safe-address 0xce66d19a86600f3c6eb61edd6c431ded5cc92b21 \ --old-module-address 0x5d46d0c5279fd85ce7365e4d668f415685922839 \ --deployment-nonce 123456 \ --private-key 59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d \ --provider-url "http://localhost:8545"hopli safe-module new-module \ --network anvil-localhost \ --contracts-root "../ethereum/contracts" \ --identity-directory "./test" \ --password-path "./test/pwd" \ --node-address 0x47f2710069F01672D01095cA252018eBf08bF85e,0x0D07Eb66Deb54D48D004765E13DcC028cf56592b \ --safe-address 0xce66d19a86600f3c6eb61edd6c431ded5cc92b21 \ --deployment-nonce 123456 \ --private-key 59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d \ --provider-url "http://localhost:8545"hopli safe-module add-target \ --network anvil-localhost \ --contracts-root "../ethereum/contracts" \ --safe-address 0xce66d19a86600f3c6eb61edd6c431ded5cc92b21 \ --module-address 0x5d46d0c5279fd85ce7365e4d668f415685922839 \ --private-key 59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d \ --provider-url "http://localhost:8545"Summary by CodeRabbit
New Features
Refactor
Breaking Changes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.