fix: use plain string for grpc address#6881
fix: use plain string for grpc address#6881SWvheerden merged 12 commits intotari-project:developmentfrom
Conversation
WalkthroughThe changes refactor how gRPC addresses are handled across multiple components. The old function that computed a socket address from a multi-address has been removed and replaced with simpler user prompt functions that return string representations of the addresses. Several configuration types have been updated to use strings instead of multi-address types. Additionally, dependency declarations and the logging configuration have been updated, and endpoint initialization has been streamlined by switching from a Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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 (
|
Test Results (CI) 3 files 129 suites 43m 16s ⏱️ Results for commit 954cc71. ♻️ This comment has been updated with latest results. |
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Please update the related config toml files as well for base_node_grpc_address and p2pool_node_grpc_address:
- f_merge_mining_proxy.toml
- g_miner.toml
I suppose these are also affected: - e_validator_node.toml
- h_collectibles.toml
- i_indexer.toml
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
applications/minotari_app_utilities/src/parse_miner_input.rs (1)
71-82: New P2Pool address prompt with hardcoded portGood addition that follows the same pattern as the base node address prompt.
Note that the port
18145is hardcoded here. Consider whether this port should be obtained from a configuration constant or function similar to howgrpc_default_portis used for the base node, to improve maintainability.- .default(format!("http://127.0.0.1:{}", 18145)) + .default(format!("http://127.0.0.1:{}", grpc_default_port(ApplicationType::ShaP2Pool, network)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
applications/minotari_app_utilities/src/parse_miner_input.rs(1 hunks)applications/minotari_merge_mining_proxy/src/run_merge_miner.rs(3 hunks)applications/minotari_miner/src/run_miner.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- applications/minotari_miner/src/run_miner.rs
- applications/minotari_merge_mining_proxy/src/run_merge_miner.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (2)
applications/minotari_app_utilities/src/parse_miner_input.rs (2)
54-69: New function simplifies base node address handlingGood change. This function simplifies the interface by returning a string representation of the address instead of converting to a SocketAddr. This aligns with the PR title's intention to use plain strings for gRPC addresses, which is more appropriate for gRPC's HTTP/2-based implementation.
A few notes:
- The function returns early within the loop, meaning the loop will only execute once
- No validation is performed on the address format beyond whitespace trimming
23-42:Details
❓ Verification inconclusive
Consider removing unused imports
With the removal of the
base_node_socket_addressfunction, some imports may no longer be used directly in this file, such asmultiaddr_to_socketaddrand possiblySocketAddrandMultiaddr.
🏁 Script executed:
#!/bin/bash # Check for unused imports in the file rg --no-line-number --no-filename "use .*?\{(.+?)\}" applications/minotari_app_utilities/src/parse_miner_input.rs | sed 's/use .*?\{//g; s/\};//g' | tr ',' '\n' | sed 's/^[ \t]*//g; s/[ \t]*$//g' | grep -v "^$" | while read -r import; do if ! rg -q "\b${import}\b" --type rust applications/minotari_app_utilities/src/parse_miner_input.rs; then echo "Potentially unused import: $import" fi doneLength of output: 272
Review Update: Verify Unused Imports Manually
The automated check for unused imports encountered a regex error. Please confirm if the following imports in the file are indeed unused:
multiaddr_to_socketaddr(fromtari_comms::utils::multiaddr)SocketAddr(fromstd::net)Multiaddr(fromtari_comms)It appears that with the removal of the
base_node_socket_addressfunction, these imports might no longer be required. However, since our automated script returned an error, please manually verify their usage in non-import code within the file before making any removals.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
applications/minotari_app_utilities/src/parse_miner_input.rs (1)
78-94: New function to prompt for p2pool address using string format.This function follows the same pattern as
prompt_for_base_node_address, providing a consistent approach for user interaction. It has a hardcoded default port (18145) which might be worth extracting to a constant or configuration if it's used elsewhere in the codebase.Consider extracting the hardcoded port
18145to a named constant for better maintainability:+const DEFAULT_P2POOL_PORT: u16 = 18145; pub fn prompt_for_p2pool_address() -> Result<String, ParseInputError> { loop { let mut address = InputPrompt::<String>::new() .with_prompt("Please enter 'p2pool-grpc-address' ('quit' or 'exit' to quit) ") - .default(format!("http://127.0.0.1:{}", 18145)) + .default(format!("http://127.0.0.1:{}", DEFAULT_P2POOL_PORT)) .interact() .unwrap();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_app_utilities/src/parse_miner_input.rs(3 hunks)applications/minotari_merge_mining_proxy/src/run_merge_miner.rs(3 hunks)applications/minotari_miner/src/run_miner.rs(4 hunks)clients/rust/wallet_grpc_client/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/rust/wallet_grpc_client/src/lib.rs
- applications/minotari_miner/src/run_miner.rs
🧰 Additional context used
🧬 Code Definitions (1)
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs (1)
applications/minotari_app_utilities/src/parse_miner_input.rs (2) (2)
prompt_for_base_node_address(57-76)prompt_for_p2pool_address(78-94)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (8)
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs (5)
262-262: Variable name should be more specific.The variable
base_node_addris misleading in this context as it's being used for the p2pool address. Consider renaming it top2pool_node_addrfor clarity.- let base_node_addr; + let p2pool_node_addr;
263-267: Address handling changed to use string representation.The code now properly checks for
p2pool_node_grpc_addressin the configuration and falls back to prompting the user if not provided. This aligns with the PR objective of using plain strings for gRPC addresses, making the code more straightforward.
228-232: Address handling simplified to use string representation.The code now properly checks for
base_node_grpc_addressin the configuration and falls back to prompting the user if not provided. This change simplifies the address handling by using direct string representations rather than converting from multi-addresses.
235-235: Endpoint creation method changed fromfrom_strtonew.The code now uses
Endpoint::new()instead ofEndpoint::from_str(), which is more appropriate for working with string addresses directly. This is a good change that aligns with the PR objective.
269-269: Endpoint creation method changed fromfrom_strtonew.Consistent with the change for base node endpoint creation, the code now uses
Endpoint::new()instead ofEndpoint::from_str()for p2pool as well. This provides a consistent approach across the codebase.applications/minotari_app_utilities/src/parse_miner_input.rs (3)
23-23: Import simplified to only include necessary functionality.The import has been simplified to only include
FromStr, removing unnecessary imports related to multiaddress handling since the code now works with plain strings.
42-46: Tonic imports updated to support URI validation.The imports have been updated to include
Urifor validating addresses as URIs, which is a better approach for gRPC addresses than using multiaddresses. This is aligned with the PR objective.
57-76: New function to prompt for base node address using string format.This new function replaces the previous
base_node_socket_addressfunction, simplifying the address handling by:
- Returning a string instead of a socket address
- Using
Uri::from_strfor validation instead of complex multiaddress handling- Using HTTP format by default, which is more intuitive for gRPC addresses
The implementation is clean and handles error cases appropriately.
Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs (1)
262-268:⚠️ Potential issueFix variable name inconsistency.
The variable is declared as
p2pool_node_addrbut thenbase_node_addris incorrectly used in the subsequent lines. This will cause runtime errors.let p2pool_node_addr; if let Some(ref a) = config.p2pool_node_grpc_address { - base_node_addr = a.clone(); + p2pool_node_addr = a.clone(); } else { - base_node_addr = prompt_for_p2pool_address()?; + p2pool_node_addr = prompt_for_p2pool_address()?; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs(3 hunks)applications/minotari_miner/src/run_miner.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/minotari_miner/src/run_miner.rs
🧰 Additional context used
🧬 Code Definitions (1)
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs (1)
applications/minotari_app_utilities/src/parse_miner_input.rs (2)
prompt_for_base_node_address(57-76)prompt_for_p2pool_address(78-94)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: file licenses
- GitHub Check: ci
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (3)
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs (3)
28-31: LGTM: Import changes align with new functionality.The new imports for
prompt_for_base_node_addressandprompt_for_p2pool_addresscorrectly support the changes in address handling throughout this file.
227-232: LGTM: Improved address handling.The code now properly checks for a configured base node address and falls back to user prompting if not available. This simplifies address management by using string representations directly.
235-235: LGTM: Streamlined endpoint creation.Changed from
Endpoint::from_str(&base_node_addr)toEndpoint::new(base_node_addr)which is more direct and aligns with the move to string-based addresses.
applications/minotari_merge_mining_proxy/src/run_merge_miner.rs
Outdated
Show resolved
Hide resolved
* development: (412 commits) chore: new release v1.13.1-pre.0 (tari-project#6898) fix: excess sig order in the tx tab (tari-project#6897) chore: ensure thread safety (tari-project#6896) fix: peer order (tari-project#6894) fix: use plain string for grpc address (tari-project#6881) fix: startup arg (tari-project#6889) feat: add num connections to network state (tari-project#6884) refactor: reduce logging to make it less noisy (tari-project#6882) feat: check coinbase count (tari-project#6880) chore(deps): bump dorny/test-reporter from 1 to 2 (tari-project#6883) chore: update readme (tari-project#6878) fix: libtor cli option (tari-project#6877) chore: new version v1.13.0-pre.0 (tari-project#6875) chore: reset network (tari-project#6874) fix: prune mode validation (tari-project#6873) perf: remove duplicate metadata signature verification (tari-project#6866) feat: remove static moneroD response (tari-project#6867) chore(ci): updates to pull version from workspace (tari-project#6868) chore(ci): binaries build continue-on-error when release (tari-project#6865) chore: handle seed words env var (tari-project#6855) ...
4 participants
Summary by CodeRabbit